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

Mon Jul 18 12:16:17 UTC 2016

Backing up a step ...

On 18/07/2016 10:12 PM, David Holmes wrote:
> 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)

Why do you use HOTSPOT_TARGET_CPU_DEFINE? I would have expected 
OPENJDK_TARGET_CPU. (There are still far too many "arch" related 
variables in the build :(.)

Thanks,
David

> 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.
>>>>>>>>>
>>>>>>>>> ------------------------------------------------------------------------------
>>>>>>>>>
>>>>>>>>