Hi, 8066474 has not been promoted. I'll remove the strlen // fix for aix from this change and push it without it. I'd like to get this done.
Best regards, Goetz. > -----Original Message----- > From: David Holmes [mailto:david.hol...@oracle.com] > Sent: Donnerstag, 8. Dezember 2016 23:03 > To: daniel.daughe...@oracle.com; Lindenmaier, Goetz > <goetz.lindenma...@sap.com>; 'Dmitry Samersoff' > <dmitry.samers...@oracle.com>; Java Core Libs <core-libs- > d...@openjdk.java.net>; serviceability-dev (serviceability- > d...@openjdk.java.net) <serviceability-...@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.hol...@oracle.com] > >>>> Sent: Donnerstag, 8. Dezember 2016 09:14 > >>>> To: Lindenmaier, Goetz <goetz.lindenma...@sap.com>; 'Dmitry > Samersoff' > >>>> <dmitry.samers...@oracle.com>; Java Core Libs <core-libs- > >>>> d...@openjdk.java.net>; serviceability-dev (serviceability- > >>>> d...@openjdk.java.net) <serviceability-...@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.samers...@oracle.com] > >>>>>> Sent: Wednesday, December 07, 2016 2:43 PM > >>>>>> To: Lindenmaier, Goetz <goetz.lindenma...@sap.com>; Java Core Libs > >>>>>> <core-libs-dev@openjdk.java.net>; serviceability-dev (serviceability- > >>>>>> d...@openjdk.java.net) <serviceability-...@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. > >