Hi Jc,
On 13/11/2018 10:50 AM, JC Beyler wrote:
Hi David,
Yes sorry, I had not seen that Mark Ludwig had only sent to me his
comment that included this part: "if you're going to bother with
strlen(), you know exactly how many bytes to copy, so don't use any
strXXX API at all - just memcpy()."
Does that make sense?
Sure - micro-optimization but that's fine.
Cheers,
David
Thanks,
Jc
On Mon, Nov 12, 2018 at 4:20 PM David Holmes <david.hol...@oracle.com
<mailto:david.hol...@oracle.com>> wrote:
Hi Jc,
On 13/11/2018 10:08 AM, JC Beyler wrote:
> Hi all,
>
> @Mark: good point, fixed in the new webrev
I assume this was the strncpy -> memcpy change as I haven't see any
email from Mark. What was the issue?
Update is fine anyway.
Thanks,
David
-----
> @David: also good point, just because originally it was written
> differently and I moved the code to this format and didn't move
the +1
> to the "right" spot
> @Michal: do you mind if I take over the bug and push this fix,
could you
> review it as well?
>
> Here is the new webrev:
> http://cr.openjdk.java.net/~jcbeyler/8213622/webrev.01/
<http://cr.openjdk.java.net/%7Ejcbeyler/8213622/webrev.01/>
> <http://cr.openjdk.java.net/%7Ejcbeyler/8213622/webrev.01/>
> Here is the bug: https://bugs.openjdk.java.net/browse/JDK-8213622
>
> Thanks,
> Jc
>
> On Mon, Nov 12, 2018 at 2:08 PM David Holmes
<david.hol...@oracle.com <mailto:david.hol...@oracle.com>
> <mailto:david.hol...@oracle.com
<mailto:david.hol...@oracle.com>>> wrote:
>
> Hi Jc,
>
> This seems okay to me. Only minor query is why you do the +1
> (presumably
> for terminating NUL) on the return_len instead of len ?
>
> Thanks,
> David
>
> On 13/11/2018 7:11 AM, JC Beyler wrote:
> > Hi all,
> >
> > I created this change instead:
> > http://cr.openjdk.java.net/~jcbeyler/8213622/webrev.00/
<http://cr.openjdk.java.net/%7Ejcbeyler/8213622/webrev.00/>
> <http://cr.openjdk.java.net/%7Ejcbeyler/8213622/webrev.00/>
> > <http://cr.openjdk.java.net/%7Ejcbeyler/8213622/webrev.00/>
> >
> > It removes the sprintf calls for strlen and strncpy calls.
I checked
> > that the code works if I force an error on GetObjectClass for
> example:
> >
> > FATAL ERROR in native method: GetObjectClass : Return is NULL
> > at
> >
>
nsk.share.gc.lock.CriticalSectionTimedLocker.criticalSection(CriticalSectionTimedLocker.java:47)
> > at
> >
>
nsk.share.gc.lock.CriticalSectionLocker$1.run(CriticalSectionLocker.java:56)
> > at
>
nsk.share.gc.lock.jniref.JNILocalRefLocker.criticalNative(Native
Method)
> > at
> >
>
nsk.share.gc.lock.jniref.JNILocalRefLocker.criticalSection(JNILocalRefLocker.java:44)
> > at
> >
>
nsk.share.gc.lock.CriticalSectionTimedLocker.criticalSection(CriticalSectionTimedLocker.java:47)
> > at
> >
>
nsk.share.gc.lock.CriticalSectionLocker$1.run(CriticalSectionLocker.java:56)
> > at java.lang.Thread.run(java.base@12-internal/Thread.java:835)
> >
> > I'll pass it through the submit repo if this looks like a
better fix.
> >
> > Let me know what you think,
> > Jc
> >
> > On Sun, Nov 11, 2018 at 10:38 PM JC Beyler
<jcbey...@google.com <mailto:jcbey...@google.com>
> <mailto:jcbey...@google.com <mailto:jcbey...@google.com>>
> > <mailto:jcbey...@google.com <mailto:jcbey...@google.com>
<mailto:jcbey...@google.com <mailto:jcbey...@google.com>>>> wrote:
> >
> > Hi all,
> >
> > If I've caught up with the conversation, it sounds like:
> > - My code breaks VS2013 build but that soon that
won't be
> supported
> > - We can't just do a renaming macro due to my use of
> sprintf with
> > an empty buffer
> > - So we need to either do what was suggested by
Kim:
> > "That first snprintf call could be rewritten using several
> calls to
> > strlen to calculate the needed size in some different
manner. The
> > later call could also be rewritten to use several
calls to strcpy
> > rather than snprintf."
> > Or something to that effect
> >
> > I can work on a fix that will handle this if we want, I'm
> just not
> > sure if that's the path that is being recommended here
though. It
> > seems that in this particular case, it is not really
hard to
> fix the
> > code for now; the day VS2013 is no longer build-able
by other
> pieces
> > of code we can come back to this solution. To be
honest, the
> reason
> > this is like this is due to not being able to have C++
> strings when
> > I tried initially. The day we can have those, then
this code can
> > become even simpler.
> >
> > Let me know what you think and would prefer,
> > Jc
> >
> > On Sun, Nov 11, 2018 at 9:01 PM David Holmes
> > <david.hol...@oracle.com
<mailto:david.hol...@oracle.com> <mailto:david.hol...@oracle.com
<mailto:david.hol...@oracle.com>>
> <mailto:david.hol...@oracle.com
<mailto:david.hol...@oracle.com> <mailto:david.hol...@oracle.com
<mailto:david.hol...@oracle.com>>>>
> wrote:
> >
> > Hi Michal,
> >
> > On 12/11/2018 2:18 PM, Michal Vala wrote:
> > >
> > >
> > > On 11/10/18 12:07 AM, David Holmes wrote:
> > >> cc'ing JC Beyler as this was his code.
> > >>
> > >> On 10/11/2018 4:35 AM, Kim Barrett wrote:
> > >>>> On Nov 9, 2018, at 11:43 AM, Michal Vala
> <mv...@redhat.com <mailto:mv...@redhat.com>
<mailto:mv...@redhat.com <mailto:mv...@redhat.com>>
> > <mailto:mv...@redhat.com <mailto:mv...@redhat.com>
<mailto:mv...@redhat.com <mailto:mv...@redhat.com>>>> wrote:
> > >>>>
> > >>>> Hi,
> > >>>>
> > >>>> please review following patch fixing build
failure on
> > Windows with
> > >>>> VS2013 toolchain.
> > >>
> > >> Not sure we're still supporting VS2013 ... ??
> > >
> > > Minimum accepted by configure is 2010. 2013 is
2nd in
> > priorities after
> > > 2017. It has `VS_SUPPORTED_2013=false` though,
but why not
> > keep it
> > > buildable?
> >
> > That depends on how painful it is to achieve that.
As Kim has
> > explained
> > the suggested fix may allow building but it isn't
correct.
> >
> > And we may not be able to keep this ability to
build with
> VS2013
> > going
> > forward.
> >
> > Thanks,
> > David
> >
> >
> > >>
> > >>>>
> > >>>>
> >
http://cr.openjdk.java.net/~mvala/jdk/jdk/JDK-8213622/webrev.00/
<http://cr.openjdk.java.net/%7Emvala/jdk/jdk/JDK-8213622/webrev.00/>
>
<http://cr.openjdk.java.net/%7Emvala/jdk/jdk/JDK-8213622/webrev.00/>
> >
>
<http://cr.openjdk.java.net/%7Emvala/jdk/jdk/JDK-8213622/webrev.00/>
> > >>>
> > >>> The failure is in a hotspot test. It should
be using
> > os::snprintf.
> > >>
> > >> Tests don't have access to os::snprintf. The
test is just
> > C++ code.
> > >>
> > >> Cheers,
> > >> David
> > >>
> > >>
> > >
> >
> >
> >
> > --
> >
> > Thanks,
> > Jc
> >
> >
> >
> > --
> >
> > Thanks,
> > Jc
>
>
>
> --
>
> Thanks,
> Jc
--
Thanks,
Jc
