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

[Vladimir Ivanov]

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

Reviewed.

Best regards,
Vladimir Ivanov


On Aug 20, 2018, at 11:07 PM, Igor Ignatyev  wrote:


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 :)

sure, it will take some time for me to do that, hopefully will upload new 
webrevs tomorrow morning PT. but the basic idea is to leave files in 
test/hotspot/jtreg/compiler, runtime, gc, native_sanity, serviceability, 
testlibrary as .c files, exactly as they were before, and restore corresponding 
filenames in make/test/JtregNativeHotspot.gmk.

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

[Igor Ignatyev]

Hi Erik,

thanks for your review!

-- Igor

> On Aug 28, 2018, at 9:01 AM, Erik Joelsson  wrote:
> 
> Hello,
> 
> I have nothing to add in addition to Magnus' comments. If that is fixed, this 
> looks good from a build point of view.
> 
> /Erik
> 
> 
> On 2018-08-28 07:51, Igor Ignatyev wrote:
>> Hi Magnus,
>> 
>> thanks for reviewing this.
>> 
>> please see my answers inline.
>> 
>> Cheers,
>> -- Igor
>> 
>>> On Aug 28, 2018, at 4:14 AM, Magnus Ihse Bursie 
>>>  wrote:
>>> 
>>> Hi Igor,
>>> 
>>> Sorry for my late arrival to this discussion.
>>> 
>>> I tried to figure out what the latest version of your patch is. My guess is 
>>> that it is http://cr.openjdk.java.net/~iignatyev//8209611/webrev.02/ + the 
>>> inlined patch from this mail?
>> yes, that's right
>>> In JtregNativeHotspot.gmk, you are removing:
>>> ifeq ($(TOOLCHAIN_TYPE), solstudio)
>>>BUILD_HOTSPOT_JTREG_LIBRARIES_CFLAGS_libji06t001 += 
>>> -erroff=E_END_OF_LOOP_CODE_NOT_REACHED
>>> endif
>>> 
>>> Is this related to the current fix?
>> y, C++ compiler of SS doesn't recognize E_END_OF_LOOP_CODE_NOT_REACHED as a 
>> valid tag, it seems this tag is only defined for C compiler
>> 
>>> In the patch below, why are you removing the comment to the "$$(foreach 
>>> file..." loop? I think it's still relevant.
>> no reason, will put it back.
>> 
>>> In this code:
>>>   $$(shell $$(FIND) $$($1_SOURCE_DIRS) -type f \( -name 
>>> "$$($1_PREFIX)*.c" \
>>>-o -name 
>>> "$$($1_PREFIX)*.cpp" \))
>>> 
>>> While I appreciate your concerns of aligning the "-name" arguments, this 
>>> kind of padding is something we try to avoid in the build system. There are 
>>> never any logical places to break the arguments, and this just leads to big 
>>> reshufflings whenever a command line changes, with little gain in 
>>> readability. Instead, just indent the line following the \ with four more 
>>> spaces (and no tabs!). (See 
>>> http://openjdk.java.net/groups/build/doc/code-conventions.html)
>> sure thing, I'll remove all redundant spaces.
>>> Otherwise, this looks fine.
>>> 
>>> /Magnus
>>> 
>>> 
>>> On 2018-08-24 23:20, Igor Ignatyev wrote:
 ok, it worked just fine, here is the final diff for 
 TestFilesCompilation.gmk:
 
> diff -r 028076b2832a -r caca95a834e3 make/common/TestFilesCompilation.gmk
> --- a/make/common/TestFilesCompilation.gmkThu Aug 16 16:28:03 
> 2018 -0700
> +++ b/make/common/TestFilesCompilation.gmkFri Aug 24 14:12:37 
> 2018 -0700
> @@ -60,13 +60,13 @@
>ifeq ($$($1_TYPE), LIBRARY)
>  $1_PREFIX = lib
>  $1_OUTPUT_SUBDIR := lib
> -$1_CFLAGS := $(CFLAGS_TESTLIB)
> +$1_CFLAGS += $(CFLAGS_TESTLIB)
>  $1_LDFLAGS := $(LDFLAGS_TESTLIB) $(call SET_SHARED_LIBRARY_ORIGIN)
>  $1_COMPILATION_TYPE := LIBRARY
>else ifeq ($$($1_TYPE), PROGRAM)
>  $1_PREFIX = exe
>  $1_OUTPUT_SUBDIR := bin
> -$1_CFLAGS := $(CFLAGS_TESTEXE)
> +$1_CFLAGS += $(CFLAGS_TESTEXE)
>  $1_LDFLAGS := $(LDFLAGS_TESTEXE)
>  $1_COMPILATION_TYPE := EXECUTABLE
>else
> @@ -75,12 +75,12 @@
>  # Locate all files with the matching prefix
>$1_FILE_LIST := \
> -  $$(shell $$(FIND) $$($1_SOURCE_DIRS) -type f -name 
> "$$($1_PREFIX)*.c")
> +  $$(shell $$(FIND) $$($1_SOURCE_DIRS) -type f \( -name 
> "$$($1_PREFIX)*.c" \
> +   -o -name 
> "$$($1_PREFIX)*.cpp" \))
>  $1_EXCLUDE_PATTERN := $$(addprefix %/, $$($1_EXCLUDE))
>$1_FILTERED_FILE_LIST := $$(filter-out $$($1_EXCLUDE_PATTERN), 
> $$($1_FILE_LIST))
>  -  # Setup a compilation for each and every one of them
>$$(foreach file, $$($1_FILTERED_FILE_LIST),\
>  $$(eval name := $$(strip $$(basename $$(notdir $$(file) \
>  $$(eval unprefixed_name := $$(patsubst $$($1_PREFIX)%, %, $$(name))) 
> \
> @@ -94,6 +94,7 @@
>  CFLAGS := $$($1_CFLAGS) $$($1_CFLAGS_$$(name)), \
>  LDFLAGS := $$($1_LDFLAGS) $$($1_LDFLAGS_$$(name)), \
>  LIBS := $$($1_LIBS_$$(name)), \
> +TOOLCHAIN := $(if $$(filter %.cpp, $$(file)), 
> TOOLCHAIN_LINK_CXX, TOOLCHAIN_DEFAULT), \
>  OPTIMIZATION := LOW, \
>  COPY_DEBUG_SYMBOLS := false, \
>  STRIP_SYMBOLS := false, \
 Thanks,
 -- Igor
 
 
> On Aug 24, 2018, at 10:36 AM, Igor Ignatev  
> wrote:
> 
> Hi Erik,
> 
> Unfortunately, just adding .cpp files to file list isn’t enough, as I 
> mentioned in one of my previous emails[1], initially I did exactly that 
> and linux-slowdebug build fails b/c c-linker was be used for .o files 
> produced by cpp-compiler.
> 
> I guess something like [2] might work. I'll play w/ this idea and send 
> final patch latch.
> 
> — Igor
> 
> [1] 
> http://mail.open

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

[Erik Joelsson]

Hello,

I have nothing to add in addition to Magnus' comments. If that is fixed, 
this looks good from a build point of view.


/Erik


On 2018-08-28 07:51, Igor Ignatyev wrote:

Hi Magnus,

thanks for reviewing this.

please see my answers inline.

Cheers,
-- Igor


On Aug 28, 2018, at 4:14 AM, Magnus Ihse Bursie  
wrote:

Hi Igor,

Sorry for my late arrival to this discussion.

I tried to figure out what the latest version of your patch is. My guess is 
that it is http://cr.openjdk.java.net/~iignatyev//8209611/webrev.02/ + the 
inlined patch from this mail?

yes, that's right

In JtregNativeHotspot.gmk, you are removing:
ifeq ($(TOOLCHAIN_TYPE), solstudio)
BUILD_HOTSPOT_JTREG_LIBRARIES_CFLAGS_libji06t001 += 
-erroff=E_END_OF_LOOP_CODE_NOT_REACHED
endif

Is this related to the current fix?

y, C++ compiler of SS doesn't recognize E_END_OF_LOOP_CODE_NOT_REACHED as a 
valid tag, it seems this tag is only defined for C compiler


In the patch below, why are you removing the comment to the "$$(foreach 
file..." loop? I think it's still relevant.

no reason, will put it back.


In this code:
   $$(shell $$(FIND) $$($1_SOURCE_DIRS) -type f \( -name "$$($1_PREFIX)*.c" 
\
-o -name 
"$$($1_PREFIX)*.cpp" \))

While I appreciate your concerns of aligning the "-name" arguments, this kind 
of padding is something we try to avoid in the build system. There are never any logical 
places to break the arguments, and this just leads to big reshufflings whenever a command 
line changes, with little gain in readability. Instead, just indent the line following 
the \ with four more spaces (and no tabs!). (See 
http://openjdk.java.net/groups/build/doc/code-conventions.html)

sure thing, I'll remove all redundant spaces.

Otherwise, this looks fine.

/Magnus


On 2018-08-24 23:20, Igor Ignatyev wrote:

ok, it worked just fine, here is the final diff for TestFilesCompilation.gmk:


diff -r 028076b2832a -r caca95a834e3 make/common/TestFilesCompilation.gmk
--- a/make/common/TestFilesCompilation.gmk  Thu Aug 16 16:28:03 2018 -0700
+++ b/make/common/TestFilesCompilation.gmk  Fri Aug 24 14:12:37 2018 -0700
@@ -60,13 +60,13 @@
ifeq ($$($1_TYPE), LIBRARY)
  $1_PREFIX = lib
  $1_OUTPUT_SUBDIR := lib
-$1_CFLAGS := $(CFLAGS_TESTLIB)
+$1_CFLAGS += $(CFLAGS_TESTLIB)
  $1_LDFLAGS := $(LDFLAGS_TESTLIB) $(call SET_SHARED_LIBRARY_ORIGIN)
  $1_COMPILATION_TYPE := LIBRARY
else ifeq ($$($1_TYPE), PROGRAM)
  $1_PREFIX = exe
  $1_OUTPUT_SUBDIR := bin
-$1_CFLAGS := $(CFLAGS_TESTEXE)
+$1_CFLAGS += $(CFLAGS_TESTEXE)
  $1_LDFLAGS := $(LDFLAGS_TESTEXE)
  $1_COMPILATION_TYPE := EXECUTABLE
else
@@ -75,12 +75,12 @@
  # Locate all files with the matching prefix
$1_FILE_LIST := \
-  $$(shell $$(FIND) $$($1_SOURCE_DIRS) -type f -name "$$($1_PREFIX)*.c")
+  $$(shell $$(FIND) $$($1_SOURCE_DIRS) -type f \( -name "$$($1_PREFIX)*.c" 
\
+   -o -name 
"$$($1_PREFIX)*.cpp" \))
  $1_EXCLUDE_PATTERN := $$(addprefix %/, $$($1_EXCLUDE))
$1_FILTERED_FILE_LIST := $$(filter-out $$($1_EXCLUDE_PATTERN), 
$$($1_FILE_LIST))
  -  # Setup a compilation for each and every one of them
$$(foreach file, $$($1_FILTERED_FILE_LIST),\
  $$(eval name := $$(strip $$(basename $$(notdir $$(file) \
  $$(eval unprefixed_name := $$(patsubst $$($1_PREFIX)%, %, $$(name))) \
@@ -94,6 +94,7 @@
  CFLAGS := $$($1_CFLAGS) $$($1_CFLAGS_$$(name)), \
  LDFLAGS := $$($1_LDFLAGS) $$($1_LDFLAGS_$$(name)), \
  LIBS := $$($1_LIBS_$$(name)), \
+TOOLCHAIN := $(if $$(filter %.cpp, $$(file)), TOOLCHAIN_LINK_CXX, 
TOOLCHAIN_DEFAULT), \
  OPTIMIZATION := LOW, \
  COPY_DEBUG_SYMBOLS := false, \
  STRIP_SYMBOLS := false, \

Thanks,
-- Igor



On Aug 24, 2018, at 10:36 AM, Igor Ignatev  wrote:

Hi Erik,

Unfortunately, just adding .cpp files to file list isn’t enough, as I mentioned 
in one of my previous emails[1], initially I did exactly that and 
linux-slowdebug build fails b/c c-linker was be used for .o files produced by 
cpp-compiler.

I guess something like [2] might work. I'll play w/ this idea and send final 
patch latch.

— Igor

[1] http://mail.openjdk.java.net/pipermail/build-dev/2018-August/022922.html
[2] TOOLCHAIN := $(if $$(filter %.cpp, $$(file)), TOOLCHAIN_LINK_CXX, 
TOOLCHAIN_DEFAULT)


On Aug 23, 2018, at 11:31 PM, Erik Joelsson  wrote:

Hello Igor,

In TestFilesCompilation.gmk, there is no need to duplicate the whole macro 
call. If you want to find .cpp as well as .c files, just add that to the one 
list of files. The SetupNativeCompilation macro will automatically treat .c and 
.cpp correctly.

Regarding the .c/.cpp files for your vmtestbase tests that include all the 
other files, this is an unfortunate solution, but I guess OK if it works. We 
certainly didn't intend it that way. The macro SetupTestFilesCompilation was 
intend

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

[Igor Ignatyev]

Hi Magnus,

thanks for reviewing this.

please see my answers inline.

Cheers,
-- Igor

> On Aug 28, 2018, at 4:14 AM, Magnus Ihse Bursie 
>  wrote:
> 
> Hi Igor,
> 
> Sorry for my late arrival to this discussion.
> 
> I tried to figure out what the latest version of your patch is. My guess is 
> that it is http://cr.openjdk.java.net/~iignatyev//8209611/webrev.02/ + the 
> inlined patch from this mail?
yes, that's right
> 
> In JtregNativeHotspot.gmk, you are removing:
> ifeq ($(TOOLCHAIN_TYPE), solstudio)
>BUILD_HOTSPOT_JTREG_LIBRARIES_CFLAGS_libji06t001 += 
> -erroff=E_END_OF_LOOP_CODE_NOT_REACHED
> endif
> 
> Is this related to the current fix?

y, C++ compiler of SS doesn't recognize E_END_OF_LOOP_CODE_NOT_REACHED as a 
valid tag, it seems this tag is only defined for C compiler

> 
> In the patch below, why are you removing the comment to the "$$(foreach 
> file..." loop? I think it's still relevant.
no reason, will put it back.

> In this code:
>   $$(shell $$(FIND) $$($1_SOURCE_DIRS) -type f \( -name 
> "$$($1_PREFIX)*.c" \
>-o -name 
> "$$($1_PREFIX)*.cpp" \))
> 
> While I appreciate your concerns of aligning the "-name" arguments, this kind 
> of padding is something we try to avoid in the build system. There are never 
> any logical places to break the arguments, and this just leads to big 
> reshufflings whenever a command line changes, with little gain in 
> readability. Instead, just indent the line following the \ with four more 
> spaces (and no tabs!). (See 
> http://openjdk.java.net/groups/build/doc/code-conventions.html)
sure thing, I'll remove all redundant spaces. 
> 
> Otherwise, this looks fine.
> 
> /Magnus
> 
> 
> On 2018-08-24 23:20, Igor Ignatyev wrote:
>> ok, it worked just fine, here is the final diff for TestFilesCompilation.gmk:
>> 
>>> diff -r 028076b2832a -r caca95a834e3 make/common/TestFilesCompilation.gmk
>>> --- a/make/common/TestFilesCompilation.gmk  Thu Aug 16 16:28:03 2018 -0700
>>> +++ b/make/common/TestFilesCompilation.gmk  Fri Aug 24 14:12:37 2018 -0700
>>> @@ -60,13 +60,13 @@
>>>ifeq ($$($1_TYPE), LIBRARY)
>>>  $1_PREFIX = lib
>>>  $1_OUTPUT_SUBDIR := lib
>>> -$1_CFLAGS := $(CFLAGS_TESTLIB)
>>> +$1_CFLAGS += $(CFLAGS_TESTLIB)
>>>  $1_LDFLAGS := $(LDFLAGS_TESTLIB) $(call SET_SHARED_LIBRARY_ORIGIN)
>>>  $1_COMPILATION_TYPE := LIBRARY
>>>else ifeq ($$($1_TYPE), PROGRAM)
>>>  $1_PREFIX = exe
>>>  $1_OUTPUT_SUBDIR := bin
>>> -$1_CFLAGS := $(CFLAGS_TESTEXE)
>>> +$1_CFLAGS += $(CFLAGS_TESTEXE)
>>>  $1_LDFLAGS := $(LDFLAGS_TESTEXE)
>>>  $1_COMPILATION_TYPE := EXECUTABLE
>>>else
>>> @@ -75,12 +75,12 @@
>>>  # Locate all files with the matching prefix
>>>$1_FILE_LIST := \
>>> -  $$(shell $$(FIND) $$($1_SOURCE_DIRS) -type f -name 
>>> "$$($1_PREFIX)*.c")
>>> +  $$(shell $$(FIND) $$($1_SOURCE_DIRS) -type f \( -name 
>>> "$$($1_PREFIX)*.c" \
>>> +   -o -name 
>>> "$$($1_PREFIX)*.cpp" \))
>>>  $1_EXCLUDE_PATTERN := $$(addprefix %/, $$($1_EXCLUDE))
>>>$1_FILTERED_FILE_LIST := $$(filter-out $$($1_EXCLUDE_PATTERN), 
>>> $$($1_FILE_LIST))
>>>  -  # Setup a compilation for each and every one of them
>>>$$(foreach file, $$($1_FILTERED_FILE_LIST),\
>>>  $$(eval name := $$(strip $$(basename $$(notdir $$(file) \
>>>  $$(eval unprefixed_name := $$(patsubst $$($1_PREFIX)%, %, $$(name))) \
>>> @@ -94,6 +94,7 @@
>>>  CFLAGS := $$($1_CFLAGS) $$($1_CFLAGS_$$(name)), \
>>>  LDFLAGS := $$($1_LDFLAGS) $$($1_LDFLAGS_$$(name)), \
>>>  LIBS := $$($1_LIBS_$$(name)), \
>>> +TOOLCHAIN := $(if $$(filter %.cpp, $$(file)), TOOLCHAIN_LINK_CXX, 
>>> TOOLCHAIN_DEFAULT), \
>>>  OPTIMIZATION := LOW, \
>>>  COPY_DEBUG_SYMBOLS := false, \
>>>  STRIP_SYMBOLS := false, \
>> Thanks,
>> -- Igor
>> 
>> 
>>> On Aug 24, 2018, at 10:36 AM, Igor Ignatev  wrote:
>>> 
>>> Hi Erik,
>>> 
>>> Unfortunately, just adding .cpp files to file list isn’t enough, as I 
>>> mentioned in one of my previous emails[1], initially I did exactly that and 
>>> linux-slowdebug build fails b/c c-linker was be used for .o files produced 
>>> by cpp-compiler.
>>> 
>>> I guess something like [2] might work. I'll play w/ this idea and send 
>>> final patch latch.
>>> 
>>> — Igor
>>> 
>>> [1] http://mail.openjdk.java.net/pipermail/build-dev/2018-August/022922.html
>>> [2] TOOLCHAIN := $(if $$(filter %.cpp, $$(file)), TOOLCHAIN_LINK_CXX, 
>>> TOOLCHAIN_DEFAULT)
>>> 
 On Aug 23, 2018, at 11:31 PM, Erik Joelsson  
 wrote:
 
 Hello Igor,
 
 In TestFilesCompilation.gmk, there is no need to duplicate the whole macro 
 call. If you want to find .cpp as well as .c files, just add that to the 
 one list of files. The SetupNativeCompilation macro will automatically 
 treat .c and .cpp correctly.
 
 Regarding the .c/.cpp files for your vmtestbase tests tha

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

[Magnus Ihse Bursie]

Hi Igor,

Sorry for my late arrival to this discussion.

I tried to figure out what the latest version of your patch is. My guess 
is that it is http://cr.openjdk.java.net/~iignatyev//8209611/webrev.02/ 
+ the inlined patch from this mail?


In JtregNativeHotspot.gmk, you are removing:
ifeq ($(TOOLCHAIN_TYPE), solstudio)
   BUILD_HOTSPOT_JTREG_LIBRARIES_CFLAGS_libji06t001 += 
-erroff=E_END_OF_LOOP_CODE_NOT_REACHED

endif

Is this related to the current fix?

In the patch below, why are you removing the comment to the "$$(foreach 
file..." loop? I think it's still relevant.


In this code:
  $$(shell $$(FIND) $$($1_SOURCE_DIRS) -type f \( -name 
"$$($1_PREFIX)*.c" \
   -o -name 
"$$($1_PREFIX)*.cpp" \))


While I appreciate your concerns of aligning the "-name" arguments, this 
kind of padding is something we try to avoid in the build system. There 
are never any logical places to break the arguments, and this just leads 
to big reshufflings whenever a command line changes, with little gain in 
readability. Instead, just indent the line following the \ with four 
more spaces (and no tabs!). (See 
http://openjdk.java.net/groups/build/doc/code-conventions.html)


Otherwise, this looks fine.

/Magnus


On 2018-08-24 23:20, Igor Ignatyev wrote:

ok, it worked just fine, here is the final diff for TestFilesCompilation.gmk:


diff -r 028076b2832a -r caca95a834e3 make/common/TestFilesCompilation.gmk
--- a/make/common/TestFilesCompilation.gmk  Thu Aug 16 16:28:03 2018 -0700
+++ b/make/common/TestFilesCompilation.gmk  Fri Aug 24 14:12:37 2018 -0700
@@ -60,13 +60,13 @@
ifeq ($$($1_TYPE), LIBRARY)
  $1_PREFIX = lib
  $1_OUTPUT_SUBDIR := lib
-$1_CFLAGS := $(CFLAGS_TESTLIB)
+$1_CFLAGS += $(CFLAGS_TESTLIB)
  $1_LDFLAGS := $(LDFLAGS_TESTLIB) $(call SET_SHARED_LIBRARY_ORIGIN)
  $1_COMPILATION_TYPE := LIBRARY
else ifeq ($$($1_TYPE), PROGRAM)
  $1_PREFIX = exe
  $1_OUTPUT_SUBDIR := bin
-$1_CFLAGS := $(CFLAGS_TESTEXE)
+$1_CFLAGS += $(CFLAGS_TESTEXE)
  $1_LDFLAGS := $(LDFLAGS_TESTEXE)
  $1_COMPILATION_TYPE := EXECUTABLE
else
@@ -75,12 +75,12 @@
  
# Locate all files with the matching prefix

$1_FILE_LIST := \
-  $$(shell $$(FIND) $$($1_SOURCE_DIRS) -type f -name "$$($1_PREFIX)*.c")
+  $$(shell $$(FIND) $$($1_SOURCE_DIRS) -type f \( -name "$$($1_PREFIX)*.c" 
\
+   -o -name 
"$$($1_PREFIX)*.cpp" \))
  
$1_EXCLUDE_PATTERN := $$(addprefix %/, $$($1_EXCLUDE))

$1_FILTERED_FILE_LIST := $$(filter-out $$($1_EXCLUDE_PATTERN), 
$$($1_FILE_LIST))
  
-  # Setup a compilation for each and every one of them

$$(foreach file, $$($1_FILTERED_FILE_LIST),\
  $$(eval name := $$(strip $$(basename $$(notdir $$(file) \
  $$(eval unprefixed_name := $$(patsubst $$($1_PREFIX)%, %, $$(name))) \
@@ -94,6 +94,7 @@
  CFLAGS := $$($1_CFLAGS) $$($1_CFLAGS_$$(name)), \
  LDFLAGS := $$($1_LDFLAGS) $$($1_LDFLAGS_$$(name)), \
  LIBS := $$($1_LIBS_$$(name)), \
+TOOLCHAIN := $(if $$(filter %.cpp, $$(file)), TOOLCHAIN_LINK_CXX, 
TOOLCHAIN_DEFAULT), \
  OPTIMIZATION := LOW, \
  COPY_DEBUG_SYMBOLS := false, \
  STRIP_SYMBOLS := false, \

Thanks,
-- Igor



On Aug 24, 2018, at 10:36 AM, Igor Ignatev  wrote:

Hi Erik,

Unfortunately, just adding .cpp files to file list isn’t enough, as I mentioned 
in one of my previous emails[1], initially I did exactly that and 
linux-slowdebug build fails b/c c-linker was be used for .o files produced by 
cpp-compiler.

I guess something like [2] might work. I'll play w/ this idea and send final 
patch latch.

— Igor

[1] http://mail.openjdk.java.net/pipermail/build-dev/2018-August/022922.html
[2] TOOLCHAIN := $(if $$(filter %.cpp, $$(file)), TOOLCHAIN_LINK_CXX, 
TOOLCHAIN_DEFAULT)


On Aug 23, 2018, at 11:31 PM, Erik Joelsson  wrote:

Hello Igor,

In TestFilesCompilation.gmk, there is no need to duplicate the whole macro 
call. If you want to find .cpp as well as .c files, just add that to the one 
list of files. The SetupNativeCompilation macro will automatically treat .c and 
.cpp correctly.

Regarding the .c/.cpp files for your vmtestbase tests that include all the 
other files, this is an unfortunate solution, but I guess OK if it works. We 
certainly didn't intend it that way. The macro SetupTestFilesCompilation was 
intended for easily writing single file native exe and lib binaries without 
having to modify any makefile. The expectation was that if anything more 
complicated was needed (like multiple files per binary), we would just write 
explicit SetupNativeCompilation calls for them in JtregNativeHotspot.gmk. It 
now looks like we have a new pattern of source files/directories that turns 
into native libraries, and we could of course create a new macro that 
automatically generates compilation setups for them as well (given that file or 
directory names makes i

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

[Igor Ignatyev]

ok, it worked just fine, here is the final diff for TestFilesCompilation.gmk:

> diff -r 028076b2832a -r caca95a834e3 make/common/TestFilesCompilation.gmk
> --- a/make/common/TestFilesCompilation.gmkThu Aug 16 16:28:03 2018 -0700
> +++ b/make/common/TestFilesCompilation.gmkFri Aug 24 14:12:37 2018 -0700
> @@ -60,13 +60,13 @@
>ifeq ($$($1_TYPE), LIBRARY)
>  $1_PREFIX = lib
>  $1_OUTPUT_SUBDIR := lib
> -$1_CFLAGS := $(CFLAGS_TESTLIB)
> +$1_CFLAGS += $(CFLAGS_TESTLIB)
>  $1_LDFLAGS := $(LDFLAGS_TESTLIB) $(call SET_SHARED_LIBRARY_ORIGIN)
>  $1_COMPILATION_TYPE := LIBRARY
>else ifeq ($$($1_TYPE), PROGRAM)
>  $1_PREFIX = exe
>  $1_OUTPUT_SUBDIR := bin
> -$1_CFLAGS := $(CFLAGS_TESTEXE)
> +$1_CFLAGS += $(CFLAGS_TESTEXE)
>  $1_LDFLAGS := $(LDFLAGS_TESTEXE)
>  $1_COMPILATION_TYPE := EXECUTABLE
>else
> @@ -75,12 +75,12 @@
>  
># Locate all files with the matching prefix
>$1_FILE_LIST := \
> -  $$(shell $$(FIND) $$($1_SOURCE_DIRS) -type f -name "$$($1_PREFIX)*.c")
> +  $$(shell $$(FIND) $$($1_SOURCE_DIRS) -type f \( -name 
> "$$($1_PREFIX)*.c" \
> +   -o -name 
> "$$($1_PREFIX)*.cpp" \))
>  
>$1_EXCLUDE_PATTERN := $$(addprefix %/, $$($1_EXCLUDE))
>$1_FILTERED_FILE_LIST := $$(filter-out $$($1_EXCLUDE_PATTERN), 
> $$($1_FILE_LIST))
>  
> -  # Setup a compilation for each and every one of them
>$$(foreach file, $$($1_FILTERED_FILE_LIST),\
>  $$(eval name := $$(strip $$(basename $$(notdir $$(file) \
>  $$(eval unprefixed_name := $$(patsubst $$($1_PREFIX)%, %, $$(name))) \
> @@ -94,6 +94,7 @@
>  CFLAGS := $$($1_CFLAGS) $$($1_CFLAGS_$$(name)), \
>  LDFLAGS := $$($1_LDFLAGS) $$($1_LDFLAGS_$$(name)), \
>  LIBS := $$($1_LIBS_$$(name)), \
> +TOOLCHAIN := $(if $$(filter %.cpp, $$(file)), TOOLCHAIN_LINK_CXX, 
> TOOLCHAIN_DEFAULT), \
>  OPTIMIZATION := LOW, \
>  COPY_DEBUG_SYMBOLS := false, \
>  STRIP_SYMBOLS := false, \

Thanks,
-- Igor


> On Aug 24, 2018, at 10:36 AM, Igor Ignatev  wrote:
> 
> Hi Erik,
> 
> Unfortunately, just adding .cpp files to file list isn’t enough, as I 
> mentioned in one of my previous emails[1], initially I did exactly that and 
> linux-slowdebug build fails b/c c-linker was be used for .o files produced by 
> cpp-compiler.
> 
> I guess something like [2] might work. I'll play w/ this idea and send final 
> patch latch.
> 
> — Igor
> 
> [1] http://mail.openjdk.java.net/pipermail/build-dev/2018-August/022922.html
> [2] TOOLCHAIN := $(if $$(filter %.cpp, $$(file)), TOOLCHAIN_LINK_CXX, 
> TOOLCHAIN_DEFAULT)
> 
>> On Aug 23, 2018, at 11:31 PM, Erik Joelsson  wrote:
>> 
>> Hello Igor,
>> 
>> In TestFilesCompilation.gmk, there is no need to duplicate the whole macro 
>> call. If you want to find .cpp as well as .c files, just add that to the one 
>> list of files. The SetupNativeCompilation macro will automatically treat .c 
>> and .cpp correctly.
>> 
>> Regarding the .c/.cpp files for your vmtestbase tests that include all the 
>> other files, this is an unfortunate solution, but I guess OK if it works. We 
>> certainly didn't intend it that way. The macro SetupTestFilesCompilation was 
>> intended for easily writing single file native exe and lib binaries without 
>> having to modify any makefile. The expectation was that if anything more 
>> complicated was needed (like multiple files per binary), we would just write 
>> explicit SetupNativeCompilation calls for them in JtregNativeHotspot.gmk. It 
>> now looks like we have a new pattern of source files/directories that turns 
>> into native libraries, and we could of course create a new macro that 
>> automatically generates compilation setups for them as well (given that file 
>> or directory names makes it possible to automatically determine everything 
>> needed). Of course, that change would be a separate cleanup possible if you 
>> want to.
>> 
>> /Erik
>> 
>>> On 2018-08-20 15:59, 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.
>>> 
>>> 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
 93

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

[Igor Ignatev]

Hi Erik,

Unfortunately, just adding .cpp files to file list isn’t enough, as I mentioned 
in one of my previous emails[1], initially I did exactly that and 
linux-slowdebug build fails b/c c-linker was be used for .o files produced by 
cpp-compiler.

I guess something like [2] might work. I'll play w/ this idea and send final 
patch latch.

— Igor

[1] http://mail.openjdk.java.net/pipermail/build-dev/2018-August/022922.html
[2] TOOLCHAIN := $(if $$(filter %.cpp, $$(file)), TOOLCHAIN_LINK_CXX, 
TOOLCHAIN_DEFAULT)

> On Aug 23, 2018, at 11:31 PM, Erik Joelsson  wrote:
> 
> Hello Igor,
> 
> In TestFilesCompilation.gmk, there is no need to duplicate the whole macro 
> call. If you want to find .cpp as well as .c files, just add that to the one 
> list of files. The SetupNativeCompilation macro will automatically treat .c 
> and .cpp correctly.
> 
> Regarding the .c/.cpp files for your vmtestbase tests that include all the 
> other files, this is an unfortunate solution, but I guess OK if it works. We 
> certainly didn't intend it that way. The macro SetupTestFilesCompilation was 
> intended for easily writing single file native exe and lib binaries without 
> having to modify any makefile. The expectation was that if anything more 
> complicated was needed (like multiple files per binary), we would just write 
> explicit SetupNativeCompilation calls for them in JtregNativeHotspot.gmk. It 
> now looks like we have a new pattern of source files/directories that turns 
> into native libraries, and we could of course create a new macro that 
> automatically generates compilation setups for them as well (given that file 
> or directory names makes it possible to automatically determine everything 
> needed). Of course, that change would be a separate cleanup possible if you 
> want to.
> 
> /Erik
> 
>> On 2018-08-20 15:59, 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.
>> 
>> 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

[Erik Joelsson]

Hello Igor,

In TestFilesCompilation.gmk, there is no need to duplicate the whole 
macro call. If you want to find .cpp as well as .c files, just add that 
to the one list of files. The SetupNativeCompilation macro will 
automatically treat .c and .cpp correctly.


Regarding the .c/.cpp files for your vmtestbase tests that include all 
the other files, this is an unfortunate solution, but I guess OK if it 
works. We certainly didn't intend it that way. The macro 
SetupTestFilesCompilation was intended for easily writing single file 
native exe and lib binaries without having to modify any makefile. The 
expectation was that if anything more complicated was needed (like 
multiple files per binary), we would just write explicit 
SetupNativeCompilation calls for them in JtregNativeHotspot.gmk. It now 
looks like we have a new pattern of source files/directories that turns 
into native libraries, and we could of course create a new macro that 
automatically generates compilation setups for them as well (given that 
file or directory names makes it possible to automatically determine 
everything needed). Of course, that change would be a separate cleanup 
possible if you want to.


/Erik

On 2018-08-20 15:59, 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.

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 Ignatyev]

Hi JC,

thanks for looking at it and even paying attention to the changes;)

regarding your nits. a space before comma is straightforward and must be done 
for test code to be aligned w/ regular hotspot code style, static_case<> is 
more controversial, it's not used much by hotspot. I do recall some discussion 
around it 3-4 years ago, but I can't recall the outcome. anyhow, I'd prefer to 
clean it up later, as these editorial/style fixes can be done more gradually, 
and this patch kinda blocks your work on 8209547.

Thanks,
-- Igor


> On Aug 22, 2018, at 11:53 AM, JC Beyler  wrote:
> 
> Hi Igor,
> 
> I looked over the changes and tried to pay attention to all the mechanical 
> changes done. They look good to me as they are the step in the direction of 
> getting it all in C++. I will be happy to start on 8209547 once this goes in. 
> 
> (As always, not an official reviewer)
> 
> Two nits:
> 
> - We could use static_cast?
> +*new_bytes = (u1*) realloc(gen, *new_length);
> 
> - Add a space since we are doing the change (this is one example)?
> -arrayOrig[i]=env->GetObjectArrayElement(orig,i); CE
> -arrayClone[i]=env->GetObjectArrayElement(clone,i); CE
> +arrayOrig[i]=(jarray) env->GetObjectArrayElement(orig,i); CE
> +arrayClone[i]=(jarray) env->GetObjectArrayElement(clone,i); CE
> 
> On the other hand, we could just clean this up later once this big move to 
> C++ happens, so I'd be happy to see us tackle it in a second/third step.
> 
> So looks good to me and thanks for doing it!
> Jc
> 
> On Tue, Aug 21, 2018 at 9:58 PM Igor Ignatyev  > wrote:
> Hi David,
> 
> thanks for looking at the webrev and all your comments. my answers are 
> inlined.
> 
> enjoy your vacation!
> 
> -- Igor 
> 
> > On Aug 21, 2018, at 9:28 PM, David Holmes  > > wrote:
> > 
> > Hi Igor,
> > 
> > On 22/08/2018 9:04 AM, Igor Ignatyev wrote:
> >> http://cr.openjdk.java.net/~iignatyev//8209611/webrev.02/index.html 
> >>  
> >>  >> > 
> >> is a new version of patch, which moves only vmTestbase tests.
> > 
> > This seems okay in principle. I didn't verify all the mechanical changes 
> > individually.
> > 
> > make/common/TestFilesCompilation.gmk
> > 
> > I suspect you can just find all .c and .cpp files and process them 
> > together, rather than duplicate all the logic. But I'll leave that to 
> > Magnus to comment on.
> I had to duplicate this foreach cycle to specify a correct TOOLCHAIN, w/o it 
> linking fails on linux-slowdebug. 
> > 
> > make/test/JtregNativeHotspot.gmk
> > 
> > + BUILD_HOTSPOT_JTREG_LIBRARIES_CFLAGS := -D__STDC_FORMAT_MACROS 
> > -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS
> > 
> > Just a note that defining these is obsolete once we use C11/C++11 - which I 
> > think is an intent for JDK 12 if possible.
> initially I wanted to use  instead of  + 
> -D__STDC_FORMAT_MACROS, but solaris studio apparently doesn't have cinttypes 
> header file.
> 
> hotspot makefiles also use -D__STDC_, so I hope both places will be fixed 
> together.
> 
> > 
> > test/hotspot/jtreg/vmTestbase/gc/g1/unloading/libdefine.cpp
> > 
> >  #ifdef __cplusplus
> > ! #define JNI_ENV_ARG(x, y) y
> >  #define JNI_ENV_PTR(x) x
> >  #else
> >  #define JNI_ENV_ARG(x, y) x , y
> >  #define JNI_ENV_PTR(x) (*x)
> >  #endif
> > 
> > If this is now a C++ source file why do you a still have the code that 
> > allows for either C or C++? Is this stuff that will be cleaned up in the 
> > later RFE?
> yes, 8209547 will clean up JNI_ENV_ARG macros.
> 
> > 
> > ---
> > 
> > The switch to C++ means a lot of code now needs:
> > 
> > + #ifdef __cplusplus
> > + extern "C" {
> > + #endif
> 
> that's true, and this patch adds 'extern "C"' very conservatively (basically 
> to everywhere). this, if needed, can be cleaned up later.
> 
> > 
> > I still have to wonder whether it better to leave these as C tests and only 
> > convert to C++ as and when needed ... seems like so much work.
> > 
> > Anyway I'm off on vacation after today so I'll leave it to others to take 
> > this up.
> > 
> > Thanks,
> > David
> > 
> >> Thanks,
> >> -- Igor
> >>> On Aug 20, 2018, at 11:07 PM, Igor Ignatyev  >>>   >>> >> wrote:
> >>> 
> > 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 :)
> >>> sure, it will take some time for me to do that, hopefully will upload new 
> >>>

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

[Igor Ignatyev]

Hi David,

thanks for looking at the webrev and all your comments. my answers are inlined.

enjoy your vacation!

-- Igor 

> On Aug 21, 2018, at 9:28 PM, David Holmes  wrote:
> 
> Hi Igor,
> 
> On 22/08/2018 9:04 AM, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev//8209611/webrev.02/index.html 
>>  is a 
>> new version of patch, which moves only vmTestbase tests.
> 
> This seems okay in principle. I didn't verify all the mechanical changes 
> individually.
> 
> make/common/TestFilesCompilation.gmk
> 
> I suspect you can just find all .c and .cpp files and process them together, 
> rather than duplicate all the logic. But I'll leave that to Magnus to comment 
> on.
I had to duplicate this foreach cycle to specify a correct TOOLCHAIN, w/o it 
linking fails on linux-slowdebug. 
> 
> make/test/JtregNativeHotspot.gmk
> 
> + BUILD_HOTSPOT_JTREG_LIBRARIES_CFLAGS := -D__STDC_FORMAT_MACROS 
> -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS
> 
> Just a note that defining these is obsolete once we use C11/C++11 - which I 
> think is an intent for JDK 12 if possible.
initially I wanted to use  instead of  + 
-D__STDC_FORMAT_MACROS, but solaris studio apparently doesn't have cinttypes 
header file.

hotspot makefiles also use -D__STDC_, so I hope both places will be fixed 
together.

> 
> test/hotspot/jtreg/vmTestbase/gc/g1/unloading/libdefine.cpp
> 
>  #ifdef __cplusplus
> ! #define JNI_ENV_ARG(x, y) y
>  #define JNI_ENV_PTR(x) x
>  #else
>  #define JNI_ENV_ARG(x, y) x , y
>  #define JNI_ENV_PTR(x) (*x)
>  #endif
> 
> If this is now a C++ source file why do you a still have the code that allows 
> for either C or C++? Is this stuff that will be cleaned up in the later RFE?
yes, 8209547 will clean up JNI_ENV_ARG macros.

> 
> ---
> 
> The switch to C++ means a lot of code now needs:
> 
> + #ifdef __cplusplus
> + extern "C" {
> + #endif

that's true, and this patch adds 'extern "C"' very conservatively (basically to 
everywhere). this, if needed, can be cleaned up later.

> 
> I still have to wonder whether it better to leave these as C tests and only 
> convert to C++ as and when needed ... seems like so much work.
> 
> Anyway I'm off on vacation after today so I'll leave it to others to take 
> this up.
> 
> Thanks,
> David
> 
>> Thanks,
>> -- Igor
>>> On Aug 20, 2018, at 11:07 PM, Igor Ignatyev >> > wrote:
>>> 
> 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 :)
>>> sure, it will take some time for me to do that, hopefully will upload new 
>>> webrevs tomorrow morning PT. but the basic idea is to leave files in 
>>> test/hotspot/jtreg/compiler, runtime, gc, native_sanity, serviceability, 
>>> testlibrary as .c files, exactly as they were before, and restore 
>>> corresponding filenames in make/test/JtregNativeHotspot.gmk.

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

[David Holmes]

Hi Igor,

On 22/08/2018 9:04 AM, Igor Ignatyev wrote:
http://cr.openjdk.java.net/~iignatyev//8209611/webrev.02/index.html 
 is 
a new version of patch, which moves only vmTestbase tests.


This seems okay in principle. I didn't verify all the mechanical changes 
individually.


make/common/TestFilesCompilation.gmk

I suspect you can just find all .c and .cpp files and process them 
together, rather than duplicate all the logic. But I'll leave that to 
Magnus to comment on.


make/test/JtregNativeHotspot.gmk

+ BUILD_HOTSPOT_JTREG_LIBRARIES_CFLAGS := -D__STDC_FORMAT_MACROS 
-D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS


Just a note that defining these is obsolete once we use C11/C++11 - 
which I think is an intent for JDK 12 if possible.


---

 test/hotspot/jtreg/vmTestbase/gc/g1/unloading/libdefine.cpp

  #ifdef __cplusplus
! #define JNI_ENV_ARG(x, y) y
  #define JNI_ENV_PTR(x) x
  #else
  #define JNI_ENV_ARG(x, y) x , y
  #define JNI_ENV_PTR(x) (*x)
  #endif

If this is now a C++ source file why do you a still have the code that 
allows for either C or C++? Is this stuff that will be cleaned up in the 
later RFE?


---

The switch to C++ means a lot of code now needs:

+ #ifdef __cplusplus
+ extern "C" {
+ #endif

I still have to wonder whether it better to leave these as C tests and 
only convert to C++ as and when needed ... seems like so much work.


Anyway I'm off on vacation after today so I'll leave it to others to 
take this up.


Thanks,
David


Thanks,
-- Igor

On Aug 20, 2018, at 11:07 PM, Igor Ignatyev > wrote:


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 :)
sure, it will take some time for me to do that, hopefully will upload 
new webrevs tomorrow morning PT. but the basic idea is to leave files 
in test/hotspot/jtreg/compiler, runtime, gc, native_sanity, 
serviceability, testlibrary as .c files, exactly as they were before, 
and restore corresponding filenames in make/test/JtregNativeHotspot.gmk.

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

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8209611/webrev.02/index.html 
 is a new 
version of patch, which moves only vmTestbase tests.

Thanks,
-- Igor 

> On Aug 20, 2018, at 11:07 PM, Igor Ignatyev  wrote:
> 
>>> 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 :)
> sure, it will take some time for me to do that, hopefully will upload new 
> webrevs tomorrow morning PT. but the basic idea is to leave files in 
> test/hotspot/jtreg/compiler, runtime, gc, native_sanity, serviceability, 
> testlibrary as .c files, exactly as they were before, and restore 
> corresponding filenames in make/test/JtregNativeHotspot.gmk.

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

[Igor Ignatev]

Because we would like to clean them up, which includes , but not limited to, 
removing JNI_ENV* and the like macros, other instances of ifndef __cplusplus. 

— Igor

> On Aug 20, 2018, at 11:39 PM, David Holmes  wrote:
> 
> Okay, but why do we need to change any of these existing tests to be compiled 
> as C++?

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

[David Holmes]

On 21/08/2018 4:07 PM, Igor Ignatyev wrote:

On Aug 20, 2018, at 10:06 PM, David Holmes  wrote:

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++.


currently our build system supports only 1-1 relation b/w tests lib/exec and 
source file; most of native libraries in vmTestbase have more than one source 
file. to work that around, we introduced a .c file for each library and theses 
.c files include all other required .c files.

yes, a .c file will be compiled as C and a .cpp file as C++, but as we have .c 
files which include .c files, moving only part of .c files, we will end up w/ 
.c files being included into both .c and .cpp files and thus compiled as both C 
and C++. let's take 
test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS301/hs301t004, if 
we want its native part to be compiled as C++, we rename libhs301t004.c to 
libhs301t004.cpp, but libhs301t004.c includes 11 other files, there only one is 
a test specific fill and all the other are used by other tests and included by 
their .c files.  so we have 10 files which now will be compiled by both C and 
C++ compilers.


Okay, but why do we need to change any of these existing tests to be 
compiled as C++?


David
--




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.


agree, but if we want to verify how C/C++ linked libraries work w/ hotspot, it 
should be done by the tests specifically written for that purpose rather that 
relaying on indirect coverage from other tests, and I guess it hasn't been done 
as we always relied on JCK to test that.




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 :)

sure, it will take some time for me to do that, hopefully will upload new 
webrevs tomorrow morning PT. but the basic idea is to leave files in 
test/hotspot/jtreg/compiler, runtime, gc, native_sanity, serviceability, 
testlibrary as .c files, exactly as they were before, and restore corresponding 
filenames in make/test/JtregNativeHotspot.gmk.

Cheers,
-- Igor



Thanks,
David


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 

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

[Igor Ignatyev]

> On Aug 20, 2018, at 10:06 PM, David Holmes  wrote:
> 
> 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++.

currently our build system supports only 1-1 relation b/w tests lib/exec and 
source file; most of native libraries in vmTestbase have more than one source 
file. to work that around, we introduced a .c file for each library and theses 
.c files include all other required .c files.

yes, a .c file will be compiled as C and a .cpp file as C++, but as we have .c 
files which include .c files, moving only part of .c files, we will end up w/ 
.c files being included into both .c and .cpp files and thus compiled as both C 
and C++. let's take 
test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS301/hs301t004, if 
we want its native part to be compiled as C++, we rename libhs301t004.c to 
libhs301t004.cpp, but libhs301t004.c includes 11 other files, there only one is 
a test specific fill and all the other are used by other tests and included by 
their .c files.  so we have 10 files which now will be compiled by both C and 
C++ 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.
> 
> Sure but JCK comes into play much later and I'd rather spot issues during 
> developer testing than promotion testing.

agree, but if we want to verify how C/C++ linked libraries work w/ hotspot, it 
should be done by the tests specifically written for that purpose rather that 
relaying on indirect coverage from other tests, and I guess it hasn't been done 
as we always relied on JCK to test that.

> 
>> 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 :)
sure, it will take some time for me to do that, hopefully will upload new 
webrevs tomorrow morning PT. but the basic idea is to leave files in 
test/hotspot/jtreg/compiler, runtime, gc, native_sanity, serviceability, 
testlibrary as .c files, exactly as they were before, and restore corresponding 
filenames in make/test/JtregNativeHotspot.gmk.

Cheers,
-- Igor

> 
> 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.j

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

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