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

David Holmes david.holmes at oracle.com
Thu Dec 5 23:02:06 UTC 2019 


On 6/12/2019 2:06 am, Daniel D. Daugherty wrote:
> 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/

Thanks Dan - fat fingers :) I need a keyboard with bigger spaces between 
keys

David

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