-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8290840: Refactor the "os" class #9600
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
👋 Welcome back iklam! A progress list of the required criteria for merging this PR into |
Webrevs
|
| ::umask(umsk); | ||
| st->print("umask: %04o (", (unsigned) umsk); | ||
| print_umask(st, umsk); | ||
| os::Posix::print_umask(st, umsk); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's really no reason print_umask has to be part of the os::Posix class, it could just be a static file function in os_posix.cpp. I suspect the same may be true for other things in os::Posix - I would think we only need things in os::Posix that are explicitly called from os::Linux or os::Bsd etc.
dholmes-ora
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updates look good thanks. Overall this seems good to me. I think there may be more refinement possible with regards to the os::XXX classes, but that can be future cleanup if you want to proceed with this version for now.
Thanks.
dholmes-ora
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mis-clicked. Approved.
|
@iklam This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
gerard-ziemski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, however, I would prefer not to have to provide the default implementations for register_code_area() or resolve_function_descriptor()
| #ifndef HAVE_FUNCTION_DESCRIPTORS | ||
| inline void* os::resolve_function_descriptor(void* p) { | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unnecessary and could be removed if we did:
+#ifdef HAVE_FUNCTION_DESCRIPTORS
// Used only on PPC.
inline static void* resolve_function_descriptor(void* p);
+#endif
in os.hpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this PR, os.hpp no longer includes any os- or cpu-specific os_xxx.hpp files. I did this to avoid including large amount of os-specific declarations (such as os::Linux, which is about 400 lines) into generic source files, which aren't interested in these declarations.
As a result of this change, there's no way to let os.hpp know whether HAVE_FUNCTION_DESCRIPTORS is defined for the current OS+CPU. If we want to do that, we need to either:
- include os_xxx.hpp back in os.hpp (which I want to avoid)
- add new header files such as
os_<os>.defs.hppos_<os>_<cpu>.defs.hppto defineHAVE_FUNCTION_DESCRIPTORS(this will result in lots of header files that usually have nothing inside)
Since there are only 4 functions that fall under this pattern (functions that are used in generic code but most platforms have dummy implementations), I think what I have is a good compromise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we do:
#if defined(LINUX) && defined(PPC) && !defined(ABI_ELFv2)
#define HAVE_FUNCTION_DESCRIPTORS
#endif
in os.hpp and whatever the equivalent for AIX/PPC is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#if defined(PPC) && !defined(ABI_ELFv2)
#define HAVE_FUNCTION_DESCRIPTORS
#endif
should do the trick. AIX uses XCOFF, not Elf, so ABI_ELFv2 should never be defined.
But I have no emotions. We can leave it like this. Or do it in a follow up.
| #ifndef _WINDOWS | ||
| // Currently used only on Windows. | ||
| inline bool os::register_code_area(char *low, char *high) { | ||
| return true; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use some custom define, similar to HAVE_FUNCTION_DESCRIPTORS and remove this default impl just like with resolve_function_descriptor()?
tstuefe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
src/hotspot/share/runtime/os.hpp
Outdated
| // | ||
| // The Porting APIs declared as "inline" in os.hpp must be | ||
| // implemented in one of the two .inline.hpp files, depending on whether | ||
| // the feature is generic to the OS, or specific to a particular CPU |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generic? Did you mean specific? E.g. "depending on whether the feature is specific to a certain OS or an OS+CPU combination" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I clarified the comment:
// The Porting APIs declared as "inline" in os.hpp must be
// implemented in one of the two .inline.hpp files, depending on
// whether the feature is specific to a particular CPU architecture
// for this OS.
src/hotspot/os/aix/os_aix.hpp
Outdated
| // Returns true if ok, false if error. | ||
| static bool get_meminfo(meminfo_t* pmi); | ||
|
|
||
| // PPC only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since your are here, remove comment? There has not been an AIX on something other than power for ~25 years.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
| // Below are inline functions that are rarely implemented by the platforms. | ||
| // Provide default empty implementation. | ||
|
|
||
| #ifndef PLATFORM_PRINT_NATIVE_STACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you rename this to HAVE_PLATFORM_PRINT_NATIVE_STACK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
| // Information about the protection of the page at address '0' on this os. | ||
| inline bool os::zero_page_read_protected() { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the feeling many of these functions are really just global constants and that the coding could be simplified further in followup RFEs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah lets do more clean up in follow-up RFEs.
src/hotspot/os/linux/os_linux.cpp
Outdated
| // perform the workaround | ||
| Linux::capture_initial_stack(JavaThread::stack_size_at_create()); | ||
| workaround_expand_exec_shield_cs_limit(); | ||
| Linux::workaround_expand_exec_shield_cs_limit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could move workaround_expand_exec_shield_cs_limit() into this file, declare it static, and remove all mentionings from headers. It now lives in os_linux_x86.cpp, but since it has to be wrapped with !zero !64bit already it is an odd misfit anyway. So its not much more "wrong" for it to live here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
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) Putting linux_blah() into the os class seems pointless. |
tstuefe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still good.
src/hotspot/os/linux/os_linux.cpp
Outdated
| * Affects IA32: RHEL 5 & 6, Ubuntu 10.04 (LTS), 10.10, 11.04, 11.10, 12.04. | ||
| * @see JDK-8023956 | ||
| */ | ||
| void workaround_expand_exec_shield_cs_limit() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make static?
|
I've simplified 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. |
dholmes-ora
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still seems okay to me.
|
|
||
| #ifdef HAVE_FUNCTION_DESCRIPTORS | ||
| void* os::Linux::resolve_function_descriptor(void* p) { | ||
| #if !defined(ABI_ELFv2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think many people will be able to map this to any specific platform. At a minimum some comment is needed to explain this.
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 I guess this can mostly be avoided. E.g. there is zero reason why |
Thanks for pointing this out. I've updated the comments in os.hpp to be more clear about the inline functions. The design is -- if Conversely, if a function is NOT declared "inline" in Hence, any source file that references this function must include Without such a rule, it's possible for the build to succeed on some platforms (especially if the compiler is more lenient on missing inline function definitions) and fail on others. As a result, it would be easier to break the repo as not everyone is able to build all platforms before integrating a PR. This rule is similar to how we handle "inline" declarations in regular HotSpot headers. |
Yes, this can certainly be improved in follow-up RFE. |
stefank
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love seeing the os files being cleaned up and restructured.
I started to look at the patch and found a significant number of nits that I think would be good to fix.
src/hotspot/os/posix/os_posix.cpp
Outdated
| #include "utilities/globalDefinitions.hpp" | ||
| #include "utilities/macros.hpp" | ||
| #include "utilities/vmError.hpp" | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other files don't add a blank line before the conditional includes.
| #define OS_POSIX_OS_POSIX_HPP | ||
|
|
||
| #include "runtime/os.hpp" | ||
| #include <errno.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually add blank lines before system includes
|
|
||
| #include "runtime/mutex.hpp" | ||
| #include "runtime/os.hpp" | ||
| #include "os_posix.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .hpp file of an .inline.hpp file should be included as the first include. The comment above was an indication that we previously didn't follow that rule, because of how we sometimes structure platform includes. But it seems like you are changing this now, and I hope that we now can follow that rule and change the code to:
#include "os_posix.hpp"
#include "runtime/mutex.hpp"
#include "runtime/os.hpp"
| #include "runtime/perfMemory.hpp" | ||
| #include "services/memTracker.hpp" | ||
| #include "utilities/exceptions.hpp" | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove blank line
| #if defined(LINUX) | ||
| #include "os_linux.hpp" | ||
| #endif | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels "wrong" that the posix files include linux files. Maybe that's something that future restructuring of code could fix.
|
|
||
| #include "precompiled.hpp" | ||
| #include "pdh_interface.hpp" | ||
| #include "os_windows.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sort
| #include "os_posix.hpp" | ||
| #include "os_linux.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sort
| #include "os_posix.hpp" | ||
| #include "os_linux.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sort
| #include OS_HEADER(os) | ||
| #include OS_HEADER_INLINE(os) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually only explicitly include .inline.hpp and don't and let that file bring in the corresponding .hpp file.
| #include "unittest.hpp" | ||
| #include "runtime/frame.inline.hpp" | ||
| #include "runtime/threads.hpp" | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove blank line
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... |
…iptor()" This reverts commit 72efa9c.
Hi Gerard, thanks for the review. I've reverted the #ifdef change and gone back to the original way. See commit cf93fb7 |
stefank
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Three more nits, otherwise looks good.
| #include "runtime/os.hpp" | ||
| #include "os_bsd.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to:
#include "os_bsd.hpp"
#include "runtime/os.hpp"
| #include "runtime/os.hpp" | ||
| #include "os_linux.hpp" | ||
| #include "os_posix.inline.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to:
#include "os_linux.hpp"
#include "runtime/os.hpp"
#include "os_posix.inline.hpp"
| // os_windows.hpp included by os.hpp | ||
|
|
||
| #include "os_windows.hpp" | ||
| #include "runtime/javaThread.hpp" | ||
| #include "runtime/mutex.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to:
#include "os_windows.hpp"
#include "runtime/javaThread.hpp"
#include "runtime/mutex.hpp"
|
Thanks @tstuefe @gerard-ziemski @stefank @dholmes-ora for the review. |
|
Going to push as commit b6b0317. |
Please see JDK-8290840 for the detailed proposal.
The
osclass, declared in os.hpp, forms the major part of the HotSpot porting interface. Its structure has gradually deteriorated over the years as new ports are created and new APIs are added.This RFE tries to address the following:
os*.cppandos*.hppfiles.os::Linux class) by platform-independent source files.Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9600/head:pull/9600$ git checkout pull/9600Update a local copy of the PR:
$ git checkout pull/9600$ git pull https://git.openjdk.org/jdk pull/9600/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 9600View PR using the GUI difftool:
$ git pr show -t 9600Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9600.diff