RFR: 8290840: Refactor the "os" class [v4]

Ioi Lam iklam at openjdk.org
Tue Aug 2 07:04:04 UTC 2022


On Tue, 2 Aug 2022 06:53:49 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Ioi Lam has updated the pull request incrementally with four additional commits since the last revision:
>> 
>>  - clarify comment
>>  - PLATFORM_PRINT_NATIVE_STACK -> HAVE_PLATFORM_PRINT_NATIVE_STACK
>>  - simplified rarely-defined function os::resolve_function_descriptor()
>>  - Made workaround_expand_exec_shield_cs_limit() a static function in os_linux.cpp
>
> Still good.

@tstuefe @gerard-ziemski 

I've simplified `os::resolve_function_descriptor()` according to your suggestions, but I changed the #ifdefs to be more exact:


  // Used only on PPC for AIX, or LINUX (ppc but not ppcle). 
  static void* resolve_function_descriptor(void* p)
#if defined(PPC) && (defined(AIX) || (defined(LINUX) && !defined(ABI_ELFv2)))
    ;
#else
  { return NULL; }
#endif


If we use "#if defined(PPC) && !defined(ABI_ELFv2)))" it would be very difficult to understand.

To be honest, I am not a big fan of adding #ifdefs into os.hpp just to save a few lines of code. The #ifdefs are hard to understand. They are also fragile if you need to add a new platform that also support this functionality.

I am (semi) OK with doing this for resolve_function_descriptor(), as that's a PPC-only thing that's unlikely to apply for other platforms. However, I don't want to use the same pattern for os::platform_print_native_stack() because we may want to implement this for other planforms.

-------------

PR: https://git.openjdk.org/jdk/pull/9600


More information about the hotspot-dev mailing list