RFR(L): 8161259: Simplify including platform files.
David Holmes
david.holmes at oracle.com
Mon Jul 18 12:23:22 UTC 2016
On 18/07/2016 10:18 PM, Lindenmaier, Goetz wrote:
> Hi David,
>
> I'll introduce CPU_HEADER_H, but actually this should be fixed
> in the closed code.
Bear with me, I'm trying to figure out how to do just that. :)
The use of HOTSPOT_TARGET_CPU_DEFINE as the value for the ifdef is not
going to work for our closed ports, unless I change that value to match
the open naming scheme. But that in turn will lead to other problems as
we also need that value the way it is currently defined.
I'll tackle this again in the morning when I'm fresher.
Thanks,
David
> 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.
>>>>>>>>>>
>>>>>>>>>> ------------------------------------------------------------------------------
>>>>>>>>>
More information about the hotspot-runtime-dev
mailing list