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

[Kim Barrett]

> On Nov 12, 2018, at 4:11 PM, JC Beyler  wrote:
> 
> Hi all,
> 
> I created this change instead:
> http://cr.openjdk.java.net/~jcbeyler/8213622/webrev.00/

test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp
  80 full_message[len] = '\0’;

That should be
  80 full_message[len - 1] = '\0’;

len includes the space for the terminating NUL.

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

[Michal Vala]

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


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

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



 

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

[David Holmes]

Hi Jc,

On 13/11/2018 10:50 AM, JC Beyler wrote:

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?


Sure - micro-optimization but that's fine.

Cheers,
David


Thanks,
Jc

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

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

 >     
 >      > 
 >      >
 >      > 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
mailto:jcbey...@google.com>
 >     >
 >      > 
      >
 >      >     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
 > 

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

[JC Beyler]

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 
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/
> > 
> > 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  > > 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/
> > 
> >  > 
> >  >
> >  > 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  > 
> >  > >> 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
> >  > mailto:david.hol...@oracle.com>
> > >>
> > wrote:
> >  >
> >  > Hi Michal,
> >  >
> >  > On 12/11/2018 2:18 PM, Michal Vala wrote:
> > 

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

[David Holmes]

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/ 


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

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

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



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

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

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

[David Holmes]

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


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

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

[Kim Barrett]

> On Nov 9, 2018, at 6:07 PM, 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  wrote:
>>> 
>>> Hi,
>>> 
>>> please review following patch fixing build failure on Windows with VS2013 
>>> toolchain.
> 
> Not sure we're still supporting VS2013 ... ??

For now, we still claim to support VS2013; it's listed in the
supported versions in make/autoconf/toolchains_windows.m4.  That will
change with JDK-8208089 (if not sooner).

>>> http://cr.openjdk.java.net/~mvala/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.

Drat!  Yeah, we don't seem to have access to VM code from these test
support codes.  That's too bad.

However, the proposed change doesn't work as intended.

There are two calls to snprintf in ExceptionCheckingJniEnv.cpp, which
would be affected by the proposed change. The first is calling
snprintf with an empty buffer to determine the size of the needed
buffer. _snprintf returns -1 on buffer overflow, rather than the
number of characters that would have been printed if the buffer size
was sufficient to hold the output. So replacing that snprintf call
with _snprintf will return a very different result from what is
expected. I don't see a simple way to solve this while retaining the
source calls to snprintf.

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.

I think there currently aren't any other calls that would be affected,
even though there are other #includes of the file being changed.  But
that's not trivial to check, and could change, possibly leading to
hard to fathom bugs.

There are 8 other calls to snprintf in test/hotspot/jtreg.  Most of
them are linux-only or solaris-only, so don't trip over the lack of
snprintf on windows.

However, one if them, in jvmti_tools.cpp, I think is applicable to
Windows.  It builds because it uses the renaming macro approach, with
the macro being in jvmti_tools.h.  Yuck (IMO).

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

[Michal Vala]

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






http://cr.openjdk.java.net/~mvala/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




--
Michal Vala
OpenJDK QE
Red Hat Czech

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

[David Holmes]

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  wrote:

Hi,

please review following patch fixing build failure on Windows with VS2013 
toolchain.


Not sure we're still supporting VS2013 ... ??



http://cr.openjdk.java.net/~mvala/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

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

[Kim Barrett]

> On Nov 9, 2018, at 11:43 AM, Michal Vala  wrote:
> 
> Hi,
> 
> please review following patch fixing build failure on Windows with VS2013 
> toolchain.
> 
> http://cr.openjdk.java.net/~mvala/jdk/jdk/JDK-8213622/webrev.00/

The failure is in a hotspot test.  It should be using os::snprintf.

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

[Michal Vala]

Hi,

please review following patch fixing build failure on Windows with VS2013 
toolchain.

http://cr.openjdk.java.net/~mvala/jdk/jdk/JDK-8213622/webrev.00/

--
Michal Vala
OpenJDK QE
Red Hat Czech