[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR (S) 8201224: Make string buffer size dynamic
From: Ioi Lam <ioi.lam () oracle ! com>
Date: 2018-08-22 15:54:11
Message-ID: d23f021f-a52d-2046-56fc-6a72d4d6cbc5 () oracle ! com
[Download RAW message or body]
[Attachment #2 (text/plain)]
Hi Jc,
79 size_t len;
80 char * result;
81 const char * const format = "%s .%s :" JLONG_FORMAT;
82 char dummy = 0;
83
84 pMN = getMethodName(pJvmtiEnv, method);
85 if ( ! pMN )
86 return strdup("NONE");
87
88 len = snprintf(&dummy, sizeof(dummy), format, pMN->classSig,
pMN->methodName, location) + 1;
89
90 if (len <= 0) {
size_t is unsigned, so len will never be less than zero. I think you
should use an int type instead.
Also, I changed the bug title to "Make string buffer size dynamic in
mlvmJvmtiUtils.c". The original name is too generic. Please use this
longer description (feel free to improve it) when you push the change.
The rest of the changes looks good.
Thanks
- Ioi
On 8/21/18 10:20 AM, JC Beyler wrote:
> Hi Alex,
>
> Thanks for the review, I fixed the white-spaces here:
> http://cr.openjdk.java.net/~jcbeyler/8201224/webrev.03/
> <http://cr.openjdk.java.net/%7Ejcbeyler/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@oracle.com
> <mailto:alexey.menkov@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/>
> > <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@oracle.com <mailto:alexey.menkov@oracle.com>
> > <mailto:alexey.menkov@oracle.com
> <mailto:alexey.menkov@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/>
> > > <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@google.com <mailto:jcbeyler@google.com>
> > <mailto:jcbeyler@google.com <mailto:jcbeyler@google.com>>
> > > <mailto:jcbeyler@google.com <mailto:jcbeyler@google.com>
> <mailto:jcbeyler@google.com <mailto:jcbeyler@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/>
> > >
> <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@google.com <mailto:jcbeyler@google.com>
> > <mailto:jcbeyler@google.com <mailto:jcbeyler@google.com>>
> > > <mailto:jcbeyler@google.com
> <mailto:jcbeyler@google.com> <mailto:jcbeyler@google.com
> <mailto:jcbeyler@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@oracle.com
> <mailto:daniel.daugherty@oracle.com>
> > <mailto:daniel.daugherty@oracle.com
> <mailto:daniel.daugherty@oracle.com>>
> > > <mailto:daniel.daugherty@oracle.com
> <mailto:daniel.daugherty@oracle.com>
> > <mailto:daniel.daugherty@oracle.com
> <mailto:daniel.daugherty@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/>
> > >>
> > <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
[Attachment #3 (text/html)]
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Hi Jc,</p>
<p> 79 size_t len;<br>
80 char * result;<br>
81 const char * const format = "%s .%s :" JLONG_FORMAT;<br>
82 char dummy = 0;<br>
83 <br>
84 pMN = getMethodName(pJvmtiEnv, method);<br>
85 if ( ! pMN )<br>
86 return strdup("NONE");<br>
87 <br>
88 len = snprintf(&dummy, sizeof(dummy), format,
pMN->classSig, pMN->methodName, location) + 1;<br>
89 <br>
90 if (len <= 0) {<br>
</p>
<p>size_t is unsigned, so len will never be less than zero. I think
you should use an int type instead.</p>
<p><br>
</p>
<p>Also, I changed the bug title to "Make string buffer size dynamic
in mlvmJvmtiUtils.c". The original name is too generic. Please use
this longer description (feel free to improve it) when you push
the change.</p>
<p>The rest of the changes looks good.</p>
<p><br>
</p>
<p>Thanks</p>
<p>- Ioi<br>
</p>
<p><br>
</p>
<br>
<div class="moz-cite-prefix">On 8/21/18 10:20 AM, JC Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF9BGBwT2Azk1=82eYp+fOf2i3KReB3OV0bX4CWq7QdvKyQohw@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<div dir="ltr">Hi Alex,
<div><br>
</div>
<div>Thanks for the review, I fixed the white-spaces here:</div>
<div><a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.03/"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8201224/webrev.03/</a><br>
</div>
<div><br>
</div>
<div>I'm missing a second reviewer though.</div>
<div><br>
</div>
<div>Thanks in advance,</div>
<div>Jc</div>
<div><br>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Wed, Aug 15, 2018 at 3:58 PM Alex Menkov <<a
href="mailto:alexey.menkov@oracle.com"
moz-do-not-send="true">alexey.menkov@oracle.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Jc,<br>
<br>
Looks good to me. I'm okay with using "buffer=NULL, len=0"
approach.<br>
The only note - could you fix indent of the inserted lines for
<br>
consistency (they have 2 spaces indents, but the files use 4
spaces <br>
indents). No need to resend webrev.<br>
<br>
--alex<br>
<br>
On 08/14/2018 21:26, JC Beyler wrote:<br>
> Hi Alex,<br>
> <br>
> Thanks for looking at it. That simplifies the webrev to
this then:<br>
> <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.02/"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8201224/webrev.02/</a> \
<br> > <<a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.02/"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.02/</a>><br>
> <br>
> I inlined a few comments on your answers.<br>
> <br>
> Thanks for the review and hopefully the new one is better
:-)<br>
> <br>
> Thanks,<br>
> Jc<br>
> <br>
> <br>
> On Tue, Aug 14, 2018 at 3:16 PM Alex Menkov <<a
href="mailto:alexey.menkov@oracle.com" target="_blank"
moz-do-not-send="true">alexey.menkov@oracle.com</a> <br>
> <mailto:<a href="mailto:alexey.menkov@oracle.com"
target="_blank" \
moz-do-not-send="true">alexey.menkov@oracle.com</a>>> wrote:<br>
> <br>
> Hi Jc,<br>
> <br>
> Calculation of the required buffer size ("old fashion
way") looks<br>
> error-prone.<br>
> I don't think we need to care about ancient SUSv2, I
suppose C99 is<br>
> supported by all (or almost all) compilers.<br>
> <br>
> But if you want to keep the code SUSv2-compatible,
you can do<br>
> something like<br>
> char dummy = 0;<br>
> int requiredSize = snprintf(&dummy,
sizeof(dummy),<br>
> formatString,...);<br>
> <br>
> <br>
> Perfect :) For some reason I misread that snprintf would
return the <br>
> number of bytes written and thus would be at max the
length.<br>
> <br>
> In this case, you cannot do it this way, the compiler
produces warnings <br>
> due to the dummy character can't even take the format. To
solve it, I <br>
> would have to create a buffer at least big enough for
empty strings and <br>
> small numbers. We can go down that route if you prefer to
instead of <br>
> using NULL.<br>
> <br>
> <br>
> Other note:<br>
> "if (obtained_size == len) " is useless<br>
> obtained_size is a result of snprintf(result, len,
...)<br>
> and snprintf returns number of chars _excluding_
terminating null, so<br>
> obtained_size is always less than len.<br>
> <br>
> <br>
> Technically this was to guard the case that we were not
going over the <br>
> limit as I misread the return of the method. You are
right, sorry about <br>
> that.<br>
> <br>
> Thanks again!<br>
> Jc<br>
> <br>
> <br>
> --alex<br>
> <br>
> On 08/10/2018 07:55, JC Beyler wrote:<br>
> > Hi all,<br>
> ><br>
> > Anybody motivated to look at this change? :)<br>
> ><br>
> > Webrev: <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.01/"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8201224/webrev.01/</a><br>
> <<a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.01/"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.01/</a>><br>
> > <<a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.01/"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.01/</a>><br>
> > Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8201224"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8201224</a><br> > \
><br> > > Let me know what you think!<br>
> > Jc<br>
> ><br>
> ><br>
> > On Wed, Aug 8, 2018 at 11:34 AM JC Beyler <<a
href="mailto:jcbeyler@google.com" target="_blank"
moz-do-not-send="true">jcbeyler@google.com</a><br>
> <mailto:<a href="mailto:jcbeyler@google.com"
target="_blank" moz-do-not-send="true">jcbeyler@google.com</a>><br>
> > <mailto:<a href="mailto:jcbeyler@google.com"
target="_blank" moz-do-not-send="true">jcbeyler@google.com</a>
<mailto:<a href="mailto:jcbeyler@google.com"
target="_blank" \
moz-do-not-send="true">jcbeyler@google.com</a>>>> wrote:<br>
> ><br>
> > Hi all,<br>
> ><br>
> > Here is a revised webrev that only does the
stated fix in the<br>
> bug. I<br>
> > filed <a
href="https://bugs.openjdk.java.net/browse/JDK-8209153"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8209153</a> for \
fixing<br> > > the style.<br>
> ><br>
> > Webrev:<br>
> <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.01/"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8201224/webrev.01/</a><br>
> <<a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.01/"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.01/</a>><br>
> > <<a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.01/"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.01/</a>><br>
> > Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8201224"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8201224</a><br> > \
><br> > > Let me know what you think!<br>
> ><br>
> > Thanks!<br>
> > Jc<br>
> ><br>
> > On Fri, Aug 3, 2018 at 9:12 AM JC Beyler
<<a href="mailto:jcbeyler@google.com" target="_blank"
moz-do-not-send="true">jcbeyler@google.com</a><br>
> <mailto:<a href="mailto:jcbeyler@google.com"
target="_blank" moz-do-not-send="true">jcbeyler@google.com</a>><br>
> > <mailto:<a
href="mailto:jcbeyler@google.com" target="_blank"
moz-do-not-send="true">jcbeyler@google.com</a> <mailto:<a
href="mailto:jcbeyler@google.com" target="_blank"
moz-do-not-send="true">jcbeyler@google.com</a>>>>
wrote:<br>
> ><br>
> > Yes that was my train of thought as
well (do it in various<br>
> > webrevs and via various bugs for
tracking). Is there a<br>
> will or<br>
> > interest to doing a few passes to just
put these tests<br>
> into the<br>
> > same format once and for all? If so,
I'll create a few<br>
> bugs for<br>
> > a few cleanup webrevs.<br>
> ><br>
> > On the other hand, often there is a
worry that we lose<br>
> history<br>
> > due to the cleanup passes modifying
each line of each file.<br>
> ><br>
> > Hence me asking before doing something
like that :-)<br>
> > Jc<br>
> ><br>
> > On Fri, Aug 3, 2018 at 7:45 AM Daniel
D. Daugherty<br>
> > <<a
href="mailto:daniel.daugherty@oracle.com" target="_blank"
moz-do-not-send="true">daniel.daugherty@oracle.com</a><br>
> <mailto:<a
href="mailto:daniel.daugherty@oracle.com" target="_blank"
moz-do-not-send="true">daniel.daugherty@oracle.com</a>><br>
> > <mailto:<a
href="mailto:daniel.daugherty@oracle.com" target="_blank"
moz-do-not-send="true">daniel.daugherty@oracle.com</a><br>
> <mailto:<a
href="mailto:daniel.daugherty@oracle.com" target="_blank"
moz-do-not-send="true">daniel.daugherty@oracle.com</a>>>>
wrote:<br>
> ><br>
> > > Ps: A lot of these tests seem
to be in GCC style<br>
> (where<br>
> > spaces are all<br>
> > > over the place, 4 space
indent, etc.), should we do a<br>
> > one-pass per<br>
> > > folder to make it into more
standard hotspot style?<br>
> ><br>
> > If you do whitespace/style
cleanups, please do them<br>
> with a<br>
> > separate bug.<br>
> ><br>
> > Dan<br>
> ><br>
> ><br>
> ><br>
> > On 8/3/18 12:11 AM, JC Beyler
wrote:<br>
> >> Hi all,<br>
> >><br>
> >> Small change to the
locationToString method to make the<br>
> >> string dynamically generated.<br>
> >><br>
> >> I added a bit of paranoia to
handle the case where<br>
> things<br>
> >> might be changed so that an
error would propagate to<br>
> the test.<br>
> >><br>
> >> Igor asked me to also at least
fix the NSK_VERIFY<br>
> tests in<br>
> >> the file.<br>
> >><br>
> >> Let me know what you think:<br>
> >><br>
> >> Webrev:<br>
> >> <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.00/"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8201224/webrev.00/</a><br>
> <<a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.00/"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.00/</a>><br>
> >> <br>
> <<a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.00/"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.00/</a>><br>
> >> Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8201224"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8201224</a><br> > \
>><br> > >> Thanks,<br>
> >> Jc<br>
> >><br>
> >> Ps: A lot of these tests seem
to be in GCC style (where<br>
> >> spaces are all over the place,
4 space indent, etc.),<br>
> >> should we do a one-pass per
folder to make it into more<br>
> >> standard hotspot style?<br>
> ><br>
> ><br>
> ><br>
> > --<br>
> ><br>
> > Thanks,<br>
> > Jc<br>
> ><br>
> ><br>
> ><br>
> > --<br>
> ><br>
> > Thanks,<br>
> > Jc<br>
> ><br>
> ><br>
> ><br>
> > --<br>
> ><br>
> > Thanks,<br>
> > Jc<br>
> <br>
> <br>
> <br>
> -- <br>
> <br>
> Thanks,<br>
> Jc<br>
</blockquote>
</div>
<br clear="all">
<div><br>
</div>
-- <br>
<div dir="ltr" class="gmail_signature"
data-smartmail="gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic