Omr: Apply consistent use of OS macros in compiler

Created on 24 Jan 2018  路  6Comments  路  Source: eclipse/omr

The OMR compiler provides a number of macros to standardize operating system defines. The use of these should be made consistent throughout the compiler codebase (they are, for the most part).

For example, #if defined(LINUX) should be #if (HOST_OS == OMR_LINUX).

The current set of macros is defined in compiler/env/defines.h

#  define OMR_LINUX   201
#  define OMR_WINDOWS 202
#  define OMR_AIX     203
#  define OMR_ZOS     204
#  define OMR_OSX     205
architecture review pending backlog compiler good first issue help wanted

Most helpful comment

While valuable for consistency in the compiler, I will broaden the scope of this issue to include all of OMR. This will be discussed at the next OMR Architecture Meeting (#4425).

All 6 comments

I'd like to work on this item. After taking a look, I have two questions:

  • Should the above OS macros change when they're used in a larger conditional including platforms (e.g. LINUXPPC)?
  • Should OS macros the .inc and other non c/cpp/h files change?

@0xdaryl Do we need to change OS macros wherever they are used ,e.g. LINUXPPC? as there was no mention about it in compiler/env/defines.h

This seems like a good time to apply consistent use of OS macros in entire OMR. Grepping for #if def in OMR root directory I see the following (cherry picked examples since we got lots of irrelevant hits):

include_core/thread_api.h:#if defined(J9ZOS390)
include_core/thrtypes.h:#if defined(OMR_OS_WINDOWS)
include_core/thrtypes.h:#if defined(LINUX)
include_core/unix/thrdsup.h:#if defined(LINUX) && defined(J9X86)
include_core/unix/thrdsup.h:#if defined(MVS) || defined (J9ZOS390)
include_core/ute_module.h:#if defined(LINUX) || defined(OSX)
jitbuilder/runtime/JBCodeCacheManager.cpp:#if defined(OMR_OS_WINDOWS)
jitbuilder/runtime/JBCodeCacheManager.cpp:   #if defined(__APPLE__)

The list goes on and on and on. These macros already seem to be defined outside of the compiler component, so I don't think we should be redefining them internally in compiler/env/defines.h as noted. We should just use the same ones the rest of OMR defines.

@0xdaryl @Avinash2198 can this work be extended to the entire repository and not limited to the compiler component? @charliegracie for any input.

There are also things like [1] which we should address. No reason why we should not have OMR_OS_ZOS rather than using J9ZOS390 throughout the codebase.

[1] https://github.com/eclipse/omr/blob/b8ed7176897ffa6199bb5cbff970a15717dccc9f/compiler/env/FEBase_t.hpp#L68-L75

While valuable for consistency in the compiler, I will broaden the scope of this issue to include all of OMR. This will be discussed at the next OMR Architecture Meeting (#4425).

Is there any reason why the compiler uses the if-equal style, as in #if (HOST_OS == OMR_LINUX), rather than an if-def style, as in #if defined(OMR_LINUX)? It seems to me that ifdefs are the more conventional approach, and are usually a fair bit more concise.

I don't know of any valid reason. We should change them to all be consistent, whatever that may be. No preference from my side.

Was this page helpful?
0 / 5 - 0 ratings