Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v62]
> Please review a patch to add support for Markdown syntax in documentation > comments, as described in the associated JEP. > > Notable features: > > * support for `///` documentation comments in `JavaTokenizer` > * new module `jdk.internal.md` -- a private copy of the `commonmark-java` > library > * updates to `DocCommentParser` to treat `///` comments as Markdown > * updates to the standard doclet to render Markdown comments in HTML Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision: update copyright years - Changes: - all: https://git.openjdk.org/jdk/pull/16388/files - new: https://git.openjdk.org/jdk/pull/16388/files/20a384b6..6576d024 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=61 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=60-61 Stats: 68 lines in 62 files changed: 0 ins; 7 del; 61 mod Patch: https://git.openjdk.org/jdk/pull/16388.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16388/head:pull/16388 PR: https://git.openjdk.org/jdk/pull/16388
Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v61]
> Please review a patch to add support for Markdown syntax in documentation > comments, as described in the associated JEP. > > Notable features: > > * support for `///` documentation comments in `JavaTokenizer` > * new module `jdk.internal.md` -- a private copy of the `commonmark-java` > library > * updates to `DocCommentParser` to treat `///` comments as Markdown > * updates to the standard doclet to render Markdown comments in HTML Jonathan Gibbons has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 85 commits: - Merge remote-tracking branch 'upstream/master' into 8298405.doclet-markdown-v3 - update commonmark-java from 0.21.0 to 0.22.0 - Remove links to `jdk.javadoc` module from `java.compiler` module` - Suppress warnings building tests - Merge with upstream/master - address review feedback for updates to Elements and friends - address review feedback for updates to Elements and friends - add support for JDK-8329296: Update Elements for '///' documentation comments - add support for `--disable-line-doc-comments` - Merge branch '8298405.doclet-markdown-v3' of https://github.com/jonathan-gibbons/jdk into 8298405.doclet-markdown-v3 - ... and 75 more: https://git.openjdk.org/jdk/compare/b96b38c2...20a384b6 - Changes: https://git.openjdk.org/jdk/pull/16388/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=60 Stats: 26326 lines in 243 files changed: 25679 ins; 260 del; 387 mod Patch: https://git.openjdk.org/jdk/pull/16388.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16388/head:pull/16388 PR: https://git.openjdk.org/jdk/pull/16388
Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v60]
> Please review a patch to add support for Markdown syntax in documentation > comments, as described in the associated JEP. > > Notable features: > > * support for `///` documentation comments in `JavaTokenizer` > * new module `jdk.internal.md` -- a private copy of the `commonmark-java` > library > * updates to `DocCommentParser` to treat `///` comments as Markdown > * updates to the standard doclet to render Markdown comments in HTML Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision: update commonmark-java from 0.21.0 to 0.22.0 - Changes: - all: https://git.openjdk.org/jdk/pull/16388/files - new: https://git.openjdk.org/jdk/pull/16388/files/fadc130b..74c86f51 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=59 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=58-59 Stats: 2382 lines in 53 files changed: 2020 ins; 258 del; 104 mod Patch: https://git.openjdk.org/jdk/pull/16388.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16388/head:pull/16388 PR: https://git.openjdk.org/jdk/pull/16388
Re: RFR: 8329257: AIX: Switch HOTSPOT_TOOLCHAIN_TYPE from xlc to gcc [v3]
On Tue, 30 Apr 2024 15:19:47 GMT, Joachim Kern wrote: >> It might be easier to get input if you create a new PR with the change. This >> discussion is hidden deep down in a closed PR. > > I will do after labor day and create a PR with this suggested solution in > your JDK-8330539. I think I still prefer just unconditionally including in globalDefinitions_gcc.hpp. For gcc/clang we are using `-std=c++14` + `-D_GNU_SOURCE` instead of `-std=gnu++14`. I forget exactly why. I don't really want to be messing with `__STRICT_ANSI__`. - PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1585181094
Re: RFR: 8329257: AIX: Switch HOTSPOT_TOOLCHAIN_TYPE from xlc to gcc [v3]
On Tue, 30 Apr 2024 14:39:29 GMT, Magnus Ihse Bursie wrote: >> Ok for me. Let's hear what @kimbarrett thinks. > > It might be easier to get input if you create a new PR with the change. This > discussion is hidden deep down in a closed PR. I will do after labor day and create a PR with this suggested solution in your JDK-8330539. - PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1585020690
Re: RFR: 8329257: AIX: Switch HOTSPOT_TOOLCHAIN_TYPE from xlc to gcc [v3]
On Tue, 30 Apr 2024 14:00:25 GMT, Martin Doerr wrote: >> For the impatient, I suggest adopting mechanism 2, i.e. unconditionally >> include in globalDefinitions_gcc.hpp. >> >> We can't include in shared code, and there is a use in shared code >> (in the relatively recently added JavaThread::pretouch_stack). >> >> When I questioned whether we needed to include at all, I referred >> to a Linux man page I'd found on the internet (the same page mdoerr linked >> to), which says (in part) >> >> "By default, modern compilers automatically translate all uses of alloca() >> into the built-in ..." >> >> Apparently I should have kept digging, because it seems that page is >> old/incorrect. A seemingly more recent Linux man page describes a different >> way of handling it that is closer to what we're seeing, but still not quite >> correct. >> >> glibc's includes if __USE_MISC is defined. >> One of the ways __USE_MISC can become defined is if _GNU_SOURCE is defined, >> and we define that for both gcc and clang toolchains. >> >> We include in globalDefinitions_gcc.hpp. So when building with >> gcc, >> globalDefinitions.hpp implicitly includes . >> >> The glibc definition of alloca is >> >> #ifdef __GNUC__ >> # define alloca(size)__builtin_alloca (size) >> #endif /* GCC. */ >> >> So that explains why we don't need any explicit include of when >> building with gcc. I expect there's something similar going on with Visual >> Studio and Xcode/clang. But apparently not with Open XLC clang. > > Ok for me. Let's hear what @kimbarrett thinks. It might be easier to get input if you create a new PR with the change. This discussion is hidden deep down in a closed PR. - PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1584947979
Re: RFR: 8331020: Assign LANG to C++ for Windows code that uses C++
On Tue, 30 Apr 2024 13:50:22 GMT, Julian Waters wrote: >> make/modules/java.desktop/lib/AwtLibraries.gmk line 102: >> >>> 100: $(eval $(call SetupJdkLibrary, BUILD_LIBAWT, \ >>> 101: NAME := awt, \ >>> 102: LANG := $(if $(filter $(OPENJDK_TARGET_OS), windows), C++, C), \ >> >> No, this is not okay. You need to do this as for the LIBJSOUND_LINK_TYPE >> above. The same goes for the one below, too. > > Oh, I'm surprised! I thought that you'd prefer the more lambda-like approach. > I guess the other way of LIBAWT_LINK_TYPE works too in that case There are two reasons: 1) To keep up with the style elsewhere, were we use "ifeq (... platform)" to setup platform-specific arguments. Even if this was not an ideal style, it would still make sense to keep to one way of doing it. 2) I actually think that is better. We have gravitated towards that solution over the years. The make syntax is hard to read and easy to get wrong. We try to make the arguments in the Setup calls trivial, and if we can't do that in place, then we create a "local" variable (by prefixing it with the name of the lib) outside the Setup call, were we can use more space to clearly show what is going on. In fact, I really dislike the `$(if...)` syntax, and use it only if I must. It is hopeless to see what is the if-clause and the else-clause, and it is way too easy to get a "false positive" since you do not compare the variable with another value, but checks if it evaluates to non-empty. - PR Review Comment: https://git.openjdk.org/jdk/pull/18927#discussion_r1584903238
Re: RFR: 8331020: Assign LANG to C++ for Windows code that uses C++
On Tue, 30 Apr 2024 13:53:11 GMT, Julian Waters wrote: > I switched it back to LANG since in the original change you switched it to > LINK_TYPE from LANG after one of my objections. I had since retracted that > objection and have been feeling bad about it. Have you since changed your > mind about LANG vs LINK_TYPE in that time? Yes, I have changed my mind. I think your objection back then was valid; I created an argument which implied a wider scope than it really delivered, with some vague hand-waving about future extensions. It is better to be more concrete here and now, and rename the parameter if we ever add more meaning to it. - PR Comment: https://git.openjdk.org/jdk/pull/18927#issuecomment-2085433698
Re: RFR: 8329257: AIX: Switch HOTSPOT_TOOLCHAIN_TYPE from xlc to gcc [v3]
On Thu, 18 Apr 2024 04:26:21 GMT, Kim Barrett wrote: >> I opened https://bugs.openjdk.org/browse/JDK-8330539 so we don't lose track >> of this, but we can keep the discussion/voting here. > > For the impatient, I suggest adopting mechanism 2, i.e. unconditionally > include in globalDefinitions_gcc.hpp. > > We can't include in shared code, and there is a use in shared code > (in the relatively recently added JavaThread::pretouch_stack). > > When I questioned whether we needed to include at all, I referred > to a Linux man page I'd found on the internet (the same page mdoerr linked > to), which says (in part) > > "By default, modern compilers automatically translate all uses of alloca() > into the built-in ..." > > Apparently I should have kept digging, because it seems that page is > old/incorrect. A seemingly more recent Linux man page describes a different > way of handling it that is closer to what we're seeing, but still not quite > correct. > > glibc's includes if __USE_MISC is defined. > One of the ways __USE_MISC can become defined is if _GNU_SOURCE is defined, > and we define that for both gcc and clang toolchains. > > We include in globalDefinitions_gcc.hpp. So when building with gcc, > globalDefinitions.hpp implicitly includes . > > The glibc definition of alloca is > > #ifdef__GNUC__ > # define alloca(size) __builtin_alloca (size) > #endif /* GCC. */ > > So that explains why we don't need any explicit include of when > building with gcc. I expect there's something similar going on with Visual > Studio and Xcode/clang. But apparently not with Open XLC clang. Ok for me. Let's hear what @kimbarrett thinks. - PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1584884372
Re: RFR: 8331020: Assign LANG to C++ for Windows code that uses C++
On Tue, 30 Apr 2024 12:27:42 GMT, Magnus Ihse Bursie wrote: > Please revert back to the LINK_TYPE name. As long as it is not used for > anything else, this is a better description. If we start to use it to have a > broader meaning, we can rename it then. I switched it back to LANG since in the original change you switched it to LINK_TYPE from LANG after one of my objections. I had since retracted that objection and have been feeling bad about it. Have you since changed your mind about LANG vs LINK_TYPE in that time? It might take me a bit of time to address these reviews, sorry about that - PR Comment: https://git.openjdk.org/jdk/pull/18927#issuecomment-2085401560
Re: RFR: 8331020: Assign LANG to C++ for Windows code that uses C++
On Tue, 30 Apr 2024 12:32:09 GMT, Magnus Ihse Bursie wrote: >> Currently, on Windows LANG is not assigned to C++ for some code that does >> use C++. This just works because link.exe does not bother about what kind of >> code it is currently linking. gcc however, does. It doesn't hurt to assign >> LANG to C++ as a formality in such cases, which this changeset does. This >> also renames LINK_TYPE to LANG, which the original change to remove the >> TOOLCHAIN parameter used to do > > make/modules/java.desktop/lib/AwtLibraries.gmk line 102: > >> 100: $(eval $(call SetupJdkLibrary, BUILD_LIBAWT, \ >> 101: NAME := awt, \ >> 102: LANG := $(if $(filter $(OPENJDK_TARGET_OS), windows), C++, C), \ > > No, this is not okay. You need to do this as for the LIBJSOUND_LINK_TYPE > above. The same goes for the one below, too. Oh, I'm surprised! I thought that you'd prefer the more lambda-like approach. I guess the other way of LIBAWT_LINK_TYPE works too in that case - PR Review Comment: https://git.openjdk.org/jdk/pull/18927#discussion_r1584867458
Re: RFR: 8329257: AIX: Switch HOTSPOT_TOOLCHAIN_TYPE from xlc to gcc [v3]
On Tue, 30 Apr 2024 12:46:31 GMT, Magnus Ihse Bursie wrote: >> I got it. And what about simply disabling the `__STRICT_ANSI__` with >> `CFLAGS_OS_DEF_JVM="-DAIX -D_LARGE_FILES -U__STRICT_ANSI__"` in >> flags-cflags.m4 for AIX. This worked too. The build is fine. > > So what you are saing is basically replacing > > CFLAGS_OS_DEF_JVM="-DAIX -Dalloca'(size)'=__builtin_alloca'(size)' > -D_LARGE_FILES" > ``` > with > > CFLAGS_OS_DEF_JVM="-DAIX -D_LARGE_FILES -U__STRICT_ANSI__" > ``` > ? > > Yeah, that'll work, I guess. "strict ansi" sounds like a problematic thing to > have enabled, and that it is added by `-std=c++14` sounds close to a bug in > my ears. So a "workaround" where this is disabled seem reasonable. Yes this would be the replacement. This is our 4th way to fix the issue. Anyone else who would prefer this too? - PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1584847372
Re: RFR: 8329257: AIX: Switch HOTSPOT_TOOLCHAIN_TYPE from xlc to gcc [v3]
On Tue, 30 Apr 2024 12:33:19 GMT, Joachim Kern wrote: >> I don't think leaving out `-std=c++14` for AIX is a good solution. > > I got it. And what about simply disabling the `__STRICT_ANSI__` with > `CFLAGS_OS_DEF_JVM="-DAIX -D_LARGE_FILES -U__STRICT_ANSI__"` in > flags-cflags.m4 for AIX. This worked too. The build is fine. So what you are saing is basically replacing CFLAGS_OS_DEF_JVM="-DAIX -Dalloca'(size)'=__builtin_alloca'(size)' -D_LARGE_FILES" ``` with CFLAGS_OS_DEF_JVM="-DAIX -D_LARGE_FILES -U__STRICT_ANSI__" ``` ? Yeah, that'll work, I guess. "strict ansi" sounds like a problematic thing to have enabled, and that it is added by `-std=c++14` sounds close to a bug in my ears. So a "workaround" where this is disabled seem reasonable. - PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1584754261
Re: RFR: 8331331: :tier1 target explanation in doc/testing.md is incorrect
On Mon, 29 Apr 2024 16:14:45 GMT, SendaoYan wrote: > Hi, > > In doc/testing.md file, it says: > > As an example, :tier1 will expand to jtreg:$(TOPDIR)/test/hotspot/jtreg:tier1 > jtreg:$(TOPDIR)/test/jdk:tier1 jtreg:$(TOPDIR)/test/langtools:tier1 > jtreg:$(TOPDIR)/test/nashorn:tier1 jtreg:$(TOPDIR)/test/jaxp:tier1. > > The actual situation is :tier1 doesn't contains test/nashorn:tier1, and the > document missed test/lib-test:tier1 > > $ make -n test-tier1 &> test-tier1.log > $ grep "Running test " test-tier1.log > Running test 'jtreg:test/hotspot/jtreg:tier1' > Running test 'jtreg:test/jdk:tier1' > Running test 'jtreg:test/langtools:tier1' > Running test 'jtreg:test/jaxp:tier1' > Running test 'jtreg:test/lib-test:tier1' > > Only change the document, no risk. I agree with Magnus and David, that is a better approach. - PR Comment: https://git.openjdk.org/jdk/pull/19002#issuecomment-2085224067
Re: Hermetic Java project meeting
On 2024-04-26 03:15, Jiangli Zhou wrote: On Thu, Apr 25, 2024 at 9:28 AM Magnus Ihse Bursie wrote: Just to be more clear, that's with using `objcopy` to localize non-exported symbols for all JDK static libraries and libjvm.a, not just libjvm.a right? Yes. Can you please include the compiler or linker errors on linux/clang? It is a bit tricky. The problem arises at the partial linking step. The problem seem to arise out of how clang converts a request to link into an actual call to ld. I enabled debug code (printing the command line, and running clang with `-v` to get it to print the actual command line used to run ld) and ran it on GHA, where it worked fine. This is how it looks there: WILL_RUN: /usr/bin/clang -v -m64 -r -o /home/runner/work/jdk/jdk/build/linux-x64/support/native/java.rmi/librmi/static/librmi_relocatable.o /home/runner/work/jdk/jdk/build/linux-x64/support/native/java.rmi/librmi/static/GC.o Ubuntu clang version 14.0.0-1ubuntu1.1 Target: x86_64-pc-linux-gnu Thread model: posix InstalledDir: /usr/bin Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/10 Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/11 Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/12 Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/13 Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/9 Selected GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/13 Candidate multilib: .;@m64 Selected multilib: .;@m64 "/usr/bin/ld" -z relro --hash-style=gnu --build-id --eh-frame-hdr -m elf_x86_64 -dynamic-linker /lib64/ld-linux-x86-64.so.2 -o /home/runner/work/jdk/jdk/build/linux-x64/support/native/java.rmi/librmi/static/librmi_relocatable.o -L/usr/bin/../lib/gcc/x86_64-linux-gnu/13 -L/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../lib64 -L/lib/x86_64-linux-gnu -L/lib/../lib64 -L/usr/lib/x86_64-linux-gnu -L/usr/lib/../lib64 -L/usr/lib/llvm-14/bin/../lib -L/lib -L/usr/lib -r /home/runner/work/jdk/jdk/build/linux-x64/support/native/java.rmi/librmi/static/GC.o In contrast, on my machine it looks like this: WILL_RUN: /usr/local/clang+llvm-13.0.1-x86_64-linux-gnu-ubuntu-18.04/bin/clang -v -m64 -r -o /localhome/git/jdk-ALT/build/clangherm/support/native/java.rmi/librmi/static/librmi_relocatable.o /localhome/git/jdk-ALT/build/clangherm/support/native/java.rmi/librmi/static/GC.o clang version 13.0.1 Target: x86_64-unknown-linux-gnu Thread model: posix InstalledDir: /usr/local/clang+llvm-13.0.1-x86_64-linux-gnu-ubuntu-18.04/bin Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/9 Selected GCC installation: /usr/lib/gcc/x86_64-linux-gnu/9 Candidate multilib: .;@m64 Candidate multilib: 32;@m32 Candidate multilib: x32;@mx32 Selected multilib: .;@m64 "/usr/bin/ld" --hash-style=both --eh-frame-hdr -m elf_x86_64 -dynamic-linker /lib64/ld-linux-x86-64.so.2 -o /localhome/git/jdk-ALT/build/clangherm/support/native/java.rmi/librmi/static/librmi_relocatable.o /lib/x86_64-linux-gnu/crt1.o /lib/x86_64-linux-gnu/crti.o /usr/lib/gcc/x86_64-linux-gnu/9/crtbegin.o -L/usr/lib/gcc/x86_64-linux-gnu/9 -L/usr/lib/gcc/x86_64-linux-gnu/9/../../../../lib64 -L/lib/x86_64-linux-gnu -L/lib/../lib64 -L/usr/lib/x86_64-linux-gnu -L/usr/lib/../lib64 -L/usr/local/clang+llvm-13.0.1-x86_64-linux-gnu-ubuntu-18.04/bin/../lib -L/lib -L/usr/lib -r /localhome/git/jdk-ALT/build/clangherm/support/native/java.rmi/librmi/static/GC.o -lgcc --as-needed -lgcc_s --no-as-needed -lc -lgcc --as-needed -lgcc_s --no-as-needed /usr/lib/gcc/x86_64-linux-gnu/9/crtend.o /lib/x86_64-linux-gnu/crtn.o /usr/bin/ld: cannot find -lgcc_s /usr/bin/ld: cannot find -lgcc_s clang-13: error: linker command failed with exit code 1 (use -v to see invocation) I don't understand what makes clang think it should include "-lgcc --as-needed -lgcc_s" and the crt*.o files when doing a partial link. In fact, the entire process on how clang (and gcc) builds up the linker command line is bordering on black magic to me. I think it can be affected by variables set at compile time (at least this was the case for gcc, last I checked), or maybe it picks up some kind of script from the environment. That's why I believe my machine could just be messed up. I could get a bit further by passing "-nodefaultlibs" (or whatever it was), but then the generated .o file were messed up wrt to library symbols and it failed dramatically when trying to do the final link of the static java launcher. Looks like you are using /usr/bin/ld and not lld. I haven't run into this type of issue. Have you tried -fuse-ld=lld? I am not sure why clang insisted on picking up ld and not lld. I remeber trying with -fuse-ld=lld, and that it did not work either. Unfortunately, I don't remember exactly what the problems were. I started reinstalling my Linux workstation yesterday, but something went wrong, and it failed so hard that it got semi-bricked by the new installation, so
Re: RFR: 8329257: AIX: Switch HOTSPOT_TOOLCHAIN_TYPE from xlc to gcc [v3]
On Tue, 30 Apr 2024 10:19:30 GMT, Magnus Ihse Bursie wrote: >> The compiler flag introducing __STRICT_ANSI__ is -std=c++14. If I omit this >> explicit compiler flag the default is used, which is also c++14. But the >> default does not set __STRICT_ANSI__ but 2 other defines. I will try a build >> without -std=c++14 and if this works, we have a solution. Nevertheless i >> will interrogate IBM what the hell this behavior should be. > > I don't think leaving out `-std=c++14` for AIX is a good solution. I got it. And what about simply disabling the `__STRICT_ANSI__` with `CFLAGS_OS_DEF_JVM="-DAIX -D_LARGE_FILES -U__STRICT_ANSI__"` in flags-cflags.m4 for AIX. This worked too. The build is fine. - PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1584725053
Re: RFR: 8331020: Assign LANG to C++ for Windows code that uses C++
On Wed, 24 Apr 2024 01:02:36 GMT, Julian Waters wrote: > Currently, on Windows LANG is not assigned to C++ for some code that does use > C++. This just works because link.exe does not bother about what kind of code > it is currently linking. gcc however, does. It doesn't hurt to assign LANG to > C++ as a formality in such cases, which this changeset does. This also > renames LINK_TYPE to LANG, which the original change to remove the TOOLCHAIN > parameter used to do Please revert back to the LINK_TYPE name. As long as it is not used for anything else, this is a better description. If we start to use it to have a broader meaning, we can rename it then. make/hotspot/gensrc/GensrcAdlc.gmk line 45: > 43: endif > 44: else ifeq ($(call isBuildOs, windows), true) > 45: ifeq ($(TOOLCHAIN_TYPE), microsoft) You're kind of sneaking in some of your "support other toolchain than ms on windows" changes here. While it does not matter that much, right now we have the assumption that platform=windows <=> toolchain=microsoft in the entire code base. With that assumption, this change looks strange. So I'd rather not take this in now, but instead do a complete integration of the changes needed to support multiple toolchains on Windows. make/hotspot/lib/CompileGtest.gmk line 98: > 96: -I$(GTEST_FRAMEWORK_SRC)/googlemock/include \ > 97: $(addprefix -I,$(GTEST_TEST_SRC)), \ > 98: CFLAGS_windows := -EHsc, \ Just to clarify: these kind of changes are okay, since for mainline it is equivalent if you do `CFLAGS_windows` or `CFLAGS_microsoft`, so if it helps you I am completely okay with converting one kind of check to another. (It was the double-checking that I objected to.) make/modules/java.desktop/lib/AwtLibraries.gmk line 102: > 100: $(eval $(call SetupJdkLibrary, BUILD_LIBAWT, \ > 101: NAME := awt, \ > 102: LANG := $(if $(filter $(OPENJDK_TARGET_OS), windows), C++, C), \ No, this is not okay. You need to do this as for the LIBJSOUND_LINK_TYPE above. The same goes for the one below, too. - PR Comment: https://git.openjdk.org/jdk/pull/18927#issuecomment-2085199170 PR Review Comment: https://git.openjdk.org/jdk/pull/18927#discussion_r1584720911 PR Review Comment: https://git.openjdk.org/jdk/pull/18927#discussion_r1584722581 PR Review Comment: https://git.openjdk.org/jdk/pull/18927#discussion_r1584723694
Re: RFR: 8331331: :tier1 target explanation in doc/testing.md is incorrect
On Mon, 29 Apr 2024 16:14:45 GMT, SendaoYan wrote: > Hi, > > In doc/testing.md file, it says: > > As an example, :tier1 will expand to jtreg:$(TOPDIR)/test/hotspot/jtreg:tier1 > jtreg:$(TOPDIR)/test/jdk:tier1 jtreg:$(TOPDIR)/test/langtools:tier1 > jtreg:$(TOPDIR)/test/nashorn:tier1 jtreg:$(TOPDIR)/test/jaxp:tier1. > > The actual situation is :tier1 doesn't contains test/nashorn:tier1, and the > document missed test/lib-test:tier1 > > $ make -n test-tier1 &> test-tier1.log > $ grep "Running test " test-tier1.log > Running test 'jtreg:test/hotspot/jtreg:tier1' > Running test 'jtreg:test/jdk:tier1' > Running test 'jtreg:test/langtools:tier1' > Running test 'jtreg:test/jaxp:tier1' > Running test 'jtreg:test/lib-test:tier1' > > Only change the document, no risk. This fix works, but I like David's idea better. It was meant as an example, not necessarily kept up to date, so by deliberately making it shorter and ading `...` we show that this is not am exhaustive list. - PR Comment: https://git.openjdk.org/jdk/pull/19002#issuecomment-2085193115
Re: Usage of iconv()
On 2024-04-25 20:30, Philip Race wrote: On 4/24/24 4:24 AM, Magnus Ihse Bursie wrote: That is a good question. libiconv is used only on macOS and AIX, for a few libraries, as you say. I just tried removing -liconv from the macOS dependencies and recompiled, just to see what would happen. There were three instances for macOS: libsplashscreen, libjdwp and libinstrument. libsplashscreen uses it in splashscreen_sys.m, where it is used to convert the jar file name. This is called from the launcher, in java.base/share/native/libjli/java.c libjdwp uses it in utf_util.c, where it is used to convert file name and command lines, iiuc. It is likely that we have similar (but better) ways to convert charsets elsewhere in our libraries that can be used instead of libiconv. I guess these are not the only two places where a filename must be converted from the platform charset to UTF8. So whatever replacement there might be, it needs to be something that is available very early in the life of the VM, in fact before there is a VM running. Agreed. But it seems to be that this is something that needs to be handled by libjli, to properly deal with paths and command lines. I'm wondering if the places which to *not* use iconv (or similar) is actually incorrect. /Magnus -phil.
Re: RFR: 8329257: AIX: Switch HOTSPOT_TOOLCHAIN_TYPE from xlc to gcc [v3]
On Tue, 30 Apr 2024 09:36:52 GMT, Joachim Kern wrote: >> On AIX `stdlib.h` also would define `alloca`, if `__STRICT_ANSI__` wouldn't >> be set. >> >> >> 780 #if !defined(__xlC__) || defined(__ibmxl__) || defined(__cplusplus) >> 781 #if defined(__IBMCPP__) && !defined(__ibmxl__) >> 782extern "builtin" char *__alloca (size_t); >> 783 # define alloca __alloca >> 784 #elif defined(__GNUC__) && !defined(__STRICT_ANSI__) >> 785#undef alloca >> 786#define alloca(size) __builtin_alloca (size) >> 787 #endif >> >> >> A small plain Testprogramm not using all of the flags we used in jdk build, >> does not set `__STRICT_ANSI__` and then `alloca` is defined correct. > > The compiler flag introducing __STRICT_ANSI__ is -std=c++14. If I omit this > explicit compiler flag the default is used, which is also c++14. But the > default does not set __STRICT_ANSI__ but 2 other defines. I will try a build > without -std=c++14 and if this works, we have a solution. Nevertheless i will > interrogate IBM what the hell this behavior should be. I don't think leaving out `-std=c++14` for AIX is a good solution. - PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1584529538
RFR: 8331020: Assign LANG to C++ for Windows code that uses C++
Currently, on Windows LANG is not assigned to C++ for some code that does use C++. This just works because link.exe does not bother about what kind of code it is currently linking. gcc however, does. It doesn't hurt to assign LANG to C++ as a formality in such cases, which this changeset does. This also renames LINK_TYPE to LANG, which the original change to remove the TOOLCHAIN parameter used to do - Commit messages: - Assign LANG to C++ in Lib.gmk - Assign LANG to C++ in Lib.gmk - Assign LANG to C++ in Launcher.gmk - Assign LANG to C++ in Lib.gmk - Assign LANG to C++ in AwtLibraries.gmk - Rename import in Lib.gmk - Rename ClientLibraries.gmk to 2dLibraries.gmk - LINK_TYPE to LANG in ClientLibraries.gmk - LINK_TYPE to LANG in JdkNativeCompilation.gmk - Add C++ to Windows in Lib.gmk - ... and 11 more: https://git.openjdk.org/jdk/compare/7a895552...8d0701bd Changes: https://git.openjdk.org/jdk/pull/18927/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18927&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8331020 Stats: 52 lines in 19 files changed: 15 ins; 3 del; 34 mod Patch: https://git.openjdk.org/jdk/pull/18927.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18927/head:pull/18927 PR: https://git.openjdk.org/jdk/pull/18927
Re: RFR: 8329257: AIX: Switch HOTSPOT_TOOLCHAIN_TYPE from xlc to gcc [v3]
On Mon, 29 Apr 2024 16:17:13 GMT, Joachim Kern wrote: >> For the impatient, I suggest adopting mechanism 2, i.e. unconditionally >> include in globalDefinitions_gcc.hpp. >> >> We can't include in shared code, and there is a use in shared code >> (in the relatively recently added JavaThread::pretouch_stack). >> >> When I questioned whether we needed to include at all, I referred >> to a Linux man page I'd found on the internet (the same page mdoerr linked >> to), which says (in part) >> >> "By default, modern compilers automatically translate all uses of alloca() >> into the built-in ..." >> >> Apparently I should have kept digging, because it seems that page is >> old/incorrect. A seemingly more recent Linux man page describes a different >> way of handling it that is closer to what we're seeing, but still not quite >> correct. >> >> glibc's includes if __USE_MISC is defined. >> One of the ways __USE_MISC can become defined is if _GNU_SOURCE is defined, >> and we define that for both gcc and clang toolchains. >> >> We include in globalDefinitions_gcc.hpp. So when building with >> gcc, >> globalDefinitions.hpp implicitly includes . >> >> The glibc definition of alloca is >> >> #ifdef __GNUC__ >> # define alloca(size)__builtin_alloca (size) >> #endif /* GCC. */ >> >> So that explains why we don't need any explicit include of when >> building with gcc. I expect there's something similar going on with Visual >> Studio and Xcode/clang. But apparently not with Open XLC clang. > > On AIX `stdlib.h` also would define `alloca`, if `__STRICT_ANSI__` wouldn't > be set. > > > 780 #if !defined(__xlC__) || defined(__ibmxl__) || defined(__cplusplus) > 781 #if defined(__IBMCPP__) && !defined(__ibmxl__) > 782extern "builtin" char *__alloca (size_t); > 783 # define alloca __alloca > 784 #elif defined(__GNUC__) && !defined(__STRICT_ANSI__) > 785#undef alloca > 786#define alloca(size) __builtin_alloca (size) > 787 #endif > > > A small plain Testprogramm not using all of the flags we used in jdk build, > does not set `__STRICT_ANSI__` and then `alloca` is defined correct. The compiler flag introducing __STRICT_ANSI__ is -std=c++14. If I omit this explicit compiler flag the default is used, which is also c++14. But the default does not set __STRICT_ANSI__ but 2 other defines. I will try a build without -std=c++14 and if this works, we have a solution. Nevertheless i will interrogate IBM what the hell this behavior should be. - PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1584461665
Integrated: 8331298: avoid alignment checks in UBSAN enabled build
On Mon, 29 Apr 2024 12:15:55 GMT, Matthias Baesken wrote: > Currently we run into some alignment related issues when building with > '--enable-ubsan' . Those errors already occur in the build. Fixing them might > take some time and maybe also some discussion if it is worth the effort , > So for now the alignment related checks should be disabled to get the UBSAN > build working. > Example : > > /jdk/src/hotspot/cpu/x86/macroAssembler_x86.hpp:128:13: runtime error: store > to misaligned address 0x15099c3cf4ce for type 'int', which requires 4 byte > alignment > 0x15099c3cf4ce: note: pointer points here > 00 80 0f 86 00 00 00 00 3d 06 00 00 80 76 60 3d 07 00 00 80 76 40 3d 08 00 > 00 80 76 20 3d 1e 00 > ^ > #0 0x1509b3b04f10 in MacroAssembler::pd_patch_instruction(unsigned char*, > unsigned char*, char const*, int) > /jdk/src/hotspot/cpu/x86/macroAssembler_x86.hpp:128 > #1 0x1509b3b04f10 in Label::patch_instructions(MacroAssembler*) > /jdk/src/hotspot/share/asm/assembler.cpp:201 > #2 0x1509b940b6d8 in VM_Version_StubGenerator::generate_get_cpu_info() > /jdk/src/hotspot/cpu/x86/vm_version_x86.cpp:381 > #3 0x1509b94059bd in VM_Version::initialize() > /jdk/src/hotspot/cpu/x86/vm_version_x86.cpp:2138 > #4 0x1509b93fb56e in VM_Version_init() > /jdk/src/hotspot/share/runtime/vm_version.cpp:32 > #5 0x1509b61ef947 in init_globals() > /jdk/src/hotspot/share/runtime/init.cpp:126 > #6 0x1509b8fb0e29 in Threads::create_vm(JavaVMInitArgs*, bool*) > /jdk/src/hotspot/share/runtime/threads.cpp:553 > #7 0x1509b67da3d7 in JNI_CreateJavaVM_inner > /jdk/src/hotspot/share/prims/jni.cpp:3581 > #8 0x1509b67da3d7 in JNI_CreateJavaVM > /jdk/src/hotspot/share/prims/jni.cpp:3672 > #9 0x1509c0eed957 in InitializeJVM > /jdk/src/java.base/share/native/libjli/java.c:1550 > #10 0x1509c0eed957 in JavaMain > /jdk/src/java.base/share/native/libjli/java.c:491 >... (rest of output omitted) This pull request has now been integrated. Changeset: 60b61e58 Author:Matthias Baesken URL: https://git.openjdk.org/jdk/commit/60b61e588c1252b4b1fbc64d0f818a85670f7146 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8331298: avoid alignment checks in UBSAN enabled build Reviewed-by: erikj, mdoerr - PR: https://git.openjdk.org/jdk/pull/18998
Re: RFR: 8331298: avoid alignment checks in UBSAN enabled build
On Mon, 29 Apr 2024 12:15:55 GMT, Matthias Baesken wrote: > Currently we run into some alignment related issues when building with > '--enable-ubsan' . Those errors already occur in the build. Fixing them might > take some time and maybe also some discussion if it is worth the effort , > So for now the alignment related checks should be disabled to get the UBSAN > build working. > Example : > > /jdk/src/hotspot/cpu/x86/macroAssembler_x86.hpp:128:13: runtime error: store > to misaligned address 0x15099c3cf4ce for type 'int', which requires 4 byte > alignment > 0x15099c3cf4ce: note: pointer points here > 00 80 0f 86 00 00 00 00 3d 06 00 00 80 76 60 3d 07 00 00 80 76 40 3d 08 00 > 00 80 76 20 3d 1e 00 > ^ > #0 0x1509b3b04f10 in MacroAssembler::pd_patch_instruction(unsigned char*, > unsigned char*, char const*, int) > /jdk/src/hotspot/cpu/x86/macroAssembler_x86.hpp:128 > #1 0x1509b3b04f10 in Label::patch_instructions(MacroAssembler*) > /jdk/src/hotspot/share/asm/assembler.cpp:201 > #2 0x1509b940b6d8 in VM_Version_StubGenerator::generate_get_cpu_info() > /jdk/src/hotspot/cpu/x86/vm_version_x86.cpp:381 > #3 0x1509b94059bd in VM_Version::initialize() > /jdk/src/hotspot/cpu/x86/vm_version_x86.cpp:2138 > #4 0x1509b93fb56e in VM_Version_init() > /jdk/src/hotspot/share/runtime/vm_version.cpp:32 > #5 0x1509b61ef947 in init_globals() > /jdk/src/hotspot/share/runtime/init.cpp:126 > #6 0x1509b8fb0e29 in Threads::create_vm(JavaVMInitArgs*, bool*) > /jdk/src/hotspot/share/runtime/threads.cpp:553 > #7 0x1509b67da3d7 in JNI_CreateJavaVM_inner > /jdk/src/hotspot/share/prims/jni.cpp:3581 > #8 0x1509b67da3d7 in JNI_CreateJavaVM > /jdk/src/hotspot/share/prims/jni.cpp:3672 > #9 0x1509c0eed957 in InitializeJVM > /jdk/src/java.base/share/native/libjli/java.c:1550 > #10 0x1509c0eed957 in JavaMain > /jdk/src/java.base/share/native/libjli/java.c:491 >... (rest of output omitted) Thanks for the reviews ! - PR Comment: https://git.openjdk.org/jdk/pull/18998#issuecomment-2084588114
