RFR(M): 8170663: Fix minor issues in corelib and servicabilty coding.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Wed Dec 14 10:23:25 UTC 2016
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 :)
Do you think the promotion will happen tonight, or will it take
another week?
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/
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