Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v36]
On Fri, 16 Feb 2024 07:42:47 GMT, Hannes Wallnöfer wrote: >> Jonathan Gibbons has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Support for Table Of Contents in Markdown headings >> - fix typo > > src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java > line 286: > >> 284: lineKind = (ch == '\n' || ch == '\r') ? >> LineKind.BLANK >> 285: : (indent <= 3) ? peekLineKind() >> 286: : lineKind != LineKind.OTHER ? >> LineKind.INDENTED_CODE_BLOCK > > Doesn't this cause false positives for indented code blocks? In my > understanding, indented lines [in a list > context](https://spec.commonmark.org/0.30/#example-258) and [directly > following a > blockquote](https://spec.commonmark.org/dingus/?text=%3E%20%20%20%20foo%0A%20%20%20%20%20bar%0A) > are not interpreted as code blocks (always in the case of blockquote, and > depending on the offset of text after the list marker in list items). > > Note: edited because my initial suggestion was incorrect. @hns @pavelrappo noted; working on it ... - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1494909524
Re: Hotspot symbol visibility - surprising news!
Hi Magnus, The "massive generated mapfile" is used on Windows. As you noticed, on Linux symbols are exported if they are both JNIEXPORT and listed in the mapfile. On Windows, the symbols are exported if they are either JNIEXPORT or listed in the mapfile. The Windows symbolicator needs to see the vftables to tell apart different object types, and these tables are only made available in the produced binaries through the mapfile. The Windows mapfile exports the vftables of every virtual class in libjvm, which is much more than the symbolicator actually needs, but that's probably a separate topic. The symbolicator on Linux does not need the mapfile. The assembly functions are correctly labelled with .globl; if we remove these declarations or change them to local, the code won't link. The right solution here is to also label them as .hidden, for example using this script: sed -i -n 'p;s/\.globa\?l/.hidden/p' src/hotspot/os_cpu/*/*.S The symbols from the standard libraries are not exported by any of our binaries. Libjvm removes them via the use of mapfile, and all other libraries are linked with -Wl,--exclude-libs,ALL; if you choose to remove the mapfile from libjvm, you'll need to add that parameter. Regards, Daniel
Integrated: 8325950: Make sure all files in the JDK pass jcheck
On Thu, 15 Feb 2024 12:19:31 GMT, Magnus Ihse Bursie wrote: > Since jcheck only checks file in a commit, there is a possibility of us > getting files in the repository that would not be accepted by jcheck. This > can happen when extending the set of files checked by jcheck, or if jcheck > changes how it checks files (perhaps due to bug fixes). > > I have now run jcheck over the entire code base, and fixed a handful of > issues. With these changes, jcheck accept the entire code base. This pull request has now been integrated. Changeset: 5c5a282f Author:Magnus Ihse Bursie URL: https://git.openjdk.org/jdk/commit/5c5a282f91dd28b306673ca2bcc30dec451e7a7d Stats: 113 lines in 38 files changed: 0 ins; 10 del; 103 mod 8325950: Make sure all files in the JDK pass jcheck Reviewed-by: prr, wetmore, erikj, naoto - PR: https://git.openjdk.org/jdk/pull/17871
Integrated: 8325972: Add -x to bash for building with LOG=debug
On Thu, 15 Feb 2024 15:07:46 GMT, Magnus Ihse Bursie wrote: > I don't understand why I have never thought of this before. If we add `-x` to > the set of bash arguments when running with LOG=debug, we get output of *all* > shell commands that make is running, even those for $(shell). > > This makes it s much easier to understand what is actually happening in > the makefile! (To the point where we could actually consider moving other > stuff away from the debug level.) This pull request has now been integrated. Changeset: 8668198c Author:Magnus Ihse Bursie URL: https://git.openjdk.org/jdk/commit/8668198c26bdac412f0a9d1255ca74da860761c5 Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod 8325972: Add -x to bash for building with LOG=debug Reviewed-by: erikj - PR: https://git.openjdk.org/jdk/pull/17875
Re: RFR: 8325881: Require minimum gcc version 10
On Sat, 17 Feb 2024 08:28:56 GMT, Kim Barrett wrote: > Please review this change that updates the minimum supported version of gcc > to be used for building OpenJDK from 6.0 to 10.0. > > This permits enabling C++17 (JDK-8314488), though gcc 9.0 might suffice for > that. A minimum of gcc 10 also obtains the primitives needed to support a > work-alick for std::is_constant_evaluated (added in C++20). There are a bunch > of improvements that would be enabled by that. Having it would also allow the > elimination of a bit of a mess in the HotSpot assert macros that was needed to > work around the lack of that feature (JDK-8303805). Either current or proposed > minimum versions of other supported compilers also provide the needed > primitives. > > Testing: mach5 tier1 (uses gcc13.2 on gcc-based platforms) > Locally (linux-x64) built and ran tier1 with gcc10.3. Looks good from a build perspective. - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17899#pullrequestreview-1888786735
Re: Hotspot symbol visibility - surprising news!
On 2024-02-14 11:06, Magnus Ihse Bursie wrote: I am currently pursuing improved build functionality for static libraries. One of the issues with static libraries are name collisions, which led me back to an old discussion about which symbols are exported from Hotspot, and how this is achieved. A long discussion is available in JDK-8017234 [1], which was created in 2013 and has been on the back burner ever since, coming back to life every now and then. There are basically two different problems here. They are both distinct and interrelated, and we likely need to solve both in tandem. The first problem is how Hotspot should say which symbols (functions) that should be exported from libjvm.so. The historical, and still used, system is to have a mapfile. In the "new" (not so new anymore) build system, this was simplified to a list of function names in make/data/hotspot-symbols, from which a mapfile is built. This is in contrast with how all other libraries in the JDK are built, and also with modern best practices. A better approach would be to annotate the functions that should be exported in the source code. In fact, such annotation already exists, even in Hotspot, as JNIEXPORT, but this is currently not used in the compilation of Hotspot. The second problem is that in addition to these explicitly exported functions, we also export basically all other functions as well in Hotspot. (So currently we could basically just forgo this complicated machinery and just export everything by default...) The reason for this seem somewhat unclear. The specifics are probably forever lost in the mists of time past, but the essential point is that these symbols are needed for SA to do it's job. So what we do is that we list all symbols in hotspot after compiling (but before linking), and selects some (most, I think) of them using regexp patterns, and add these to the mapfile. Actually, we are not doing what I thought we're doing, and what I assume whoever wrote the original code thought we were doing. Seems no-one has ever bothered to check the effect of this massive generated mapfile. :-/ At least not on linux/gcc (which is the only platform I've looked at so far). It turns out that the gcc linker never removes the "hidden" visibility from symbols. Even if you list them in a mapfile under "global:", if they were compiled with visibility "hidden", they will still be hidden. The only thing you can ever hope to achieve with a mapfile is to make symbols with "default" visibility be "hidden", by placing them under the "local:" header. In hotspot, only functions marked JNIEXPORT have the "default" visibility. The upshot of all this is that the mapfile has very little impact on the resulting symbols in Hotspot, and a lot of the difference it makes seems to be incorrect. :-/ In other words, all the mapfile can ever do is hide stuff that would otherwise have been visible. So what symbols are then made hidden by the mapfile? In effect, symbols that are marked JNIEXPORT but are not listed in make/data/hotspot-symbols, and symbols that arise from outside hotspot source code. I have divided these symbols into four categories: 1) All symbols from debug.cpp that are marked JNIEXPORT (most of them). 2) All symbols from jvmciCompilerToVM.cpp and that are marked JNIEXPORT (most of them), and data from vmStructs_jvmci.cpp that are marked JNIEXPORT. 3) Assembly functions such as _Copy_arrayof_conjoint_bytes that are marked .globl 4) Symbols not present in hotspot source code, but which originates with the compiler or standard libraries, e.g. __bss_start, __cxa_begin_catch, _ZTIN10__cxxabiv117__class_type_infoE, _ZTSN9__gnu_cxx20recursive_init_errorE, etc. My interpretation of this is: a) It is actually an error not to export functions marked JNIEXPORT, so 1 and 2 are actually more correct this way. b) The assembly functions are incorrectly marked .globl. This should just be removed. c) As for the symbols from compiler and standard libraries; I don't know. I assume these are exported by default by normal libs, so it should probably be fine to do the same with Hotspot. Possibly there are other ways to make these local, if we really want that (linker flags, or something like that?). The most important observation, however, is that generating a long list of symbols from the object file to make them "available" to SA seems to be a complete misunderstanding of what is happening. No hidden symbols can ever be made visible in this manner. /Magnus
Re: RFR: 8325963: Clean up NativeCompilation.gmk and its newly created parts
On Thu, 15 Feb 2024 14:13:19 GMT, Magnus Ihse Bursie wrote: > This is a follow-up to > [JDK-8325877](https://bugs.openjdk.org/browse/JDK-8325877). I was careful in > that bug not to change order of any lines, only split up the original file > into parts. > > In this PR, I use the new structure to improve the design. I collect the > functionality into three clearly separate phases, preparation, compilation > and linking. I use consistent naming to reveal whether a function just sets > up variables, or create output. I simplify and split up a few extra hard to > read functions. I move some code around so it is placed more logically. > > It can be hard to convince yourself that the changes are correct if you just > look at the end result. I have tried to make small and clear changes in each > separate commit, and to explain in the commit title what I am doing. I > recommend reviewing this PR [commit by > commit](https://github.com/openjdk/jdk/pull/17873/commits). Marked as reviewed by jwaters (Committer). - PR Review: https://git.openjdk.org/jdk/pull/17873#pullrequestreview-1888308883
Re: RFR: 8325963: Clean up NativeCompilation.gmk and its newly created parts
On Thu, 15 Feb 2024 14:13:19 GMT, Magnus Ihse Bursie wrote: > This is a follow-up to > [JDK-8325877](https://bugs.openjdk.org/browse/JDK-8325877). I was careful in > that bug not to change order of any lines, only split up the original file > into parts. > > In this PR, I use the new structure to improve the design. I collect the > functionality into three clearly separate phases, preparation, compilation > and linking. I use consistent naming to reveal whether a function just sets > up variables, or create output. I simplify and split up a few extra hard to > read functions. I move some code around so it is placed more logically. > > It can be hard to convince yourself that the changes are correct if you just > look at the end result. I have tried to make small and clear changes in each > separate commit, and to explain in the commit title what I am doing. I > recommend reviewing this PR [commit by > commit](https://github.com/openjdk/jdk/pull/17873/commits). Oh boy, this isn't going to be fun, but no worries, I'll find a way :) I've actually avoided pulling in the previous change into the gcc port since MinGW is packaging a version of Java that predates it, I'll probably pull it in once a new tag that includes that changeset is released - PR Comment: https://git.openjdk.org/jdk/pull/17873#issuecomment-1952338357
Re: RFR: 8325963: Clean up NativeCompilation.gmk and its newly created parts
On Thu, 15 Feb 2024 14:13:19 GMT, Magnus Ihse Bursie wrote: > This is a follow-up to > [JDK-8325877](https://bugs.openjdk.org/browse/JDK-8325877). I was careful in > that bug not to change order of any lines, only split up the original file > into parts. > > In this PR, I use the new structure to improve the design. I collect the > functionality into three clearly separate phases, preparation, compilation > and linking. I use consistent naming to reveal whether a function just sets > up variables, or create output. I simplify and split up a few extra hard to > read functions. I move some code around so it is placed more logically. > > It can be hard to convince yourself that the changes are correct if you just > look at the end result. I have tried to make small and clear changes in each > separate commit, and to explain in the commit title what I am doing. I > recommend reviewing this PR [commit by > commit](https://github.com/openjdk/jdk/pull/17873/commits). I have fully verified using COMPARE_BUILD that the resulting native code is byte-by-byte identical on Linux, macOS and Windows, so that should confirm the correctness of this refactoring. And @TheShermanTanker, I apologize, but this one is probably going to be much harder for you to integrate into your port. :-( - PR Comment: https://git.openjdk.org/jdk/pull/17873#issuecomment-1952314727 PR Comment: https://git.openjdk.org/jdk/pull/17873#issuecomment-1952316464
RFR: 8325963: Clean up NativeCompilation.gmk and its newly created parts
This is a follow-up to [JDK-8325877](https://bugs.openjdk.org/browse/JDK-8325877). I was careful in that bug not to change order of any lines, only split up the original file into parts. In this PR, I use the new structure to improve the design. I collect the functionality into three clearly separate phases, preparation, compilation and linking. I use consistent naming to reveal whether a function just sets up variables, or create output. I simplify and split up a few extra hard to read functions. I move some code around so it is placed more logically. It can be hard to convince yourself that the changes are correct if you just look at the end result. I have tried to make small and clear changes in each separate commit, and to explain in the commit title what I am doing. I recommend reviewing this PR [commit by commit](https://github.com/openjdk/jdk/pull/17873/commits). - Commit messages: - Inline SetupDebugSymbols into SetupCompilerFlags - Fix indentation in the new functions - Extract CreateStaticLibrary and CreateDynamicLibraryOrExecutable - Move GetEntitlement setup to SetupLinking. Inline GetSymbols (for static builds). - Simplify SetupCompileFileFlags by passing in $$($1_BASE) as $2 - Clearly separate the steps into preparation, compilation and linking. - Be consistent (and explicit) about the use of Setup vs Create. Collect all Setup functions at the start. - Combine the linking preparations - Combine the three SetupBasicVariables functions Changes: https://git.openjdk.org/jdk/pull/17873/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17873&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8325963 Stats: 385 lines in 6 files changed: 132 ins; 130 del; 123 mod Patch: https://git.openjdk.org/jdk/pull/17873.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17873/head:pull/17873 PR: https://git.openjdk.org/jdk/pull/17873
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v50]
On Sun, 21 Jan 2024 07:58:11 GMT, Julian Waters wrote: >> We should set the -permissive- flag for the Microsoft Visual C compiler, as >> was requested by the now backed out >> [JDK-8241499](https://bugs.openjdk.org/browse/JDK-8241499). Doing so makes >> the Visual C compiler much less accepting of ill formed code, which will >> improve code quality on Windows in the future. > > Julian Waters has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 84 commits: > > - Merge branch 'openjdk:master' into patch-10 > - awt_Window.cpp > - awt_Frame.cpp > - awt_Component.cpp > - awt_Canvas.cpp > - Merge branch 'openjdk:master' into patch-10 > - Merge branch 'openjdk:master' into patch-10 > - Fix awt_Window.cpp > - Fix awt_PrintJob.cpp > - -Zc:stringStrings no longer needed with -permissive- flags-cflags.m4 > - ... and 74 more: https://git.openjdk.org/jdk/compare/a474b372...0f34608b bot-keep-alive - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-1952121717
