RFR: 8213622 - Windows VS2013 build failure - "'snprintf': identifier not found"

David Holmes david.holmes at oracle.com
Tue Nov 13 00:20:37 UTC 2018


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


More information about the build-dev mailing list