Re: RFR(S) : 8238943: switch to jtreg 5.0

[Igor Ignatev]

Oh, I’m sorry I actually changed it to 5.0 when were (re)doing testing, and 
apparently forgot to replace the webrev, the right is 
http://cr.openjdk.java.net/~iignatyev//8238943/webrev.01 ; with version field 
value being the only difference b/w .00 and .01

Thanks,
— Igor

> On Feb 13, 2020, at 9:23 AM, Erik Joelsson  wrote:
> 
> Looks good, but could you change the "version" field to "5.0", it should 
> work now.
> 
> /Erik
> 
>>> On 2020-02-13 08:50, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev//8238943/webrev.00
>>> 10 lines changed: 1 ins; 0 del; 9 mod;
>> Hi all,
>> could you please review the patch which changes jtreg version used in 
>> jdk/jdk to the latest and greatest -- jtreg 5.0? and as (recently became) 
>> usually, this patch also bumps requiredVersion in all the jtreg test suites 
>> in order to reduce possible discrepancy in results.
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8238943
>> webrev: http://cr.openjdk.java.net/~iignatyev/8238943/webrev.00
>> testing: tier1-4
>> Thanks,
>> -- Igor

Re: [13] RFR (S): 8217404: --with-jvm-features doesn't work when multiple features are explicitly disabled

[Igor Ignatev]

Still looks good to me. 

— Igor

> On Jan 18, 2019, at 5:26 PM, Vladimir Ivanov  
> wrote:
> 
> Updated webrev:
>  http://cr.openjdk.java.net/~vlivanov/8217404/webrev.01
> 
> Verified that it works as expected on Linux, Windows, MacOS, and Solaris.
> 
> Best regards,
> Vladimir Ivanov
> 
>> On 18/01/2019 16:39, Vladimir Ivanov wrote:
>> Thanks, Igor.
>>> overall your fix looks reasonable, but w/ it we can get unintentionally 
>>> disabled features (b/c grep doesn't do full word match). although this 
>>> problem wasn't really introduced by your fix, I think it's be better to fix 
>>> it as a part of your patch. I see two possible solutions:
>> I was aware of such drawback, but decided to leave it as is, since it 
>> doesn't affect existing features.
>>>   -  add "-w" to grep, but I am not sure if "-w" is supported by all grep 
>>> implementations
>>>   - use $XARGS instead of $ECHO when we get DISABLE_X. in this case you 
>>> will need to revert your changes in 'if test ...' lines
>> I'm in favor of using "-w" and I see different grep flags being used 
>> already, but would like somebody from Build team confirm they are OK with 
>> such solution.
>> Best regards,
>> Vladimir Ivanov
 On Jan 18, 2019, at 3:33 PM, Vladimir Ivanov 
  wrote:
 
 http://cr.openjdk.java.net/~vlivanov/8217404/webrev.00/
 https://bugs.openjdk.java.net/browse/JDK-8217404
 
 --with-jvm-features doesn't work properly when multiple features are 
 explicitly disabled:
 
 $ bash configure --with-jvm-features="-aot -jvmci -graal"
 ...
 checking if jvmci module jdk.internal.vm.ci should be built... yes
 checking if graal module jdk.internal.vm.compiler should be built... yes
 checking if aot should be enabled... yes
 ...
 
 The problem in the following code:
 
   DISABLE_AOT=`$ECHO $DISABLED_JVM_FEATURES | $GREP aot`
   if test "x$DISABLE_AOT" = "xaot"; then
 ENABLE_AOT="false"
   fi
 
 Since DISABLED_JVM_FEATURES ("aot jvmci graal") contains the list of 
 explicitly disabled features, grep over it returns the whole list when 
 there's a match. The subsequent check fails because there's no exact 
 match, though DISABLE_AOT contains "aot" .
 
 Proposed fix is to check there's no match instead.
 
 After the fix it works as expected:
 
 $ bash configure --with-jvm-features="-aot -jvmci -graal"
 ...
 checking if jvmci module jdk.internal.vm.ci should be built... no, forced
 checking if graal module jdk.internal.vm.compiler should be built... no, 
 forced
 checking if aot should be enabled... no, forced
 ...
 
 (The fix doesn't address the case when one feature has a name which is a 
 proper substring of another feature, but there are no such cases at the 
 moment.)
 
 Best regards,
 Vladimir Ivanov
>>> 

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: 8209520: Build fails when native code coverage is enabled

[Igor Ignatev]

Hi Leonid,

We have never supported native code coverage builds with warnings enabled as 
errors. There are bugs in gcc which cause false positive warnings, so it was 
decided to ignore all warnings from instrumented builds. It’d be much better 
and reliable to fix makefiles to always use ‘disable-warning-as-errors’ when 
‘enable-native-coverage’ is used. It should be pretty straightforward to do.

cc’ing build alias.  

Cheers,
— Igor

> On Aug 23, 2018, at 2:37 PM, Vladimir Kozlov  
> wrote:
> 
> macroassembler changes are good.
> 
> Thanks,
> Vladimir
> 
>> On 8/23/18 1:51 PM, Leonid Mesnik wrote:
>> Hi
>> Could you please review following fix which fix code so gcc doesn't complain 
>> when JDK is build with enabled native code coverage.
>> webrev: http://cr.openjdk.java.net/~lmesnik/8209520/webrev.00/ 
>> 
>> bug: https://bugs.openjdk.java.net/browse/JDK-8209520
>> These warning appeared because of change optimization settings used for 
>> getting code coverage.
>> 1) src/hotspot/cpu/x86/macroAssembler_x86.cpp, 
>> src/hotspot/share/gc/shared/genCollectedHeap.cpp
>> gcc complained about uninitialized variables, like
>> * For target hotspot_variant-server_libjvm_objs_macroAssembler_x86.o:
>> /home/lmesnik/ws/jdk-8209520/open/src/hotspot/cpu/x86/macroAssembler_x86.cpp:
>>  In member function 'void ControlWord::print() const':
>> /home/lmesnik/ws/jdk-8209520/open/src/hotspot/cpu/x86/macroAssembler_x86.cpp:5769:11:
>>  error: 'pc' may be used uninitialized in this function 
>> [-Werror=maybe-uninitialized]
>>  printf("%04x  masks = %s, %s, %s", _value & 0x, f, rc, pc);
>>  ~~^~~~
>> /home/lmesnik/ws/jdk-8209520/open/src/hotspot/cpu/x86/macroAssembler_x86.cpp:5769:11:
>>  error: 'rc' may be used uninitialized in this function 
>> [-Werror=maybe-uninitialized]
>> So I just fixed codepath to show more explicitly that variables are 
>> initialized before usage.
>> 2) src/java.desktop/share/native/libsplashscreen/splashscreen_png.c:
>> The changes to prevent waning about clobbering in splashscreen_png.c are 
>> similar to fix in:
>> 1. JDK-8080695 
>>splashscreen_png.c compile error with gcc 4.9.2
>> The another approach would be to fix build to ignore these warnings for code 
>> coverage build. While I think it makes build system even more complicated.
>> Leonid

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

