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

[David Holmes]

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? 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 :).)

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.

---

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);

? :)

---

src/share/vm/code/nmethod.cpp

Not clear why the platform include can simply be elided here ??

---

src/share/vm/jvmci/jvmciRuntime.cpp

Shouldn't:

! #endif // !LP64

be:

! #endif // LP64

---

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?

---

src/share/vm/runtime/os.hpp

Should we convert setjmp.h include to:

#ifndef _WINDOWS
#include <setjmp.h>
#endif
#ifdef __APPLE__
...
#endif

?

BTW: why is it _WINDOWS instead of WINDOWS? Is that coming from the 
compiler itself?

---

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

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