Re: RFR(M/L) : 8209611 : use C++ compiler for hotspot tests

[David Holmes]

Hi Igor,

On 21/08/2018 1:58 PM, Igor Ignatev wrote:

Hi David,

It is necessary for vmTestbase tests (if we of course want to clean them 
up) as they are coupled, and moving only part of them will require (non 
trivial) updates in the rest of code (or at least in the shared part) to 
properly serve both compilers.


Can you elaborate on the problem please as I don't see why the 
vmTestbase tests are special here.


I would expect a .c file to be compiled as C and a .cpp to be compiled 
as C++.


It absolutely not necessary for other 
tests, but I’d prefer to have some level of unification in the test 
base. That being said, I agree I might went too far and moved all the 
tests which might or might not compromise their purpose. I personally 
don’t think we should relay on our jtreg testbase to verify if C linked 
libraries can be used with hotspot. it must be verified by JCK which 
should be compiled/linked with carefully chosen compilers/linkers and 
their flags.


Sure but JCK comes into play much later and I'd rather spot issues 
during developer testing than promotion testing.


It has been discussed (not widely enough and I accept that) in 8209547 
and the related email thread b/w JC(cc'ed) and myself.


as I said, I might went a way too far, so I'll revert changes in the 
non-vmTestbase tests and made appropriate changes in makefiles. what do 
you think?


I think I need to see what you mean exactly :)

Thanks,
David


Thanks,
— Igor

On Aug 20, 2018, at 6:43 PM, David Holmes > wrote:



Hi Igor,

On 21/08/2018 8:59 AM, Igor Ignatyev wrote:
http://cr.openjdk.java.net/~iignatyev//8209611/webrev.01/index.html 


11160 lines changed: 879 ins; 61 del; 10220 mod;

Hi all,
could you please review the patch which moves all hotspot native test 
code to C++? this will guarantee that we always use C++ compilers for 
them (as an opposite to either C or C++ compiler depending on 
configuration), as a result we will be able to get rid of 
JNI_ENV_ARG[1] macros, perform other clean ups and improve overall 
quality of the test code.


Sorry but I don't see why this is necessary. If people want to be able 
to write C++ tests then we should enable that if not currently 
enabled, but I don't see why everything should be forced to C++. What 
if we did something that broke the C linkage and we didn't detect it 
because we only ever tested C++?


Was the motivation previously discussed somewhere?

Thanks,
David


the patch consists of two parts:
 - automatic: renaming .c files to .cpp, updating #include, changing 
JNI/JVMTI calls
 - semi-manual: adding extern "C" , fixing a number of compiler 
warnings (mostly types inconsistency), updating makefiles

JBS: https://bugs.openjdk.java.net/browse/JDK-8209611
webrevs:
 - automatic: 
http://cr.openjdk.java.net/~iignatyev//8209611/webrev.00/index.html 


9394 lines changed: 0 ins; 0 del; 9394 mod;
 - semi-manual: 
http://cr.openjdk.java.net/~iignatyev//8209611/webrev.0-1/index.html 


1899 lines changed: 879 ins; 61 del; 959 mod
 - whole: 
http://cr.openjdk.java.net/~iignatyev//8209611/webrev.01/index.html 


11160 lines changed: 879 ins; 61 del; 10220 mod;

testing: all hotspot tests + tier[1-3]
[1] https://bugs.openjdk.java.net/browse/JDK-8209547
Thanks,
-- Igor

Re: RFR(M/L) : 8209611 : use C++ compiler for hotspot tests

[Igor Ignatev]

Hi David,

It is necessary for vmTestbase tests (if we of course want to clean them up) as 
they are coupled, and moving only part of them will require (non trivial) 
updates in the rest of code (or at least in the shared part) to properly serve 
both compilers. It absolutely not necessary for other tests, but I’d prefer to 
have some level of unification in the test base. That being said, I agree I 
might went too far and moved all the tests which might or might not compromise 
their purpose. I personally don’t think we should relay on our jtreg testbase 
to verify if C linked libraries can be used with hotspot. it must be verified 
by JCK which should be compiled/linked with carefully chosen compilers/linkers 
and their flags.

