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 <igor.ignat...@oracle.com> 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 <erik.joels...@oracle.com> 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

Reply via email to