RFR(M): 8170663: Fix minor issues in corelib and servicabilty coding.

David Holmes david.holmes at oracle.com
Thu Dec 8 22:03:00 UTC 2016 

On 9/12/2016 7:31 AM, Daniel D. Daugherty wrote:
> On 12/8/16 1:59 PM, David Holmes wrote:
>> On 9/12/2016 12:21 AM, Lindenmaier, Goetz wrote:
>>> Hi David,
>>>
>>> thanks for looking at the change.  New webrev:
>>> http://cr.openjdk.java.net/~goetz/wr16/8170663-corlib_s11y/webrev.04/
>>>
>>>> src/java.base/share/native/libjli/java.c
>>>> As far as I can see the existing code is working perfectly fine.
>>> Ah, thanks for the explanation, now I got it!  Removed.
>>>
>>>> Again I'm not sure what the bug is here. On AIX the output string is
>>>> expanded with:
>>>> "%s/lib/%s/jli:"
>>> I first edited this against jdk9/hs, where the arch is gone since
>>> 8066474,
>>> http://hg.openjdk.java.net/jdk9/hs/jdk/rev/81508186e5bc
>>> but the // was not adapted. Then I moved the change to jdk9/dev because
>>> I thought I have to push it there. And yes, in that coding // would
>>> be correct.
>>> So I have to wait until hs is promoted ...
>>
>> So just based on the current file, the change proposed - to remove one
>> / - is not correct until the arch directory is removed.
>
> That change is already in JDK9-hs:
>
> Changeset: c14f9a7b4cab
> Author:    erikj
> Date:      2016-12-05 17:55 +0100
> URL:       http://hg.openjdk.java.net/jdk9/hs/rev/c14f9a7b4cab
>
> 8066474: Remove the lib/ directory from Linux and Solaris images
> Reviewed-by: tbell, ihse
>
> along with changesets in the jdk and hotspot repos along with a few
> closed repos.

Thanks Dan. So we need to see a webrev based on latest hs code - where 
removing the extra / would be correct.

David
-----

