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

Mon Jul 18 09:06:00 UTC 2016

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