RFR: 8234185: Cleanup usage of canonicalize function between libjava, hotspot and libinstrument

David Holmes david.holmes at oracle.com
Thu Dec 5 13:00:23 UTC 2019 

Hi Christoph,

On 5/12/2019 9:55 pm, Langer, Christoph wrote:
> Hi David,
> 
> thanks again for your efforts.
> 
> Here is a version that ran successfully through jdk-submit (mach5-one-clanger-JDK-8234185-3-20191205-1051-7247172): http://cr.openjdk.java.net/~clanger/webrevs/8234185.2/
> 
> I avoid the inclusion of jdk_util.h in src/java.base/windows/native/libjava/canonicalize_md.c. Do you think this is good to go?

Avoiding the include does seem to be the way to go. That seems so 
obvious now.

src/java.base/windows/native/libjava/canonicalize_md.c

+// We can't include jdk_util.h here because the file is used in Oracle 
in another context
+// #include "jdk_util.h"

Seems to me clients of JDK_Canonicalize need to include jdk_util.h, not 
the files that implement it. So there is no reason to include it here 
and so no need for the comment. Further, does:

src/java.base/unix/native/libjava/canonicalize_md.c

need to include jdk_util.h? I think not.

+/* The appropriate location of getPrefixed() is io_util_md.c, but it is
+   also used in a non-OpenJDK context within Oracle. There, 
canonicalize_md.c
+   is already pulled in and compiled, so to avoid more complicate solutions
+   we keep this method here. */

I don't like to have comments like this but I guess it is needed here. 
Please put the */ on a newline. Also s/complicate/complicates/

src/java.base/windows/native/libjava/io_util_md.c

should now be unchanged, but you've left in the copyright update.

A second review is still needed for the final version of this.

Thanks,
David


