Hi Erik, thanks for your review!
-- Igor > On Aug 28, 2018, at 9:01 AM, Erik Joelsson <erik.joels...@oracle.com> 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 >>> <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 >
