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

[David Holmes]

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