RFR(M): 8170663: Fix minor issues in corelib and servicabilty coding.
David Holmes
david.holmes at oracle.com
Wed Dec 14 11:28:39 UTC 2016
On 14/12/2016 9:00 PM, Lindenmaier, Goetz wrote:
> Hi,
>
> You're right, I had removed the change to e_asin.c.
You only pointed Joe to that file AFAICS. I would expect he would also
object to the change to k_standard.c for the same reason.
> New webrev anyways:
> http://cr.openjdk.java.net/~goetz/wr16/8170663-corlib_s11y/webrev.05/
>
> java_md_solinux.c adds JLI_MemFree(new_jvmpath) that was missing.
Okay. Thanks.
David
> Best regards,
> Goetz.
>
>
>> -----Original Message-----
>> From: David Holmes [mailto:david.holmes at oracle.com]
>> Sent: Mittwoch, 14. Dezember 2016 11:42
>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>;
>> daniel.daugherty at oracle.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 14/12/2016 8:23 PM, Lindenmaier, Goetz wrote:
>>> Hi David,
>>>
>>> I found "8169317: [s390] Various minor bug fixes and adaptions."
>>> in jdk9/dev this morning, so I thought it has been promoted based
>>> on some older change. I waited for that quite a while because tests
>>> in jdk9/dev kept failing on s390.
>>> How can I get the information when what was promoted? This always
>>> is a wild guess for me ... as well as when what will be promoted :)
>>
>> Yeah there's no notification in the bug report of when hs pushes up to
>> dev. The changeset email is the only record of exactly when it happens.
>>
>>> Do you think the promotion will happen tonight, or will it take
>>> another week?
>>
>> hs goes through Pre-Integration Testing (PIT) before it is pushed to
>> dev. There have been delays in getting that started and so a delay in it
>> finishing and being analyzed. It may still be a couple of days.
>>
>>> I removed
>>> - JLI_StrLen(jrepath) + JLI_StrLen(arch) + JLI_StrLen("/lib//jli:") +
>>> + JLI_StrLen(jrepath) + JLI_StrLen(arch) + JLI_StrLen("/lib/jli:") +
>>> from
>>> http://cr.openjdk.java.net/~goetz/wr16/8170663-corlib_s11y/webrev.04/
>>
>> With that gone that file only has the jvmpath rename - which isn't
>> fixing anything.
>>
>> Joe also asked for the fdlibm changes to dropped.
>>
>> Thanks,
>> David
>>
>>> Best regards,
>>> Goetz.
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: David Holmes [mailto:david.holmes at oracle.com]
>>>> Sent: Mittwoch, 14. Dezember 2016 11:04
>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>;
>>>> daniel.daugherty at oracle.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 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