RFR(M): 8170663: Fix minor issues in corelib and servicabilty coding.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Mon Dec 12 08:47:35 UTC 2016
Hi Joe,
I'm not happy with this, but I'll remove the change to asin from
my patch. Thanks for looking at it.
Best regards,
Goetz.
> -----Original Message-----
> From: Joseph D. Darcy [mailto:joe.darcy at oracle.com]
> Sent: Freitag, 9. Dezember 2016 23:29
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> Cc: Java Core Libs <core-libs-dev at openjdk.java.net>
> Subject: Re: RFR(M): 8170663: Fix minor issues in corelib and servicabilty
> coding.
>
> Hi Goetz,
>
> Please leave fdlibm asin alone as part of this change. The fdlibm
> library has some anachronistic coding idioms and certainly at least in
> this one case misleading code, but I'd prefer to leave the code as-is
> until it is ported to Java.
>
> Thanks,
>
> -Joe
>
> On 12/8/2016 6:32 AM, Lindenmaier, Goetz wrote:
> > Hi Joe,
> >
> > could you please have a look at my change to e_asin.c:
> > http://cr.openjdk.java.net/~goetz/wr16/8170663-
> corlib_s11y/webrev.04/src/java.base/share/native/libfdlibm/e_asin.c.udiff.html
> >
> > The change conserves the situation, I just added {} and comments.
> > I would appreciate to clean this up as it's hard to understand and our
> > code scan does not like it the way it is.
> >
> > We had talked about this in my thread where I learned to read this code :)
> > http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-
> December/045165.html
> >
> > Best regards,
> > Goetz.
> >
> >
> >> -----Original Message-----
> >> From: Martin Buchholz [mailto:martinrb at google.com]
> >> Sent: Mittwoch, 7. Dezember 2016 18:09
> >> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Joe Darcy
> >> <joe.darcy at oracle.com>
> >> Cc: 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.
> >>
> >> This changes fdlibm, which has historically been untouched. Don't commit
> >> without Joe Darcy's approval.
> >>
> >> On Wed, Dec 7, 2016 at 7:26 AM, Lindenmaier, Goetz
> >> <goetz.lindenmaier at sap.com <mailto:goetz.lindenmaier at sap.com> >
> 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/ <http://cr.openjdk.java.net/~goetz/wr16/8170663-
> >> corlib_s11y/webrev.03/>
> >>
> >> Best regards,
> >> Goetz.
> >>
> >> > -----Original Message-----
> >> > From: Dmitry Samersoff [mailto:dmitry.samersoff at oracle.com
> >> <mailto:dmitry.samersoff at oracle.com> ]
> >> > Sent: Wednesday, December 07, 2016 2:43 PM
> >> > To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com
> >> <mailto:goetz.lindenmaier at sap.com> >; Java Core Libs
> >> > <core-libs-dev at openjdk.java.net <mailto:core-libs-
> >> dev at openjdk.java.net> >; serviceability-dev (serviceability-
> >> > dev at openjdk.java.net <mailto:dev at openjdk.java.net> )
> >> <serviceability-dev at openjdk.java.net <mailto: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 <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-
> >> <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