RFR(M): 8170663: Fix minor issues in corelib and servicabilty coding.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Wed Dec 14 11:46:23 UTC 2016
> object to the change to k_standard.c for the same reason.
Ok, I remove that too.
Best regards,
Goetz.
> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.com]
> Sent: Mittwoch, 14. Dezember 2016 12:29
> 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 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