RFR(M): 8170663: Fix minor issues in corelib and servicabilty coding.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Wed Dec 14 11:00:46 UTC 2016
Hi,
You're right, I had removed the change to e_asin.c.
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.
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 serviceability-dev
mailing list