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/>
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>> 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/>
>
> 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>>> 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>>>
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>>> 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/>
> >>>
> >>> 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
