Sure, will change the before I push. I considered that but didn’t run with it.
Cheers, Henry > On Apr 6, 2020, at 7:43 PM, David Holmes <david.hol...@oracle.com> wrote: > > Hi Henry, > > On 7/04/2020 3:36 am, Henry Jen wrote: >> Here is the combined webrev[1] which I think is what we should have. By >> using __solaris__, we make sure no extra define from build and assuming that >> all solaris build will have gethrtime() and use that. > > This looks good to me and means the Solaris code should start working > correctly again, as well as providing an implementation on all other > platforms. > > Only minor suggestion is to move > > 33 #include <sys/time.h> > > outside of the ifdef in the header file as all platforms will include it > anyway. You can then also remove the include in the .c file > > 819 #include <sys/time.h> > > Thanks, > David > ----- > >> The current build only use that for static build launcher, not custom >> launcher use libjli. >> Cheers, >> Henry >> [1] http://cr.openjdk.java.net/~henryjen/jdk/8241638.2/webrev/ >>> On Apr 5, 2020, at 9:21 PM, David Holmes <david.hol...@oracle.com> wrote: >>> >>> On 6/04/2020 12:37 pm, David Holmes wrote: >>>> On 6/04/2020 12:20 pm, Henry Jen wrote: >>>>> On Apr 5, 2020, at 7:15 PM, David Holmes <david.hol...@oracle.com> wrote: >>>>>> >>>>>> On 6/04/2020 11:50 am, David Holmes wrote: >>>>>>> There is something not right here ... >>>>>>> On 4/04/2020 3:13 pm, Henry Jen wrote: >>>>>>>> Internal test shows that inline implementation is not working for some >>>>>>>> Solaris artifacts, because the -DHAVE_GETHRTIME is not consistently >>>>>>>> defined, so it is actually broken. :) >>>>>>>> >>>>>>>>> [2020-04-03T15:59:26,981Z] Creating >>>>>>>>> support/test/hotspot/jtreg/native/bin/jvm-test-launcher from 1 file(s) >>>>>>> Are you sure that line actually pertains to any error? The test defines >>>>>>> a custom launcher which doesn't use libjli so should never be including >>>>>>> the header file we are discussing. >>>>>>>>> [2020-04-03T16:02:10,984Z] >>>>>>>>> [2020-04-03T16:02:10,984Z] ERROR: Build failed for target 'default >>>>>>>>> (product-bundles test-bundles static-libs-bundles)' in configuration >>>>>>>>> 'solaris-sparcv9-open' (exit code 2) >>>>>>>>> [2020-04-03T16:02:11,051Z] >>>>>>>>> [2020-04-03T16:02:11,051Z] === Output from failing command(s) >>>>>>>>> repeated here === >>>>>>>>> [2020-04-03T16:02:11,055Z] * For target >>>>>>>>> support_native_java.base_libjli_BUILD_LIBJLI_link: >>>>>>>>> [2020-04-03T16:02:11,061Z] Undefined first referenced >>>>>>>>> [2020-04-03T16:02:11,061Z] symbol in file >>>>>>>>> [2020-04-03T16:02:11,061Z] getTimeMicros >>>>>>>>> /export/home/opt/mach5/mesos/work_dir/e101deec-9613-4d46-a64d-559e689496aa/workspace/build/solaris-sparcv9-open/support/native/java.base/libjli/java.o >>>>>>>>> [2020-04-03T16:02:11,061Z] ld: fatal: symbol referencing errors >>>>>>> This looks like a direct linkage error. AFAICS >>>>>>> ./launcher/LauncherCommon.gmk only defines -DHAVE_GETHRTIME for >>>>>>> launcher executables. But if we are building libjli it is not an >>>>>>> executable. I'm suspecting there is actually a long standing build bug >>>>>>> here from when libjli was introduced. Possibly only evident on an >>>>>>> incremental build. >>>>>> >>>>>> I can confirm that the flags set in LauncherCommon.gmk are not passed to >>>>>> the compilation of java_md_solinux.c (I added a custom -DDAVIDH to the >>>>>> linux flags and checked the build). So I have no idea how this has been >>>>>> working, if indeed it actually has. >>>>>> >>>>> >>>>> I should say it’s the inconsistency for building java.c for launcher >>>>> executable and libjli.so, thus cause libjli.so failed to build. It wasn’t >>>>> detected before because CounterGet is defined as no-op, so nothing to >>>>> link with. >>>> My point is that this seems completely broken. HAVE_GETHRTIME is never >>>> defined when building libjli, only when building the launcher executables, >>>> and the launcher executable code never calls CounterGet(). This suggests >>>> that the launcher debug code on Solaris has been using the same code as >>>> linux and thus should be seen to be reporting the same thing ... which it >>>> is ... >>>> _JAVA_LAUNCHER_DEBUG=true >>>> /java/re/jdk/9/promoted/latest/binaries/solaris-x64/bin/java -version >>>> ----_JAVA_LAUNCHER_DEBUG---- >>>> Launcher state: >>>> First application arg index: -1 >>>> debug:on >>>> javargs:off >>>> program name:java >>>> launcher name:java >>>> javaw:off >>>> fullversion:9+181 >>>> Command line args: >>>> argv[0] = /java/re/jdk/9/promoted/latest/binaries/solaris-x64/bin/java >>>> argv[1] = -version >>>> JRE path is .../java_re/jdk/9/fcs/181/binaries/solaris-x64 >>>> jvm.cfg[0] = ->-server<- >>>> jvm.cfg[1] = ->-client<- >>>> 1 micro seconds to parse jvm.cfg >>>> Default VM: server >>>> Does >>>> `.../export/java_re/jdk/9/fcs/181/binaries/solaris-x64/lib/server/libjvm.so' >>>> exist ... yes. >>>> mustsetenv: FALSE >>>> JVM path is >>>> .../export/java_re/jdk/9/fcs/181/binaries/solaris-x64/lib/server/libjvm.so >>>> 1 micro seconds to LoadJavaVM >>>> Always reports 1 microsecond just like Linux. >>>> This whole setup is completely broken at the build level. >>> >>> This worked correctly in JDK 6 (6u181 is latest I could test) but is broken >>> in JDK 7. >>> >>> David >>> ----- >>> >>>> Cheers, >>>> David >>>>> Cheers, >>>>> Henry >>>>> >>>>> >>>>>> David >>>>>> >>>>>>> David >>>>>>> ----- >>>>>>>>> [2020-04-03T16:02:11,082Z] >>>>>>>>> [2020-04-03T16:02:11,082Z] * All command lines available in >>>>>>>>> /export/home/opt/mach5/mesos/work_dir/e101deec-9613-4d46-a64d-559e689496aa/workspace/build/solaris-sparcv9-open/make-support/failure-logs. >>>>>>>>> [2020-04-03T16:02:11,086Z] === End of repeated output === >>>>>>>>> [2020-04-03T16:02:11,094Z] >>>>>>>>> [2020-04-03T16:02:11,094Z] === Make failed targets repeated here === >>>>>>>>> [2020-04-03T16:02:11,736Z] CoreLibraries.gmk:206: recipe for target >>>>>>>>> '/export/home/opt/mach5/mesos/work_dir/e101deec-9613-4d46-a64d-559e689496aa/workspace/build/solaris-sparcv9-open/support/modules_libs/java.base/libjli.so' >>>>>>>>> failed >>>>>>>>> [2020-04-03T16:02:11,737Z] make/Main.gmk:195: recipe for target >>>>>>>>> 'java.base-libs' failed >>>>>>>>> [2020-04-03T16:02:11,739Z] === End of repeated output === >>>>>>>>> [2020-04-03T16:02:11,741Z] >>>>>>>> >>>>>>>> >>>>>>>> I verified that either move implementation into .c as a function >>>>>>>> body[1] or change to #ifdef __solaris__[2] will fix that. So I think >>>>>>>> we will change to detect __solaris__ as webrev[2] rather than have an >>>>>>>> extra #define. If some other build want to have that, they can be >>>>>>>> modify that #ifdef easily. >>>>>>>> >>>>>>>> As I look into it, I found Mac have similar implementation with minor >>>>>>>> mistake, so I fixed that as well. Please review following based on >>>>>>>> zhanglin’s patch. >>>>>>>> >>>>>>>> I’ll push [2] once I got a +1. >>>>>>>> >>>>>>>> [1] http://cr.openjdk.java.net/~henryjen/jdk/8241638.0/webrev/ >>>>>>>> [2] http://cr.openjdk.java.net/~henryjen/jdk/8241638.1/webrev/ >>>>>>>> >>>>>>>> Cheers, >>>>>>>> Henry >>>>>>>> >>>>>>>>> On Apr 2, 2020, at 6:17 AM, Alan Bateman <alan.bate...@oracle.com> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>> On 02/04/2020 11:26, linzang(臧琳) wrote: >>>>>>>>>> : >>>>>>>>>> Here is the updated webrev : >>>>>>>>>> http://cr.openjdk.java.net/~lzang/8241638/webrev04/ >>>>>>>>>> Thanks for your help! >>>>>>>>>> >>>>>>>>> webrev04 looks good. My preference was to was to replace #ifdef >>>>>>>>> HAVE_GETHRTIME with #ifdef __solaris__ but there doesn't seem to be >>>>>>>>> appetite to do this now. I think Henry has offered to help sponsor. >>>>>>>>> >>>>>>>>> -Alan.
