RFR: 8213622 - Windows VS2013 build failure - "'snprintf': identifier not found"
JC Beyler
jcbeyler at google.com
Tue Nov 13 00:50:08 UTC 2018
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.holmes at 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.holmes at oracle.com
> > <mailto:david.holmes at 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 at 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 <jcbeyler at google.com
> > <mailto:jcbeyler at google.com>
> > > <mailto:jcbeyler at google.com <mailto:jcbeyler at 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.holmes at oracle.com <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com <mailto:david.holmes at 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
> > <mvala at redhat.com <mailto:mvala at redhat.com>
> > > <mailto:mvala at redhat.com <mailto:mvala at 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
More information about the build-dev
mailing list