RFR(L): 8161259: Simplify including platform files.

Mon Jul 18 12:18:02 UTC 2016

Hi David,

I'll introduce CPU_HEADER_H, but actually this should be fixed
in the closed code.

Best regards,
  Goetz.

> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.com]
> Sent: Montag, 18. Juli 2016 14:13
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Kim Barrett
> <kim.barrett at oracle.com>
> Cc: hotspot-compiler-dev at openjdk.java.net; hotspot-runtime-
> dev at openjdk.java.net
> Subject: Re: RFR(L): 8161259: Simplify including platform files.
> 
> Hi Goetz,
> 
> On 18/07/2016 8:21 PM, Lindenmaier, Goetz wrote:
> > Hi David,
> >
> >>>> src/share/vm/prims/jni_md.h
> >>>> src/share/vm/prims/jvm.h
> >>>>
> >>>> // Can not use CPU_HEADER() macro, as it appends .hpp
> >>>>
> >>>> can we not define a second form, eg CPU_HEADER_H, that appends.h
> >>>> instead?
> >>> I'll need one for OS, one for CPU, and each will be used only once.
> >>> So I figured not to do it.  But probably I should do it to get is
> >>> similar everywhere.
> >>
> >> We actually need this as the simple arch names need not be mutually
> >> exclusive - eg arm and aarch64. Otherwise this needs to be an if-else
> >> construct in the correct order. ;-)
> > They should be mutually exclusive, as they are set in CompileJvm.gmk
> > in the same statement as the INCLUDE_SUFFIX_CPU:
> > -D$(HOTSPOT_TARGET_CPU_DEFINE)
> 
> Unfortunately we also have -DARM coming in from the closed part of
> spec.gmk as we don't use AARCH64 for our port. So we get both defined
> and try to include two platform files.
> 
> Not sure how to try and resolve this yet. Trying to understand what the
> role of HOTSPOT_TARGET_CPU_DEFINE is intended to be, as we are not
> overriding it for our port - which I think is the current problem, but
> changing it may break something else.
> 
> Thanks,
> David
> 
> 
> > Best regards,
> >   Goetz
> >
> >
> >
> >> -----Original Message-----
> >> From: David Holmes [mailto:david.holmes at oracle.com]
> >> Sent: Montag, 18. Juli 2016 11:06
> >> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Kim Barrett
> >> <kim.barrett at oracle.com>
> >> Cc: hotspot-compiler-dev at openjdk.java.net; hotspot-runtime-
> >> dev at openjdk.java.net
> >> Subject: Re: RFR(L): 8161259: Simplify including platform files.
> >>
> >> On 18/07/2016 5:15 PM, Lindenmaier, Goetz wrote:
> >>> Hi David,
> >>>
> >>> thanks for looking at all these files in more detail!
> >>>
> >>>> -----Original Message-----
> >>>> From: David Holmes [mailto:david.holmes at oracle.com]
> >>>> Sent: Montag, 18. Juli 2016 08:03
> >>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Kim Barrett
> >>>> <kim.barrett at oracle.com>
> >>>> Cc: hotspot-compiler-dev at openjdk.java.net; hotspot-runtime-
> >>>> dev at openjdk.java.net
> >>>> Subject: Re: RFR(L): 8161259: Simplify including platform files.
> >>>>
> >>>> Hi Goetz,
> >>>>
> >>>> Again thanks for tackling this!
> >>>>
> >>>> On 15/07/2016 10:17 PM, Lindenmaier, Goetz wrote:
> >>>>> Hi,
> >>>>>
> >>>>> I made a new webrev:
> >>>>>  - '_' is added in makefile
> >>>>>  - uses Kim's macros
> >>>>>  - macros are capitalized
> >>>>>  - more comments in macros.hpp
> >>>>> http://cr.openjdk.java.net/~goetz/wr16/8161259-
> >> newPfmIncl/webrev.02/
> >>>>
> >>>> Generally looks okay, a couple of clarifications and comments below.
> >>>>
> >>>>
> >>>> make/gensrc/GensrcAdlc.gmk
> >>>>
> >>>> So the change from HOTSPOT_TARGET_CPU to
> >>>> HOTSPOT_TARGET_CPU_ARCH only
> >>>> affects the generated files right?
> >>> Yes.
> >>>> For our closed ports we still have
> >>>> the _32/_64 source files in a common arch directory, but I don't think
> >>>> that needs to be reflected in the generated files. (Sorry I haven't had
> >>>> the time yet to apply this patch and see what needs to be changed on
> the
> >>>> closed side - but will start that once I send this :).)
> >>> If I run plain configure, it generates a directory that has the word size
> >>> in it's name:
> >>>     linux-x86_64-normal-server-release
> >>> If I configure --with-target-bits=32, it builds to
> >>>     linux-x86-normal-server-release,
> >>> so I figured this should be fine.
> >>
> >> Yes generated files are fine.
> >>
> >>>> make/lib/CompileJvm.gmk
> >>>>
> >>>> Hmm but this change assumes no more _32/_64 header files if I read it
> >>>> right ?? So I'll need a common file that dispatches internally for
> >>>> 32-bit and 64-bit.
> >>> Yes.
> >>> There is a single file in cpu/x86 where this did not hold: stubRoutines
> >>> But this was rather small. And there anyways was a common file
> >>> that was included in both.
> >>>
> >>>> src/os/posix/vm/os_posix.cpp
> >>>>
> >>>> I'm wondering if we can use a similar trick to avoid this kind of
> >>>> switch(OS) statement:
> >>>>
> >>>>   #ifdef TARGET_OS_FAMILY_linux
> >>>>      Linux::ucontext_set_pc(ctx, pc);
> >>>>   #elif defined(TARGET_OS_FAMILY_solaris)
> >>>>      Solaris::ucontext_set_pc(ctx, pc);
> >>>>   ...
> >>>>
> >>>> ie something like:
> >>>>
> >>>>    SUB(TARGET_OS)::ucontext_set_pc(ctx, pc);
> >>>>
> >>>> ? :)
> >>> Hmm, I had to add the '_' to the string in TARGET_SO...
> >>> Actually I think the implementation should be moved to
> >>> os_linux.cpp/os_bsd.cpp etc.
> >>>
> >>>>
> >>>> ---
> >>>>
> >>>> src/share/vm/code/nmethod.cpp
> >>>>
> >>>> Not clear why the platform include can simply be elided here ??
> >>> It already includes code/nativeInst.hpp, which is the umbrella header
> >>> for all the platform files.
> >>>
> >>>> src/share/vm/jvmci/jvmciRuntime.cpp
> >>>>
> >>>> Shouldn't:
> >>>>
> >>>> ! #endif // !LP64
> >>>>
> >>>> be:
> >>>>
> >>>> ! #endif // LP64
> >>> Well, it ends the #else part ... but fixed anyways.
> >>>
> >>>>
> >>>> ---
> >>>>
> >>>> src/share/vm/prims/jni_md.h
> >>>> src/share/vm/prims/jvm.h
> >>>>
> >>>> // Can not use CPU_HEADER() macro, as it appends .hpp
> >>>>
> >>>> can we not define a second form, eg CPU_HEADER_H, that appends.h
> >>>> instead?
> >>> I'll need one for OS, one for CPU, and each will be used only once.
> >>> So I figured not to do it.  But probably I should do it to get is
> >>> similar everywhere.
> >>
> >> We actually need this as the simple arch names need not be mutually
> >> exclusive - eg arm and aarch64. Otherwise this needs to be an if-else
> >> construct in the correct order. ;-)
> >>
> >> Thanks,
> >> David
> >> -----
> >>
> >>>>
> >>>> ---
> >>>>
> >>>> src/share/vm/runtime/os.hpp
> >>>>
> >>>> Should we convert setjmp.h include to:
> >>>>
> >>>> #ifndef _WINDOWS
> >>>> #include <setjmp.h>
> >>>> #endif
> >>>> #ifdef __APPLE__
> >>>> ...
> >>>> #endif
> >>>> ?
> >>> Fixed.
> >>>
> >>>
> >>>>
> >>>> BTW: why is it _WINDOWS instead of WINDOWS? Is that coming from
> the
> >>>> compiler itself?
> >>> I wondered about that, too. Therefore I defined WINDOWS in the
> >>> prototype webrev. Then I saw that our build is defining -D_WINDOWS
> >> *and* -DWIN32,
> >>> and removed it again using _WINDOWS instead.
> >>> Eventually both should be replaced by WINDOWS, but not in this change.
> >>>
> >>>
> >>>> ---
> >>>>
> >>>> src/os_cpu/aix_ppc/vm/bytes_aix_ppc.inline.hpp
> >>>>
> >>>> Don't understand the point of this:
> >>>>
> >>>>    28 #if defined(VM_LITTLE_ENDIAN)
> >>>>    29 // Aix is not littel endian.
> >>>>    30 #endif // VM_LITTLE_ENDIAN
> >>>>
> >>>> also typo: littel
> >>> I just copied the linux_ppc code and removed the unused
> implementation.
> >>> I wanted to document why there is this empty file.
> >>>
> >>> Best regards,
> >>>   Goetz.
> >>>
> >>>
> >>>
> >>>> Thanks,
> >>>> David
> >>>> -----
> >>>>
> >>>>> Best regards,
> >>>>>   Goetz.
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: David Holmes [mailto:david.holmes at oracle.com]
> >>>>>> Sent: Freitag, 15. Juli 2016 11:11
> >>>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Kim Barrett
> >>>>>> <kim.barrett at oracle.com>
> >>>>>> Cc: hotspot-compiler-dev at openjdk.java.net; hotspot-runtime-
> >>>>>> dev at openjdk.java.net
> >>>>>> Subject: Re: RFR(L): 8161259: Simplify including platform files.
> >>>>>>
> >>>>>> On 15/07/2016 5:30 PM, Lindenmaier, Goetz wrote:
> >>>>>>> HI Kim,
> >>>>>>>
> >>>>>>> thanks for this version of the macros, it's working on all the
> platforms
> >>>>>>> I can build.
> >>>>>>>
> >>>>>>> Actually, I would prefer having the '_' in the macros and not on the
> >>>>>>> command line. This way, parts of the name construction are in
> >>>>>>> CompileJvm.gmk, other parts are in macros.hpp.  But constructing
> >>>>>>> the search path is also in the makefile, and uses the very same
> string
> >>>>>>> as the file suffixes. (Without '_', one could include that in the
> macro,
> >>>> too.)
> >>>>>>>
> >>>>>>> But overall, I consider this a detail and am as fine with your solution
> >>>>>>> as with the SUB() macros.  What is better is that the linux=1 etc
> >>>>>>> problems are avoided.
> >>>>>>>
> >>>>>>> Any more opinions whether the macros should be upper case?
> >>>>>>
> >>>>>> Yeah they probably should be.
> >>>>>>
> >>>>>> Thanks,
> >>>>>> David
> >>>>>>
> >>>>>>> Best regards,
> >>>>>>>   Goetz.
> >>>>>>>
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Kim Barrett [mailto:kim.barrett at oracle.com]
> >>>>>>>> Sent: Donnerstag, 14. Juli 2016 23:20
> >>>>>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> >>>>>>>> Cc: Andrew Haley <aph at redhat.com>; hotspot-compiler-
> >>>>>>>> dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net
> >>>>>>>> Subject: Re: RFR(L): 8161259: Simplify including platform files.
> >>>>>>>>
> >>>>>>>>> On Jul 14, 2016, at 3:18 PM, Lindenmaier, Goetz
> >>>>>>>> <goetz.lindenmaier at sap.com> wrote:
> >>>>>>>>>
> >>>>>>>>> Hi everybody,
> >>>>>>>>>
> >>>>>>>>> please take into account that these macros are only used within
> 20
> >>>> lines
> >>>>>> of
> >>>>>>>>> the file macro.hpp.  The code everybody needs to understand is
> >>>>>>>>>    #include cpu_header(bytes)
> >>>>>>>>> which, in this example, is in file bytes.hpp and expands to
> >>>>>>>> bytes_<cpu>.hpp.
> >>>>>>>>> There are six of these, for cpu/os/os_cpu and .hpp/.inline.hpp.
> >>>>>>>>>
> >>>>>>>>> I really would appreciate if I don't have to spend days editing the
> >>>>>>>>> 12 lines that use SUB().
> >>>>>>>>>
> >>>>>>>>> @Kim
> >>>>>>>>> I'm working on the s390 port, and posted my current progress
> >> claiming
> >>>>>>>>> that the biggest shared change I need to do is adding the
> #includes.
> >>>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2016-
> >>>>>>>> July/023782.html
> >>>>>>>>> This arose the discussion about the includes.
> >>>>>>>>> Later I posted a prototype of what Volker proposed in that
> >> discussion
> >>>>>>>>> to that thread:
> >>>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2016-
> >>>>>>>> July/023934.html
> >>>>>>>>> which I now turned into a RFR.
> >>>>>>>>
> >>>>>>>> Thanks.
> >>>>>>>>
> >>>>>>>> I think I see what happened to the email thread for me; it looks
> like
> >> one
> >>>> of
> >>>>>>>> Andrew’s replies went to hotspot-compiler-dev but not hotspot-
> >>>> runtime-
> >>>>>>>> dev,
> >>>>>>>> and I was not subscribed to the compiler list this morning.
> >>>>>>>>
> >>>>>>>> I like the idea, just quibbling over details.  I've only reviewed
> >>>>>>>> macros.hpp so far; the rest looks like it can wait until the details
> >>>>>>>> are settled.
> >>>>>>>>
> >>>>>>>> ------------------------------------------------------------------------------
> >>>>>>>> src/share/vm/utilities/macros.hpp
> >>>>>>>>  470 // Helper macros to workaround existing #defines that spoil
> >>>>>>>>  471 // the macro expansion. Detected so far: linux=1, sparc=1.
> >>>>>>>>
> >>>>>>>> This issue can be dodged by making the leading underscore part
> of
> >> the
> >>>>>>>> expansion for INCLUDE_SUFFIX_*, e.g. in
> >> make/lib/CompileJvm.gmk,
> >>>> use
> >>>>>>>> instead
> >>>>>>>>
> >>>>>>>>   66 JVM_CFLAGS_TARGET_DEFINES += \
> >>>>>>>>   67     -DINCLUDE_SUFFIX_OS=_$(HOTSPOT_TARGET_OS) \
> >>>>>>>>   68     -
> DINCLUDE_SUFFIX_CPU=_$(HOTSPOT_TARGET_CPU_ARCH)
> >> \
> >>>>>>>>
> >>>>>>>> Note the insertion of leading "_" for the values.
> >>>>>>>>
> >>>>>>>> Then the whole macro block can be written as
> >>>>>>>>
> >>>>>>>> #define cpu_header_stem(basename)
> PASTE_TOKENS(basename,
> >>>>>>>> INCLUDE_SUFFIX_CPU)
> >>>>>>>> #define os_header_stem(basename) PASTE_TOKENS(basename,
> >>>>>>>> INCLUDE_SUFFIX_OS)
> >>>>>>>> #define os_cpu_header_stem(basename)
> >> PASTE_TOKENS(basename,
> >>>>>>>> PASTE_TOKENS(INCLUDE_SUFFIX_OS, INCLUDE_SUFFIX_CPU))
> >>>>>>>>
> >>>>>>>> #define cpu_header(basename)
> >>>>>> XSTR(cpu_header_stem(basename).hpp)
> >>>>>>>> #define cpu_header_inline(basename)
> >>>>>>>> XSTR(cpu_header_stem(basename).inline.hpp)
> >>>>>>>> #define os_header(basename)
> >>>> XSTR(os_header_stem(basename).hpp)
> >>>>>>>> #define os_header_inline(basename)
> >>>>>>>> XSTR(os_header_stem(basename).inline.hpp)
> >>>>>>>> #define os_cpu_header(basename)
> >>>>>>>> XSTR(os_cpu_header_stem(basename).hpp)
> >>>>>>>> #define os_cpu_header_inline(basename)
> >>>>>>>> XSTR(os_cpu_header_stem(basename).inline.hpp)
> >>>>>>>>
> >>>>>>>> And SUB is no longer used...
> >>>>>>>>
> >>>>>>>> If some future build system wants brackets instead of strings, just
> >>>>>>>> replace XSTR(...) with <...>.
> >>>>>>>>
> >>>>>>>> We lose if _linux or _sparc (for example) are defined, but that's
> true
> >>>>>>>> for the webrev.01 code too.  But note that underscore followed
> by a
> >>>>>>>> lowercase letter is not in the reserved word pattern for C/C++.
> >>>>>>>>
> >>>>>>>> BTW, my preference would be to use uppercase for the macro
> >> names.
> >>>> I
> >>>>>>>> don't know what others think about that.
> >>>>>>>>
> >>>>>>>> ------------------------------------------------------------------------------
> >>>>>>>