Re: RFR (XS): 8141134: Remove unnecessary pragma warning(disable:4355) from GC code

[Thomas Schatzl]

Hi,

On Wed, 2015-11-04 at 12:12 +0100, Volker Simonis wrote: 
> Hi Erik,
> 
> thanks a lot for your understanding :)
> 
> Your suggestion is perfectly fine for me and we can live with both
> versions of the fix.

  I went for the change in the globalDefinitions* file.

New webrevs:
http://cr.openjdk.java.net/~tschatzl/8141134/webrev.1/
http://cr.openjdk.java.net/~tschatzl/8141134/webrev.0_to_1/

It built fine on the official MS compiler, but I did not check on VS
2010.

Thanks,
  Thomas

Re: RFR (XS): 8141134: Remove unnecessary pragma warning(disable:4355) from GC code

[Volker Simonis]

Hi Thomas,

the change looks good. Thomas St. is just checking the VS2010 build.
We'll let you new the results once it's finished.

Thank you and best regards,
Volker


On Wed, Nov 4, 2015 at 4:04 PM, Erik Helin  wrote:
> On 2015-11-04, Thomas Schatzl wrote:
>> Hi,
>>
>> On Wed, 2015-11-04 at 12:12 +0100, Volker Simonis wrote:
>> > Hi Erik,
>> >
>> > thanks a lot for your understanding :)
>> >
>> > Your suggestion is perfectly fine for me and we can live with both
>> > versions of the fix.
>>
>>   I went for the change in the globalDefinitions* file.
>>
>> New webrevs:
>> http://cr.openjdk.java.net/~tschatzl/8141134/webrev.1/
>> http://cr.openjdk.java.net/~tschatzl/8141134/webrev.0_to_1/
>
> Looks good to me, Reviewed.
>
>> It built fine on the official MS compiler, but I did not check on VS
>> 2010.
>
> Volker, can you check if this compiles on VS 2010?
>
> Thanks,
> Erik
>
>> Thanks,
>>   Thomas
>>
>>

Re: RFR (XS): 8141134: Remove unnecessary pragma warning(disable:4355) from GC code

[Magnus Ihse Bursie]

On 2015-11-04 11:46, Erik Helin wrote:


Maybe we can come up with something that removes these #pragmas from the
GC code while still allowing you to compile with VS2010. What do you
think about the following suggestions:
- We change the Makefiles to add the flag /Wd4355 if we use VS2010 or
   older. This will results in the warning being disabled for all files
   (one could potentially also disable the warning per file).
- We add the code:
   #if _MSC_VER < 1000
 #pragma warning( disable:4355 )
   #endif // _MSC_VER < 1800
   to globalDefinitions_visCPP.hpp. This file gets included in
   globalDefinitions.hpp which in turn gets included basically
   everywhere. This will turn off the warning for essentially all files.

I'm leaning more towards the Makefile solution since that is where it
makes most sense to me to disable warnings. OTOH,
globalDefinitions_visCPP.hpp already disables quite a few warnings...

build-dev, I don't know if you guys have any other suggestion?
Normally, I'd prefer to keep warning handling as compiler flags, but in 
this case I think Volker's suggestion makes most sense.


/Magnus

Re: RFR (XS): 8141134: Remove unnecessary pragma warning(disable:4355) from GC code

[Thomas Stüfe]

Hi Eric,

On Wed, Nov 4, 2015 at 11:46 AM, Erik Helin  wrote:

> (adding build-dev as well)
>
> On 2015-11-03, Volker Simonis wrote:
> > Hi Erik,
> >
> > while I agree that this is a nice clean up I want to second Thomas in
> > his attempt to save the VS 2010 build. I understand that the official
> > Oracle build tool chain for jdk9 on Windows is VS 2013, but this may
> > be different for others. I also think that we shouldn't sacrifice the
> > possibility to use  VS 2010 without any hard requirements like for
> > example the need to use new language features which are not supported
> > in the old version.
>
> Hi Volker,
>
> I understand that you guys want to keep building VS2010 but I know that
> you also understand that we want to get rid of this cruft from the GC
> code.
>
> > If you and others still insist in doing this change, I'd strongly vote
> > to first completely disable the support for VS 2010 in the 'configure'
> > process before we start to do source code changes.
> >
> > Instead, I'd prefer if you could just change the code from:
> >
> > #ifdef _MSC_VER
> >
> > to:
> >
> > #if _MSC_VER < 1800
> >
> > like it was already done for
> > https://bugs.openjdk.java.net/browse/JDK-8137329. This makes the
> > dependency on a specific compiler version explicit (although it would
> > hardly count as a "clean up" any more :)
>
> Maybe we can come up with something that removes these #pragmas from the
> GC code while still allowing you to compile with VS2010. What do you
> think about the following suggestions:
> - We change the Makefiles to add the flag /Wd4355 if we use VS2010 or
>   older. This will results in the warning being disabled for all files
>   (one could potentially also disable the warning per file).
> - We add the code:
>   #if _MSC_VER < 1000
> #pragma warning( disable:4355 )
>   #endif // _MSC_VER < 1800
>   to globalDefinitions_visCPP.hpp. This file gets included in
>   globalDefinitions.hpp which in turn gets included basically
>   everywhere. This will turn off the warning for essentially all files.
>
> I'm leaning more towards the Makefile solution since that is where it
> makes most sense to me to disable warnings. OTOH,
> globalDefinitions_visCPP.hpp already disables quite a few warnings...
>
>
Thanks!

