RFR (S) 8201224: Make string buffer size dynamic
JC Beyler
jcbeyler at google.com
Tue Aug 21 21:31:21 UTC 2018
Hi Serguei,
Thanks for the review! Fixed as you requested here:
http://cr.openjdk.java.net/~jcbeyler/8201224/webrev.04/
I tested it in release + slowdebug for vm/mlvm, all 44 tests passed.
Let me know what you think,
Jc
On Tue, Aug 21, 2018 at 1:43 PM serguei.spitsyn at oracle.com <
serguei.spitsyn at oracle.com> wrote:
> Hi Jc,
>
> It looks pretty good to me.
>
> One minor comment:
>
>
> http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.03/test/hotspot/jtreg/vmTestbase/vm/mlvm/indy/func/jvmti/share/IndyRedefineClass.c.udiff.html
>
> locStr = locationToString(jvmti_env, method, location);++ if (locStr == NULL) {+ NSK_DISPLAY0("Error in Single step event: locationToString failed\n");+ gIsErrorOccured = JNI_TRUE;+ }+
> NSK_DISPLAY1("Single step event: %s\n", locStr);
> free(locStr);
>
> I'd suggest to put the use of locStr to the else statement as it is done in stepBreakPopReturn.c:
>
> } else {
> NSK_DISPLAY1("Single step event: %s\n", locStr);
> free(locStr);
> }
>
>
> How did you test it?
> Di you run all vm/mlvm tests?
>
> Thanks,
> Serguei
>
>
> On 8/21/18 10:20, JC Beyler wrote:
>
> Hi Alex,
>
> Thanks for the review, I fixed the white-spaces here:
> http://cr.openjdk.java.net/~jcbeyler/8201224/webrev.03/
>
> I'm missing a second reviewer though.
>
> Thanks in advance,
> Jc
>
>
> On Wed, Aug 15, 2018 at 3:58 PM Alex Menkov <alexey.menkov at oracle.com>
> wrote:
>
>> Hi Jc,
>>
>> Looks good to me. I'm okay with using "buffer=NULL, len=0" approach.
>> The only note - could you fix indent of the inserted lines for
>> consistency (they have 2 spaces indents, but the files use 4 spaces
>> indents). No need to resend webrev.
>>
>> --alex
>>
>> On 08/14/2018 21:26, JC Beyler wrote:
>> > Hi Alex,
>> >
>> > Thanks for looking at it. That simplifies the webrev to this then:
>> > http://cr.openjdk.java.net/~jcbeyler/8201224/webrev.02/
>> > <http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.02/>
>> >
>> > I inlined a few comments on your answers.
>> >
>> > Thanks for the review and hopefully the new one is better :-)
>> >
>> > Thanks,
>> > Jc
>> >
>> >
>> > On Tue, Aug 14, 2018 at 3:16 PM Alex Menkov <alexey.menkov at oracle.com
>> > <mailto:alexey.menkov at oracle.com>> wrote:
>> >
>> > Hi Jc,
>> >
>> > Calculation of the required buffer size ("old fashion way") looks
>> > error-prone.
>> > I don't think we need to care about ancient SUSv2, I suppose C99 is
>> > supported by all (or almost all) compilers.
>> >
>> > But if you want to keep the code SUSv2-compatible, you can do
>> > something like
>> > char dummy = 0;
>> > int requiredSize = snprintf(&dummy, sizeof(dummy),
>> > formatString,...);
>> >
>> >
>> > Perfect :) For some reason I misread that snprintf would return the
>> > number of bytes written and thus would be at max the length.
>> >
>> > In this case, you cannot do it this way, the compiler produces warnings
>> > due to the dummy character can't even take the format. To solve it, I
>> > would have to create a buffer at least big enough for empty strings and
>> > small numbers. We can go down that route if you prefer to instead of
>> > using NULL.
>> >
>> >
>> > Other note:
>> > "if (obtained_size == len) " is useless
>> > obtained_size is a result of snprintf(result, len, ...)
>> > and snprintf returns number of chars _excluding_ terminating null,
>> so
>> > obtained_size is always less than len.
>> >
>> >
>> > Technically this was to guard the case that we were not going over the
>> > limit as I misread the return of the method. You are right, sorry about
>> > that.
>> >
>> > Thanks again!
>> > Jc
>> >
>> >
>> > --alex
>> >
>> > On 08/10/2018 07:55, JC Beyler wrote:
>> > > Hi all,
>> > >
>> > > Anybody motivated to look at this change? :)
>> > >
>> > > Webrev: http://cr.openjdk.java.net/~jcbeyler/8201224/webrev.01/
>> > <http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.01/>
>> > > <http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.01/>
>> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8201224
>> > >
>> > > Let me know what you think!
>> > > Jc
>> > >
>> > >
>> > > On Wed, Aug 8, 2018 at 11:34 AM JC Beyler <jcbeyler at google.com
>> > <mailto:jcbeyler at google.com>
>> > > <mailto:jcbeyler at google.com <mailto:jcbeyler at google.com>>>
>> wrote:
>> > >
>> > > Hi all,
>> > >
>> > > Here is a revised webrev that only does the stated fix in the
>> > bug. I
>> > > filed https://bugs.openjdk.java.net/browse/JDK-8209153 for
>> fixing
>> > > the style.
>> > >
>> > > Webrev:
>> > http://cr.openjdk.java.net/~jcbeyler/8201224/webrev.01/
>> > <http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.01/>
>> > > <http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.01/>
>> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8201224
>> > >
>> > > Let me know what you think!
>> > >
>> > > Thanks!
>> > > Jc
>> > >
>> > > On Fri, Aug 3, 2018 at 9:12 AM JC Beyler <
>> jcbeyler at google.com
>> > <mailto:jcbeyler at google.com>
>> > > <mailto:jcbeyler at google.com <mailto:jcbeyler at google.com>>>
>> wrote:
>> > >
>> > > Yes that was my train of thought as well (do it in
>> various
>> > > webrevs and via various bugs for tracking). Is there a
>> > will or
>> > > interest to doing a few passes to just put these tests
>> > into the
>> > > same format once and for all? If so, I'll create a few
>> > bugs for
>> > > a few cleanup webrevs.
>> > >
>> > > On the other hand, often there is a worry that we lose
>> > history
>> > > due to the cleanup passes modifying each line of each
>> file.
>> > >
>> > > Hence me asking before doing something like that :-)
>> > > Jc
>> > >
>> > > On Fri, Aug 3, 2018 at 7:45 AM Daniel D. Daugherty
>> > > <daniel.daugherty at oracle.com
>> > <mailto:daniel.daugherty at oracle.com>
>> > > <mailto:daniel.daugherty at oracle.com
>> > <mailto:daniel.daugherty at oracle.com>>> wrote:
>> > >
>> > > > Ps: A lot of these tests seem to be in GCC style
>> > (where
>> > > spaces are all
>> > > > over the place, 4 space indent, etc.), should we
>> do a
>> > > one-pass per
>> > > > folder to make it into more standard hotspot
>> style?
>> > >
>> > > If you do whitespace/style cleanups, please do them
>> > with a
>> > > separate bug.
>> > >
>> > > Dan
>> > >
>> > >
>> > >
>> > > On 8/3/18 12:11 AM, JC Beyler wrote:
>> > >> Hi all,
>> > >>
>> > >> Small change to the locationToString method to make
>> the
>> > >> string dynamically generated.
>> > >>
>> > >> I added a bit of paranoia to handle the case where
>> > things
>> > >> might be changed so that an error would propagate to
>> > the test.
>> > >>
>> > >> Igor asked me to also at least fix the NSK_VERIFY
>> > tests in
>> > >> the file.
>> > >>
>> > >> Let me know what you think:
>> > >>
>> > >> Webrev:
>> > >> http://cr.openjdk.java.net/~jcbeyler/8201224/webrev.00/
>> > <http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.00/>
>> > >>
>> > <http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.00/>
>> > >> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8201224
>> > >>
>> > >> Thanks,
>> > >> Jc
>> > >>
>> > >> Ps: A lot of these tests seem to be in GCC style
>> (where
>> > >> spaces are all over the place, 4 space indent,
>> etc.),
>> > >> should we do a one-pass per folder to make it into
>> more
>> > >> standard hotspot style?
>> > >
>> > >
>> > >
>> > > --
>> > >
>> > > Thanks,
>> > > Jc
>> > >
>> > >
>> > >
>> > > --
>> > >
>> > > Thanks,
>> > > Jc
>> > >
>> > >
>> > >
>> > > --
>> > >
>> > > Thanks,
>> > > Jc
>> >
>> >
>> >
>> > --
>> >
>> > Thanks,
>> > Jc
>>
>
>
> --
>
> Thanks,
> Jc
>
>
>
--
Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180821/42ea84be/attachment-0001.html>
More information about the serviceability-dev
mailing list