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? Thanks, Jc On Mon, Nov 12, 2018 at 4:20 PM David Holmes <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/> > > 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 > -- Thanks, Jc