Both solutions are fine, I would prefer adding them to
globalDefinitions_visCPP.hpp,
which already contains a number of other workarounds for older MSVC
versions.

Kind Regards, Thomas



> build-dev, I don't know if you guys have any other suggestion?
>
> Thanks,
> Erik
>
> > Regards,
> > Volker
> >
> >
> >
> >
> > On Tue, Nov 3, 2015 at 3:32 PM, Thomas Stüfe 
> wrote:
> > > Hi,
> > >
> > > one question, does this mean this would not build anymore with Visual
> Studio
> > > 2010?
> > >
> > > According to :
> > > https://wiki.openjdk.java.net/display/Build/Supported+Build+Platforms
> ,
> > > VS2010 is still supported build platform.
> > >
> > > Also there seems to be enough interest in the community about keeping
> the
> > > VS2010 build alive, see
> > >
> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2015-October/015978.html
> > >
> > > We (SAP) unfortunately still use Visual Studio 2010 for our VM, it
> would be
> > > nice if that would continue to work.
> > >
> > > Best Regards, Thomas
> > >
> > >
> > >
> > > On Mon, Nov 2, 2015 at 5:17 PM, Erik Helin 
> wrote:
> > >>
> > >> On 2015-11-02, Thomas Schatzl wrote:
> > >> > Hi all,
> > >> >
> > >> >   Erik H. made me aware of another pragma 4355 in nmethod.cpp that
> can
> > >> > be removed.
> > >> >
> > >> > I updated the webrev in place because of the kind of change.
> > >>
> > >> Looks good, Reviewed.
> > >>
> > >> > Thanks,
> > >> >   Thomas
> > >> >
> > >> > On Mon, 2015-11-02 at 16:14 +0100, Thomas Schatzl wrote:
> > >> > > Hi all,
> > >> > >
> > >> > >   can I have reviews for the following change that removes some
> > >> > > pragmas
> > >> > > in GC code?
> > >> > > With the move to newer MSVC, they are not needed any more.
> > >> > >
> > >> > > CR:
> > >> > > https://bugs.openjdk.java.net/browse/JDK-8141134
> > >> > > Webrev:
> > >> > > http://cr.openjdk.java.net/~tschatzl/8141134/webrev/
> > >> > > Testing:
> > >> > > jprt
> > >> > >
> > >> > > Thanks,
> > >> > >   Thomas
> > >> > >
> > >> > >
> > >> >
> > >> >
> > >
> > >
>

Re: RFR (XS): 8141134: Remove unnecessary pragma warning(disable:4355) from GC code

[Thomas Stüfe]

Hi Eric,

your change builds fine on VS 2010.

Thank you!

Thomas

On Wed, Nov 4, 2015 at 4:04 PM, Erik Helin  wrote:

> On 2015-11-04, Thomas Schatzl wrote:
> > Hi,
> >
> > On Wed, 2015-11-04 at 12:12 +0100, Volker Simonis wrote:
> > > Hi Erik,
> > >
> > > thanks a lot for your understanding :)
> > >
> > > Your suggestion is perfectly fine for me and we can live with both
> > > versions of the fix.
> >
> >   I went for the change in the globalDefinitions* file.
> >
> > New webrevs:
> > http://cr.openjdk.java.net/~tschatzl/8141134/webrev.1/
> > http://cr.openjdk.java.net/~tschatzl/8141134/webrev.0_to_1/
>
> Looks good to me, Reviewed.
>
> > It built fine on the official MS compiler, but I did not check on VS
> > 2010.
>
> Volker, can you check if this compiles on VS 2010?
>
> Thanks,
> Erik
>
> > Thanks,
> >   Thomas
> >
> >
>

