Re: RFR 8210226: Add support for multiple project folders to idea.sh

[Magnus Ihse Bursie]

On 2018-08-30 17:12, Maurizio Cimadamore wrote:

Hi,
this patch adds proper support for -o option to the idea.sh script, 
which allows to place the .idea folder under any given output folder 
(not necessarily the JDK root).


To be able to do this, I had to revampo the logic for template 
substitution in idea.sh, as it was growing too brittle. I now have a 
much more declarative logic, where replacements can be added by 
calling the add_replacement function. This allows to replace a lot of 
the previous code with very simple and self-explanatory calls to that 
routine.


This piece was necessary, as we needed to replace a lot of references 
to the idea variable $PROJECT_DIR with the new template variable 
###ROOT_DIR### (otherwise the project is ill-formed, since 
$PROJECT_DIR merely points at the folder containing the .idea project 
folder).


Webrev:

http://cr.openjdk.java.net/~mcimadamore/8210226/


Looks good to me, with the caveat that I do not fully understand the 
idea project generator.


/Magnus

Which build platforms are supported for jdk11 and upcoming jdk12?

[Lindenmaier, Goetz]

Hi,

https://wiki.openjdk.java.net/display/Build/Supported+Build+Platforms
is quite outdated, listing only jdk9.
Is there a different site now listing this information?

(I'll also update our information on this site.)

Because of https://bugs.openjdk.java.net/browse/JDK-8210194
we think about updating our compilers used for jdk12.

Best regards,
  Goetz.

Re: 8209520: Build fails when native code coverage is enabled

[Magnus Ihse Bursie]

There are two aspects here that sound similar, but is not:

Erik says:

On Aug 30, 2018, at 6:26 AM, Erik Joelsson  wrote:

I shared your opinion at first while discussing this offline with Leonid. What 
changed my mind was the claim that the warnings cannot be truly trusted when 
GCC is generating code coverage data. If that is true, then having warnings as 
errors turned on then serves no purpose. The majority of builds will be without 
code coverage enabled, so we will still get ample protection against warnings 
in the code.


and Igor says:

Hi,

On 2018-08-30 20:36, Igor Ignatyev wrote:

I ain't saying that Leonid's fixes are wrong, I definitely support his changes 
in macroAssembler_x86 and genCollectedHeap (can't comment on splashscreen_png.c 
as I don't really understand what gcc complains there), I'm saying that b/c 
coverage-enabled builds aren't built often enough and warnings from these 
builds will always be low-priority bugs, I believe that such instrumented must 
be produced w/ warnings-as-errors disabled.



These are two different arguments for turning off warnings for code 
coverage:

1) gcc is producing incorrect warnings
2) the warnings might be correct, but we are going to treat such bugs as 
low priority



I understand and accept 1, but I do not accept 2 as a valid reason for 
turning off warnings. I tried to dig through history in the 
(Oracle-internal) bug Igor posted, but in the end I could find no 
changes was made to any compiler flags..?


I have googled around but found no discussions at all that indicate that 
gcov should emit invalid warnings.


As for argument 2: we're using code coverage as a way to increase code 
quality, after all. If we get additional warnings, that indicate ways to 
improve the code, then we should use this to improve the code, not hide 
it away. And there's no reason not to treat such bugs as severe as any 
other compile errors.


Let's get more concrete: In this case, we have three warnings. To solve 
this, Leonid fixed:


* In macroAssembler_x86.cpp, he added ShouldNotReachHere(), which 
definitely improves code quality.


* In genCollectedHeap.cpp, he he initalized two local variables to NULL. 
After studying the code, I see that this is strictly not needed, and gcc 
is apparently losing some information here that could have been used to 
determine that the code is correct. However, my initial reaction when 
reading the code was that it was indeed broken, and it took me some time 
to prove that it was not. This is not good code readability. It's 
fragile, and some future code change might end up with a non-initialized 
local variable. So I think this is good, defensive programming, no 
matter what.


* In splashscreen_png.c, I'll honestly say that I don't know wether this 
fix is correct or not. But it does seem reasonable. The 
SplashDecodePng() function uses setjmp() *shudder* and for me, that's a 
big Here Be Dragons! sign. I also note that the success variable is read 
in the done: label, just below access to row_pointers and image_data -- 
which are both, already, declared volatile. So I'm guessing that 
declaring success volatile is in fact the correct thing to do. (I 
challenge anyone with a greater understanding of setjmp to prove me 
wrong...)


So I say we should be happy we got those warnings, and fix the code.

I don't see this as an argument that gcc emits invalid warnings. If that 
happens in the future, then we can discuss eliminating those warning. 
But if we do, we should eliminate them selectively, only the incorrect 
warnings and only on the files/libraries where they occur erroneously. 
Not disable all warnings in a single stroke.


/Magnus

Re: [OpenJDK 2D-Dev] RFR JDK-8209786: gcc 7.3 compiler errors on zLinux

[Magnus Ihse Bursie]

On 2018-08-31 01:28, Philip Race wrote:
Some day, I'd like to replace a lot of medialib functionality with 
something
like the proposed Vector API. But that is far enough away that 
medialib needs
to be maintained, and unlike a previous discussion about a similar 
issue in

the JPEG library, we are on the hook for maintaining medialib.
So if there is an actual logic error in medialib, I'd prefer to fix it.
If the issue is a false positive, I'd be OK to disable the warning, 
but wrapped

in a platform-specific manner. So is it a real error here ?


Gcc is emitting a warning due to shifting of negative values, which is 
deemed undefined behavior by the spec.


Is this a real error? Well. Undefined behavior is no good. It can work 
for now and then suddenly start breaking.