[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: [11] RFR(S) 8204113: Upgrade linker used in AOT tests to be same version as build toolchain

[Igor Ignatev]

Hi Vladimir,

The fix looks good to me. 

— Igor

> On Jun 11, 2018, at 6:15 PM, Vladimir Kozlov  
> wrote:
> 
> http://cr.openjdk.java.net/~kvn/8204113/webrev.01/
> https://bugs.openjdk.java.net/browse/JDK-8204113
> 
> AOT tests need a local linker to generate AOT libraries. They load devkits 
> from our infrastructure when there is no local linker on testing machine. The 
> names of devkits are hardcoded in AotCompiler.java (java wrapper which 
> launches 'jaotc' tool).
> 
> We recently updated compilers for JDK 11. As result devkit names in 
> AotCompiler.java should be updated too.
> 
> Tested by running AOT tests on all our supported platforms.
> 
> -- 
> Thanks,
> Vladimir

Re: RFR(L) : 8199375 : [TESTBUG] Open source vm testbase monitoring tests

[Igor Ignatev]

Vladimir,

Tests are listed only in _quick test group b/c it doesn’t include all tests 
from the directory. We use this group in some of our test configurations, and 
:vmTestbase_nsk_monitoring in others. vmTestbase_nsk_monitoring is defined by 
the directory as other groups.

Thanks,
— Igor

> On May 1, 2018, at 7:54 PM, Vladimir Kozlov  
> wrote:
> 
> Igor,
> 
> Why you need to list each test in TEST.groups and not just directory as we do 
> in other cases?
> 
> Thanks,
> Vladimir
> 
>> On 5/1/18 7:10 PM, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev//8199375/webrev.00/index.html
>>> 41276 lines changed: 41274 ins; 1 del; 1 mod;
>> Hi all,
>> could you please review the patch which open sources monitoring tests from 
>> vm testbase?
>> The tests were developed to test hotspot related JMX functionality. as w/ 
>> common VM testbase code, these tests are old, they have been run from 
>> hotspot testing for a long period of time. originally, these tests were run 
>> by a test harness different from jtreg and had different build and execution 
>> schemes, some parts couldn't be easily translated to jtreg, so tests might 
>> have actions or pieces of code which look weird. in a long term, we are 
>> planning to rework them.
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8199375
>> webrev:  http://cr.openjdk.java.net/~iignatyev/8199375/webrev.00/index.html
>> testing: vmTestbase_nsk_monitoring test group
>> Thanks,
>> -- Igor