Re: RFR (XS): 8141134: Remove unnecessary pragma warning(disable:4355) from GC code

[Erik Helin]

(adding build-dev as well)

On 2015-11-03, Volker Simonis wrote:
> Hi Erik,
> 
> while I agree that this is a nice clean up I want to second Thomas in
> his attempt to save the VS 2010 build. I understand that the official
> Oracle build tool chain for jdk9 on Windows is VS 2013, but this may
> be different for others. I also think that we shouldn't sacrifice the
> possibility to use  VS 2010 without any hard requirements like for
> example the need to use new language features which are not supported
> in the old version.

Hi Volker,

I understand that you guys want to keep building VS2010 but I know that
you also understand that we want to get rid of this cruft from the GC
code.

> If you and others still insist in doing this change, I'd strongly vote
> to first completely disable the support for VS 2010 in the 'configure'
> process before we start to do source code changes.
> 
> Instead, I'd prefer if you could just change the code from:
> 
> #ifdef _MSC_VER
> 
> to:
> 
> #if _MSC_VER < 1800
> 
> like it was already done for
> https://bugs.openjdk.java.net/browse/JDK-8137329. This makes the
> dependency on a specific compiler version explicit (although it would
> hardly count as a "clean up" any more :)

Maybe we can come up with something that removes these #pragmas from the
GC code while still allowing you to compile with VS2010. What do you
think about the following suggestions:
- We change the Makefiles to add the flag /Wd4355 if we use VS2010 or
  older. This will results in the warning being disabled for all files
  (one could potentially also disable the warning per file).
- We add the code:
  #if _MSC_VER < 1000
#pragma warning( disable:4355 )
  #endif // _MSC_VER < 1800
  to globalDefinitions_visCPP.hpp. This file gets included in
  globalDefinitions.hpp which in turn gets included basically
  everywhere. This will turn off the warning for essentially all files.

I'm leaning more towards the Makefile solution since that is where it
makes most sense to me to disable warnings. OTOH,
globalDefinitions_visCPP.hpp already disables quite a few warnings...

build-dev, I don't know if you guys have any other suggestion?

Thanks,
Erik

> Regards,
> Volker
> 
> 
> 
> 
> On Tue, Nov 3, 2015 at 3:32 PM, Thomas Stüfe  wrote:
> > Hi,
> >
> > one question, does this mean this would not build anymore with Visual Studio
> > 2010?
> >
> > According to :
> > https://wiki.openjdk.java.net/display/Build/Supported+Build+Platforms ,
> > VS2010 is still supported build platform.
> >
> > Also there seems to be enough interest in the community about keeping the
> > VS2010 build alive, see
> > http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2015-October/015978.html
> >
> > We (SAP) unfortunately still use Visual Studio 2010 for our VM, it would be
> > nice if that would continue to work.
> >
> > Best Regards, Thomas
> >
> >
> >
> > On Mon, Nov 2, 2015 at 5:17 PM, Erik Helin  wrote:
> >>
> >> On 2015-11-02, Thomas Schatzl wrote:
> >> > Hi all,
> >> >
> >> >   Erik H. made me aware of another pragma 4355 in nmethod.cpp that can
> >> > be removed.
> >> >
> >> > I updated the webrev in place because of the kind of change.
> >>
> >> Looks good, Reviewed.
> >>
> >> > Thanks,
> >> >   Thomas
> >> >
> >> > On Mon, 2015-11-02 at 16:14 +0100, Thomas Schatzl wrote:
> >> > > Hi all,
> >> > >
> >> > >   can I have reviews for the following change that removes some
> >> > > pragmas
> >> > > in GC code?
> >> > > With the move to newer MSVC, they are not needed any more.
> >> > >
> >> > > CR:
> >> > > https://bugs.openjdk.java.net/browse/JDK-8141134
> >> > > Webrev:
> >> > > http://cr.openjdk.java.net/~tschatzl/8141134/webrev/
> >> > > Testing:
> >> > > jprt
> >> > >
> >> > > Thanks,
> >> > >   Thomas
> >> > >
> >> > >
> >> >
> >> >
> >
> >

Re: RFR (XS): 8141134: Remove unnecessary pragma warning(disable:4355) from GC code

[Volker Simonis]

