RFR(S): 8219142: Make libjimage strncpy uses GCC 8.x friendly

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR(S): 8219142: Make libjimage strncpy uses GCC 8.x friendly

Mikael Vidstedt

Please review this change which addresses some warnings generated by GCC 8.2 related to the uses of strncpy in libjimage/imageFile.cpp.

Bug: https://bugs.openjdk.java.net/browse/JDK-8219142
Webrev: http://cr.openjdk.java.net/~mikael/webrevs/8219142/webrev.00/open/webrev/ <http://cr.openjdk.java.net/~mikael/webrevs/8219142/webrev.00/open/webrev/>


In addition to feedback on the change itself, I’m taking suggestions on what tests to run. An earlier version of the change passed the typical tier1 testing. I’m going to run tier1 on this version as well, but let me know if there are additional tests I should run.

Cheers,
Mikael

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8219142: Make libjimage strncpy uses GCC 8.x friendly

Jim Laskey (Oracle)
I wonder if you should flag overflow so no attempt is made to search with a bogus path. It’s not necessary but prevent future misunderstandings.

Sent from my iPhone

> On Feb 15, 2019, at 5:24 PM, Mikael Vidstedt <[hidden email]> wrote:
>
>
> Please review this change which addresses some warnings generated by GCC 8.2 related to the uses of strncpy in libjimage/imageFile.cpp.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8219142
> Webrev: http://cr.openjdk.java.net/~mikael/webrevs/8219142/webrev.00/open/webrev/ <http://cr.openjdk.java.net/~mikael/webrevs/8219142/webrev.00/open/webrev/>
>
>
> In addition to feedback on the change itself, I’m taking suggestions on what tests to run. An earlier version of the change passed the typical tier1 testing. I’m going to run tier1 on this version as well, but let me know if there are additional tests I should run.
>
> Cheers,
> Mikael
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8219142: Make libjimage strncpy uses GCC 8.x friendly

Mikael Vidstedt

This is interesting. I agree that the code should handle an overflow in a better way, throwing an exception seems like the reasonable thing to do. I started looking at doing exactly that.

However, that also made me have a look at how this code is actually used, and..

It’s not obvious that it actually is..

AFAICT, ImageFileReader::location_path is only used from JIMAGE_ResourcePath (in jimage.cpp). The only “user” I can find of that function in the JDK is in hotspot:

src/hotspot/share/classfile/classLoader.cpp:static JImage_ResourcePath_t           JImageResourcePath     = NULL;
src/hotspot/share/classfile/classLoader.cpp:  JImageResourcePath = CAST_TO_FN_PTR(JImage_ResourcePath_t, os::dll_lookup(handle, "JIMAGE_ResourcePath"));
src/hotspot/share/classfile/classLoader.cpp:  guarantee(JImageResourcePath != NULL, "function JIMAGE_ResourcePath not found”);

That’s the declaration of a variable, the initialization of it, and the check to make sure the lookup actually succeeded. However, the variable isn’t actually used anywhere after that.

Is this dead code which should just be removed instead? Or is JIMAGE_ResourcePath considered exported? Can (non-JDK) native code expect to find and use it?

Cheers,
Mikael

> On Feb 15, 2019, at 1:58 PM, James Laskey <[hidden email]> wrote:
>
> I wonder if you should flag overflow so no attempt is made to search with a bogus path. It’s not necessary but prevent future misunderstandings.
>
> Sent from my iPhone
>
>> On Feb 15, 2019, at 5:24 PM, Mikael Vidstedt <[hidden email]> wrote:
>>
>>
>> Please review this change which addresses some warnings generated by GCC 8.2 related to the uses of strncpy in libjimage/imageFile.cpp.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8219142
>> Webrev: http://cr.openjdk.java.net/~mikael/webrevs/8219142/webrev.00/open/webrev/ <http://cr.openjdk.java.net/~mikael/webrevs/8219142/webrev.00/open/webrev/>
>>
>>
>> In addition to feedback on the change itself, I’m taking suggestions on what tests to run. An earlier version of the change passed the typical tier1 testing. I’m going to run tier1 on this version as well, but let me know if there are additional tests I should run.
>>
>> Cheers,
>> Mikael
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8219142: Make libjimage strncpy uses GCC 8.x friendly

Mikael Vidstedt


> On Feb 15, 2019, at 2:31 PM, Mikael Vidstedt <[hidden email]> wrote:
>
>
> This is interesting. I agree that the code should handle an overflow in a better way, throwing an exception seems like the reasonable thing to do. I started looking at doing exactly that.

Sorry, throwing an exception isn’t a reasonable thing to do, it would have to be flagged some other way, however, the more important question is still: is this code dead?

>
> However, that also made me have a look at how this code is actually used, and..
>
> It’s not obvious that it actually is..
>
> AFAICT, ImageFileReader::location_path is only used from JIMAGE_ResourcePath (in jimage.cpp). The only “user” I can find of that function in the JDK is in hotspot:
>
> src/hotspot/share/classfile/classLoader.cpp:static JImage_ResourcePath_t           JImageResourcePath     = NULL;
> src/hotspot/share/classfile/classLoader.cpp:  JImageResourcePath = CAST_TO_FN_PTR(JImage_ResourcePath_t, os::dll_lookup(handle, "JIMAGE_ResourcePath"));
> src/hotspot/share/classfile/classLoader.cpp:  guarantee(JImageResourcePath != NULL, "function JIMAGE_ResourcePath not found”);
>
> That’s the declaration of a variable, the initialization of it, and the check to make sure the lookup actually succeeded. However, the variable isn’t actually used anywhere after that.
>
> Is this dead code which should just be removed instead? Or is JIMAGE_ResourcePath considered exported? Can (non-JDK) native code expect to find and use it?
>
> Cheers,
> Mikael
>
>> On Feb 15, 2019, at 1:58 PM, James Laskey <[hidden email]> wrote:
>>
>> I wonder if you should flag overflow so no attempt is made to search with a bogus path. It’s not necessary but prevent future misunderstandings.
>>
>> Sent from my iPhone
>>
>>> On Feb 15, 2019, at 5:24 PM, Mikael Vidstedt <[hidden email]> wrote:
>>>
>>>
>>> Please review this change which addresses some warnings generated by GCC 8.2 related to the uses of strncpy in libjimage/imageFile.cpp.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8219142
>>> Webrev: http://cr.openjdk.java.net/~mikael/webrevs/8219142/webrev.00/open/webrev/ <http://cr.openjdk.java.net/~mikael/webrevs/8219142/webrev.00/open/webrev/>
>>>
>>>
>>> In addition to feedback on the change itself, I’m taking suggestions on what tests to run. An earlier version of the change passed the typical tier1 testing. I’m going to run tier1 on this version as well, but let me know if there are additional tests I should run.
>>>
>>> Cheers,
>>> Mikael
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8219142: Make libjimage strncpy uses GCC 8.x friendly

Jim Laskey (Oracle)
In reply to this post by Mikael Vidstedt
The only user IIRC is the native class loader. It’s possible, due to the evolution of the API, that this entry is no longer used. Kill and verify.

Sent from my iPhone

> On Feb 15, 2019, at 6:31 PM, Mikael Vidstedt <[hidden email]> wrote:
>
>
> This is interesting. I agree that the code should handle an overflow in a better way, throwing an exception seems like the reasonable thing to do. I started looking at doing exactly that.
>
> However, that also made me have a look at how this code is actually used, and..
>
> It’s not obvious that it actually is..
>
> AFAICT, ImageFileReader::location_path is only used from JIMAGE_ResourcePath (in jimage.cpp). The only “user” I can find of that function in the JDK is in hotspot:
>
> src/hotspot/share/classfile/classLoader.cpp:static JImage_ResourcePath_t           JImageResourcePath     = NULL;
> src/hotspot/share/classfile/classLoader.cpp:  JImageResourcePath = CAST_TO_FN_PTR(JImage_ResourcePath_t, os::dll_lookup(handle, "JIMAGE_ResourcePath"));
> src/hotspot/share/classfile/classLoader.cpp:  guarantee(JImageResourcePath != NULL, "function JIMAGE_ResourcePath not found”);
>
> That’s the declaration of a variable, the initialization of it, and the check to make sure the lookup actually succeeded. However, the variable isn’t actually used anywhere after that.
>
> Is this dead code which should just be removed instead? Or is JIMAGE_ResourcePath considered exported? Can (non-JDK) native code expect to find and use it?
>
> Cheers,
> Mikael
>
>> On Feb 15, 2019, at 1:58 PM, James Laskey <[hidden email]> wrote:
>>
>> I wonder if you should flag overflow so no attempt is made to search with a bogus path. It’s not necessary but prevent future misunderstandings.
>>
>> Sent from my iPhone
>>
>>> On Feb 15, 2019, at 5:24 PM, Mikael Vidstedt <[hidden email]> wrote:
>>>
>>>
>>> Please review this change which addresses some warnings generated by GCC 8.2 related to the uses of strncpy in libjimage/imageFile.cpp.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8219142
>>> Webrev: http://cr.openjdk.java.net/~mikael/webrevs/8219142/webrev.00/open/webrev/ <http://cr.openjdk.java.net/~mikael/webrevs/8219142/webrev.00/open/webrev/>
>>>
>>>
>>> In addition to feedback on the change itself, I’m taking suggestions on what tests to run. An earlier version of the change passed the typical tier1 testing. I’m going to run tier1 on this version as well, but let me know if there are additional tests I should run.
>>>
>>> Cheers,
>>> Mikael
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8219142: Make libjimage strncpy uses GCC 8.x friendly

Mikael Vidstedt

FWIW, this change, which removes ResourcePath etc. also passes tier1:

http://cr.openjdk.java.net/~mikael/webrevs/8219142/webrev.01/open/webrev/ <http://cr.openjdk.java.net/~mikael/webrevs/8219142/webrev.01/open/webrev/>

So if that function isn’t meant to be used from outside the JDK I’m pretty sure the code is indeed dead and should just be removed.

Cheers,
Mikael

> On Feb 15, 2019, at 2:39 PM, James Laskey <[hidden email]> wrote:
>
> The only user IIRC is the native class loader. It’s possible, due to the evolution of the API, that this entry is no longer used. Kill and verify.
>
> Sent from my iPhone
>
>> On Feb 15, 2019, at 6:31 PM, Mikael Vidstedt <[hidden email]> wrote:
>>
>>
>> This is interesting. I agree that the code should handle an overflow in a better way, throwing an exception seems like the reasonable thing to do. I started looking at doing exactly that.
>>
>> However, that also made me have a look at how this code is actually used, and..
>>
>> It’s not obvious that it actually is..
>>
>> AFAICT, ImageFileReader::location_path is only used from JIMAGE_ResourcePath (in jimage.cpp). The only “user” I can find of that function in the JDK is in hotspot:
>>
>> src/hotspot/share/classfile/classLoader.cpp:static JImage_ResourcePath_t           JImageResourcePath     = NULL;
>> src/hotspot/share/classfile/classLoader.cpp:  JImageResourcePath = CAST_TO_FN_PTR(JImage_ResourcePath_t, os::dll_lookup(handle, "JIMAGE_ResourcePath"));
>> src/hotspot/share/classfile/classLoader.cpp:  guarantee(JImageResourcePath != NULL, "function JIMAGE_ResourcePath not found”);
>>
>> That’s the declaration of a variable, the initialization of it, and the check to make sure the lookup actually succeeded. However, the variable isn’t actually used anywhere after that.
>>
>> Is this dead code which should just be removed instead? Or is JIMAGE_ResourcePath considered exported? Can (non-JDK) native code expect to find and use it?
>>
>> Cheers,
>> Mikael
>>
>>> On Feb 15, 2019, at 1:58 PM, James Laskey <[hidden email]> wrote:
>>>
>>> I wonder if you should flag overflow so no attempt is made to search with a bogus path. It’s not necessary but prevent future misunderstandings.
>>>
>>> Sent from my iPhone
>>>
>>>> On Feb 15, 2019, at 5:24 PM, Mikael Vidstedt <[hidden email]> wrote:
>>>>
>>>>
>>>> Please review this change which addresses some warnings generated by GCC 8.2 related to the uses of strncpy in libjimage/imageFile.cpp.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8219142
>>>> Webrev: http://cr.openjdk.java.net/~mikael/webrevs/8219142/webrev.00/open/webrev/ <http://cr.openjdk.java.net/~mikael/webrevs/8219142/webrev.00/open/webrev/>
>>>>
>>>>
>>>> In addition to feedback on the change itself, I’m taking suggestions on what tests to run. An earlier version of the change passed the typical tier1 testing. I’m going to run tier1 on this version as well, but let me know if there are additional tests I should run.
>>>>
>>>> Cheers,
>>>> Mikael
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8219142: Make libjimage strncpy uses GCC 8.x friendly

Alan Bateman
In reply to this post by Mikael Vidstedt
On 15/02/2019 21:24, Mikael Vidstedt wrote:
> Please review this change which addresses some warnings generated by GCC 8.2 related to the uses of strncpy in libjimage/imageFile.cpp.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8219142
> Webrev: http://cr.openjdk.java.net/~mikael/webrevs/8219142/webrev.00/open/webrev/ <http://cr.openjdk.java.net/~mikael/webrevs/8219142/webrev.00/open/webrev/>
>
>
> In addition to feedback on the change itself, I’m taking suggestions on what tests to run. An earlier version of the change passed the typical tier1 testing. I’m going to run tier1 on this version as well, but let me know if there are additional tests I should run.
>
The jrtfs tests cover this area, the path to specify to jtreg is
jdk/jdk/internal/jrtfs.

I skimmed through the changes and all red looks good :-)  I assume the
bug description can be changed as it's now about removing unused jimage
functions rather than changes to works with a newer version of gcc.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8219142: Make libjimage strncpy uses GCC 8.x friendly

Mikael Vidstedt


> On Feb 18, 2019, at 12:03 AM, Alan Bateman <[hidden email]> wrote:
>
> On 15/02/2019 21:24, Mikael Vidstedt wrote:
>> Please review this change which addresses some warnings generated by GCC 8.2 related to the uses of strncpy in libjimage/imageFile.cpp.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8219142
>> Webrev: http://cr.openjdk.java.net/~mikael/webrevs/8219142/webrev.00/open/webrev/ <http://cr.openjdk.java.net/~mikael/webrevs/8219142/webrev.00/open/webrev/>
>>
>>
>> In addition to feedback on the change itself, I’m taking suggestions on what tests to run. An earlier version of the change passed the typical tier1 testing. I’m going to run tier1 on this version as well, but let me know if there are additional tests I should run.
>>
> The jrtfs tests cover this area, the path to specify to jtreg is jdk/jdk/internal/jrtfs.

Thanks! I ran the jrtfs tests (all of tier2 even) and they all pass.

> I skimmed through the changes and all red looks good :-)  I assume the bug description can be changed as it's now about removing unused jimage functions rather than changes to works with a newer version of gcc.

Updated the summary to reflect the new charter of the enhancement.

Can I please get a review from somebody in the runtime team as well for the hotspot changes?

Cheers,
Mikael