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

Daniel D. Daugherty daniel.daugherty at oracle.com
Thu Dec 5 16:06:58 UTC 2019 

On 12/5/19 8:00 AM, David Holmes wrote:
> 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/

It should be:

s/complicate/complicated/

Dan

>
> 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