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

Tue Nov 13 09:32:06 UTC 2018

Hi,

On 11/13/18 1:08 AM, JC Beyler wrote:
> Hi all,
> 
> @Mark: good point, fixed in the new webrev
> @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?

Please, take it. I'm not jdk reviewer nor have anough C++ skills to do a 
reviews. I don't see any issue there. I've also tried to build it with VS2013 
and it works fine. Thanks!

> 
> Here is the new webrev:
> http://cr.openjdk.java.net/~jcbeyler/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>
> 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/>
>>>
>>> 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
>>
> 
> 

-- 
Michal Vala
OpenJDK QE
Red Hat Czech