> Dan
>
>
>
>>
>> David
>> -----
>>
>>>> The jvmpath -> jvm_newpath change wasn't really necessary - a
>>>> comment on
>>>> the strdup would have sufficed IMO.
>>> Dmitry asked me to add it.  But I think it's ok.
>>>
>>> Can I consider this reviewed now?  I.e. could I push it once 8066474 is
>>> promoted and Joe Darcy agreed?
>>>
>>> Best regards,
>>>   Goetz.
>>>
>>>
>>>> -----Original Message-----
>>>> From: David Holmes [mailto:david.holmes at oracle.com]
>>>> Sent: Donnerstag, 8. Dezember 2016 09:14
>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; 'Dmitry Samersoff'
>>>> <dmitry.samersoff at oracle.com>; Java Core Libs <core-libs-
>>>> dev at openjdk.java.net>; serviceability-dev (serviceability-
>>>> dev at openjdk.java.net) <serviceability-dev at openjdk.java.net>
>>>> Subject: Re: RFR(M): 8170663: Fix minor issues in corelib and
>>>> servicabilty
>>>> coding.
>>>>
>>>> Hi Goetz,
>>>>
>>>> On 8/12/2016 1:26 AM, Lindenmaier, Goetz wrote:
>>>>> Hi Dmitry,
>>>>>
>>>>> yes, new_jvmpath is consistent with the other variables.
>>>>> I also merged the ifs in SDE.c.
>>>>>
>>>>> new webrev:
>>>>> http://cr.openjdk.java.net/~goetz/wr16/8170663-corlib_s11y/webrev.03/
>>>>
>>>> src/java.base/share/native/libjli/java.c
>>>>
>>>> As far as I can see the existing code is working perfectly fine.
>>>> Given a
>>>> jvm.cfg with:
>>>>
>>>> -server KNOWN
>>>> -client IGNORE
>>>> -myvm KNOWN
>>>> -oldvm ALIASED_TO -server
>>>>
>>>> The usage text is:
>>>>
>>>>      -server       to select the "server" VM
>>>>      -myvm         to select the "myvm" VM
>>>>      -oldvm        is a synonym for the "server" VM [deprecated]
>>>>                    The default VM is server,
>>>>                    because you are running on a server-class machine.
>>>>
>>>> which is exactly what I would expect. Why do you think there is a bug?
>>>>
>>>> ---
>>>>
>>>> src/java.base/unix/native/libjli/java_md_solinux.c
>>>>
>>>> Again I'm not sure what the bug is here. On AIX the output string is
>>>> expanded with:
>>>>
>>>> "%s/lib/%s/jli:"
>>>>
>>>> so it needs to account for the extra "/lib//jli:" characters - and that
>>>> is exactly what the existing code does:
>>>>
>>>> + JLI_StrLen("/lib//jli:")
>>>>
>>>> The jvmpath -> jvm_newpath change wasn't really necessary - a
>>>> comment on
>>>> the strdup would have sufficed IMO.
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>
>>>>
>>>>
>>>>> Best regards,
>>>>>   Goetz.
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Dmitry Samersoff [mailto:dmitry.samersoff at oracle.com]
>>>>>> Sent: Wednesday, December 07, 2016 2:43 PM
>>>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Java Core Libs
>>>>>> <core-libs-dev at openjdk.java.net>; serviceability-dev (serviceability-
>>>>>> dev at openjdk.java.net) <serviceability-dev at openjdk.java.net>
>>>>>> Subject: Re: RFR(M): 8170663: Fix minor issues in corelib and
>>>>>> servicabilty
>>>>>> coding.
>>>>>>
>>>>>> Goetz,
>>>>>>
>>>>>> SDE.c:
>>>>>>
>>>>>> You might combine if at ll. 260 and 263 to one but it's just
>>>>>> matter of test.
>>>>>>
>>>>>>  if (sti == baseStratumIndex || sti < 0) {
>>>>>>         return; /* Java stratum - return unchanged */
>>>>>>  }
>>>>>>
>>>>>>> I'm not sure what you mean. I tried to fix it, but please
>>>>>>> double-check the new webrev.
>>>>>>
>>>>>> if cnt is <= 0 loop at l.267
>>>>>>
>>>>>> for (; cnt-- > 0; ++fromEntry) {
>>>>>>
>>>>>> is never run and we effectively do
>>>>>>
>>>>>>  *entryCountPtr = 0;
>>>>>>
>>>>>> at l.283
>>>>>>
>>>>>> So if we you suspect that cnt may become negative or 0:
>>>>>> (your v.01 changes)
>>>>>>
>>>>>>  260         if (sti < 0 && cnt > 0) {
>>>>>>  261             return;
>>>>>>  262         }
>>>>>>
>>>>>> it's better to check it early.
>>>>>>
>>>>>> But I'm not sure we have to care about negative/zero cnt here.
>>>>>>
>>>>>> -Dmitry
>>>>>>
>>>>>> On 2016-12-07 11:37, Lindenmaier, Goetz wrote:
>>>>>>> Hi Dmitry,
>>>>>>>
>>>>>>> thanks for looking at my change!
>>>>>>> Updated webrev:
>>>>>>> http://cr.openjdk.java.net/~goetz/wr16/8170663-corlib_s11y/webrev.02
>>>>>>>
>>>>>>>> * src/java.base/unix/native/libjli/java_md_solinux.c
>>>>>>>> Is this line correct?
>>>>>>>> 519             jvmpath = JLI_StringDup(jvmpath);
>>>>>>>
>>>>>>> It seems pointless. Should I remove it?  (The whole file is a
>>>>>>> horror.)
>>>>>>>
>>>>>>>> * src/jdk.jdwp.agent/share/native/libjdwp/SDE.c
>>>>>>>> It might be better to return immediately if cnt < 0,
>>>>>>>> regardless of value of sti.
>>>>>>>
>>>>>>> I'm not sure what you mean. I tried to fix it, but please
>>>>>>> double-check the new webrev.
>>>>>>>
>>>>>>>> * src/jdk.jdwp.agent/unix/native/libdt_socket/socket_md.c
>>>>>>>> It might be better to write
>>>>>>>>   arg.l_linger = (on) ? (unsigned short)value.i : 0;
>>>>>>>> and leave one copy of setsockopt() call
>>>>>>>
>>>>>>> Yes, looks better.
>>>>>>>
>>>>>>> Best regards,
>>>>>>>   Goetz
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> -Dmitry
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2016-12-06 16:12, Lindenmaier, Goetz wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This change fixes some minor issues found in our code scans.
>>>>>>>>>
>>>>>>>>> I hope this correctly addresses corelib and serviceability issues.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Please review:
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~goetz/wr16/8170663-
>>>>>> corlib_s11y/webrev.01/
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Best regards,
>>>>>>>>>
>>>>>>>>>   Goetz.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Changes in detail:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> e_asin.c
>>>>>>>>>
>>>>>>>>> Code scan reports missing {}.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> The code "if (huge+x>one) {" is only there to set the inexact
>>>>>>>>> flag of
>>>>>>>>> the processor.
>>>>>>>>> It's a way to avoid the C compiler to optimize the code away.
>>>>>>>>> It is
>>>>>>>>> always true,
>>>>>>>>> so the parenthesis of the outer else don't matter.
>>>>>>>>>
>>>>>>>>> Although this basically just adds the {} I would like to submit
>>>>>>>>> this to
>>>>>>>>>
>>>>>>>>> avoid anybody else spends more the 30sec on understanding these
>>>>>>>>>
>>>>>>>>> if statements.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> k_standard.c
>>>>>>>>>
>>>>>>>>> exc.retval is returned below and thus should always be
>>>>>>>>> initialized.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> imageDecompressor.cpp
>>>>>>>>>
>>>>>>>>> Wrong destructor is used.  Reported also by David CARLIER
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> java.c
>>>>>>>>>
>>>>>>>>> in line 1865 'name' was used, it should be 'alias'.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> java_md_solinux.c
>>>>>>>>>
>>>>>>>>> "//" in path is useless. Further down a free is missing.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> SDE.c
>>>>>>>>>
>>>>>>>>> Call to stratumTableIndex can return negative value if
>>>>>>>>> defaultStratumId
>>>>>>>>> == null.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> socket_md.c
>>>>>>>>>
>>>>>>>>> arg.l_linger is passed to setsockopt uninitialized. Its use is
>>>>>>>>> hidden in
>>>>>>>>> the macros.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> unpack.cpp
>>>>>>>>>
>>>>>>>>> n.slice should not get negative argument for end, which is
>>>>>>>>> passed from
>>>>>>>>> dollar1.
>>>>>>>>> But dollar1 can get negative where it is set to the result of
>>>>>>>>> lastIndexOf(DOLLAR_MIN, DOLLAR_MAX, n, dollar2 - 1).
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Dmitry Samersoff
>>>>>>>> Oracle Java development team, Saint Petersburg, Russia
>>>>>>>> * I would love to change the world, but they won't give me the
>>>>>>>> sources.
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Dmitry Samersoff
>>>>>> Oracle Java development team, Saint Petersburg, Russia
>>>>>> * I would love to change the world, but they won't give me the
>>>>>> sources.
>

More information about the serviceability-dev mailing list