RFR(M): 8170663: Fix minor issues in corelib and servicabilty coding.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Fri Dec 9 07:25:19 UTC 2016
Hi,
I'll post a new webrev once the hs code has been propagated to
dev.
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