RFR(M): 8170663: Fix minor issues in corelib and servicabilty coding.
David Holmes
david.holmes at oracle.com
Wed Dec 14 10:42:26 UTC 2016
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 serviceability-dev
mailing list