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

David Holmes david.holmes at oracle.com
Wed Dec 14 10:03:53 UTC 2016 

On 14/12/2016 7:48 PM, Lindenmaier, Goetz wrote:
> Hi,
>
> 8066474 has not been promoted.

hs has not been able to push up to dev yet.

> I'll remove the strlen // fix for aix from this change and
> push it without it. I'd like to get this done.

I've lost track of what is now left in this set of changes ??

David

> Best regards,
>   Goetz.
>
>> -----Original Message-----
>> From: David Holmes [mailto:david.holmes at oracle.com]
>> Sent: Donnerstag, 8. Dezember 2016 23:03
>> To: daniel.daugherty at oracle.com; 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.
>>
>> 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 core-libs-dev mailing list