RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

Langer, Christoph

Hi,

 

please review the first extracted patch from 8192978. This one is about correcting the jlong format string in src/jdk.jdi/share/native/libdt_shmem/shmemBase.c.

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8193183

WebRev: http://cr.openjdk.java.net/~clanger/webrevs/8193183.0/

 

I believe the libdt_shmem is only built on windows.

 

Thanks,

Christoph

 

Reply | Threaded
Open this post in threaded view
|

Re: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

Chris Plummer
Looks good to me. I can sponsor the push for you.

thanks,

Chris

On 12/7/17 7:21 AM, Langer, Christoph wrote:

Hi,

 

please review the first extracted patch from 8192978. This one is about correcting the jlong format string in src/jdk.jdi/share/native/libdt_shmem/shmemBase.c.

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8193183

WebRev: http://cr.openjdk.java.net/~clanger/webrevs/8193183.0/

 

I believe the libdt_shmem is only built on windows.

 

Thanks,

Christoph

 


Reply | Threaded
Open this post in threaded view
|

Re: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

serguei.spitsyn@oracle.com
Hi Christoph,

It looks good.
Thank you taking care about it!

Yes, the libdt_shmem is for Windows only.

Thanks,
Serguei


On 12/7/17 10:13, Chris Plummer wrote:
Looks good to me. I can sponsor the push for you.

thanks,

Chris

On 12/7/17 7:21 AM, Langer, Christoph wrote:

Hi,

 

please review the first extracted patch from 8192978. This one is about correcting the jlong format string in src/jdk.jdi/share/native/libdt_shmem/shmemBase.c.

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8193183

WebRev: http://cr.openjdk.java.net/~clanger/webrevs/8193183.0/

 

I believe the libdt_shmem is only built on windows.

 

Thanks,

Christoph

 



Reply | Threaded
Open this post in threaded view
|

Re: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

Alex Menkov-2
In reply to this post by Langer, Christoph
Looks like we already have definitions to workaround Visual Studio
issues (including absence of PRId64) in
open/src/hotspot/share/utilities/globalDefinitions_visCPP.hpp
It would be better to reuse it (to have all workarounds in a single place)

I suppose to utilize it you need
#include "utilities/globalDefinitions.hpp"
It includes corresponding "globalDefinitions_*.hpp" depending on the
compiler used.

--alex

On 12/07/2017 07:21, Langer, Christoph wrote:

> Hi,
>
> please review the first extracted patch from 8192978. This one is about
> correcting the jlong format string in
> src/jdk.jdi/share/native/libdt_shmem/shmemBase.c.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8193183
>
> WebRev: http://cr.openjdk.java.net/~clanger/webrevs/8193183.0/
>
> I believe the libdt_shmem is only built on windows.
>
> Thanks,
>
> Christoph
>
Reply | Threaded
Open this post in threaded view
|

RE: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

Langer, Christoph
In reply to this post by Chris Plummer

Thanks Chris and Serguei for reviewing.

 

I can do the push myself as it isn’t hotspot. I’ll send out other webrevs tomorrow for the other stuff.

 

Best regards

Christoph

 

From: Chris Plummer [mailto:[hidden email]]
Sent: Donnerstag, 7. Dezember 2017 19:14
To: Langer, Christoph <[hidden email]>; [hidden email]
Cc: Lindenmaier, Goetz <[hidden email]>
Subject: Re: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

 

Looks good to me. I can sponsor the push for you.

thanks,

Chris

On 12/7/17 7:21 AM, Langer, Christoph wrote:

Hi,

 

please review the first extracted patch from 8192978. This one is about correcting the jlong format string in src/jdk.jdi/share/native/libdt_shmem/shmemBase.c.

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8193183

WebRev: http://cr.openjdk.java.net/~clanger/webrevs/8193183.0/

 

I believe the libdt_shmem is only built on windows.

 

Thanks,

Christoph

 

 

Reply | Threaded
Open this post in threaded view
|

Re: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

Chris Plummer
In reply to this post by Alex Menkov-2
I had noticed that when I first looked at these changes, but figured
that JDI code cannot leverage globalDefinitions.hpp. However, I'm not
certain of that.