It has been discussed (not widely enough and I accept that) in 8209547 and the 
related email thread b/w JC(cc'ed) and myself.

as I said, I might went a way too far, so I'll revert changes in the 
non-vmTestbase tests and made appropriate changes in makefiles. what do you 
think? 

Thanks,
— Igor

On Aug 20, 2018, at 6:43 PM, David Holmes mailto:david.hol...@oracle.com>> wrote:

> Hi Igor,
> 
> On 21/08/2018 8:59 AM, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev//8209611/webrev.01/index.html 
>> 
>>> 11160 lines changed: 879 ins; 61 del; 10220 mod;
>> Hi all,
>> could you please review the patch which moves all hotspot native test code 
>> to C++? this will guarantee that we always use C++ compilers for them (as an 
>> opposite to either C or C++ compiler depending on configuration), as a 
>> result we will be able to get rid of JNI_ENV_ARG[1] macros, perform other 
>> clean ups and improve overall quality of the test code.
> 
> Sorry but I don't see why this is necessary. If people want to be able to 
> write C++ tests then we should enable that if not currently enabled, but I 
> don't see why everything should be forced to C++. What if we did something 
> that broke the C linkage and we didn't detect it because we only ever tested 
> C++?
> 
> Was the motivation previously discussed somewhere?
> 
> Thanks,
> David
> 
>> the patch consists of two parts:
>>  - automatic: renaming .c files to .cpp, updating #include, changing 
>> JNI/JVMTI calls
>>  - semi-manual: adding extern "C" , fixing a number of compiler warnings 
>> (mostly types inconsistency), updating makefiles
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8209611 
>> 
>> webrevs:
>>  - automatic: 
>> http://cr.openjdk.java.net/~iignatyev//8209611/webrev.00/index.html 
>> 
>>> 9394 lines changed: 0 ins; 0 del; 9394 mod;
>>  - semi-manual: 
>> http://cr.openjdk.java.net/~iignatyev//8209611/webrev.0-1/index.html 
>> 
>>> 1899 lines changed: 879 ins; 61 del; 959 mod
>>  - whole: 
>> http://cr.openjdk.java.net/~iignatyev//8209611/webrev.01/index.html 
>> 
>>> 11160 lines changed: 879 ins; 61 del; 10220 mod;
>> testing: all hotspot tests + tier[1-3]
>> [1] https://bugs.openjdk.java.net/browse/JDK-8209547
>> Thanks,
>> -- Igor

Re: RFR(M/L) : 8209611 : use C++ compiler for hotspot tests

[David Holmes]

Hi Igor,

On 21/08/2018 8:59 AM, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8209611/webrev.01/index.html

11160 lines changed: 879 ins; 61 del; 10220 mod;


Hi all,

could you please review the patch which moves all hotspot native test code to 
C++? this will guarantee that we always use C++ compilers for them (as an 
opposite to either C or C++ compiler depending on configuration), as a result 
we will be able to get rid of JNI_ENV_ARG[1] macros, perform other clean ups 
and improve overall quality of the test code.


Sorry but I don't see why this is necessary. If people want to be able 
to write C++ tests then we should enable that if not currently enabled, 
but I don't see why everything should be forced to C++. What if we did 
something that broke the C linkage and we didn't detect it because we 
only ever tested C++?


Was the motivation previously discussed somewhere?

Thanks,
David


the patch consists of two parts:
  - automatic: renaming .c files to .cpp, updating #include, changing JNI/JVMTI 
calls
  - semi-manual: adding extern "C" , fixing a number of compiler warnings 
(mostly types inconsistency), updating makefiles

JBS: https://bugs.openjdk.java.net/browse/JDK-8209611
webrevs:
  - automatic: 
http://cr.openjdk.java.net/~iignatyev//8209611/webrev.00/index.html

9394 lines changed: 0 ins; 0 del; 9394 mod;


  - semi-manual: 
http://cr.openjdk.java.net/~iignatyev//8209611/webrev.0-1/index.html

1899 lines changed: 879 ins; 61 del; 959 mod


  - whole: http://cr.openjdk.java.net/~iignatyev//8209611/webrev.01/index.html

11160 lines changed: 879 ins; 61 del; 10220 mod;


testing: all hotspot tests + tier[1-3]

[1] https://bugs.openjdk.java.net/browse/JDK-8209547

Thanks,
-- Igor

Re: Linux + Clang + execstack

[David Holmes]

On 21/08/2018 9:39 AM, Arthur Eubanks wrote:
On Mon, Aug 20, 2018 at 4:18 PM David Holmes > wrote:


Hi Arthur,

cc'ing build-dev as this is currently a build issue.

On 21/08/2018 3:11 AM, Arthur Eubanks wrote:
 > Hi,
 >
 > At Google we're trying to build hotspot on Linux with clang. One
thing that
 > happens is that the resulting libjvm.so is stack executable. When
running
 > hotspot we get warnings about the stack being executable.
 >
 > Compiling an assembly file into the final .so results in the
stack being
 > executable. In this case the file is linux_x86_64.s. This doesn't
happen
 > with gcc because "-Wl,-z,noexecstack" is passed as a hotspot
linker flag
 > with gcc in flags-ldflags.m4. When using clang that linker flag isn't
 > passed.
 >
 > Doing something like the solution in
 > https://wiki.ubuntu.com/SecurityTeam/Roadmap/ExecutableStacks
 > fixes the problem without the use of linker flags.

You mean the source code directives for the linker?

Sorry, I wasn't specific enough, I meant the flags for the assembler.
#if defined(__linux__) && defined(__ELF__)
.section        .note.GNU-stack, "", %progbits
#endif


I think I prefer to see this handled explicitly in the build as is
currently done. Can we just adjust ./make/autoconf/flags-ldflags.m4 to
pass the linker flags for gcc and clang?

I don't mind this solution, but it seems like the right thing to do is 
to fix things at the source level and remove extra unnecessary linker 
flags.


Personally I see this as source code pollution. The concept of 
executable stacks has nothing to do with what is being expressed by the 
source code, or the language used for it.


Just my 2c. I'll defer to build folk ... though they are still on 
vacation at the moment.


I removed "-Wl,-z,noexecstack" from the flags after adding the 
above assembler flags and libjvm.so is still correctly not stack 
executable. I don't really mind either way though. Maybe it's good to 
have an extra safeguard in the linker flags.



 > The jtreg test test/hotspot/jtreg/runtime/execstack/TestCheckJDK.java
 > checks for the stack being executable.
 >
 > Any thoughts? If there are no objections, I can propose a patch
that works
 > for both gcc and clang on Linux. Also, I'm not sure how/if macOS
handles
 > this problem given that it uses clang.

We don't seem to handle it at all on OS X. Does OS X prevent executable
stacks itself?

A quick search, according to Wikipedia 
(https://en.wikipedia.org/wiki/Executable_space_protection#macOS), 
64-bit executables on macOS aren't stack or heap executable. Not sure if 
that information is accurate though.


Seems to be:

https://developer.apple.com/library/archive/documentation/Security/Conceptual/SecureCodingGuide/Articles/BufferOverflows.html

"macOS and iOS provide two features that can make it harder to exploit 
stack and buffer overflows: address space layout randomization (ASLR) 
and a non-executable stack and heap."


Cheers,
David



David

Re: [12] RFR JDK-8167314: Enable the check to detect duplicate provides in in GenModuleInfoSource

[Lance Andersen]

looks good Mandy
On Aug 20, 2018, at 6:35 PM, mandy chung  wrote:

> A simple patch to enable the check to enforce no duplicate provides
> in module-info.java.extra, same checks as javac.  This check
> was disabled in the fix for JDK-8202941 because of duplicated
> provides generated for jdk.internal.vm.compiler.  Since JDK-8208463
> is now resolved [1], we can enable this check.
> 
> Webrev:
>  http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8167314/webrev.00/
> 
> Mandy
> [1] http://hg.openjdk.java.net/jdk/jdk/rev/c5461fe16efb




Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com

Re: Linux + Clang + execstack

[David Holmes]

Hi Arthur,

cc'ing build-dev as this is currently a build issue.

On 21/08/2018 3:11 AM, Arthur Eubanks wrote:

Hi,

At Google we're trying to build hotspot on Linux with clang. One thing that
happens is that the resulting libjvm.so is stack executable. When running
hotspot we get warnings about the stack being executable.

Compiling an assembly file into the final .so results in the stack being
executable. In this case the file is linux_x86_64.s. This doesn't happen
with gcc because "-Wl,-z,noexecstack" is passed as a hotspot linker flag
with gcc in flags-ldflags.m4. When using clang that linker flag isn't
passed.

Doing something like the solution in
https://wiki.ubuntu.com/SecurityTeam/Roadmap/ExecutableStacks
fixes the problem without the use of linker flags.


You mean the source code directives for the linker?

I think I prefer to see this handled explicitly in the build as is 
currently done. Can we just adjust ./make/autoconf/flags-ldflags.m4 to 
pass the linker flags for gcc and clang?



The jtreg test test/hotspot/jtreg/runtime/execstack/TestCheckJDK.java
checks for the stack being executable.

Any thoughts? If there are no objections, I can propose a patch that works
for both gcc and clang on Linux. Also, I'm not sure how/if macOS handles
this problem given that it uses clang.


We don't seem to handle it at all on OS X. Does OS X prevent executable 
stacks itself?


David

Re: RFR (XS): 8209760 merge error: restore ea in make/conf/jib-profiles.js

[Mikael Vidstedt]

Looks good, thanks for fixing and sorry for getting it wrong to start with.

Cheers,
Mikael

> On Aug 20, 2018, at 4:11 PM, Tim Bell  wrote:
> 
> All-
> 
> Please review this change to restore ea in make/conf/jib-profiles.js.
> 
> This is a file we use to control some internal build and release 
> infrastructure.  Since it is in the open repository, I am circulating the RFR 
> here.
> 
> Bug #8209760:
> https://bugs.openjdk.java.net/browse/JDK-8209760
> 
> Webrev:
> http://cr.openjdk.java.net/~tbell/8209760/webrev/
> 
> Thanks in advance-
> 
> Tim

RFR (XS): 8209760 merge error: restore ea in make/conf/jib-profiles.js

[Tim Bell]

All-

Please review this change to restore ea in make/conf/jib-profiles.js.

This is a file we use to control some internal build and release 
infrastructure.  Since it is in the open repository, I am circulating 
the RFR here.


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

Webrev:
http://cr.openjdk.java.net/~tbell/8209760/webrev/

Thanks in advance-

Tim

RFR(M/L) : 8209611 : use C++ compiler for hotspot tests

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8209611/webrev.01/index.html
> 11160 lines changed: 879 ins; 61 del; 10220 mod;

Hi all,

could you please review the patch which moves all hotspot native test code to 
C++? this will guarantee that we always use C++ compilers for them (as an 
opposite to either C or C++ compiler depending on configuration), as a result 
we will be able to get rid of JNI_ENV_ARG[1] macros, perform other clean ups 
and improve overall quality of the test code.

the patch consists of two parts:
 - automatic: renaming .c files to .cpp, updating #include, changing JNI/JVMTI 
calls
 - semi-manual: adding extern "C" , fixing a number of compiler warnings 
(mostly types inconsistency), updating makefiles

JBS: https://bugs.openjdk.java.net/browse/JDK-8209611
webrevs:
 - automatic: 
http://cr.openjdk.java.net/~iignatyev//8209611/webrev.00/index.html
> 9394 lines changed: 0 ins; 0 del; 9394 mod; 

 - semi-manual: 
http://cr.openjdk.java.net/~iignatyev//8209611/webrev.0-1/index.html
> 1899 lines changed: 879 ins; 61 del; 959 mod

 - whole: http://cr.openjdk.java.net/~iignatyev//8209611/webrev.01/index.html
> 11160 lines changed: 879 ins; 61 del; 10220 mod;

testing: all hotspot tests + tier[1-3]

[1] https://bugs.openjdk.java.net/browse/JDK-8209547

Thanks,
-- Igor

[12] RFR JDK-8167314: Enable the check to detect duplicate provides in in GenModuleInfoSource

[mandy chung]

A simple patch to enable the check to enforce no duplicate provides
in module-info.java.extra, same checks as javac.  This check
was disabled in the fix for JDK-8202941 because of duplicated
provides generated for jdk.internal.vm.compiler.  Since JDK-8208463
is now resolved [1], we can enable this check.

Webrev:
  http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8167314/webrev.00/

Mandy
[1] http://hg.openjdk.java.net/jdk/jdk/rev/c5461fe16efb

Re: custom extension for make/SourceRevision.gmk

[Christian Thalinger]

> On Jul 19, 2018, at 9:17 PM, Christian Thalinger  
> wrote:
> 
> 
> 
>> On Jul 19, 2018, at 2:31 PM, Erik Joelsson > > wrote:
>> 
>> I can do that. Do you have a bug?
>> 
> No.

Sorry, was on vacation… I don’t see the change in the repo. Did you file one?

>> /Erik
>> 
>> On 2018-07-19 10:57, Christian Thalinger wrote:
>>> 
>>> 
>>> On Thu, Jul 19, 2018 at 1:11 PM Erik Joelsson >> > wrote:
>>> This looks good to me, but will need coordination when pushed as I said 
>>> earlier.
>>> 
>>> 
>>> Do you want to push it so it’s easier?
>>> 
>>> /Erik
>>> 
>>> On 2018-07-19 10:04, Christian Thalinger wrote:
 
 
> On Jul 19, 2018, at 12:57 PM, Erik Joelsson  > wrote:
> 
> 
> 
> On 2018-07-19 09:54, Christian Thalinger wrote:
>> 
>> 
>>> On Jul 19, 2018, at 12:44 PM, Erik Joelsson >> > wrote:
>>> 
>>> 
>>> On 2018-07-19 09:16, Christian Thalinger wrote:
 
 
 Well, the issue is this:
 
 exploded-image: exploded-image-base release-file
 
   release-file: create-source-revision-tracker
 
 store-source-revision:
 +($(CD) $(TOPDIR)/make && $(MAKE) $(MAKE_ARGS) -f 
 SourceRevision.gmk store-source-revision)
 
 create-source-revision-tracker:
 +($(CD) $(TOPDIR)/make && $(MAKE) $(MAKE_ARGS) -f 
 SourceRevision.gmk create-source-revision-tracker)
 
 We need these targets because all isn’t really used.
 
>>> Ah, the all target is tricking me and should be removed if not called 
>>> from anywhere. Then your suggested patch is good (except for missing 
>>> the :=).
>> 
>> Do you want me to remove the all: target?
>> 
> Yes, that would be a good cleanup to avoid confusion.
 
 How about this:
 
 diff --git a/make/SourceRevision.gmk b/make/SourceRevision.gmk
 index 10dd943..6d4a706 100644
 --- a/make/SourceRevision.gmk
 +++ b/make/SourceRevision.gmk
 @@ -1,5 +1,5 @@
  #
 -# Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved.
 +# Copyright (c) 2016, 2018, Oracle and/or its affiliates. All rights 
 reserved.
  # DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  #
  # This code is free software; you can redistribute it and/or modify it
 @@ -23,12 +23,10 @@
  # questions.
  #
  
 -default: all
 -
  include $(SPEC)
  include MakeBase.gmk
  
 -$(eval $(call IncludeCustomExtension, SourceRevision.gmk))
 +$(eval $(call IncludeCustomExtension, SourceRevision-pre.gmk))
  
  
 
  # Keep track of what source revision is used to create the build, by 
 creating
 @@ -94,11 +92,14 @@ ifneq ($(and $(HG), $(wildcard $(TOPDIR)/.hg)), )
  
$(eval $(call CreateSourceRevisionFile, $(STORED_SOURCE_REVISION)))
  
 -  store-source-revision: $(STORED_SOURCE_REVISION)
 +  hg-store-source-revision: $(STORED_SOURCE_REVISION)
  
$(eval $(call CreateSourceRevisionFile, $(SOURCE_REVISION_TRACKER)))
  
 -  create-source-revision-tracker: $(SOURCE_REVISION_TRACKER)
 +  hg-create-source-revision-tracker: $(SOURCE_REVISION_TRACKER)
 +
 +  STORE_SOURCE_REVISION_TARGET := hg-store-source-revision
 +  CREATE_SOURCE_REVISION_TRACKER_TARGET := 
 hg-create-source-revision-tracker
  
  else
# Not using HG
 @@ -106,28 +107,39 @@ else
ifneq ($(wildcard $(STORED_SOURCE_REVISION)), )
  # We have a stored source revision (.src-rev)
  
 -store-source-revision:
 +src-store-source-revision:
 $(call LogInfo, No mercurial configuration present$(COMMA) not 
 updating .src-rev)
  
  $(SOURCE_REVISION_TRACKER): $(STORED_SOURCE_REVISION)
 $(install-file)
  
 -create-source-revision-tracker: $(SOURCE_REVISION_TRACKER)
 +src-create-source-revision-tracker: $(SOURCE_REVISION_TRACKER)
else
  # We don't have a stored source revision. Can't do anything, really.
  
 -store-source-revision:
 +src-store-source-revision:
 $(call LogWarn, Error: No mercurial configuration present$(COMMA) 
 cannot create .src-rev)
 exit 2
  
 -create-source-revision-tracker:
 +src-create-source-revision-tracker:
 $(call LogWarn, Warning: No mercurial configuration present and no 
 .src-rev)
endif
  
 +  STORE_SOURCE_REVISION_TARGET := src-store-source-revision
 +  CREATE_SOURCE_REVISION_TRACKER_TARGET := 
 src-create-source-revision-tracker
 +
  endif
  
 -all: store-source-revision