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@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 <jcbey...@google.com
<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
<david.hol...@oracle.com <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 <mv...@redhat.com
<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
>>
>>
>
--
Thanks,
Jc
--
Thanks,
Jc