Chris

On 12/7/17 10:50 AM, Alex Menkov wrote:

> Looks like we already have definitions to workaround Visual Studio
> issues (including absence of PRId64) in
> open/src/hotspot/share/utilities/globalDefinitions_visCPP.hpp
> It would be better to reuse it (to have all workarounds in a single
> place)
>
> I suppose to utilize it you need
> #include "utilities/globalDefinitions.hpp"
> It includes corresponding "globalDefinitions_*.hpp" depending on the
> compiler used.
>
> --alex
>
> On 12/07/2017 07:21, Langer, Christoph wrote:
>> Hi,
>>
>> please review the first extracted patch from 8192978. This one is
>> about correcting the jlong format string in
>> src/jdk.jdi/share/native/libdt_shmem/shmemBase.c.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8193183
>>
>> WebRev: http://cr.openjdk.java.net/~clanger/webrevs/8193183.0/
>>
>> I believe the libdt_shmem is only built on windows.
>>
>> Thanks,
>>
>> Christoph
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

Chris Plummer
In reply to this post by Langer, Christoph
Hi Christoph,

You can do the push, but please let me do some internal testing first. I want to make sure all our platforms are covered, and our closed tests are turn.

thanks,

Chris

On 12/7/17 11:02 AM, Langer, Christoph wrote:

Thanks Chris and Serguei for reviewing.

 

I can do the push myself as it isn’t hotspot. I’ll send out other webrevs tomorrow for the other stuff.

 

Best regards

Christoph

 

From: Chris Plummer [[hidden email]]
Sent: Donnerstag, 7. Dezember 2017 19:14
To: Langer, Christoph [hidden email]; [hidden email]
Cc: Lindenmaier, Goetz [hidden email]
Subject: Re: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

 

Looks good to me. I can sponsor the push for you.

thanks,

Chris

On 12/7/17 7:21 AM, Langer, Christoph wrote:

Hi,

 

please review the first extracted patch from 8192978. This one is about correcting the jlong format string in src/jdk.jdi/share/native/libdt_shmem/shmemBase.c.

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8193183

WebRev: http://cr.openjdk.java.net/~clanger/webrevs/8193183.0/

 

I believe the libdt_shmem is only built on windows.

 

Thanks,

Christoph

 

 


Reply | Threaded
Open this post in threaded view
|

Re: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

Chris Plummer
Clicked send to fast. That should be "run", not "turn".

Chris

On 12/7/17 11:26 AM, Chris Plummer wrote:
Hi Christoph,

You can do the push, but please let me do some internal testing first. I want to make sure all our platforms are covered, and our closed tests are turn.

thanks,

Chris

On 12/7/17 11:02 AM, Langer, Christoph wrote:

Thanks Chris and Serguei for reviewing.

 

I can do the push myself as it isn’t hotspot. I’ll send out other webrevs tomorrow for the other stuff.

 

Best regards

Christoph

 

From: Chris Plummer [[hidden email]]
Sent: Donnerstag, 7. Dezember 2017 19:14
To: Langer, Christoph [hidden email]; [hidden email]
Cc: Lindenmaier, Goetz [hidden email]
Subject: Re: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

 

Looks good to me. I can sponsor the push for you.

thanks,

Chris

On 12/7/17 7:21 AM, Langer, Christoph wrote:

Hi,

 

please review the first extracted patch from 8192978. This one is about correcting the jlong format string in src/jdk.jdi/share/native/libdt_shmem/shmemBase.c.

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8193183

WebRev: http://cr.openjdk.java.net/~clanger/webrevs/8193183.0/

 

I believe the libdt_shmem is only built on windows.

 

Thanks,

Christoph

 

 



Reply | Threaded
Open this post in threaded view
|

Re: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

Alex Menkov-2
In reply to this post by Chris Plummer
well, maybe code duplication is lesser evil in the case :)

--alex

On 12/07/2017 11:25, Chris Plummer wrote:

