Hi Magnus, thanks for reviewing this.
please see my answers inline. Cheers, -- Igor > On Aug 28, 2018, at 4:14 AM, Magnus Ihse Bursie > <magnus.ihse.bur...@oracle.com> 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 <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 >
