> On May 5, 2020, at 4:48 PM, serguei.spit...@oracle.com wrote:
>
> Hi Mikael,
>
> The fixes in webrev look good to me.
>
> I've just noticed a couple of more serviceability related things can be
> missed.
> (Not sure if they are included into different chunk of fixes.)
>
> One is libjvm_db.so which is for Solaris Pstack support:
> make/hotspot/lib/CompileDtraceLibraries.gmk: # Note that libjvm_db.c has
> tests for COMPILER2, but this was never set by
> make/hotspot/lib/CompileDtraceLibraries.gmk: LIBJVM_DB_OUTPUTDIR :=
> $(JVM_VARIANT_OUTPUTDIR)/libjvm_db
> make/hotspot/lib/CompileDtraceLibraries.gmk: NAME := jvm_db, \
> make/hotspot/lib/CompileDtraceLibraries.gmk: SRC :=
> $(TOPDIR)/src/java.base/solaris/native/libjvm_db, \
>
> Another is DTrace support which also includes jhelper.d (support for DTrace
> jstack action which is for Solaris only):
> make/hotspot/gensrc/GensrcDtrace.gmk
> make/hotspot/lib/JvmDtraceObjects.gmk
> It also impacts some other make files.
I believe you’ll find that these changes were included in the build system
patch:
https://mail.openjdk.java.net/pipermail/build-dev/2020-May/027342.html
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/build/open/webrev/
Let me know if I missed something.
> Also, these lines can be just removed:
> open/src/jdk.jdwp.agent/unix/native/libjdwp/path_md.h:#ifndef
> _JAVASOFT_SOLARIS_PATH_MD_H_
> open/src/jdk.jdwp.agent/unix/native/libjdwp/path_md.h:#define
> _JAVASOFT_SOLARIS_PATH_MD_H_
> open/src/jdk.jdwp.agent/unix/native/libjdwp/path_md.h:#endif /*
> !_JAVASOFT_SOLARIS_PATH_MD_H_ */
Remove or rather rename? The corresponding Windows header also has the guard,
any particular reason why it should be *removed*?
Cheers,
Mikael
>
> Thanks,
> Serguei
>
>
> On 5/5/20 14:34, Mikael Vidstedt wrote:
>> All good points! I deliberately chose *not* to update comments where it
>> wasn’t immediately 100% obvious exactly how to update them. For example, in
>> many cases I found that the comments are already incomplete or stale, and
>> for each such case we’ll want to consider how exactly to update the comment
>> (remove/switch to Unix/etc.). I would prefer to handle this as follow-up(s),
>> separate from the JEP. Does that sounds reasonable?
>>
>> Apart from the comments - do the changes look good? If so, can I count you
>> as a reviewer?
>>
>> Cheers,
>> Mikael
>>
>>
>>> On May 4, 2020, at 12:20 AM, serguei.spit...@oracle.com wrote:
>>>
>>> HI Mikael,
>>>
>>> Some quick comments.
>>>
>>> Some extra references to Solaris/solaris, SunOS or SPARC are listed below:
>>>
>>> src/java.instrument/unix/native/libinstrument/FileSystemSupport_md.c (2
>>> refs to Solaris/solaris)
>>> src/java.management/share/classes/javax/management/loading/MLet.java (refs
>>> to Solaris, SPARC/sparc and SunOS)
>>> src/jdk.management.agent/unix/classes/jdk/internal/agent/FileSystemImpl.java
>>> (ref to Solaris)
>>>
>>> src/hotspot/share/prims/forte.cpp:// Solaris SPARC Compiler1 needs an
>>> additional check on the grandparent
>>> src/hotspot/share/prims/forte.cpp:// on Linux X86, Solaris SPARC and
>>> Solaris X86.
>>> src/hotspot/share/prims/jvmti.xml: On the <tm>Solaris</tm> Operating
>>> Environment, an agent library is a shared
>>> src/hotspot/share/prims/jvmti.xml: <code>LD_LIBRARY_PATH</code> under the
>>> <tm>Solaris</tm> operating
>>> src/hotspot/share/prims/jvmti.xml: example, in the Java 2 SDK a
>>> CTRL-Break on Win32 and a CTRL-\ on Solaris
>>> src/hotspot/share/prims/methodHandles.cpp:#undef CS // Solaris builds
>>> complain
>>>
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>> On 5/3/20 22:12, Mikael Vidstedt wrote:
>>>> Please review this change which implements part of JEP 381:
>>>>
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
>>>> webrev:
>>>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/serviceability/open/webrev/
>>>> JEP: https://bugs.openjdk.java.net/browse/JDK-8241787
>>>>
>>>>
>>>> Note: When reviewing this, please be aware that this exercise was
>>>> *extremely* mind-numbing, so I appreciate your help reviewing all the
>>>> individual changes carefully. You may want to get that coffee cup filled
>>>> up (or whatever keeps you awake)!
>>>>
>>>>
>>>> Background:
>>>>
>>>> Because of the size of the total patch and wide range of areas touched,
>>>> this patch is one out of in total six partial patches which together make
>>>> up the necessary changes to remove the Solaris and SPARC ports. The other
>>>> patches are being sent out for review to mailing lists appropriate for the
>>>> respective areas the touch. An email will be sent to jdk-dev summarizing
>>>> all the patches/reviews. To be clear: this patch is *not* in itself
>>>> complete and stand-alone - all of the (six) patches are needed to form a
>>>> complete patch. Some changes in this patch may look wrong or incomplete
>>>> unless also looking at the corresponding changes in other areas.
>>>>
>>>> For convenience, I’m including a link below[1] to the full webrev, but in
>>>> case you have comments on changes in other areas, outside of the files
>>>> included in this thread, please provide those comments directly in the
>>>> thread on the appropriate mailing list for that area if possible.
>>>>
>>>> In case it helps, the changes were effectively produced by searching for
>>>> and updating any code mentioning “solaris", “sparc”, “solstudio”, “sunos”,
>>>> etc. More information about the areas impacted can be found in the JEP
>>>> itself.
>>>>
>>>>
>>>> Testing:
>>>>
>>>> A slightly earlier version of this change successfully passed tier1-8, as
>>>> well as client tier1-2. Additional testing will be done after the first
>>>> round of reviews has been completed.
>>>>
>>>> Cheers,
>>>> Mikael
>>>>
>>>> [1]
>>>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/
>>>>
>