> I had noticed that when I first looked at these changes, but figured
> that JDI code cannot leverage globalDefinitions.hpp. However, I'm not
> certain of that.
>
> Chris
>
> On 12/7/17 10:50 AM, Alex Menkov wrote:
>> Looks like we already have definitions to workaround Visual Studio
>> issues (including absence of PRId64) in
>> open/src/hotspot/share/utilities/globalDefinitions_visCPP.hpp
>> It would be better to reuse it (to have all workarounds in a single
>> place)
>>
>> I suppose to utilize it you need
>> #include "utilities/globalDefinitions.hpp"
>> It includes corresponding "globalDefinitions_*.hpp" depending on the
>> compiler used.
>>
>> --alex
>>
>> On 12/07/2017 07:21, Langer, Christoph wrote:
>>> Hi,
>>>
>>> please review the first extracted patch from 8192978. This one is
>>> about correcting the jlong format string in
>>> src/jdk.jdi/share/native/libdt_shmem/shmemBase.c.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8193183
>>>
>>> WebRev: http://cr.openjdk.java.net/~clanger/webrevs/8193183.0/
>>>
>>> I believe the libdt_shmem is only built on windows.
>>>
>>> Thanks,
>>>
>>> Christoph
>>>
>
Reply | Threaded
Open this post in threaded view
|

RE: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

Langer, Christoph
In reply to this post by Chris Plummer

Hi Chris,

 

ok, I understand. Then please let me know when you’re done or …in that case you might as well do the push.:)

 

Thanks

Christoph

 

From: Chris Plummer [mailto:[hidden email]]
Sent: Donnerstag, 7. Dezember 2017 20:26
To: Langer, Christoph <[hidden email]>; [hidden email]; [hidden email]
Cc: Lindenmaier, Goetz <[hidden email]>
Subject: Re: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

 

Hi Christoph,

You can do the push, but please let me do some internal testing first. I want to make sure all our platforms are covered, and our closed tests are turn.

thanks,

Chris

On 12/7/17 11:02 AM, Langer, Christoph wrote:

Thanks Chris and Serguei for reviewing.

 

I can do the push myself as it isn’t hotspot. I’ll send out other webrevs tomorrow for the other stuff.

 

Best regards

Christoph

 

From: Chris Plummer [[hidden email]]
Sent: Donnerstag, 7. Dezember 2017 19:14
To: Langer, Christoph [hidden email]; [hidden email]
Cc: Lindenmaier, Goetz [hidden email]
Subject: Re: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

 

Looks good to me. I can sponsor the push for you.

thanks,

Chris

On 12/7/17 7:21 AM, Langer, Christoph wrote:

Hi,

 

please review the first extracted patch from 8192978. This one is about correcting the jlong format string in src/jdk.jdi/share/native/libdt_shmem/shmemBase.c.

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8193183

WebRev: http://cr.openjdk.java.net/~clanger/webrevs/8193183.0/

 

I believe the libdt_shmem is only built on windows.

 

Thanks,

Christoph

 

 

 

Reply | Threaded
Open this post in threaded view
|

Re: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

David Holmes
In reply to this post by Langer, Christoph
On 8/12/2017 1:21 AM, Langer, Christoph wrote:

> Hi,
>
> please review the first extracted patch from 8192978. This one is about
> correcting the jlong format string in
> src/jdk.jdi/share/native/libdt_shmem/shmemBase.c.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8193183
>
> WebRev: http://cr.openjdk.java.net/~clanger/webrevs/8193183.0/
>
> I believe the libdt_shmem is only built on windows.

Okay ... in that case why do you need the

+ #if defined(_WIN32)

? As there is no definition of PRId64 for non-Windows any attempt to
build on non-Windows will fail. If you don't use the ifdef and simply
use %I64d then any attempt to build on non-windows will also fail.

Or we can continue the illusion this might be built on non-Windows and
add a definition that works in all cases - like hotspot's INT64_FORMAT.

Cheers,
David

> Thanks,
>
> Christoph
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

Chris Plummer
On 12/7/17 7:46 PM, David Holmes wrote:

> On 8/12/2017 1:21 AM, Langer, Christoph wrote:
>> Hi,
>>
>> please review the first extracted patch from 8192978. This one is
>> about correcting the jlong format string in
>> src/jdk.jdi/share/native/libdt_shmem/shmemBase.c.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8193183
>>
>> WebRev: http://cr.openjdk.java.net/~clanger/webrevs/8193183.0/
>>
>> I believe the libdt_shmem is only built on windows.
>
> Okay ... in that case why do you need the
>
> + #if defined(_WIN32)
>
> ? As there is no definition of PRId64 for non-Windows any attempt to
> build on non-Windows will fail. If you don't use the ifdef and simply
> use %I64d then any attempt to build on non-windows will also fail.
>
> Or we can continue the illusion this might be built on non-Windows and
> add a definition that works in all cases - like hotspot's INT64_FORMAT.
INT64_FORMAT uses PRId64.

Chris
>
> Cheers,
> David
>
>> Thanks,
>>
>> Christoph
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

David Holmes
On 8/12/2017 2:16 PM, Chris Plummer wrote:

> On 12/7/17 7:46 PM, David Holmes wrote:
>> On 8/12/2017 1:21 AM, Langer, Christoph wrote:
>>> Hi,
>>>
>>> please review the first extracted patch from 8192978. This one is
>>> about correcting the jlong format string in
>>> src/jdk.jdi/share/native/libdt_shmem/shmemBase.c.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8193183
>>>
>>> WebRev: http://cr.openjdk.java.net/~clanger/webrevs/8193183.0/
>>>
>>> I believe the libdt_shmem is only built on windows.
>>
>> Okay ... in that case why do you need the
>>
>> + #if defined(_WIN32)
>>
>> ? As there is no definition of PRId64 for non-Windows any attempt to
>> build on non-Windows will fail. If you don't use the ifdef and simply
>> use %I64d then any attempt to build on non-windows will also fail.
>>
>> Or we can continue the illusion this might be built on non-Windows and
>> add a definition that works in all cases - like hotspot's INT64_FORMAT.
> INT64_FORMAT uses PRId64.

Apologies - I hadn't realized that PRId64 is the standard name.

David

> Chris
>>
>> Cheers,
>> David
>>
>>> Thanks,
>>>
>>> Christoph
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

Chris Plummer
In reply to this post by Langer, Christoph
Hi Christoph,

Testing is done. All clear to push. I'll let you have the honors. :)

Chris

On 12/7/17 1:28 PM, Langer, Christoph wrote:

Hi Chris,

 

ok, I understand. Then please let me know when you’re done or …in that case you might as well do the push.:)

 

Thanks

Christoph

 

From: Chris Plummer [[hidden email]]
Sent: Donnerstag, 7. Dezember 2017 20:26
To: Langer, Christoph [hidden email]; [hidden email]; [hidden email]
Cc: Lindenmaier, Goetz [hidden email]
Subject: Re: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

 

Hi Christoph,

You can do the push, but please let me do some internal testing first. I want to make sure all our platforms are covered, and our closed tests are turn.

thanks,

Chris

On 12/7/17 11:02 AM, Langer, Christoph wrote:

Thanks Chris and Serguei for reviewing.

 

I can do the push myself as it isn’t hotspot. I’ll send out other webrevs tomorrow for the other stuff.

 

Best regards

Christoph

 

From: Chris Plummer [[hidden email]]
Sent: Donnerstag, 7. Dezember 2017 19:14
To: Langer, Christoph [hidden email]; [hidden email]
Cc: Lindenmaier, Goetz [hidden email]
Subject: Re: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

 

Looks good to me. I can sponsor the push for you.

thanks,

Chris

On 12/7/17 7:21 AM, Langer, Christoph wrote:

Hi,

 

please review the first extracted patch from 8192978. This one is about correcting the jlong format string in src/jdk.jdi/share/native/libdt_shmem/shmemBase.c.

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8193183

WebRev: http://cr.openjdk.java.net/~clanger/webrevs/8193183.0/

 

I believe the libdt_shmem is only built on windows.

 

Thanks,

Christoph

 

 

 


Reply | Threaded
Open this post in threaded view
|

RE: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

Langer, Christoph
In reply to this post by Alex Menkov-2
Hi Alex,