On Wed, Nov 4, 2015 at 11:46 AM, Erik Helin  wrote:
> (adding build-dev as well)
>
> On 2015-11-03, Volker Simonis wrote:
>> Hi Erik,
>>
>> while I agree that this is a nice clean up I want to second Thomas in
>> his attempt to save the VS 2010 build. I understand that the official
>> Oracle build tool chain for jdk9 on Windows is VS 2013, but this may
>> be different for others. I also think that we shouldn't sacrifice the
>> possibility to use  VS 2010 without any hard requirements like for
>> example the need to use new language features which are not supported
>> in the old version.
>
> Hi Volker,
>
> I understand that you guys want to keep building VS2010 but I know that
> you also understand that we want to get rid of this cruft from the GC
> code.
>
>> If you and others still insist in doing this change, I'd strongly vote
>> to first completely disable the support for VS 2010 in the 'configure'
>> process before we start to do source code changes.
>>
>> Instead, I'd prefer if you could just change the code from:
>>
>> #ifdef _MSC_VER
>>
>> to:
>>
>> #if _MSC_VER < 1800
>>
>> like it was already done for
>> https://bugs.openjdk.java.net/browse/JDK-8137329. This makes the
>> dependency on a specific compiler version explicit (although it would
>> hardly count as a "clean up" any more :)
>
> Maybe we can come up with something that removes these #pragmas from the
> GC code while still allowing you to compile with VS2010. What do you
> think about the following suggestions:
> - We change the Makefiles to add the flag /Wd4355 if we use VS2010 or
>   older. This will results in the warning being disabled for all files
>   (one could potentially also disable the warning per file).
> - We add the code:
>   #if _MSC_VER < 1000
> #pragma warning( disable:4355 )
>   #endif // _MSC_VER < 1800
>   to globalDefinitions_visCPP.hpp. This file gets included in
>   globalDefinitions.hpp which in turn gets included basically
>   everywhere. This will turn off the warning for essentially all files.
>
> I'm leaning more towards the Makefile solution since that is where it
> makes most sense to me to disable warnings. OTOH,
> globalDefinitions_visCPP.hpp already disables quite a few warnings...
>
> build-dev, I don't know if you guys have any other suggestion?
>

Hi Erik,

thanks a lot for your understanding :)

Your suggestion is perfectly fine for me and we can live with both
versions of the fix.
But as we already have:

#if _MSC_VER < 1800
#define strtoull _strtoui64
#endif

in globalDefinitions_visCPP.hpp it's probably the simplest solutions
to just add:

#if _MSC_VER < 1800
#define strtoull _strtoui64
#pragma warning( disable:4355 )
#endif

But in the end I leave it up to you.

Thank you and best regards,
Volker


> Thanks,
> Erik
>
>> Regards,
>> Volker
>>
>>
>>
>>
>> On Tue, Nov 3, 2015 at 3:32 PM, Thomas Stüfe  wrote:
>> > Hi,
>> >
>> > one question, does this mean this would not build anymore with Visual 
>> > Studio
>> > 2010?
>> >
>> > According to :
>> > https://wiki.openjdk.java.net/display/Build/Supported+Build+Platforms ,
>> > VS2010 is still supported build platform.
>> >
>> > Also there seems to be enough interest in the community about keeping the
>> > VS2010 build alive, see
>> > http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2015-October/015978.html
>> >
>> > We (SAP) unfortunately still use Visual Studio 2010 for our VM, it would be
>> > nice if that would continue to work.
>> >
>> > Best Regards, Thomas
>> >
>> >
>> >
>> > On Mon, Nov 2, 2015 at 5:17 PM, Erik Helin  wrote:
>> >>
>> >> On 2015-11-02, Thomas Schatzl wrote:
>> >> > Hi all,
>> >> >
>> >> >   Erik H. made me aware of another pragma 4355 in nmethod.cpp that can
>> >> > be removed.
>> >> >
>> >> > I updated the webrev in place because of the kind of change.
>> >>
>> >> Looks good, Reviewed.
>> >>
>> >> > Thanks,
>> >> >   Thomas
>> >> >
>> >> > On Mon, 2015-11-02 at 16:14 +0100, Thomas Schatzl wrote:
>> >> > > Hi all,
>> >> > >
>> >> > >   can I have reviews for the following change that removes some
>> >> > > pragmas
>> >> > > in GC code?
>> >> > > With the move to newer MSVC, they are not needed any more.
>> >> > >
>> >> > > CR:
>> >> > > https://bugs.openjdk.java.net/browse/JDK-8141134
>> >> > > Webrev:
>> >> > > http://cr.openjdk.java.net/~tschatzl/8141134/webrev/
>> >> > > Testing:
>> >> > > jprt
>> >> > >
>> >> > > Thanks,
>> >> > >   Thomas
>> >> > >
>> >> > >
>> >> >
>> >> >
>> >
>> >