On Wed, 21 Feb 2024 23:32:15 GMT, Magnus Ihse Bursie <i...@openjdk.org> wrote:
> **Summary:** Finally get rid of the mapfiles in Hotspot, and replace it with > compiler options and JNIEXPORT on all platforms. > > The bug that this PR solves, > [JDK-8017234](https://bugs.openjdk.org/browse/JDK-8017234), was created in > 2013. Even back then the use of mapfiles in Hotspot was dated, so this is > really good riddance with old rubbish. > > This code touches on central but not well understood parts of the Hotspot > dynamic library, which has contributed to why this bug has stayed unresolved > for so long. I will need to explain this fix in more detail than usually > necessary. (Please bare with me if this gets long.) I also anticipate that > not all solutions that I've picked will be accepted, and we'll have to > discuss how to proceed. I think it is better to have actual concrete code to > discuss around, rather than starting by an abstract discussion. To keep this > description short, I will post the discussion as a comment to the PR. > > I have run this PR through tier 1-3 in our CI system. I have also carefully > checked how the resulting dynamic library differs with this patch (not much; > see discussion below). For build system changes, this is often the most > relevant metric. First some general notes on related build changes: This was the last place in the build system where mapfiles where used. It will now be possible to clean up a lot of this framework, but I will do that separately after this has been integrated. Also, Hotspot is now built much closer to the rest of the native libraries in the JDK, so there is potential for more unification (e.g. placing `--exclude-libs,ALL` in a common location). This too will be done separately. On Windows, we still need to do the dance of finding symbols from the object files, and add them to a list of exported files (included by `-def:`). I decoupled this logic away from the old mapfile stuff, so we can safely delete the mapfile code later on. So what might look like new code in `CompileJvm.gmk` is actually just old code from `JvmMapfile.gmk` that I have cleaned up somewhat. These symbols are consumed by SA. It might be that we can limit the amount of symbols exported this way, or maybe that we can find another solution completely in SA, but that will have to be a future topic for research. For now, we keep the same exported symbols. I have checked the resulting libjvm.so/jvm.dll using the `COMPARE_BUILD` functionality, which (among other things) analyzes differences between native libraries before and after a patch. This has verified that there is no change in the symbols of the Hotspot dynamic library on macOS/x64, macOS/aarch64 or Windows/x64. I have also gotten help from @MBaesken and @JoKern65 to confirmed that there is no effect on AIX (in fact, AIX never used the mapfile it created...). There are, however, a few minor changes on linux/x64 and linux/aarch64. I will discuss these below. When I first started doing this conversion, I got a lot more differences. First of all, the `-fvisibility=hidden` flag does not apply to assembly files, so the symbols marked `.global` in these files also needed to be marked `.hidden`. (On macOS, Apple's clang assembler uses the directive `.private_extern` instead.) The assembly files had some inconsistent whitespace (using tabs instead of spaces every once in a while), which my editor kept changing, so I gave up and fixed all of them. This makes the diff artificially larger; if someone objects to this I can revert the whitespace changes. Secondly, it turned out that there were several symbols declared `JNIEXPORT` that were not included in the symbol list, and hence were not exported on Linux. In contrast to Windows, which only relies on `JNIEXPORT`, on Linux it was necessary to both use `JNIEXPORT` and declare the symbol in the symbol list to get it actually exported. To be consistent with the old behavior, I have undefined `JNIEXPORT` in these files. It works to keep the output the same, but it does not look very nice. I suggest we either: a) say we want to remove this `#undef` and actually export the functions annotated `JNIEXPORT`. This is what I would recommend. If so, let's keep the ugly undef for now, and I'll remove it in a follow-up bug. (I really would like not to change any behavior with this patch; it is intrusive enough as it is.) b) say we really don't want to export these functions on Linux, only on Windows. In that case, we should probably replace `JNIEXPORT` with `FOO_EXPORT`, and define `FOO_EXPORT` to be `JNIEXPORT` on Windows, and blank on Linux. This is a decision for the Hotspot team; let me know which solution you chose. (It can be different in different places, of course). And thirdly, there seem to be a bug in gcc, which ignores the `-fvisibility=hidden` flag in certain circumstances involving templates. I could not pinpoint what conditions were necessary to trigger this bug, but in `accessBackend.cpp` I have manually added a `HIDDEN` attribute to a bunch of `arraycopy_conjoint` functions to keep them from being exported. It looks ugly; and we might be willing to accept that these functions are exported even though they should not be, to get rid of this. Or it might be that someone finds out if what is different with this declarations that trigger this bug, and constructs a work-around. (I gave up on it, though...) About the changes on linux/x64 and linux/aarch64: I believe the new values are in some way more correct, but I also think the difference is insignificant. The difference is that the data (variable) symbols `__bss_start`, `_edata` and `_end`, and the text (function) symbols `_fini` and `_init` has changed from local to global. Afaik, these are symbols created by the linker. Also, when we used a mapfile, the symbol `SUNWprivate_1.1` that was part of the mapfile definition was included in libjvm.so, and this is no longer the case. Finally, in the gtest libjvmo.so there are some additional differences as well. I did not really care about these, since this is only used for testing, but for the sake of completeness, here are the differences. They belong to two different categories. Group 1 includes four symbols like `_Z9type_nameIiEPKcv`. These come from this definition: template <typename T> const char* type_name(); in `test_parse_memory_size.cpp`, and is seemingly the same issue as with the `arraycopy_conjoint` functions in `accessBackend.cpp`. They have changed from local to global. Group 2 includes symbols that seem to originate in the googletest library. They have changed from local to global, but also to weak symbols. They match some different patterns: * Seven symbols like `_ZNSt15_Sp_counted_ptrIPKN7testing8internal2REELN9__gnu_cxx12_Lock_policyE2EED2Ev`. Demangled, these read `std::_Sp_counted_ptr<testing::internal::RE const*, (__gnu_cxx::_Lock_policy)2>`. * One symbol `_ZN7testing15AssertionResultlsIA12_cEERS0_RKT_` (demangled: `testing::AssertionResult& testing::AssertionResult::operator<< <char [12]>(char const (&) [12])`) * And finally, only on x64 (but not aarch64), one symbol `_ZN7testing4Test10HasFailureEv` (demangled: `testing::Test::HasFailure()`). ------------- PR Comment: https://git.openjdk.org/jdk/pull/17955#issuecomment-1958337547
