RFR(M): 8170663: Fix minor issues in corelib and servicabilty coding.
Daniel D. Daugherty
daniel.daugherty at oracle.com
Thu Dec 8 21:31:11 UTC 2016
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.
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