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
> 

Reply via email to