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

David Holmes david.holmes at oracle.com
Thu Dec 8 20:59:07 UTC 2016 

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.

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