> Somebody in Oracle could then eventually clean up things by decoupling the installer from OpenJDK's canonicalize_md.c. I'd open a bug for this.
> 
> Thanks
> Christoph
> 
>> -----Original Message-----
>> From: David Holmes <david.holmes at oracle.com>
>> Sent: Dienstag, 3. Dezember 2019 23:52
>> To: Langer, Christoph <christoph.langer at sap.com>
>> Cc: core-libs-dev at openjdk.java.net; hotspot-runtime-
>> dev at openjdk.java.net; Alan Bateman <Alan.Bateman at oracle.com>; gerard
>> ziemski <gerard.ziemski at oracle.com>
>> Subject: Re: RFR: 8234185: Cleanup usage of canonicalize function between
>> libjava, hotspot and libinstrument
>>
>> Hi Christoph,
>>
>> On 3/12/2019 7:26 pm, Langer, Christoph wrote:
>>> Hi David,
>>>
>>> thanks for taking care of this.
>>>
>>> This would be my updated patch that could hopefully be enabled by just
>> adding the include directory where "jdk_util.h" is located. It would be really
>> great if that'd work. I think it would open up a path to automatically include
>> io_util_md.h by including io_util.h.
>>>
>>> http://cr.openjdk.java.net/~clanger/webrevs/8234185.1/
>>
>> I'm afraid I cannot get this to work at our end. I get the following errors:
>>
>> t:\workspace\open\src\java.base\share\native\libjava\jdk_util.h(46):
>> error C2143: syntax error: missing ')' before '*'
>> t:\workspace\open\src\java.base\share\native\libjava\jdk_util.h(46):
>> error C2143: syntax error: missing '{' before '*'
>> t:\workspace\open\src\java.base\share\native\libjava\jdk_util.h(46):
>> warning C4142: 'size_t': benign redefinition of type
>> C:\ade\mesos\work_dir\jib-
>> ma~1\install\jpg\infra\buildd~1\devkit~1\vs2017~3.0\devkit~1.gz\VC\includ
>> e\vcruntime.h(184):
>> note: see declaration of 'size_t'
>> t:\workspace\open\src\java.base\share\native\libjava\jdk_util.h(46):
>> error C2370: 'size_t': redefinition; different storage class
>> C:\ade\mesos\work_dir\jib-
>> ma~1\install\jpg\infra\buildd~1\devkit~1\vs2017~3.0\devkit~1.gz\VC\includ
>> e\vcruntime.h(184):
>> note: see declaration of 'size_t'
>> t:\workspace\open\src\java.base\share\native\libjava\jdk_util.h(46):
>> error C2146: syntax error: missing ';' before identifier 'info_size'
>> t:\workspace\open\src\java.base\share\native\libjava\jdk_util.h(46):
>> error C2059: syntax error: ')'
>>
>> This pertains to the line:
>>
>> JDK_GetVersionInfo0(jdk_version_info* info, size_t info_size);
>>
>> There is also a second problem in our closed code that will take more
>> effort from someone familiar with that code to resolve. I will file an
>> issue for our install team to pick up.
>>
>> I would ask that this not be pushed for the moment while others figure
>> out how best to handle this.
>>
>> Thanks,
>> David
>> -----
>>
>>
>>> Cheers
>>> Christoph
>>>
>>>
>>>> -----Original Message-----
>>>> From: David Holmes <david.holmes at oracle.com>
>>>> Sent: Dienstag, 3. Dezember 2019 03:13
>>>> To: Langer, Christoph <christoph.langer at sap.com>; Alan Bateman
>>>> <Alan.Bateman at oracle.com>; gerard ziemski
>> <gerard.ziemski at oracle.com>
>>>> Cc: core-libs-dev at openjdk.java.net; hotspot-runtime-
>>>> dev at openjdk.java.net
>>>> Subject: Re: RFR: 8234185: Cleanup usage of canonicalize function
>> between
>>>> libjava, hotspot and libinstrument
>>>>
>>>> Hi Christoph,
>>>>
>>>> Can you please post your updated patch for review and I will use it to
>>>> check the fix for our internal build. Once you have your fix reviewed I
>>>> will push both fixes together.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> On 25/11/2019 10:41 pm, David Holmes wrote:
>>>>> Hi Christoph,
>>>>>
>>>>> On 25/11/2019 10:38 pm, Langer, Christoph wrote:
>>>>>> Hi David,
>>>>>>
>>>>>> thanks for your investigation. I'll prepare a fix to move back
>>>>>> getPrefixed into canonicalize_md.c. However, could you please still
>>>>>> fix your internal build in terms that it would have 'jdk_util.h' in
>>>>>> the include path?
>>>>>
>>>>> That should be simple enough to do.
>>>>>
>>>>> David
>>>>>
>>>>>> Thanks
>>>>>> Christoph
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: David Holmes <david.holmes at oracle.com>
>>>>>>> Sent: Montag, 25. November 2019 01:02
>>>>>>> To: Langer, Christoph <christoph.langer at sap.com>; Alan Bateman
>>>>>>> <Alan.Bateman at oracle.com>; gerard ziemski
>>>> <gerard.ziemski at oracle.com>
>>>>>>> Cc: core-libs-dev at openjdk.java.net; hotspot-runtime-
>>>>>>> dev at openjdk.java.net
>>>>>>> Subject: Re: RFR: 8234185: Cleanup usage of canonicalize function
>>>>>>> between
>>>>>>> libjava, hotspot and libinstrument
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 25/11/2019 8:45 am, David Holmes wrote:
>>>>>>>> On 25/11/2019 7:49 am, David Holmes wrote:
>>>>>>>>> On 25/11/2019 7:33 am, David Holmes wrote:
>>>>>>>>>> Hi Christoph,
>>>>>>>>>>
>>>>>>>>>> On 23/11/2019 12:04 am, Langer, Christoph wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> I'd like to push this change. However, running it through jdk-
>> submit
>>>>>>>>>>> shows reproducible errors:
>>>>>>>>>>>
>>>>>>>>>>> Job: mach5-one-clanger-JDK-8234185-1-20191122-0927-6913189
>>>>>>>>>>> BuildId: 2019-11-22-0926373.christoph.langer.source
>>>>>>>>>>> No failed tests
>>>>>>>>>>> Tasks Summary
>>>>>>>>>>> •    NA: 0
>>>>>>>>>>> •    NOTHING_TO_RUN: 0
>>>>>>>>>>> •    KILLED: 0
>>>>>>>>>>> •    PASSED: 76
>>>>>>>>>>> •    UNABLE_TO_RUN: 0
>>>>>>>>>>> •    EXECUTED_WITH_FAILURE: 1
>>>>>>>>>>> •    FAILED: 0
>>>>>>>>>>> •    HARNESS_ERROR: 0
>>>>>>>>>>> Build
>>>>>>>>>>> 1 Executed with failure
>>>>>>>>>>> o    windows-x64-install-windows-x64-build-19 error while
>> building,
>>>>>>>>>>> return value: 2
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Job: mach5-one-clanger-JDK-8234185-20191121-2313-6898791
>>>>>>>>>>> BuildId: 2019-11-21-2311357.christoph.langer.source
>>>>>>>>>>> No failed tests
>>>>>>>>>>> Tasks Summary
>>>>>>>>>>> •    NA: 0
>>>>>>>>>>> •    NOTHING_TO_RUN: 0
>>>>>>>>>>> •    KILLED: 0
>>>>>>>>>>> •    PASSED: 76
>>>>>>>>>>> •    UNABLE_TO_RUN: 0
>>>>>>>>>>> •    EXECUTED_WITH_FAILURE: 1
>>>>>>>>>>> •    FAILED: 0
>>>>>>>>>>> •    HARNESS_ERROR: 0
>>>>>>>>>>> Build
>>>>>>>>>>> 1 Executed with failure
>>>>>>>>>>> o    windows-x64-install-windows-x64-build-19 error while
>> building,
>>>>>>>>>>> return value: 2
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> David already had a look and let me know that the following was
>>>> the
>>>>>>>>>>> reason:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>
>>>>
>> t:/workspace/open/src/java.base/windows/native/libjava/canonicalize_md.
>>>>>>> c(41):
>>>>>>>>>>> fatal error C1083: Cannot open include file: 'jdk_util.h': No such
>>>>>>>>>>> file or directory
>>>>>>>>>>>
>>>>>>>>>>> This is not explainable to me as I see this running through my
>> local
>>>>>>>>>>> build and our nightly builds without problems. I also can't explain
>>>>>>>>>>> jdk_util.h can't be opened at this place - it should be there and
>>>>>>>>>>> part of the include directories...
>>>>>>>>>>>
>>>>>>>>>>> I'd appreciate any help...
>>>>>>>>>>
>>>>>>>>>> I just dug a little deeper and this is failing in part of our closed
>>>>>>>>>> build for the install repo. There is a library there that is using
>>>>>>>>>> canonicalize_md.c directly - i.e. it adds that file to its source
>>>>>>>>>> files list. The build instructions don't include that directory on
>>>>>>>>>> the include directory list - hence the failure. But it will also fail
>>>>>>>>>> due to the name change you made.
>>>>>>>>>
>>>>>>>>> Actually it appears that the other source code doesn't actually refer
>>>>>>>>> to the canonicalize function at all, so a simple fix may be possible
>>>>>>>>> at the build level on our side. I'm testing that now.
>>>>>>>>
>>>>>>>> It isn't the canonicalize function that is used, it is getPrefixed,
>>>>>>>> which has now been moved to the io_util_md.c file. So a fix will be a
>>>>>>>> bit more involved.
>>>>>>>
>>>>>>> I tried adding io_util_md.c to the library source list instead of
>>>>>>> canonicalize_md.c but that just caused a slew of other compilation
>>>>>>> failures, so I don't see any quick fix for us here.
>>>>>>>
>>>>>>> David
>>>>>>>
>>>>>>>>
>>>>>>>> David
>>>>>>>>
>>>>>>>>>
>>>>>>>>> David
>>>>>>>>> -----
>>>>>>>>>
>>>>>>>>>> Someone will need to work with you to make the necessary
>> changes
>>>> to
>>>>>>>>>> our code.
>>>>>>>>>>
>>>>>>>>>> David
>>>>>>>>>>
>>>>>>>>>>> Thanks
>>>>>>>>>>> Christoph
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>> From: Langer, Christoph
>>>>>>>>>>>> Sent: Donnerstag, 21. November 2019 14:19
>>>>>>>>>>>> To: Alan Bateman <Alan.Bateman at oracle.com>; core-libs-
>>>>>>>>>>>> dev at openjdk.java.net; hotspot-runtime-
>> dev at openjdk.java.net
>>>>>>>>>>>> Subject: RE: RFR: 8234185: Cleanup usage of canonicalize
>> function
>>>>>>>>>>>> between
>>>>>>>>>>>> libjava, hotspot and libinstrument
>>>>>>>>>>>>
>>>>>>>>>>>> Hi Alan,
>>>>>>>>>>>>
>>>>>>>>>>>> thanks for the review. I'll push it then after running through
>>>>>>>>>>>> jdk-submit.
>>>>>>>>>>>>
>>>>>>>>>>>> /Christoph
>>>>>>>>>>>>
>>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>>> From: Alan Bateman <Alan.Bateman at oracle.com>
>>>>>>>>>>>>> Sent: Donnerstag, 21. November 2019 09:51
>>>>>>>>>>>>> To: Langer, Christoph <christoph.langer at sap.com>; core-libs-
>>>>>>>>>>>>> dev at openjdk.java.net; hotspot-runtime-
>>>> dev at openjdk.java.net
>>>>>>>>>>>>> Subject: Re: RFR: 8234185: Cleanup usage of canonicalize
>>>> function
>>>>>>>>>>>>> between
>>>>>>>>>>>>> libjava, hotspot and libinstrument
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 14/11/2019 15:37, Langer, Christoph wrote:
>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> please review this cleanup change regarding function
>>>>>>>>>>>>>> "canonicalize" of
>>>>>>>>>>>>> libjava.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8234185
>>>>>>>>>>>>>> Webrev:
>>>> http://cr.openjdk.java.net/~clanger/webrevs/8234185.0/
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The goal is to cleanup how this function is defined and used.
>>>> One
>>>>>>>>>>>>>> thing is,
>>>>>>>>>>>>> that there was an unnecessary wrapper function
>> "Canonicalize"
>>>> in
>>>>>>>>>>>>> jni_util.c.
>>>>>>>>>>>>> It wrapped the call to "canonicalize". We can get rid of this
>>>>>>>>>>>>> wrapper.
>>>>>>>>>>>>> Unfortunately, it is not possible to just export "canonicalize"
>>>>>>>>>>>>> since this will
>>>>>>>>>>>>> conflict with a method signature from the math library, at
>> least
>>>>>>>>>>>>> on modern
>>>>>>>>>>>>> Linuxes. So I decided to call the method JDK_Canonicalize and
>>>> will
>>>>>>>>>>>>> correctly
>>>>>>>>>>>>> define it in jdk_util.h which can be included everywhere.
>>>>>>>>>>>>>>
>>>>>>>>>>>>> I think this change is okay. My main concern when initially
>> seeing
>>>>>>>>>>>>> this
>>>>>>>>>>>>> go by was that it would leak the \\?\ or \\?\UNC\ prefix into
>> the
>>>>>>>>>>>>> canonical File when it wasn't there previously, this would of
>>>>>>>>>>>>> course
>>>>>>>>>>>>> have several implications. But I think you have it right and this
>>>>>>>>>>>>> is, as
>>>>>>>>>>>>> you position, just refactoring/cleanup.
>>>>>>>>>>>>>
>>>>>>>>>>>>> -Alan

More information about the core-libs-dev mailing list