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

[Gerard Ziemski]

On Tue, 2 Aug 2022 07:12:33 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>>> Very good work. Only small remarks/nitpickings.
>>> 
>>> One question though, how does it work if I want to use an API implemented on a per-platform base in shared code? E.g. `zero_page_read_protected()`? I include `os.hpp` to get the definition, but how do I get the implementation? I have to include the platform-specific inline header too? Still with the OS_HEADER macro?
>>> 
>>> Cheers, Thomas
>> 
>> I think we should avoid creating non-portable os::xxx APIs and then call them from shared code. There were some calls to os::Posix::xxx from the shared code that I fixed in this PR.
>> 
>> If you really want to make a platform-specific call, you should just do this (sadly we have many such cases)
>> 
>> 
>> #ifdef LINUX
>> linux_blah();
>> #endif
>> 
>> 
>> Putting linux_blah() into the os class seems pointless.
>
>> 
>> I think we should avoid creating non-portable os::xxx APIs and then call them from shared code. There were some calls to os::Posix::xxx from the shared code that I fixed in this PR.
> 
> I don't think that's what I meant. I meant APIs that exist for all OSes but have different expressions on each OS. So the prototype should live in shared os. But I want to pull a different implementation for each OS. 
> 
> This only affects inline functions. If the different implementations live in os_xx.cpp it works automatically through link time resolution.
> 
> I see there are only a few examples for what I meant. Many inline functions return platform dependend values and those live in variables in shared code (e.g. os::vm_page_size(), ...). Others are just not inline (os.create_thread(), ...). Which leaves us with the likes of `zero_page_read_protected()` etc, and I see you put them into os.inline.hpp and there you include the OS specific headers too.
> 
> I guess this can mostly be avoided. E.g. there is zero reason why `zero_page_read_protected()` cannot be a static boolean variable in os. For future RFEs.

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

Thank you for trying. I am not completely happy about the new #ifdef and it doesn't look as clean to me as I imagined it would, so I am OK with you going with the original way it way. I guess, we could address it in a follow up if we come up with a nicer way of doing this...

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

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