as far as I know, it's not possible to include hotspot headers in the JDK, so we have to leave it local.

Best regards
Christoph

-----Original Message-----
From: serviceability-dev [mailto:[hidden email]] On Behalf Of Alex Menkov
Sent: Donnerstag, 7. Dezember 2017 21:16
To: Chris Plummer <[hidden email]>; [hidden email]
Subject: Re: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

well, maybe code duplication is lesser evil in the case :)

--alex

On 12/07/2017 11:25, Chris Plummer wrote:

> I had noticed that when I first looked at these changes, but figured
> that JDI code cannot leverage globalDefinitions.hpp. However, I'm not
> certain of that.
>
> Chris
>
> On 12/7/17 10:50 AM, Alex Menkov wrote:
>> Looks like we already have definitions to workaround Visual Studio
>> issues (including absence of PRId64) in
>> open/src/hotspot/share/utilities/globalDefinitions_visCPP.hpp
>> It would be better to reuse it (to have all workarounds in a single
>> place)
>>
>> I suppose to utilize it you need
>> #include "utilities/globalDefinitions.hpp"
>> It includes corresponding "globalDefinitions_*.hpp" depending on the
>> compiler used.
>>
>> --alex
>>
>> On 12/07/2017 07:21, Langer, Christoph wrote:
>>> Hi,
>>>
>>> please review the first extracted patch from 8192978. This one is
>>> about correcting the jlong format string in
>>> src/jdk.jdi/share/native/libdt_shmem/shmemBase.c.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8193183
>>>
>>> WebRev: http://cr.openjdk.java.net/~clanger/webrevs/8193183.0/
>>>
>>> I believe the libdt_shmem is only built on windows.
>>>
>>> Thanks,
>>>
>>> Christoph
>>>
>
Reply | Threaded
Open this post in threaded view
|

RE: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

Langer, Christoph
In reply to this post by Chris Plummer

Done: http://hg.openjdk.java.net/jdk/jdk/rev/8ad12da0cbc7

 

Thanks

Christoph

 

From: Chris Plummer [mailto:[hidden email]]
Sent: Freitag, 8. Dezember 2017 06:48
To: Langer, Christoph <[hidden email]>; [hidden email]; [hidden email]
Cc: Lindenmaier, Goetz <[hidden email]>
Subject: Re: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

 

Hi Christoph,

Testing is done. All clear to push. I'll let you have the honors. :)

Chris

On 12/7/17 1:28 PM, Langer, Christoph wrote:

Hi Chris,

 

ok, I understand. Then please let me know when you’re done or …in that case you might as well do the push.:)

 

Thanks

Christoph

 

From: Chris Plummer [[hidden email]]
Sent: Donnerstag, 7. Dezember 2017 20:26
To: Langer, Christoph [hidden email]; [hidden email]; [hidden email]
Cc: Lindenmaier, Goetz [hidden email]
Subject: Re: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

 

Hi Christoph,

You can do the push, but please let me do some internal testing first. I want to make sure all our platforms are covered, and our closed tests are turn.

thanks,

Chris

On 12/7/17 11:02 AM, Langer, Christoph wrote:

Thanks Chris and Serguei for reviewing.

 

I can do the push myself as it isn’t hotspot. I’ll send out other webrevs tomorrow for the other stuff.

 

Best regards

Christoph

 

From: Chris Plummer [[hidden email]]
Sent: Donnerstag, 7. Dezember 2017 19:14
To: Langer, Christoph [hidden email]; [hidden email]
Cc: Lindenmaier, Goetz [hidden email]
Subject: Re: RFR (XS): 8193183: Fix format string in libdt_shmem/shmemBase.c

 

Looks good to me. I can sponsor the push for you.

thanks,

Chris

On 12/7/17 7:21 AM, Langer, Christoph wrote:

Hi,

 

please review the first extracted patch from 8192978. This one is about correcting the jlong format string in src/jdk.jdi/share/native/libdt_shmem/shmemBase.c.

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8193183

WebRev: http://cr.openjdk.java.net/~clanger/webrevs/8193183.0/

 

I believe the libdt_shmem is only built on windows.

 

Thanks,

Christoph