The originally suggested code change 
(http://cr.openjdk.java.net/~aleonard/8209786/webrev.00) seems 
reasonable to me. It is at the very least much clearer what the 
intention is. And it's conformant with the spec.


So I'd advocate using that fix, if you want to continue supporting the 
library.


/Magnus






-phil.

On 8/30/18, 3:37 PM, Brian Burkhalter wrote:

Hi Andrew,

As noted in the issue comments, the fdlibm C code is obsolescent and 
eventually to be superseded by equivalent Java implementations. As 
for the mediaLib code being moribund I cannot speak but am copying 
2d-dev which owns java.desktop.


Thanks,

Brian

On Aug 30, 2018, at 6:03 AM, Andrew Leonard 
mailto:andrew_m_leon...@uk.ibm.com>> 
wrote:



Thanks Magnus,
Yes, these libraries are moribound it seems, hence their preference 
for compile options..

Cheers
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com 






From: Magnus Ihse Bursie >
To: Andrew Leonard >, Brian Burkhalter 
mailto:brian.burkhal...@oracle.com>>
Cc: core-libs-...@openjdk.java.net 
, build-dev 
mailto:build-dev@openjdk.java.net>>

Date: 30/08/2018 13:49
Subject: Re: RFR JDK-8209786: gcc 7.3 compiler errors on zLinux
 





Andrew,

If you want to make changes to the build system (files in make/*),
please always include build-dev in the reviews.

From a build point, this fix looks okay. My general preference is to
fix shady code instead of disabling warnings, but in this case it's in
two libraries that are either external or moribound, so if the
maintainer's of the respective libraries want to avoid code changes, I
accept that.

/Magnus


On 2018-08-30 14:18, Andrew Leonard wrote:
> Hi Brian,
> Thanks for taking a look at this, I have just done a rebuild with 
a new

> patch with appropriate gcc disable warnings for these libraries:
> http://cr.openjdk.java.net/~aleonard/8209786/webrev.01/ 

> This works fine, so if you think this is a more favourable 
approach for

> these libraries? i'd like to get this merged please.
> Thanks
> Andrew
>
> Andrew Leonard
> Java Runtimes Development
> IBM Hursley
> IBM United Kingdom Ltd
> Phone internal: 245913, external: 01962 815913
> internet email: andrew_m_leon...@uk.ibm.com 


>
>
>
>
> From:   Brian Burkhalter >
> To: Andrew Leonard >
> Cc: core-libs-...@openjdk.java.net 


> Date:   28/08/2018 15:52
> Subject:    Re: RFR JDK-8209786: gcc 7.3 compiler errors on 
zLinux

>
>
>
> Hi Andrew,
>
> It was suggested that it would be preferable to dial down the 
compilation
> settings for the fdlibm code rather than make a source code 
change. Was

> this investigated?
>
> Thanks,
>
> Brian
>
> On Aug 28, 2018, at 7:18 AM, Andrew Leonard 
mailto:andrew_m_leon...@uk.ibm.com>>

> wrote:
>
> We have discovered issues with gcc 7.3 on zLinux, combined with 
OpenJDK's
> default compiler options has highlighted a couple of native code 
issues,

> with undefined behaviours:
>   - validating loop test array bounds
>   - left shifts of negative values
> I have created bug https://bugs.openjdk.java.net/browse/JDK-8209786
> and attached the webrev fix here:
> http://cr.openjdk.java.net/~aleonard/8209786/webrev.00/ 


>
> This has already been discussed and refined on the "s390x-port-dev"
> maillist
> and as it was pointed out, it should have been posted here...
>
> I'd like to request a sponsor for this fix please?
>
>
>
>
> Unless stated otherwise above:
> IBM United Kingdom Limited - Registered in England and Wales with 
number

> 741598.
> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire 
PO6 3AU







Unless stated otherwise above:
IBM United 

Re: [patch] add zero support for x86_64-linux-gnux32 target

[Magnus Ihse Bursie]

The necroposter strikes back! :-)

I'm currently trying to fix or close all long standing bugs on 
infrastructure/build, and now the time has come to JDK-8165440.


This patch had a bit of bad timing when it was posted, since it could 
not be accepted into mainline due to feature freeze, and there were no 
other repo to accept it.


I adjusted the patch to the current code base (which means that most 
parts of it were not needed). What remains are two files. However, I 
can't test if this works. Matthias, can you verify that this is a 
working patch for jdk/jdk for the gnux32 target? If so, I'll sponsor 
this patch.


diff --git a/make/autoconf/platform.m4 b/make/autoconf/platform.m4
--- a/make/autoconf/platform.m4
+++ b/make/autoconf/platform.m4
@@ -35,6 +35,10 @@
   VAR_CPU_ARCH=x86
   VAR_CPU_BITS=64
   VAR_CPU_ENDIAN=little
+  case "$host" in *x32)
+    VAR_CPU=x32
+    VAR_CPU_BITS=32
+  esac
   ;;
 i?86)
   VAR_CPU=x86
@@ -455,6 +459,8 @@
 HOTSPOT_$1_CPU_DEFINE=IA32
   elif test "x$OPENJDK_$1_CPU" = xx86_64; then
 HOTSPOT_$1_CPU_DEFINE=AMD64
+  elif test "x$OPENJDK_$1_CPU" = xx32; then
+    HOTSPOT_$1_CPU_DEFINE=X32
   elif test "x$OPENJDK_$1_CPU" = xsparcv9; then
 HOTSPOT_$1_CPU_DEFINE=SPARC
   elif test "x$OPENJDK_$1_CPU" = xaarch64; then
diff --git a/src/hotspot/os/linux/os_linux.cpp 
b/src/hotspot/os/linux/os_linux.cpp

--- a/src/hotspot/os/linux/os_linux.cpp
+++ b/src/hotspot/os/linux/os_linux.cpp
@@ -1742,7 +1742,7 @@

 #if  (defined IA32)
   static  Elf32_Half running_arch_code=EM_386;
-#elif   (defined AMD64)
+#elif   (defined AMD64) || defined(X32)
   static  Elf32_Half running_arch_code=EM_X86_64;
 #elif  (defined IA64)
   static  Elf32_Half running_arch_code=EM_IA_64;

/Magnus

On 2016-09-06 01:01, David Holmes wrote:

Hi Severin, Matthias,

On 5/09/2016 10:16 PM, Severin Gehwolf wrote:

On Mon, 2016-09-05 at 14:03 +0200, Matthias Klose wrote:

The attached patch adds support for building zero for the x86_64-
linux-gnux32
target, having changes in the build system, hotspot and jdk.

 - the build system currently only derives the target from
   the cpu in PLATFORM_EXTRACT_VARS_FROM_CPU; that is not enough
   for the new target, which only differs by the ending of the
   triplet. However the $host macro should be available anywhere.

 - the hotspot part just handles the new "cpu"

 - GensrcX11Wrappers.gmk assumes that there is a black/white
   decision about -m32/-m64. The patch works around it. However
   the real patch should be to get these flags from the build
   system, and not hardcode itself.

 - the sysctl system call is unsupported in the x32 kernel, and
   just the include leads to a build error.  From my point of view
   the header is not needed. I had successful builds on all other
   targets without including it. If you want to keep the include,
   then it should be guarded with
   #if !(defined(_ILP32) && defined(__x86_64__))

Matthias


I've filed this bug for this:
https://bugs.openjdk.java.net/browse/JDK-8165440


Please note that as a P4 issue this can not be fixed given we have hit 
RDP1:


http://mail.openjdk.java.net/pipermail/jdk9-dev/2016-August/004777.html

Further this is filed as "bug" but seems to clearly be an enhancement, 
so you would need approval for it to come in post-Feature-Complete.


Please consider if this is something that must be fixed for 9 or can 
be deferred. Otherwise you will need to follow additional approval 
processes.


Sorry.

David (just the messenger!)


Unfortunately, I have no way of testing it.

Cheers,
Severin

Re: [patch] add zero support for x86_64-linux-gnux32 target

[John Paul Adrian Glaubitz]

On 08/31/2018 10:43 AM, Magnus Ihse Bursie wrote:
> I adjusted the patch to the current code base (which means that most parts of 
> it were not needed). What remains are two files. However, I can't test if 
> this works. Matthias, can you verify that this is a working patch for jdk/jdk 
> for the gnux32 target? If so, I'll sponsor this patch.
I can test it. Didn't know that Matthias posted a patch for x32
support. The current patch that the Debian openjdk-11 package
is using at the moment can be found in [1].

Adrian

> [1] 
> https://sources.debian.org/src/openjdk-11/11%7E27-1/debian/patches/zero-x32.diff/

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [patch] add zero support for x86_64-linux-gnux32 target

[John Paul Adrian Glaubitz]

On 08/31/2018 10:57 AM, John Paul Adrian Glaubitz wrote:
> On 08/31/2018 10:43 AM, Magnus Ihse Bursie wrote:
>> I adjusted the patch to the current code base (which means that most parts 
>> of it were not needed). What remains are two files. However, I can't test if 
>> this works. Matthias, can you verify that this is a working patch for 
>> jdk/jdk for the gnux32 target? If so, I'll sponsor this patch.
> I can test it. Didn't know that Matthias posted a patch for x32
> support. The current patch that the Debian openjdk-11 package
> is using at the moment can be found in [1].

Oh, and here's the build log of openjdk-11 in Debian with that
patch [1].

Adrian

> [1] 
> https://buildd.debian.org/status/fetch.php?pkg=openjdk-11&arch=x32&ver=11%7E27-1&stamp=1534788109&raw=0

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [OpenJDK 2D-Dev] RFR JDK-8209786: gcc 7.3 compiler errors on zLinux

[Andrew Leonard]

Hi,
So there seems to be varying opinion here, taking the 2D view point since 
it is going to be maintained, the opinion seems to be more with the fix (
http://cr.openjdk.java.net/~aleonard/8209786/webrev.00/). This would be my 
personal preference also, but previous comments seemed to prefer compiler 
options.

Looking at each error:
- /libfdlibm/k_rem_pio2.c : This is a simple "for" loop bound check, which 
with good programming practice should really have been there to prevent 
ArrayOutOfBounds.. or native overwrites.. Simple fix.
- libmlib_image/mlib_ImageLookUp_Bit.c : This is -ve bit shifting which is 
spec undefined, which in theory could mean it breaks in the future if we 
upgrade/change compiler... However, this is complex code, we need to be 
very sure the new fix is "correct", myself and my colleague here have 
examined it closely and are happy, but i'd appreciate your in-depth 
analysis please. 

Magnus, Philip, Brian, Goetz, can we have a vote? => "Fix" or 
"DisableWarnings" ?

Thanks
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com 




From:   Magnus Ihse Bursie 
To: Philip Race , Brian Burkhalter 

Cc: 2d-dev <2d-...@openjdk.java.net>, build-dev 
, Andrew Leonard 
, core-libs-dev 

Date:   31/08/2018 09:27
Subject:Re: [OpenJDK 2D-Dev] RFR JDK-8209786: gcc 7.3 compiler 
errors on zLinux



On 2018-08-31 01:28, Philip Race wrote:
> Some day, I'd like to replace a lot of medialib functionality with 
> something
> like the proposed Vector API. But that is far enough away that 
> medialib needs
> to be maintained, and unlike a previous discussion about a similar 
> issue in
> the JPEG library, we are on the hook for maintaining medialib.
> So if there is an actual logic error in medialib, I'd prefer to fix it.
> If the issue is a false positive, I'd be OK to disable the warning, 
> but wrapped
> in a platform-specific manner. So is it a real error here ?

Gcc is emitting a warning due to shifting of negative values, which is 
deemed undefined behavior by the spec.

Is this a real error? Well. Undefined behavior is no good. It can work 
for now and then suddenly start breaking.

The originally suggested code change 
(
http://cr.openjdk.java.net/~aleonard/8209786/webrev.00
) seems 
reasonable to me. It is at the very least much clearer what the 
intention is. And it's conformant with the spec.

So I'd advocate using that fix, if you want to continue supporting the 
library.

/Magnus




>
> -phil.
>
> On 8/30/18, 3:37 PM, Brian Burkhalter wrote:
>> Hi Andrew,
>>
>> As noted in the issue comments, the fdlibm C code is obsolescent and 
>> eventually to be superseded by equivalent Java implementations. As 
>> for the mediaLib code being moribund I cannot speak but am copying 
>> 2d-dev which owns java.desktop.
>>
>> Thanks,
>>
>> Brian
>>
>> On Aug 30, 2018, at 6:03 AM, Andrew Leonard 
>> mailto:andrew_m_leon...@uk.ibm.com>> 
>> wrote:
>>
>>> Thanks Magnus,
>>> Yes, these libraries are moribound it seems, hence their preference 
>>> for compile options..
>>> Cheers
>>> Andrew
>>>
>>> Andrew Leonard
>>> Java Runtimes Development
>>> IBM Hursley
>>> IBM United Kingdom Ltd
>>> Phone internal: 245913, external: 01962 815913
>>> internet email: andrew_m_leon...@uk.ibm.com 
>>> 
>>>
>>>
>>>
>>>
>>> From: Magnus Ihse Bursie >> >
>>> To: Andrew Leonard >> >, Brian Burkhalter 
>>> mailto:brian.burkhal...@oracle.com>>
>>> Cc: core-libs-...@openjdk.java.net 
>>> , build-dev 
>>> mailto:build-dev@openjdk.java.net>>
>>> Date: 30/08/2018 13:49
>>> Subject: Re: RFR JDK-8209786: gcc 7.3 compiler errors on zLinux
>>> 
 
>>>
>>>
>>>
>>>
>>> Andrew,
>>>
>>> If you want to make changes to the build system (files in make/*),
>>> please always include build-dev in the reviews.
>>>
>>> From a build point, this fix looks okay. My general preference is to
>>> fix shady code instead of disabling warnings, but in this case it's in
>>> two libraries that are either external or moribound, so if the
>>> maintainer's of the respective libraries want to avoid code changes, I
>>> accept that.
>>>
>>> /Magnus
>>>
>>>
>>> On 2018-08-30 14:18, Andrew Leonard wrote:
>>> > Hi Brian,
>>> > Thanks for taking a look at this, I have just done a rebuild with 
>>> a new
>>> > patch with appropriate gcc disable warnings for these libraries:
>>> > 
http://cr.openjdk.java.net/~aleonard/8209786/webrev.01/ 

>>> <
http://cr.openjdk.java.net/%7Ealeonard/8209786/webrev.01/
>
>>> > This works fine, so if you think this is a more favourable 
>>> approach for
>>> > these libraries? i'd like to get this merged please.
>>> > Thanks
>>> > Andrew
>>> >
>>> > Andrew Leonard
>>> > Java Runtimes Developm

Re: [patch] add zero support for x86_64-linux-gnux32 target

[Magnus Ihse Bursie]

On 2018-08-31 10:57, John Paul Adrian Glaubitz wrote:

On 08/31/2018 10:43 AM, Magnus Ihse Bursie wrote:

I adjusted the patch to the current code base (which means that most parts of 
it were not needed). What remains are two files. However, I can't test if this 
works. Matthias, can you verify that this is a working patch for jdk/jdk for 
the gnux32 target? If so, I'll sponsor this patch.

I can test it. Didn't know that Matthias posted a patch for x32
support. The current patch that the Debian openjdk-11 package
is using at the moment can be found in [1].

Hah! That's exactly the same patch as I derived. :-)

So that means, I assume, that the patch has been tested properly? If so 
I'll just sponsor and push it.


/Magnus



Adrian


[1] 
https://sources.debian.org/src/openjdk-11/11%7E27-1/debian/patches/zero-x32.diff/

Re: [patch] add zero support for x86_64-linux-gnux32 target

[John Paul Adrian Glaubitz]

On 08/31/2018 11:19 AM, Magnus Ihse Bursie wrote:
> Hah! That's exactly the same patch as I derived. :-)

Great.

> So that means, I assume, that the patch has been tested properly? If so I'll 
> just sponsor and push it.

(sid-x32-sbuild)root@epyc:/# file /usr/lib/jvm/java-11-openjdk-x32/bin/java
/usr/lib/jvm/java-11-openjdk-x32/bin/java: ELF 32-bit LSB executable, x86-64, 
version 1 (SYSV), dynamically linked, interpreter /libx32/ld-linux-x32.so.2, 
for GNU/Linux 3.4.0, BuildID[sha1]=feec974bc65365772119c032d534e49839f8df8f, 
stripped
(sid-x32-sbuild)root@epyc:/# /usr/lib/jvm/java-11-openjdk-x32/bin/java --version
openjdk 11 2018-09-25
OpenJDK Runtime Environment (build 11+27-Debian-1)
OpenJDK Zero VM (build 11+27-Debian-1, interpreted mode)
(sid-x32-sbuild)root@epyc:/#

Yes. Go ahead, please :-).

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [OpenJDK 2D-Dev] RFR JDK-8209786: gcc 7.3 compiler errors on zLinux

[Magnus Ihse Bursie]

On 2018-08-31 11:14, Andrew Leonard wrote:

Hi,
So there seems to be varying opinion here, taking the 2D view point 
since it is going to be maintained, the opinion seems to be more with 
the fix (http://cr.openjdk.java.net/~aleonard/8209786/webrev.00/ 
). This 
would be my personal preference also, but previous comments seemed to 
prefer compiler options.


Looking at each error:
- */libfdlibm/k_rem_pio2.c* : This is a simple "for" loop bound check, 
which with good programming practice should really have been there to 
prevent ArrayOutOfBounds.. or native overwrites.. Simple fix.
- *libmlib_image/mlib_ImageLookUp_Bit.c* : This is -ve bit shifting 
which is spec undefined, which in theory could mean it breaks in the 
future if we upgrade/change compiler... However, this is complex code, 
we need to be very sure the new fix is "correct", myself and my 
colleague here have examined it closely and are happy, but i'd 
appreciate your in-depth analysis please. 
It looks good. The intention of the old code has clearly been to have an 
32 or 64-bit wide all-1s bitmask, and this is what your fix delivers as 
well.





Magnus, Philip, Brian, Goetz, can we have a vote? => "Fix" or 
"DisableWarnings" ?


Note that this decision can be different for the two libraries. I'd 
argue that the maintainer of each library decides. And if so, it seems 
to be "compiler fix" for libfdlibm, and "source fix" for libmlib_image.


/Magnus





Thanks
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com




From: Magnus Ihse Bursie 
To: Philip Race , Brian Burkhalter 

Cc: 2d-dev <2d-...@openjdk.java.net>, build-dev 
, Andrew Leonard 
, core-libs-dev 


Date: 31/08/2018 09:27
Subject: Re: [OpenJDK 2D-Dev] RFR JDK-8209786: gcc 7.3 compiler errors 
on zLinux





On 2018-08-31 01:28, Philip Race wrote:
> Some day, I'd like to replace a lot of medialib functionality with
> something
> like the proposed Vector API. But that is far enough away that
> medialib needs
> to be maintained, and unlike a previous discussion about a similar
> issue in
> the JPEG library, we are on the hook for maintaining medialib.
> So if there is an actual logic error in medialib, I'd prefer to fix it.
> If the issue is a false positive, I'd be OK to disable the warning,
> but wrapped
> in a platform-specific manner. So is it a real error here ?

Gcc is emitting a warning due to shifting of negative values, which is
deemed undefined behavior by the spec.

Is this a real error? Well. Undefined behavior is no good. It can work
for now and then suddenly start breaking.

The originally suggested code change
(http://cr.openjdk.java.net/~aleonard/8209786/webrev.00 
) seems

reasonable to me. It is at the very least much clearer what the
intention is. And it's conformant with the spec.

So I'd advocate using that fix, if you want to continue supporting the
library.

/Magnus




>
> -phil.
>
> On 8/30/18, 3:37 PM, Brian Burkhalter wrote:
>> Hi Andrew,
>>
>> As noted in the issue comments, the fdlibm C code is obsolescent and
>> eventually to be superseded by equivalent Java implementations. As
>> for the mediaLib code being moribund I cannot speak but am copying
>> 2d-dev which owns java.desktop.
>>
>> Thanks,
>>
>> Brian
>>
>> On Aug 30, 2018, at 6:03 AM, Andrew Leonard
>> mailto:andrew_m_leon...@uk.ibm.com>>
>> wrote:
>>
>>> Thanks Magnus,
>>> Yes, these libraries are moribound it seems, hence their preference
>>> for compile options..
>>> Cheers
>>> Andrew
>>>
>>> Andrew Leonard
>>> Java Runtimes Development
>>> IBM Hursley
>>> IBM United Kingdom Ltd
>>> Phone internal: 245913, external: 01962 815913
>>> internet email: andrew_m_leon...@uk.ibm.com
>>> 
>>>
>>>
>>>
>>>
>>> From: Magnus Ihse Bursie >> >
>>> To: Andrew Leonard >> >, Brian Burkhalter
>>> mailto:brian.burkhal...@oracle.com>>
>>> Cc: core-libs-...@openjdk.java.net
>>> , build-dev
>>> mailto:build-dev@openjdk.java.net>>
>>> Date: 30/08/2018 13:49
>>> Subject: Re: RFR JDK-8209786: gcc 7.3 compiler errors on zLinux
>>> 


>>>
>>>
>>>
>>>
>>> Andrew,
>>>
>>> If you want to make changes to the build system (files in make/*),
>>> please always include build-dev in the reviews.
>>>
>>> From a build point, this fix looks okay. My general preference is to
>>> fix shady code instead of disabling warnings, but in this case it's in
>>> two libraries that are either external or moribound, so if the
>>> maintainer's of the respective libraries want to avoid code changes, I
>>> accept that.
>

Configure: Does enabling graal automatically disable cmsgc?

[Lindenmaier, Goetz]

Hi,

I'm just fixing jtreg/runtime/appcds/sharedStrings/IncompatibleOptions.java

It is not checking correctly whether ZGC is enabled. 

I found this code in the test:
if (!Compiler.isGraalEnabled()) { // Graal does not support CMS
  testExec(8, "-XX:+UseConcMarkSweepGC", "", "", false);
}

I think this should test for GC.CMS.isSupported(). I would like to 
fix this, too.
But this would require that configuring with graal disables cmsgc.
(Which seems to be what is desired given the comment above.)
Is this the case?

Best regards,
  Goetz.

Re: Configure: Does enabling graal automatically disable cmsgc?

[Magnus Ihse Bursie]

On 2018-08-31 11:39, Lindenmaier, Goetz wrote:

Hi,

I'm just fixing jtreg/runtime/appcds/sharedStrings/IncompatibleOptions.java

It is not checking correctly whether ZGC is enabled.

I found this code in the test:
if (!Compiler.isGraalEnabled()) { // Graal does not support CMS
   testExec(8, "-XX:+UseConcMarkSweepGC", "", "", false);
}

I think this should test for GC.CMS.isSupported(). I would like to
fix this, too.
But this would require that configuring with graal disables cmsgc.
(Which seems to be what is desired given the comment above.)
Is this the case?


Maybe I'm confused here, but are you not mixing up configure options and 
run-time options? Both graal and CMS require run-time options to be 
enabled, right? So it's an error to enable both of them. But that does 
not prohibit you to build a JVM that has support for both Graal and CMS 
(as long as you only enable at most one of them at a time).


/Magnus

RE: Configure: Does enabling graal automatically disable cmsgc?

[Lindenmaier, Goetz]

Hi Magnus,

Ok, I understand, isGraalEnabled is a runtime check, not a 
check what was set at compile time of the JVM. And the 
two tier-4 comilers can be there both.
So I understand the check in the test.

Thanks,
  Goetz

> -Original Message-
> From: Magnus Ihse Bursie 
> Sent: Freitag, 31. August 2018 12:07
> To: Lindenmaier, Goetz ; hotspot compiler
> ; build-dev (build-
> d...@openjdk.java.net) 
> Subject: Re: Configure: Does enabling graal automatically disable cmsgc?
> 
> On 2018-08-31 11:39, Lindenmaier, Goetz wrote:
> > Hi,
> >
> > I'm just fixing
> jtreg/runtime/appcds/sharedStrings/IncompatibleOptions.java
> >
> > It is not checking correctly whether ZGC is enabled.
> >
> > I found this code in the test:
> > if (!Compiler.isGraalEnabled()) { // Graal does not support CMS
> >testExec(8, "-XX:+UseConcMarkSweepGC", "", "", false);
> > }
> >
> > I think this should test for GC.CMS.isSupported(). I would like to
> > fix this, too.
> > But this would require that configuring with graal disables cmsgc.
> > (Which seems to be what is desired given the comment above.)
> > Is this the case?
> 
> Maybe I'm confused here, but are you not mixing up configure options and
> run-time options? Both graal and CMS require run-time options to be
> enabled, right? So it's an error to enable both of them. But that does
> not prohibit you to build a JVM that has support for both Graal and CMS
> (as long as you only enable at most one of them at a time).
> 
> /Magnus

Re: [patch] add zero support for x86_64-linux-gnux32 target

[John Paul Adrian Glaubitz]

Hi Magnus!

I just tested it and the following change in make/autoconf/flags.m4 is
necessary as well so that gcc is not called with "-m64":

diff -r 18afb2097ada -r 1f28530b1f46 make/autoconf/flags.m4
--- a/make/autoconf/flags.m4Fri Aug 31 11:43:06 2018 +0200
+++ b/make/autoconf/flags.m4Fri Aug 31 12:50:02 2018 +0200
@@ -241,7 +241,8 @@
   elif test "x$TOOLCHAIN_TYPE" = xsolstudio; then
 MACHINE_FLAG="-m${OPENJDK_TARGET_CPU_BITS}"
   elif test "x$TOOLCHAIN_TYPE" = xgcc || test "x$TOOLCHAIN_TYPE" = xclang; then
-if test "x$OPENJDK_TARGET_CPU_ARCH" = xx86 ||
+if test "x$OPENJDK_TARGET_CPU_ARCH" = xx86 &&
+test "x$OPENJDK_TARGET_CPU" != xx32 ||
 test "x$OPENJDK_TARGET_CPU_ARCH" = xsparc ||
 test "x$OPENJDK_TARGET_CPU_ARCH" = xppc; then
   MACHINE_FLAG="-m${OPENJDK_TARGET_CPU_BITS}"

Adrian

On 08/31/2018 10:43 AM, Magnus Ihse Bursie wrote:
> The necroposter strikes back! :-)
> 
> I'm currently trying to fix or close all long standing bugs on 
> infrastructure/build, and now the time has come to JDK-8165440.
> 
> This patch had a bit of bad timing when it was posted, since it could not be 
> accepted into mainline due to feature freeze, and there were no other repo to 
> accept it.
> 
> I adjusted the patch to the current code base (which means that most parts of 
> it were not needed). What remains are two files. However, I can't test if 
> this works. Matthias, can you verify that this is a working patch for jdk/jdk 
> for the gnux32 target? If so, I'll sponsor this patch.
> 
> diff --git a/make/autoconf/platform.m4 b/make/autoconf/platform.m4
> --- a/make/autoconf/platform.m4
> +++ b/make/autoconf/platform.m4
> @@ -35,6 +35,10 @@
>    VAR_CPU_ARCH=x86
>    VAR_CPU_BITS=64
>    VAR_CPU_ENDIAN=little
> +  case "$host" in *x32)
> +    VAR_CPU=x32
> +    VAR_CPU_BITS=32
> +  esac
>    ;;
>  i?86)
>    VAR_CPU=x86
> @@ -455,6 +459,8 @@
>  HOTSPOT_$1_CPU_DEFINE=IA32
>    elif test "x$OPENJDK_$1_CPU" = xx86_64; then
>  HOTSPOT_$1_CPU_DEFINE=AMD64
> +  elif test "x$OPENJDK_$1_CPU" = xx32; then
> +    HOTSPOT_$1_CPU_DEFINE=X32
>    elif test "x$OPENJDK_$1_CPU" = xsparcv9; then
>  HOTSPOT_$1_CPU_DEFINE=SPARC
>    elif test "x$OPENJDK_$1_CPU" = xaarch64; then
> diff --git a/src/hotspot/os/linux/os_linux.cpp 
> b/src/hotspot/os/linux/os_linux.cpp
> --- a/src/hotspot/os/linux/os_linux.cpp
> +++ b/src/hotspot/os/linux/os_linux.cpp
> @@ -1742,7 +1742,7 @@
> 
>  #if  (defined IA32)
>    static  Elf32_Half running_arch_code=EM_386;
> -#elif   (defined AMD64)
> +#elif   (defined AMD64) || defined(X32)
>    static  Elf32_Half running_arch_code=EM_X86_64;
>  #elif  (defined IA64)
>    static  Elf32_Half running_arch_code=EM_IA_64;
> 
> /Magnus
> 
> On 2016-09-06 01:01, David Holmes wrote:
>> Hi Severin, Matthias,
>>
>> On 5/09/2016 10:16 PM, Severin Gehwolf wrote:
>>> On Mon, 2016-09-05 at 14:03 +0200, Matthias Klose wrote:
 The attached patch adds support for building zero for the x86_64-
 linux-gnux32
 target, having changes in the build system, hotspot and jdk.

  - the build system currently only derives the target from
    the cpu in PLATFORM_EXTRACT_VARS_FROM_CPU; that is not enough
    for the new target, which only differs by the ending of the
    triplet. However the $host macro should be available anywhere.

  - the hotspot part just handles the new "cpu"

  - GensrcX11Wrappers.gmk assumes that there is a black/white
    decision about -m32/-m64. The patch works around it. However
    the real patch should be to get these flags from the build
    system, and not hardcode itself.

  - the sysctl system call is unsupported in the x32 kernel, and
    just the include leads to a build error.  From my point of view
    the header is not needed. I had successful builds on all other
    targets without including it. If you want to keep the include,
    then it should be guarded with
    #if !(defined(_ILP32) && defined(__x86_64__))

 Matthias
>>>
>>> I've filed this bug for this:
>>> https://bugs.openjdk.java.net/browse/JDK-8165440
>>
>> Please note that as a P4 issue this can not be fixed given we have hit RDP1:
>>
>> http://mail.openjdk.java.net/pipermail/jdk9-dev/2016-August/004777.html
>>
>> Further this is filed as "bug" but seems to clearly be an enhancement, so 
>> you would need approval for it to come in post-Feature-Complete.
>>
>> Please consider if this is something that must be fixed for 9 or can be 
>> deferred. Otherwise you will need to follow additional approval processes.
>>
>> Sorry.
>>
>> David (just the messenger!)
>>
>>> Unfortunately, I have no way of testing it.
>>>
>>> Cheers,
>>> Severin
>>>

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [patch] add zero support for x86_64-linux-gnux32 target

[Magnus Ihse Bursie]

Yes, that looks reasonable! If you want to, you can push this + Klose's fix. 

/Magnus

> 31 aug. 2018 kl. 13:25 skrev John Paul Adrian Glaubitz 
> :
> 
> Hi Magnus!
> 
> I just tested it and the following change in make/autoconf/flags.m4 is
> necessary as well so that gcc is not called with "-m64":
> 
> diff -r 18afb2097ada -r 1f28530b1f46 make/autoconf/flags.m4
> --- a/make/autoconf/flags.m4Fri Aug 31 11:43:06 2018 +0200
> +++ b/make/autoconf/flags.m4Fri Aug 31 12:50:02 2018 +0200
> @@ -241,7 +241,8 @@
>   elif test "x$TOOLCHAIN_TYPE" = xsolstudio; then
> MACHINE_FLAG="-m${OPENJDK_TARGET_CPU_BITS}"
>   elif test "x$TOOLCHAIN_TYPE" = xgcc || test "x$TOOLCHAIN_TYPE" = xclang; 
> then
> -if test "x$OPENJDK_TARGET_CPU_ARCH" = xx86 ||
> +if test "x$OPENJDK_TARGET_CPU_ARCH" = xx86 &&
> +test "x$OPENJDK_TARGET_CPU" != xx32 ||
> test "x$OPENJDK_TARGET_CPU_ARCH" = xsparc ||
> test "x$OPENJDK_TARGET_CPU_ARCH" = xppc; then
>   MACHINE_FLAG="-m${OPENJDK_TARGET_CPU_BITS}"
> 
> Adrian
> 
>> On 08/31/2018 10:43 AM, Magnus Ihse Bursie wrote:
>> The necroposter strikes back! :-)
>> 
>> I'm currently trying to fix or close all long standing bugs on 
>> infrastructure/build, and now the time has come to JDK-8165440.
>> 
>> This patch had a bit of bad timing when it was posted, since it could not be 
>> accepted into mainline due to feature freeze, and there were no other repo 
>> to accept it.
>> 
>> I adjusted the patch to the current code base (which means that most parts 
>> of it were not needed). What remains are two files. However, I can't test if 
>> this works. Matthias, can you verify that this is a working patch for 
>> jdk/jdk for the gnux32 target? If so, I'll sponsor this patch.
>> 
>> diff --git a/make/autoconf/platform.m4 b/make/autoconf/platform.m4
>> --- a/make/autoconf/platform.m4
>> +++ b/make/autoconf/platform.m4
>> @@ -35,6 +35,10 @@
>>VAR_CPU_ARCH=x86
>>VAR_CPU_BITS=64
>>VAR_CPU_ENDIAN=little
>> +  case "$host" in *x32)
>> +VAR_CPU=x32
>> +VAR_CPU_BITS=32
>> +  esac
>>;;
>>  i?86)
>>VAR_CPU=x86
>> @@ -455,6 +459,8 @@
>>  HOTSPOT_$1_CPU_DEFINE=IA32
>>elif test "x$OPENJDK_$1_CPU" = xx86_64; then
>>  HOTSPOT_$1_CPU_DEFINE=AMD64
>> +  elif test "x$OPENJDK_$1_CPU" = xx32; then
>> +HOTSPOT_$1_CPU_DEFINE=X32
>>elif test "x$OPENJDK_$1_CPU" = xsparcv9; then
>>  HOTSPOT_$1_CPU_DEFINE=SPARC
>>elif test "x$OPENJDK_$1_CPU" = xaarch64; then
>> diff --git a/src/hotspot/os/linux/os_linux.cpp 
>> b/src/hotspot/os/linux/os_linux.cpp
>> --- a/src/hotspot/os/linux/os_linux.cpp
>> +++ b/src/hotspot/os/linux/os_linux.cpp
>> @@ -1742,7 +1742,7 @@
>> 
>>  #if  (defined IA32)
>>static  Elf32_Half running_arch_code=EM_386;
>> -#elif   (defined AMD64)
>> +#elif   (defined AMD64) || defined(X32)
>>static  Elf32_Half running_arch_code=EM_X86_64;
>>  #elif  (defined IA64)
>>static  Elf32_Half running_arch_code=EM_IA_64;
>> 
>> /Magnus
>> 
>>> On 2016-09-06 01:01, David Holmes wrote:
>>> Hi Severin, Matthias,
>>> 
 On 5/09/2016 10:16 PM, Severin Gehwolf wrote:
> On Mon, 2016-09-05 at 14:03 +0200, Matthias Klose wrote:
> The attached patch adds support for building zero for the x86_64-
> linux-gnux32
> target, having changes in the build system, hotspot and jdk.
> 
>  - the build system currently only derives the target from
>the cpu in PLATFORM_EXTRACT_VARS_FROM_CPU; that is not enough
>for the new target, which only differs by the ending of the
>triplet. However the $host macro should be available anywhere.
> 
>  - the hotspot part just handles the new "cpu"
> 
>  - GensrcX11Wrappers.gmk assumes that there is a black/white
>decision about -m32/-m64. The patch works around it. However
>the real patch should be to get these flags from the build
>system, and not hardcode itself.
> 
>  - the sysctl system call is unsupported in the x32 kernel, and
>just the include leads to a build error.  From my point of view
>the header is not needed. I had successful builds on all other
>targets without including it. If you want to keep the include,
>then it should be guarded with
>#if !(defined(_ILP32) && defined(__x86_64__))
> 
> Matthias
 
 I've filed this bug for this:
 https://bugs.openjdk.java.net/browse/JDK-8165440
>>> 
>>> Please note that as a P4 issue this can not be fixed given we have hit RDP1:
>>> 
>>> http://mail.openjdk.java.net/pipermail/jdk9-dev/2016-August/004777.html
>>> 
>>> Further this is filed as "bug" but seems to clearly be an enhancement, so 
>>> you would need approval for it to come in post-Feature-Complete.
>>> 
>>> Please consider if this is something that must be fixed for 9 or can be 
>>> deferred. Otherwise you will need to follow additional approval processes.
>>> 
>>> Sorry.
>>> 
>>> David (just the mes

Re: [patch] add zero support for x86_64-linux-gnux32 target

[John Paul Adrian Glaubitz]

On 08/31/2018 01:40 PM, Magnus Ihse Bursie wrote:
> Yes, that looks reasonable! If you want to, you can push this + Klose's fix. 
Is there a bug in the JBS I can reference?

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: RFR 8210226: Add support for multiple project folders to idea.sh

[Maurizio Cimadamore]

Thanks for the reviews; once again, when doing some more testing I 
discovered some issues which had to do with coexistence with the 
intellij jtreg plugin.


The bottom of the issue is that certain IDE owned ant variable (such as 
$ModuleFileDir$) are only set when clicking on certain IDE actions, but 
not on other. This leads to a very messy behavior, where certain times 
the IDE will fail to build the project if the build is triggered 
implicitly  (e.g. upon running a jtreg test from the IDE).


I now got rid of references to such variables and replaced it with 
references to a new IDEA_DIR template variable, whose addition in the 
idea.sh script is straighforward.


To make sure that I got all bases covered, I tested in the following fashion

1) create a project for java.base and jdk.compiler module in the root folder
- verify that source files belonging to either of the above modules can 
be opened inside the IDE
- verify that jtreg tests can be run from the IDE (and that it triggers 
'make images' correctly)

- verify that the build/rebuild buttons of the IDE also work
- check consistency of IDE setting by inspecting various menus (e.g. 
Project structure and Project settings)


2) create a project for java.desktop in a folder called desktop (e.g. 
using -o desktop), then check again:
- verify that source files belonging to the java.desktop module can be 
opened inside the IDE
- verify that jtreg tests can be run from the IDE (and that it triggers 
'make images' correctly)

- verify that the build/rebuild buttons of the IDE also work
- check consistency of IDE setting by inspecting various menus (e.g. 
Project structure and Project settings)



This all worked as expected.

Webrev:

http://cr.openjdk.java.net/~mcimadamore/8210226_v2/


Sorry for the review churn!

Maurizio





On 30/08/18 16:12, Maurizio Cimadamore wrote:

Hi,
this patch adds proper support for -o option to the idea.sh script, 
which allows to place the .idea folder under any given output folder 
(not necessarily the JDK root).


To be able to do this, I had to revampo the logic for template 
substitution in idea.sh, as it was growing too brittle. I now have a 
much more declarative logic, where replacements can be added by 
calling the add_replacement function. This allows to replace a lot of 
the previous code with very simple and self-explanatory calls to that 
routine.


This piece was necessary, as we needed to replace a lot of references 
to the idea variable $PROJECT_DIR with the new template variable 
###ROOT_DIR### (otherwise the project is ill-formed, since 
$PROJECT_DIR merely points at the folder containing the .idea project 
folder).


Webrev:

http://cr.openjdk.java.net/~mcimadamore/8210226/

Cheers
Maurizio

Re: [OpenJDK 2D-Dev] RFR JDK-8209786: gcc 7.3 compiler errors on zLinux

[Brian Burkhalter]

On Aug 31, 2018, at 2:28 AM, Magnus Ihse Bursie  
wrote:

>> Magnus, Philip, Brian, Goetz, can we have a vote? => "Fix" or 
>> "DisableWarnings" ?
> 
> Note that this decision can be different for the two libraries. I'd argue 
> that the maintainer of each library decides. And if so, it seems to be 
> "compiler fix" for libfdlibm, and "source fix" for libmlib_image.

I think we can safely say “disable compiler errors” for fdlibm as indicated by 
Joe Darcy’s comment in the issue (he owns fdlibm), and source code change for 
mediaLib as Phil indicated in e-mail.

Thanks,

Brian

Re: [patch] add zero support for x86_64-linux-gnux32 target

[Severin Gehwolf]

On Fri, 2018-08-31 at 13:42 +0200, John Paul Adrian Glaubitz wrote:
> On 08/31/2018 01:40 PM, Magnus Ihse Bursie wrote:
> > Yes, that looks reasonable! If you want to, you can push this + Klose's 
> > fix. 
> 
> Is there a bug in the JBS I can reference?

https://bugs.openjdk.java.net/browse/JDK-8165440

Thanks,
Severin

Re: Which build platforms are supported for jdk11 and upcoming jdk12?

[Erik Joelsson]

Hello,

On 2018-08-31 00:38, Lindenmaier, Goetz wrote:

Hi,

https://wiki.openjdk.java.net/display/Build/Supported+Build+Platforms
is quite outdated, listing only jdk9.
Is there a different site now listing this information?

No, we are just bad at keeping it up to date. Oracle's platforms for 11 are:

Linux x64 GCC 7.3, building on OEL7 but still an OEL6 sysroot.
Solaris sparcv9, building on Solaris 11.2 with studio 12.4. (planning to 
go to 12.6 and Solaris 11.3 in JDK 12)

Macosx, building on 10.13.5, Xcode 9.4.
Windows, building on Server 2012, Visual Studio 2017 15.5.5.

I should update the site.

/Erik

(I'll also update our information on this site.)

Because of https://bugs.openjdk.java.net/browse/JDK-8210194
we think about updating our compilers used for jdk12.

Best regards,
   Goetz.

Re: 8209520: Build fails when native code coverage is enabled

[Erik Joelsson]

On 2018-08-31 01:20, Magnus Ihse Bursie wrote:


These are two different arguments for turning off warnings for code 
coverage:

1) gcc is producing incorrect warnings
2) the warnings might be correct, but we are going to treat such bugs 
as low priority



I understand and accept 1, but I do not accept 2 as a valid reason for 
turning off warnings. I tried to dig through history in the 
(Oracle-internal) bug Igor posted, but in the end I could find no 
changes was made to any compiler flags..?


I have googled around but found no discussions at all that indicate 
that gcov should emit invalid warnings.


As for argument 2: we're using code coverage as a way to increase code 
quality, after all. If we get additional warnings, that indicate ways 
to improve the code, then we should use this to improve the code, not 
hide it away. And there's no reason not to treat such bugs as severe 
as any other compile errors.



I agree and it seems I was assuming too much when I claimed 1.
Let's get more concrete: In this case, we have three warnings. To 
solve this, Leonid fixed:


* In macroAssembler_x86.cpp, he added ShouldNotReachHere(), which 
definitely improves code quality.


* In genCollectedHeap.cpp, he he initalized two local variables to 
NULL. After studying the code, I see that this is strictly not needed, 
and gcc is apparently losing some information here that could have 
been used to determine that the code is correct. However, my initial 
reaction when reading the code was that it was indeed broken, and it 
took me some time to prove that it was not. This is not good code 
readability. It's fragile, and some future code change might end up 
with a non-initialized local variable. So I think this is good, 
defensive programming, no matter what.


* In splashscreen_png.c, I'll honestly say that I don't know wether 
this fix is correct or not. But it does seem reasonable. The 
SplashDecodePng() function uses setjmp() *shudder* and for me, that's 
a big Here Be Dragons! sign. I also note that the success variable is 
read in the done: label, just below access to row_pointers and 
image_data -- which are both, already, declared volatile. So I'm 
guessing that declaring success volatile is in fact the correct thing 
to do. (I challenge anyone with a greater understanding of setjmp to 
prove me wrong...)


So I say we should be happy we got those warnings, and fix the code.

I don't see this as an argument that gcc emits invalid warnings. If 
that happens in the future, then we can discuss eliminating those 
warning. But if we do, we should eliminate them selectively, only the 
incorrect warnings and only on the files/libraries where they occur 
erroneously. Not disable all warnings in a single stroke.


Thank you for the detailed analysis. With this I definitely agree that 
it's better to fix the code.


/Erik

/Magnus

Re: RFR 8210226: Add support for multiple project folders to idea.sh

[Erik Joelsson]

Looks ok to me (and same caveat as Magnus).

/Erik


On 2018-08-31 06:59, Maurizio Cimadamore wrote:
Thanks for the reviews; once again, when doing some more testing I 
discovered some issues which had to do with coexistence with the 
intellij jtreg plugin.


The bottom of the issue is that certain IDE owned ant variable (such 
as $ModuleFileDir$) are only set when clicking on certain IDE actions, 
but not on other. This leads to a very messy behavior, where certain 
times the IDE will fail to build the project if the build is triggered 
implicitly  (e.g. upon running a jtreg test from the IDE).


I now got rid of references to such variables and replaced it with 
references to a new IDEA_DIR template variable, whose addition in the 
idea.sh script is straighforward.


To make sure that I got all bases covered, I tested in the following 
fashion


1) create a project for java.base and jdk.compiler module in the root 
folder
- verify that source files belonging to either of the above modules 
can be opened inside the IDE
- verify that jtreg tests can be run from the IDE (and that it 
triggers 'make images' correctly)

- verify that the build/rebuild buttons of the IDE also work
- check consistency of IDE setting by inspecting various menus (e.g. 
Project structure and Project settings)


2) create a project for java.desktop in a folder called desktop (e.g. 
using -o desktop), then check again:
- verify that source files belonging to the java.desktop module can be 
opened inside the IDE
- verify that jtreg tests can be run from the IDE (and that it 
triggers 'make images' correctly)

- verify that the build/rebuild buttons of the IDE also work
- check consistency of IDE setting by inspecting various menus (e.g. 
Project structure and Project settings)



This all worked as expected.

Webrev:

http://cr.openjdk.java.net/~mcimadamore/8210226_v2/


Sorry for the review churn!

Maurizio





On 30/08/18 16:12, Maurizio Cimadamore wrote:

Hi,
this patch adds proper support for -o option to the idea.sh script, 
which allows to place the .idea folder under any given output folder 
(not necessarily the JDK root).


To be able to do this, I had to revampo the logic for template 
substitution in idea.sh, as it was growing too brittle. I now have a 
much more declarative logic, where replacements can be added by 
calling the add_replacement function. This allows to replace a lot of 
the previous code with very simple and self-explanatory calls to that 
routine.


This piece was necessary, as we needed to replace a lot of references 
to the idea variable $PROJECT_DIR with the new template variable 
###ROOT_DIR### (otherwise the project is ill-formed, since 
$PROJECT_DIR merely points at the folder containing the .idea project 
folder).


Webrev:

http://cr.openjdk.java.net/~mcimadamore/8210226/

Cheers
Maurizio

Re: 8209520: Build fails when native code coverage is enabled

[Igor Ignatyev]

Hi Magnus,

herein I will be talking only about 1st two warnings.
 
although your analyze is correct, it doesn't take into account the fact that 
the warnings report situations that can't happen in current codebase, and gcc 
doesn't report them in our "regular" builds b/c it can proof that
- in macroAssembler_x86.cpp both rounding_control() and precision_control() are 
>= 0 and <= 3, so switch statements do cover all possible cases
- in genCollectedHeap.cpp, removeSmallestScratch is called only from one place 
w/ prev_ptr being an address of local variable, therefore we will always visit 
while loop, so smallest_ptr and smallest will be inited.

so these warnings are false-possitive b/c they can never happen. one can argue 
that if we change the code, these issues might become real. that's true, but 
when it happens, gcc will report them as warnings for non-instrumented builds, 
as the compiler won't be able to proof that prev_ptr is always not null, or 
precision_control is from [0;3], etc. this is one of the reasons why these bugs 
won't be ever treated the same as other compiler errors. they are 
false-possitive from practical point of view and fixing them not always improve 
code quality, it some case we might end up w/ more clumsy code just to fix 
"potential" issues w/ zero probability to happen. if we want to use compiler 
warnings to improve quality, I'd suggest us to start w/ removing currently 
disabled warnings and/or start using clang static analyzer and clang tidy 
rather than fixing issues which will never happen.

disabling warnings in instrumented builds isn't the same as disabling all 
warnings in a single stroke, it's just disabling warnings in a build 
configuration we don't treat as important as other configurations.

as for the bug I posted, it was "fixed" by passing 
'--disable-warnings-as-errors' to jprt if native-coverage is enabled.

Thanks,
-- Igor

> On Aug 31, 2018, at 9:21 AM, Erik Joelsson  wrote:
> 
> On 2018-08-31 01:20, Magnus Ihse Bursie wrote:
>> 
>> These are two different arguments for turning off warnings for code coverage:
>> 1) gcc is producing incorrect warnings
>> 2) the warnings might be correct, but we are going to treat such bugs as low 
>> priority
>> 
>> 
>> I understand and accept 1, but I do not accept 2 as a valid reason for 
>> turning off warnings. I tried to dig through history in the 
>> (Oracle-internal) bug Igor posted, but in the end I could find no changes 
>> was made to any compiler flags..?
>> 
>> I have googled around but found no discussions at all that indicate that 
>> gcov should emit invalid warnings.
>> 
>> As for argument 2: we're using code coverage as a way to increase code 
>> quality, after all. If we get additional warnings, that indicate ways to 
>> improve the code, then we should use this to improve the code, not hide it 
>> away. And there's no reason not to treat such bugs as severe as any other 
>> compile errors.
>> 
> I agree and it seems I was assuming too much when I claimed 1.
>> Let's get more concrete: In this case, we have three warnings. To solve 
>> this, Leonid fixed:
>> 
>> * In macroAssembler_x86.cpp, he added ShouldNotReachHere(), which definitely 
>> improves code quality.
>> 
>> * In genCollectedHeap.cpp, he he initalized two local variables to NULL. 
>> After studying the code, I see that this is strictly not needed, and gcc is 
>> apparently losing some information here that could have been used to 
>> determine that the code is correct. However, my initial reaction when 
>> reading the code was that it was indeed broken, and it took me some time to 
>> prove that it was not. This is not good code readability. It's fragile, and 
>> some future code change might end up with a non-initialized local variable. 
>> So I think this is good, defensive programming, no matter what.
>> 
>> * In splashscreen_png.c, I'll honestly say that I don't know wether this fix 
>> is correct or not. But it does seem reasonable. The SplashDecodePng() 
>> function uses setjmp() *shudder* and for me, that's a big Here Be Dragons! 
>> sign. I also note that the success variable is read in the done: label, just 
>> below access to row_pointers and image_data -- which are both, already, 
>> declared volatile. So I'm guessing that declaring success volatile is in 
>> fact the correct thing to do. (I challenge anyone with a greater 
>> understanding of setjmp to prove me wrong...)
>> 
>> So I say we should be happy we got those warnings, and fix the code.
>> 
>> I don't see this as an argument that gcc emits invalid warnings. If that 
>> happens in the future, then we can discuss eliminating those warning. But if 
>> we do, we should eliminate them selectively, only the incorrect warnings and 
>> only on the files/libraries where they occur erroneously. Not disable all 
>> warnings in a single stroke.
>> 
> Thank you for the detailed analysis. With this I definitely agree that it's 
> better to fix the code.
> 
> /Erik
>> /Magnus