Re: RFR: 8265315: Support for CLDR version 41 [v2]
On Fri, 8 Apr 2022 20:17:52 GMT, Naoto Sato wrote: >> This is to upgrade the CLDR data from version 39 to version 41 which was >> released yesterday. The vast majority of the changes are basically replacing >> the CLDR data, along with tools/testcase alignments. Here is the link to >> CLDR v41's release notes: https://cldr.unicode.org/index/downloads/cldr-41 > > Naoto Sato has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 22 commits: > > - Merge branch 'master' into cldr+ > - Merge branch 'master' into cldr+ > - CLDR v41 final > - CLDR v41 beta2 > - Merge branch 'master' into cldr+ > - CLDR v41 alpha4 > - Merge branch 'master' into cldr+ > - Update copyright year to 2022 > - CLDR release v40 > - Merge branch 'master' into cldr > - ... and 12 more: > https://git.openjdk.java.net/jdk/compare/a8c87526...9ef22a6d Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8150
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
On Fri, 8 Apr 2022 14:48:41 GMT, Andrew Leonard wrote: >> I am not sure why without the explicit .file directive that the FILE symbol >> in the ELF info contains an entry pointing to the .o object file, here's >> what it was before: >> >> 31712: 0 FILELOCAL DEFAULT ABS >> /home/andrew2/jdk/build/linux-aarch64-server-release/hotspot/variant-server/libjvm/objs/atomic_linux_aarch64.o >> >> It's as if the lack of the .file has caused some tooling (linker?) to create >> this entry using the .o file path. > >> @andrew-m-leonard >> >> > So I am thinking adding a .file directive to the .S file specifying the >> > "full source path" should do the trick thanks to -debug-prefix-map, I will >> > do a test.. >> >> But then you'll end up with that absolute path name embedded into `STT_FILE` >> in the object file. It's best to use the relative path name _and_ employ >> path mapping. > > @mkartashev of course yes it would make the .o non-reproducible, so yes makes > sense "relative path name _and_ employ path mapping." @andrew-m-leonard Just to clarify, if you create a new PR, where you revert the introduced relative path linking, add `.file THIS_FILE` to all assembly (*.S) files, and pass `-DTHIS_FILE=$relative_file_name` and `-fdebug-prefix-map` to gcc, then I'm perfectly happy! - PR: https://git.openjdk.java.net/jdk/pull/8124
Re: RFR: 8265315: Support for CLDR version 41 [v2]
On Fri, 8 Apr 2022 20:17:52 GMT, Naoto Sato wrote: >> This is to upgrade the CLDR data from version 39 to version 41 which was >> released yesterday. The vast majority of the changes are basically replacing >> the CLDR data, along with tools/testcase alignments. Here is the link to >> CLDR v41's release notes: https://cldr.unicode.org/index/downloads/cldr-41 > > Naoto Sato has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 22 commits: > > - Merge branch 'master' into cldr+ > - Merge branch 'master' into cldr+ > - CLDR v41 final > - CLDR v41 beta2 > - Merge branch 'master' into cldr+ > - CLDR v41 alpha4 > - Merge branch 'master' into cldr+ > - Update copyright year to 2022 > - CLDR release v40 > - Merge branch 'master' into cldr > - ... and 12 more: > https://git.openjdk.java.net/jdk/compare/a8c87526...9ef22a6d Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8150
Integrated: 8284588: Remove GensrcCommonLangtools.gmk
On Fri, 8 Apr 2022 14:57:02 GMT, Magnus Ihse Bursie wrote: > There's been unnecessary (near) duplication of functionality, and messy > logic, in the langtools gensrc step, for a long time. (Basically since we got > rid of the mercurial forest...) > > This is a first attempt at cleaning this up. The file > GensrcCommonLangtools.gmk is removed. This contained three pieces of > functionality. > > * SetupVersionProperties was moved to GensrcCommonJdk.gmk (which was renamed > to GensrcCommon.gmk). > * PropertiesParser was only used in one place, and moved there. > (jdk.compiler/Gensrc.gmk). It was also heavily rewritten to show more clearly > what it actually is doing. > * SetupCompileProperties (from langtools) were replaced with calls to > SetupCompileProperties (from GensrcCommon/"jdk"), which was modified slightly > to accomodate for the differences. (Also the Java tool needed to be slightly > modified to accept the same syntax as the JDK version of this tool.) > > A reasonable follow-up is to research if the two SetupCompileProperties tools > can be unified into one. This pull request has now been integrated. Changeset: 3357d9a1 Author:Magnus Ihse Bursie URL: https://git.openjdk.java.net/jdk/commit/3357d9a168f031e1de4fa0d203f16a6f060fd062 Stats: 367 lines in 21 files changed: 124 ins; 185 del; 58 mod 8284588: Remove GensrcCommonLangtools.gmk Reviewed-by: erikj - PR: https://git.openjdk.java.net/jdk/pull/8167
Re: RFR: 8284588: Remove GensrcCommonLangtools.gmk [v2]
> There's been unnecessary (near) duplication of functionality, and messy > logic, in the langtools gensrc step, for a long time. (Basically since we got > rid of the mercurial forest...) > > This is a first attempt at cleaning this up. The file > GensrcCommonLangtools.gmk is removed. This contained three pieces of > functionality. > > * SetupVersionProperties was moved to GensrcCommonJdk.gmk (which was renamed > to GensrcCommon.gmk). > * PropertiesParser was only used in one place, and moved there. > (jdk.compiler/Gensrc.gmk). It was also heavily rewritten to show more clearly > what it actually is doing. > * SetupCompileProperties (from langtools) were replaced with calls to > SetupCompileProperties (from GensrcCommon/"jdk"), which was modified slightly > to accomodate for the differences. (Also the Java tool needed to be slightly > modified to accept the same syntax as the JDK version of this tool.) > > A reasonable follow-up is to research if the two SetupCompileProperties tools > can be unified into one. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Simplify PARSEPROPERTIES_ARGS - Changes: - all: https://git.openjdk.java.net/jdk/pull/8167/files - new: https://git.openjdk.java.net/jdk/pull/8167/files/11ede464..e84aa180 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8167&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8167&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8167.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8167/head:pull/8167 PR: https://git.openjdk.java.net/jdk/pull/8167
Re: RFR: 8284588: Remove GensrcCommonLangtools.gmk
On Fri, 8 Apr 2022 16:59:37 GMT, Erik Joelsson wrote: >> There's been unnecessary (near) duplication of functionality, and messy >> logic, in the langtools gensrc step, for a long time. (Basically since we >> got rid of the mercurial forest...) >> >> This is a first attempt at cleaning this up. The file >> GensrcCommonLangtools.gmk is removed. This contained three pieces of >> functionality. >> >> * SetupVersionProperties was moved to GensrcCommonJdk.gmk (which was renamed >> to GensrcCommon.gmk). >> * PropertiesParser was only used in one place, and moved there. >> (jdk.compiler/Gensrc.gmk). It was also heavily rewritten to show more >> clearly what it actually is doing. >> * SetupCompileProperties (from langtools) were replaced with calls to >> SetupCompileProperties (from GensrcCommon/"jdk"), which was modified >> slightly to accomodate for the differences. (Also the Java tool needed to be >> slightly modified to accept the same syntax as the JDK version of this tool.) >> >> A reasonable follow-up is to research if the two SetupCompileProperties >> tools can be unified into one. > > make/modules/jdk.compiler/Gensrc.gmk line 63: > >> 61: $(PARSEPROPERTIES_FILES)) >> 62: >> 63: PARSEPROPERTIES_ARGS := $(foreach file, $(PARSEPROPERTIES_FILES), \ > > This could be simplified by iterating over PARSEPROPERTIES_SRC. Indeed. :) I am a bit amazed that this code still had potential to be even more simplified. I don't even remember how much junk I throw away, and I still missed this. - PR: https://git.openjdk.java.net/jdk/pull/8167
Re: RFR: 8265315: Support for CLDR version 41 [v2]
> This is to upgrade the CLDR data from version 39 to version 41 which was > released yesterday. The vast majority of the changes are basically replacing > the CLDR data, along with tools/testcase alignments. Here is the link to CLDR > v41's release notes: https://cldr.unicode.org/index/downloads/cldr-41 Naoto Sato has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 22 commits: - Merge branch 'master' into cldr+ - Merge branch 'master' into cldr+ - CLDR v41 final - CLDR v41 beta2 - Merge branch 'master' into cldr+ - CLDR v41 alpha4 - Merge branch 'master' into cldr+ - Update copyright year to 2022 - CLDR release v40 - Merge branch 'master' into cldr - ... and 12 more: https://git.openjdk.java.net/jdk/compare/a8c87526...9ef22a6d - Changes: https://git.openjdk.java.net/jdk/pull/8150/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8150&range=01 Stats: 132400 lines in 859 files changed: 96406 ins; 4216 del; 31778 mod Patch: https://git.openjdk.java.net/jdk/pull/8150.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8150/head:pull/8150 PR: https://git.openjdk.java.net/jdk/pull/8150
Re: RFR: 8265315: Support for CLDR version 41
On Thu, 7 Apr 2022 21:20:20 GMT, Naoto Sato wrote: > This is to upgrade the CLDR data from version 39 to version 41 which was > released yesterday. The vast majority of the changes are basically replacing > the CLDR data, along with tools/testcase alignments. Here is the link to CLDR > v41's release notes: https://cldr.unicode.org/index/downloads/cldr-41 Marked as reviewed by ihse (Reviewer). Great, thanks! - PR: https://git.openjdk.java.net/jdk/pull/8150
Re: RFR: 8265315: Support for CLDR version 41
On Thu, 7 Apr 2022 21:20:20 GMT, Naoto Sato wrote: > This is to upgrade the CLDR data from version 39 to version 41 which was > released yesterday. The vast majority of the changes are basically replacing > the CLDR data, along with tools/testcase alignments. Here is the link to CLDR > v41's release notes: https://cldr.unicode.org/index/downloads/cldr-41 I measured the execution time of `CLDRConverter`. Each run measures an average execution time for 5 invocations of `CLDRConverter.main()`, and I ran it 5 times for `java.base` and `jdk.localedata` modules. All average values are in seconds. - Before the fix - java.base: 11.536 - 12.836 - jdk.localedata: 19.764 - 22.584 - After the fix - java.base: 11.666 - 12.833 - jdk.localedata: 19.183 - 20.838 Looking at the results, I don't see any notable tool execution time difference with this change. - PR: https://git.openjdk.java.net/jdk/pull/8150
Re: RFR: 8284588: Remove GensrcCommonLangtools.gmk
On Fri, 8 Apr 2022 14:57:02 GMT, Magnus Ihse Bursie wrote: > There's been unnecessary (near) duplication of functionality, and messy > logic, in the langtools gensrc step, for a long time. (Basically since we got > rid of the mercurial forest...) > > This is a first attempt at cleaning this up. The file > GensrcCommonLangtools.gmk is removed. This contained three pieces of > functionality. > > * SetupVersionProperties was moved to GensrcCommonJdk.gmk (which was renamed > to GensrcCommon.gmk). > * PropertiesParser was only used in one place, and moved there. > (jdk.compiler/Gensrc.gmk). It was also heavily rewritten to show more clearly > what it actually is doing. > * SetupCompileProperties (from langtools) were replaced with calls to > SetupCompileProperties (from GensrcCommon/"jdk"), which was modified slightly > to accomodate for the differences. (Also the Java tool needed to be slightly > modified to accept the same syntax as the JDK version of this tool.) > > A reasonable follow-up is to research if the two SetupCompileProperties tools > can be unified into one. Marked as reviewed by erikj (Reviewer). make/modules/jdk.compiler/Gensrc.gmk line 63: > 61: $(PARSEPROPERTIES_FILES)) > 62: > 63: PARSEPROPERTIES_ARGS := $(foreach file, $(PARSEPROPERTIES_FILES), \ This could be simplified by iterating over PARSEPROPERTIES_SRC. - PR: https://git.openjdk.java.net/jdk/pull/8167
RFR: 8284588: Remove GensrcCommonLangtools.gmk
There's been unnecessary (near) duplication of functionality, and messy logic, in the langtools gensrc step, for a long time. (Basically since we got rid of the mercurial forest...) This is a first attempt at cleaning this up. The file GensrcCommonLangtools.gmk is removed. This contained three pieces of functionality. * SetupVersionProperties was moved to GensrcCommonJdk.gmk (which was renamed to GensrcCommon.gmk). * PropertiesParser was only used in one place, and moved there. (jdk.compiler/Gensrc.gmk). It was also heavily rewritten to show more clearly what it actually is doing. * SetupCompileProperties (from langtools) were replaced with calls to SetupCompileProperties (from GensrcCommon/"jdk"), which was modified slightly to accomodate for the differences. (Also the Java tool needed to be slightly modified to accept the same syntax as the JDK version of this tool.) A reasonable follow-up is to research if the two SetupCompileProperties tools can be unified into one. - Commit messages: - Unify SetupCompilePropertiesLangtools with SetupCompileProperties - Get rid of GensrcCommonLangtools.gmk - Clean up parseproperties Changes: https://git.openjdk.java.net/jdk/pull/8167/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8167&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284588 Stats: 367 lines in 21 files changed: 124 ins; 185 del; 58 mod Patch: https://git.openjdk.java.net/jdk/pull/8167.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8167/head:pull/8167 PR: https://git.openjdk.java.net/jdk/pull/8167
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
On Fri, 8 Apr 2022 14:42:34 GMT, Andrew Leonard wrote: >> Andrew Leonard has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Trigger checks > > I am not sure why without the explicit .file directive that the FILE symbol > in the ELF info contains an entry pointing to the .o object file, here's what > it was before: > > 31712: 0 FILELOCAL DEFAULT ABS > /home/andrew2/jdk/build/linux-aarch64-server-release/hotspot/variant-server/libjvm/objs/atomic_linux_aarch64.o > > It's as if the lack of the .file has caused some tooling (linker?) to create > this entry using the .o file path. > @andrew-m-leonard > > > So I am thinking adding a .file directive to the .S file specifying the > > "full source path" should do the trick thanks to -debug-prefix-map, I will > > do a test.. > > But then you'll end up with that absolute path name embedded into `STT_FILE` > in the object file. It's best to use the relative path name _and_ employ path > mapping. @mkartashev of course yes it would make the .o non-reproducible, so yes makes sense "relative path name _and_ employ path mapping." - PR: https://git.openjdk.java.net/jdk/pull/8124
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
On Wed, 6 Apr 2022 13:30:28 GMT, Andrew Leonard wrote: >> This PR fixes the non-deterministic behavior when building on linux with >> different userids or within >> different workspace folders. >> It fixes the following issues: >> - MakeZipReproducible.java used to produce reproducible src.zip removes the >> optional zip "extra" field containing UID/GID. >> - When absolute output paths are not allowed, enable the use of >> -fdebug-prefix-map to ensure debug symbol info does not contain the top >> level workspace folder. >> - For reproducible builds ensure the gcc random symbol name generator is >> seeded using -frandom-seed. >> - For reproducible builds when producing debug symbols use relative object >> paths for library linking to remove absolute MASM object paths. >> >> Signed-off-by: Andrew Leonard > > Andrew Leonard has updated the pull request incrementally with one additional > commit since the last revision: > > Trigger checks I am not sure why without the explicit .file directive that the FILE symbol in the ELF info contains an entry pointing to the .o object file, here's what it was before: 31712: 0 FILELOCAL DEFAULT ABS /home/andrew2/jdk/build/linux-aarch64-server-release/hotspot/variant-server/libjvm/objs/atomic_linux_aarch64.o It's as if the lack of the .file has caused some tooling (linker?) to create this entry using the .o file path. - PR: https://git.openjdk.java.net/jdk/pull/8124
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
On Fri, 8 Apr 2022 14:42:34 GMT, Andrew Leonard wrote: >> Andrew Leonard has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Trigger checks > > I am not sure why without the explicit .file directive that the FILE symbol > in the ELF info contains an entry pointing to the .o object file, here's what > it was before: > > 31712: 0 FILELOCAL DEFAULT ABS > /home/andrew2/jdk/build/linux-aarch64-server-release/hotspot/variant-server/libjvm/objs/atomic_linux_aarch64.o > > It's as if the lack of the .file has caused some tooling (linker?) to create > this entry using the .o file path. @andrew-m-leonard > So I am thinking adding a .file directive to the .S file specifying the "full > source path" should do the trick thanks to -debug-prefix-map, I will do a > test.. But then you'll end up with that absolute path name embedded into `STT_FILE` in the object file. It's best to use the relative path name *and* employ path mapping. @andrew-m-leonard > It's as if the lack of the .file has caused some tooling (linker?) to create > this entry using the .o file path. I'm sure that's exactly what happens; the linker is the culprit. - PR: https://git.openjdk.java.net/jdk/pull/8124
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
On Fri, 8 Apr 2022 14:18:50 GMT, Maxim Kartashev wrote: >> @magicus >>> Is it possible to pass the ".file" directive on the command line? >> >> I don't think so. Your other idea works quite well, though. If you have this >> in the source >> >>.file THIS_FILE >> >> and you specify this on the command line >> >> -DTHIS_FILE='"src/hotspot/os_cpu/linux_x86/linux_x86_64.S"' >> >> you get the same reproducible and debuggable result. > >> @mkartashev Why don't you try building with clang as two different users, in >> two different directories, and see if the build is non-reproducible without >> this patch, but is reproducible with it (incl. your suggested change to >> include clang as well)? > > I'm going to do this on Mac, but perhaps not in the immediate future. If > someone has the time for this right now, please jump ahead of me in the line. @mkartashev yes you're right, here's the dwarf for the compilation unit that I changed to add the .file to: COMPILE_UNIT: < 0><0x000b> DW_TAG_compile_unit DW_AT_stmt_list 0x0022146d DW_AT_low_pc0x004707c0 DW_AT_high_pc 0x00470a40 DW_AT_name atomic_linux_aarch64.S DW_AT_comp_dir make/hotspot DW_AT_producer GNU AS 2.27 DW_AT_language DW_LANG_Mips_Assembler So it needs the full source path specifying as you said. Note however -debug-prefix-map has affected the DW_AT_comp_dir, removing the absolute workspace folder. Note, previously this is what the same entry had: COMPILE_UNIT: < 0><0x000b> DW_TAG_compile_unit DW_AT_stmt_list 0x00224f82 DW_AT_low_pc0x004707c0 DW_AT_high_pc 0x00470a40 DW_AT_name /home/andrew2/jdk/src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.S DW_AT_comp_dir /home/andrew2/jdk/make/hotspot DW_AT_producer GNU AS 2.27 DW_AT_language DW_LANG_Mips_Assembler So I am thinking adding a .file directive to the .S file specifying the "full source path" should do the trick thanks to -debug-prefix-map, I will do a test.. - PR: https://git.openjdk.java.net/jdk/pull/8124
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
On Fri, 8 Apr 2022 14:16:03 GMT, Maxim Kartashev wrote: >> @mkartashev Why don't you try building with clang as two different users, in >> two different directories, and see if the build is non-reproducible without >> this patch, but is reproducible with it (incl. your suggested change to >> include clang as well)? > > @magicus >> Is it possible to pass the ".file" directive on the command line? > > I don't think so. Your other idea works quite well, though. If you have this > in the source > >.file THIS_FILE > > and you specify this on the command line > > -DTHIS_FILE='"src/hotspot/os_cpu/linux_x86/linux_x86_64.S"' > > you get the same reproducible and debuggable result. > @mkartashev Why don't you try building with clang as two different users, in > two different directories, and see if the build is non-reproducible without > this patch, but is reproducible with it (incl. your suggested change to > include clang as well)? I'm going to do this on Mac, but perhaps not in the immediate future. If someone has the time for this right now, please jump ahead of me in the line. - PR: https://git.openjdk.java.net/jdk/pull/8124
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
On Fri, 8 Apr 2022 14:11:56 GMT, Magnus Ihse Bursie wrote:
>> I did some experiments both with `gcc` and `clang`. Both respect the `.file`
>> directive and put into the resulting ELF whatever was written after that
>> keyword, so no path mapping is necessary. However, debug information suffers
>> because `gcc` puts the exact same string into DWARF that debuggers need, so
>> writing
>>
>> .file "linux_x86_64.S"
>>
>> gives you `DW_AT_NAME("linux_x86_64.S")`, which is not very useful (BTW,
>> `clang` accurately points to the absolute path name of the source file) . So
>> you have to use the relative path name there:
>>
>> .file "src/hotspot/os_cpu/linux_x86/linux_x86_64.S"
>
> @mkartashev Why don't you try building with clang as two different users, in
> two different directories, and see if the build is non-reproducible without
> this patch, but is reproducible with it (incl. your suggested change to
> include clang as well)?
@magicus
> Is it possible to pass the ".file" directive on the command line?
I don't think so. Your other idea works quite well, though. If you have this in
the source
.file THIS_FILE
and you specify this on the command line
-DTHIS_FILE='"src/hotspot/os_cpu/linux_x86/linux_x86_64.S"'
you get the same reproducible and debuggable result.
-
PR: https://git.openjdk.java.net/jdk/pull/8124
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
On Wed, 6 Apr 2022 13:30:28 GMT, Andrew Leonard wrote:
>> This PR fixes the non-deterministic behavior when building on linux with
>> different userids or within
>> different workspace folders.
>> It fixes the following issues:
>> - MakeZipReproducible.java used to produce reproducible src.zip removes the
>> optional zip "extra" field containing UID/GID.
>> - When absolute output paths are not allowed, enable the use of
>> -fdebug-prefix-map to ensure debug symbol info does not contain the top
>> level workspace folder.
>> - For reproducible builds ensure the gcc random symbol name generator is
>> seeded using -frandom-seed.
>> - For reproducible builds when producing debug symbols use relative object
>> paths for library linking to remove absolute MASM object paths.
>>
>> Signed-off-by: Andrew Leonard
>
> Andrew Leonard has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Trigger checks
I did some experiments both with `gcc` and `clang`. Both respect the `.file`
directive and put into the resulting ELF whatever was written after that
keyword, so no path mapping is necessary. However, debug information suffers
because `gcc` puts the exact same string into DWARF that debuggers need, so
writing
.file "linux_x86_64.S"
gives you `DW_AT_NAME("linux_x86_64.S")`, which is not very useful (BTW,
`clang` accurately points to the absolute path name of the source file) . So
you have to use the relative path name there:
.file "src/hotspot/os_cpu/linux_x86/linux_x86_64.S"
-
PR: https://git.openjdk.java.net/jdk/pull/8124
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
On Fri, 8 Apr 2022 14:10:05 GMT, Maxim Kartashev wrote:
>> Andrew Leonard has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Trigger checks
>
> I did some experiments both with `gcc` and `clang`. Both respect the `.file`
> directive and put into the resulting ELF whatever was written after that
> keyword, so no path mapping is necessary. However, debug information suffers
> because `gcc` puts the exact same string into DWARF that debuggers need, so
> writing
>
> .file "linux_x86_64.S"
>
> gives you `DW_AT_NAME("linux_x86_64.S")`, which is not very useful (BTW,
> `clang` accurately points to the absolute path name of the source file) . So
> you have to use the relative path name there:
>
> .file "src/hotspot/os_cpu/linux_x86/linux_x86_64.S"
@mkartashev Why don't you try building with clang as two different users, in
two different directories, and see if the build is non-reproducible without
this patch, but is reproducible with it (incl. your suggested change to include
clang as well)?
-
PR: https://git.openjdk.java.net/jdk/pull/8124
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
On Wed, 6 Apr 2022 13:30:28 GMT, Andrew Leonard wrote: >> This PR fixes the non-deterministic behavior when building on linux with >> different userids or within >> different workspace folders. >> It fixes the following issues: >> - MakeZipReproducible.java used to produce reproducible src.zip removes the >> optional zip "extra" field containing UID/GID. >> - When absolute output paths are not allowed, enable the use of >> -fdebug-prefix-map to ensure debug symbol info does not contain the top >> level workspace folder. >> - For reproducible builds ensure the gcc random symbol name generator is >> seeded using -frandom-seed. >> - For reproducible builds when producing debug symbols use relative object >> paths for library linking to remove absolute MASM object paths. >> >> Signed-off-by: Andrew Leonard > > Andrew Leonard has updated the pull request incrementally with one additional > commit since the last revision: > > Trigger checks Is it possible to pass the ".file" directive on the command line? It would certainly be nicer if this was setup by the compile command line, rather than forcing developers to change a line directive in the source code if they ever rename a file (even though the chances of this happening is rather slim.) Or perhaps at least we can do something like: .file FILENAME and then pass `-DFILENAME=$1_RELATIVE_FILENAME` on the command line? Then we'd just need to add a bunch of never-changing lines to all assembly lines, as a boilerplate kind of thing. That sounds much more palatable to me. - PR: https://git.openjdk.java.net/jdk/pull/8124
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
On Fri, 8 Apr 2022 13:23:23 GMT, Andrew Leonard wrote:
> I can't seem to find any online assembly documentation(?) that states that a
> .file symbol pointing to the absolute object file name is produced, but it
> obviously does! In fact maybe it's to do with how the linker generates them,
> as using relative path for the link changed it...?
@andrew-m-leonard
Well, I also didn't take this idea from the documentation, but having
previously worked on assemblers I thought this plausible. After all, the
assembler has to provide *some* value for the `STT_FILE` symbol, why not take
literally whatever the user specified in the `.file` directive?
> Will that not affect the ability to debug these files, as GDB won't know
> where to find source files
@erikj79
First, we should remember that these are assembly files, so even total absence
of the source code wouldn't hurt a lot (apart from comments, of which there is
little).
Having said that, there's no reason to willingly give up on the on the ability
of the debugger to find the corresponding source code. The value of `STT_FILE`
is the last resort of the debugger (`dbx` uses it, but I'm not even sure that
`gdb` does). The primary source of this info is the value of `DW_AT_name` of
`DW_TAG_compile_unit` in the file's DWARF `.debug_info` section. For instance,
this is what I'm currently getting in the release build:
build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o:
file format ELF64-x86-64
.debug_info contents:
0x: Compile Unit: length = 0x002a version = 0x0002 abbr_offset =
0x addr_size = 0x08 (next unit at 0x002e)
0x000b: DW_TAG_compile_unit
DW_AT_stmt_list (0x)
DW_AT_low_pc (0x)
DW_AT_high_pc (0x0423)
DW_AT_name
("/home/work/work/OpenJDK/jdk/src/hotspot/os_cpu/linux_x86/linux_x86_64.S")
DW_AT_comp_dir("/home/work/work/OpenJDK/jdk/make/hotspot")
DW_AT_producer("GNU AS 2.34")
DW_AT_language(DW_LANG_Mips_Assembler)
This `DW_AT_name` better not change to just `linux_x86_64.S` or else `STT_FILE`
wouldn't help even if it contained the absolute path.
-
PR: https://git.openjdk.java.net/jdk/pull/8124
RFR: 8284105: Update security libraries to use sealed classes
Please review these changes to update the security libraries to use sealed classes. See JEP 409 for more details on sealed classes. No CSR is required as all the changes are to internal classes. A few classes that did not have subclasses were simply marked final instead of sealed. - Commit messages: - Update security libraries to use sealed classes. Changes: https://git.openjdk.java.net/jdk/pull/8165/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8165&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284105 Stats: 50 lines in 20 files changed: 8 ins; 0 del; 42 mod Patch: https://git.openjdk.java.net/jdk/pull/8165.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8165/head:pull/8165 PR: https://git.openjdk.java.net/jdk/pull/8165
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
On Fri, 8 Apr 2022 13:23:23 GMT, Andrew Leonard wrote: > @magicus I will create a new PR, with .file <...S> directives added to the > linux platform assembly files, and undo the relative path linking. Does that > seem reasonable? Will that not affect the ability to debug these files, as GDB won't know where to find source files, or am I misunderstanding what the .file directive does? - PR: https://git.openjdk.java.net/jdk/pull/8124
Re: RFR: 8242181: [Linux] Show source information when printing native stack traces in hs_err files [v8]
On Fri, 8 Apr 2022 13:02:00 GMT, Erik Joelsson wrote: >> Christian Hagedorn has updated the pull request with a new target base due >> to a merge or a rebase. The pull request now contains 60 commits: >> >> - Merge branch 'master' into JDK-8242181 >> - Fix build, add GCC flag gdwarf-4 to exclude DWARF 5, add assertions >> - Apply remaining review comments from Thomas Stuefe >> - Change load_dwarf_file with DwarfFilePath and DebugInfo >> - Revert renaming on Windows >> - Merge branch 'master' into JDK-8242181 >> - Updating some comments >> - Cleanup loading dwarf file and add summary >> - Review comments of first pass by Thomas except dwarf file loading >> - Merge branch 'master' into JDK-8242181 >> - ... and 50 more: >> https://git.openjdk.java.net/jdk/compare/60281810...229f1b90 > > make/autoconf/flags-cflags.m4 line 116: > >> 114: fi >> 115: >> 116: CFLAGS_DEBUG_SYMBOLS="-g -gdwarf-4" > > We may need to guard this with a FLAGS_COMPILER_CHECK_ARGUMENTS. Perhaps it > should also be applied only on Linux since the whole feature is Linux only. > What do you think @magicus? I'm googling around for some information about -gdwarf-4 but is mostly coming up empty handed. :( I found [this](https://www.phoronix.com/scan.php?page=news_item&px=GCC-11-DWARF-5-Possible-Default) saying that dwarf-5 became default in gcc11. It also claims dwarf-4 is about 10 years old. My guess is that all our supported gcc versions do support -gdwarf-4, but this needs to be verified. In practice, we only use gcc on linux so I'm not convinced we need to have an addition check for that. If we ever are going to support gcc on other OSes I think we'll have many more, much harder problems, than to remove the -gdwarf-4 flag. - PR: https://git.openjdk.java.net/jdk/pull/7126
Integrated: 8284507: GHA: Only check test results if testing was not skipped
On Thu, 7 Apr 2022 07:53:38 GMT, Christoph Langer wrote: > In GitHub Actions the step "Check that all tests executed successfully" will > be marked as failing when the "Run tests" step did not run but some earlier > step already failed. This is irritating and it can be corrected by doing the > test check only if testing was not skipped. > > Here is a link to such a test run where the check failed although the issue > was with the cygwin installation: > https://github.com/GoeLin/jdk11u-dev/runs/5788778433?check_suite_focus=true This pull request has now been integrated. Changeset: 8eac3427 Author:Christoph Langer URL: https://git.openjdk.java.net/jdk/commit/8eac3427b1d3932378965c7dce26853d1e1a04d9 Stats: 8 lines in 1 file changed: 4 ins; 0 del; 4 mod 8284507: GHA: Only check test results if testing was not skipped Reviewed-by: shade, ihse - PR: https://git.openjdk.java.net/jdk/pull/8139
Re: RFR: 8284507: GHA: Only check test results if testing was not skipped
On Thu, 7 Apr 2022 15:29:59 GMT, Magnus Ihse Bursie wrote: >> In GitHub Actions the step "Check that all tests executed successfully" will >> be marked as failing when the "Run tests" step did not run but some earlier >> step already failed. This is irritating and it can be corrected by doing the >> test check only if testing was not skipped. >> >> Here is a link to such a test run where the check failed although the issue >> was with the cygwin installation: >> https://github.com/GoeLin/jdk11u-dev/runs/5788778433?check_suite_focus=true > > Marked as reviewed by ihse (Reviewer). Thanks @magicus - PR: https://git.openjdk.java.net/jdk/pull/8139
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
On Fri, 8 Apr 2022 13:11:32 GMT, Maxim Kartashev wrote: >>> While we're at it, let me repeat my question from the alias: >>> >>> I was wondering why were the changes in `make/autoconf/flags-cflags.m4` >>> made under: >>> >>> ``` >>> if test "x$TOOLCHAIN_TYPE" = xgcc; then ... >>> ``` >>> >>> but not also propagated to the clang branch down below >>> >>> ``` >>> elif test "x$TOOLCHAIN_TYPE" = xclang; then ... >>> ``` >>> >>> ? >>> >>> It seems like the same would be required to make builds by `clang` >>> reproducible and `clang` does support the same option, spelling and all. >> >> That is simply because I have not test full reproducibility on MacOS, the >> intention of this PR was for Linux platforms. >> Although MacOS is my next port of investigation for reproducibility as a >> whole.. > >> That is simply because I have not test full reproducibility on MacOS, the >> intention of this PR was for Linux platforms. >> Although MacOS is my next port of investigation for reproducibility as a >> whole.. > > @andrew-m-leonard Thank you for the clarification! > > Again, FWIW doing same on MacOS achieved the same reproducible result as on > Linux. The only thing I won't vouch for is how necessary it is on MacOS; > maybe some options can be omitted for `clang`, for example. I simply did the > exact same thing without much investigation as to why it worked. @mkartashev Maxim, just tried your solution on one of the assembly files, and it does resolve the absolute path for it. I did a readelf, and I can see the entry that was absolute before is now what I specified in the .S assembly: 31712: 0 FILELOCAL DEFAULT ABS atomic_linux_aarch64.S 31714: 0 FILELOCAL DEFAULT ABS /home/andrew3/jdk/build/l 31725: 0 FILELOCAL DEFAULT ABS /home/andrew3/jdk/build/l I can't seem to find any online assembly documentation(?) that states that a .file symbol pointing to the absolute object file name is produced, but it obviously does! In fact maybe it's to do with how the linker generates them, as using relative path for the link changed it...? @magicus I will create a new PR, with .file <...S> directives added to the linux platform assembly files, and undo the relative path linking. Does that seem reasonable? - PR: https://git.openjdk.java.net/jdk/pull/8124
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
On Fri, 8 Apr 2022 12:57:58 GMT, Andrew Leonard wrote: > That is simply because I have not test full reproducibility on MacOS, the > intention of this PR was for Linux platforms. > Although MacOS is my next port of investigation for reproducibility as a > whole.. @andrew-m-leonard Thank you for the clarification! Again, FWIW doing same on MacOS achieved the same reproducible result as on Linux. The only thing I won't vouch for is how necessary it is on MacOS; maybe some options can be omitted for `clang`, for example. I simply did the exact same thing without much investigation as to why it worked. - PR: https://git.openjdk.java.net/jdk/pull/8124
Re: RFR: 8242181: [Linux] Show source information when printing native stack traces in hs_err files [v8]
On Fri, 8 Apr 2022 11:11:29 GMT, Christian Hagedorn wrote: >> When printing the native stack trace on Linux (mostly done for hs_err >> files), it only prints the method with its parameters and a relative offset >> in the method: >> >> Stack: [0x7f6e01739000,0x7f6e0183a000], sp=0x7f6e01838110, >> free space=1020k >> Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native >> code) >> V [libjvm.so+0x620d86] Compilation::~Compilation()+0x64 >> V [libjvm.so+0x624b92] Compiler::compile_method(ciEnv*, ciMethod*, int, >> bool, DirectiveSet*)+0xec >> V [libjvm.so+0x8303ef] >> CompileBroker::invoke_compiler_on_method(CompileTask*)+0x899 >> V [libjvm.so+0x82f067] CompileBroker::compiler_thread_loop()+0x3df >> V [libjvm.so+0x84f0d1] CompilerThread::thread_entry(JavaThread*, >> JavaThread*)+0x69 >> V [libjvm.so+0x1209329] JavaThread::thread_main_inner()+0x15d >> V [libjvm.so+0x12091c9] JavaThread::run()+0x167 >> V [libjvm.so+0x1206ada] Thread::call_run()+0x180 >> V [libjvm.so+0x1012e55] thread_native_entry(Thread*)+0x18f >> >> This makes it sometimes difficult to see where exactly the methods were >> called from and sometimes almost impossible when there are multiple >> invocations of the same method within one method. >> >> This patch improves this by providing source information (filename + line >> number) to the native stack traces on Linux similar to what's already done >> on Windows (see >> [JDK-8185712](https://bugs.openjdk.java.net/browse/JDK-8185712)): >> >> Stack: [0x7f34fca18000,0x7f34fcb19000], sp=0x7f34fcb17110, >> free space=1020k >> Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native >> code) >> V [libjvm.so+0x620d86] Compilation::~Compilation()+0x64 >> (c1_Compilation.cpp:607) >> V [libjvm.so+0x624b92] Compiler::compile_method(ciEnv*, ciMethod*, int, >> bool, DirectiveSet*)+0xec (c1_Compiler.cpp:250) >> V [libjvm.so+0x8303ef] >> CompileBroker::invoke_compiler_on_method(CompileTask*)+0x899 >> (compileBroker.cpp:2291) >> V [libjvm.so+0x82f067] CompileBroker::compiler_thread_loop()+0x3df >> (compileBroker.cpp:1966) >> V [libjvm.so+0x84f0d1] CompilerThread::thread_entry(JavaThread*, >> JavaThread*)+0x69 (compilerThread.cpp:59) >> V [libjvm.so+0x1209329] JavaThread::thread_main_inner()+0x15d >> (thread.cpp:1297) >> V [libjvm.so+0x12091c9] JavaThread::run()+0x167 (thread.cpp:1280) >> V [libjvm.so+0x1206ada] Thread::call_run()+0x180 (thread.cpp:358) >> V [libjvm.so+0x1012e55] thread_native_entry(Thread*)+0x18f >> (os_linux.cpp:705) >> >> For Linux, we need to parse the debug symbols which are generated by GCC in >> DWARF - a standardized debugging format. This patch adds support for DWARF >> 4, the default of GCC 10.x, for 32 and 64 bit architectures (tested with >> x86_32, x86_64 and AArch64). DWARF 5 is not supported as it was still >> experimental and not generated for HotSpot. However, newer GCC version may >> soon generate DWARF 5 by default in which case this parser either needs to >> be extended or the build of HotSpot configured to only emit DWARF 4. >> >> The code follows the parsing steps described in the official DWARF 4 spec: >> https://dwarfstd.org/doc/DWARF4.pdf >> I added references to the corresponding sections throughout the code. >> However, I tried to explain the steps from the DWARF spec directly in the >> code (method names, comments etc.). This allows to follow the code without >> the need to actually deep dive into the spec. >> >> The comments at the `Dwarf` class in the `elf.hpp` file explain in more >> detail how a DWARF file is structured and how the parsing algorithm works to >> get to the filename and line number information. There are more class >> comments throughout the `elf.hpp` file about how different DWARF sections >> are structured and how the parsing algorithm needs to fetch the required >> information. Therefore, I will not repeat the exact workings of the >> algorithm here but refer to the code comments. I've tried to add as much >> information as possible to improve the readability. >> >> Generally, I've tried to stay away from adding any assertions as this code >> is almost always executed when already processing a VM error. Instead, the >> DWARF parser aims to just exit gracefully and possibly omit source >> information for a stack frame instead of risking to stop writing the hs_err >> file when an assertion would have failed. To debug failures, `-Xlog:dwarf` >> can be used with `info`, `debug` or `trace` which provides logging messages >> throughout parsing. >> >> **Testing:** >> Apart from manual testing, I've added two kinds of tests: >> - A JTreg test: Spawns new VMs to let them crash in various ways. The test >> reads the created hs_err files to check if the DWARF parsing could correctly >> find the filename and line number. For normal HotSpot files, I could not >> check against hardcoded fil
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
On Fri, 8 Apr 2022 12:43:46 GMT, Maxim Kartashev wrote: > While we're at it, let me repeat my question from the alias: > > I was wondering why were the changes in `make/autoconf/flags-cflags.m4` made > under: > > ``` > if test "x$TOOLCHAIN_TYPE" = xgcc; then ... > ``` > > but not also propagated to the clang branch down below > > ``` > elif test "x$TOOLCHAIN_TYPE" = xclang; then ... > ``` > > ? > > It seems like the same would be required to make builds by `clang` > reproducible and `clang` does support the same option, spelling and all. That is simply because I have not test full reproducibility on MacOS, the intention of this PR was for Linux platforms. Although MacOS is my next port of investigation for reproducibility as a whole.. - PR: https://git.openjdk.java.net/jdk/pull/8124
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
On Wed, 6 Apr 2022 13:30:28 GMT, Andrew Leonard wrote: >> This PR fixes the non-deterministic behavior when building on linux with >> different userids or within >> different workspace folders. >> It fixes the following issues: >> - MakeZipReproducible.java used to produce reproducible src.zip removes the >> optional zip "extra" field containing UID/GID. >> - When absolute output paths are not allowed, enable the use of >> -fdebug-prefix-map to ensure debug symbol info does not contain the top >> level workspace folder. >> - For reproducible builds ensure the gcc random symbol name generator is >> seeded using -frandom-seed. >> - For reproducible builds when producing debug symbols use relative object >> paths for library linking to remove absolute MASM object paths. >> >> Signed-off-by: Andrew Leonard > > Andrew Leonard has updated the pull request incrementally with one additional > commit since the last revision: > > Trigger checks While we're at it, let me repeat my question from the alias: I was wondering why were the changes in `make/autoconf/flags-cflags.m4` made under: if test "x$TOOLCHAIN_TYPE" = xgcc; then ... but not also propagated to the clang branch down below elif test "x$TOOLCHAIN_TYPE" = xclang; then ... ? It seems like the same would be required to make builds by `clang` reproducible and `clang` does support the same option, spelling and all. - PR: https://git.openjdk.java.net/jdk/pull/8124
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
On Fri, 8 Apr 2022 12:24:38 GMT, Maxim Kartashev wrote: > FWIW, I (locally) solved the problem of absolute path names in the compiled > assembly by adding the `.file` directive. For example: > > ``` > --- a/src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.S > +++ b/src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.S > @@ -19,7 +19,7 @@ > // or visit www.oracle.com if you need additional information or have any > // questions. > > - > +.file "atomic_linux_aarch64.S" > > .text > ``` @mkartashev Maxim, I think that might be worth a LOT :-) thank you, i'll have a look at that, that maybe the solution, I hadn't grasped the problem might be assembly compiled object path or something, cheers - PR: https://git.openjdk.java.net/jdk/pull/8124
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
On Wed, 6 Apr 2022 13:30:28 GMT, Andrew Leonard wrote: >> This PR fixes the non-deterministic behavior when building on linux with >> different userids or within >> different workspace folders. >> It fixes the following issues: >> - MakeZipReproducible.java used to produce reproducible src.zip removes the >> optional zip "extra" field containing UID/GID. >> - When absolute output paths are not allowed, enable the use of >> -fdebug-prefix-map to ensure debug symbol info does not contain the top >> level workspace folder. >> - For reproducible builds ensure the gcc random symbol name generator is >> seeded using -frandom-seed. >> - For reproducible builds when producing debug symbols use relative object >> paths for library linking to remove absolute MASM object paths. >> >> Signed-off-by: Andrew Leonard > > Andrew Leonard has updated the pull request incrementally with one additional > commit since the last revision: > > Trigger checks FWIW, I (locally) solved the problem of absolute path names in the compiled assembly by adding the `.file` directive. For example: --- a/src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.S +++ b/src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.S @@ -19,7 +19,7 @@ // or visit www.oracle.com if you need additional information or have any // questions. - +.file "atomic_linux_aarch64.S" .text - PR: https://git.openjdk.java.net/jdk/pull/8124
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
On Wed, 6 Apr 2022 13:30:28 GMT, Andrew Leonard wrote: >> This PR fixes the non-deterministic behavior when building on linux with >> different userids or within >> different workspace folders. >> It fixes the following issues: >> - MakeZipReproducible.java used to produce reproducible src.zip removes the >> optional zip "extra" field containing UID/GID. >> - When absolute output paths are not allowed, enable the use of >> -fdebug-prefix-map to ensure debug symbol info does not contain the top >> level workspace folder. >> - For reproducible builds ensure the gcc random symbol name generator is >> seeded using -frandom-seed. >> - For reproducible builds when producing debug symbols use relative object >> paths for library linking to remove absolute MASM object paths. >> >> Signed-off-by: Andrew Leonard > > Andrew Leonard has updated the pull request incrementally with one additional > commit since the last revision: > > Trigger checks @magicus now i've done a build without using -fdebug-prefix-map, and the debuginfo obviously contains lots of absolute paths to the "src" cpp/c/hpp/.. files, HOWEVER, if I grep for ".o" file names, I still only get the 3 hotspot assembly objects: andrew@andrew:~$ strings /home/andrew2/jdk/build/linux-aarch64-server-release/images/jdk/lib/server/libjvm.debuginfo | grep andrew | grep ".o" - PR: https://git.openjdk.java.net/jdk/pull/8124
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
On Wed, 6 Apr 2022 13:30:28 GMT, Andrew Leonard wrote: >> This PR fixes the non-deterministic behavior when building on linux with >> different userids or within >> different workspace folders. >> It fixes the following issues: >> - MakeZipReproducible.java used to produce reproducible src.zip removes the >> optional zip "extra" field containing UID/GID. >> - When absolute output paths are not allowed, enable the use of >> -fdebug-prefix-map to ensure debug symbol info does not contain the top >> level workspace folder. >> - For reproducible builds ensure the gcc random symbol name generator is >> seeded using -frandom-seed. >> - For reproducible builds when producing debug symbols use relative object >> paths for library linking to remove absolute MASM object paths. >> >> Signed-off-by: Andrew Leonard > > Andrew Leonard has updated the pull request incrementally with one additional > commit since the last revision: > > Trigger checks @magicus Hmm, this is interesting, so with gcc 10.3, using -fdebug-prefix-map, I am seeing 3 absolute paths for the 3 hotspot assembly files, but note if I do a dwarfdump it shows no absolute paths, only the .FILE debuginfo strings have the path: andrew@andrew:~$ strings /home/andrew3/jdk/build/linux-aarch64-server-release/images/jdk/lib/server/libjvm.debuginfo | grep andrew - PR: https://git.openjdk.java.net/jdk/pull/8124
Re: RFR: 8242181: [Linux] Show source information when printing native stack traces in hs_err files [v8]
> When printing the native stack trace on Linux (mostly done for hs_err files), > it only prints the method with its parameters and a relative offset in the > method: > > Stack: [0x7f6e01739000,0x7f6e0183a000], sp=0x7f6e01838110, free > space=1020k > Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native > code) > V [libjvm.so+0x620d86] Compilation::~Compilation()+0x64 > V [libjvm.so+0x624b92] Compiler::compile_method(ciEnv*, ciMethod*, int, > bool, DirectiveSet*)+0xec > V [libjvm.so+0x8303ef] > CompileBroker::invoke_compiler_on_method(CompileTask*)+0x899 > V [libjvm.so+0x82f067] CompileBroker::compiler_thread_loop()+0x3df > V [libjvm.so+0x84f0d1] CompilerThread::thread_entry(JavaThread*, > JavaThread*)+0x69 > V [libjvm.so+0x1209329] JavaThread::thread_main_inner()+0x15d > V [libjvm.so+0x12091c9] JavaThread::run()+0x167 > V [libjvm.so+0x1206ada] Thread::call_run()+0x180 > V [libjvm.so+0x1012e55] thread_native_entry(Thread*)+0x18f > > This makes it sometimes difficult to see where exactly the methods were > called from and sometimes almost impossible when there are multiple > invocations of the same method within one method. > > This patch improves this by providing source information (filename + line > number) to the native stack traces on Linux similar to what's already done on > Windows (see [JDK-8185712](https://bugs.openjdk.java.net/browse/JDK-8185712)): > > Stack: [0x7f34fca18000,0x7f34fcb19000], sp=0x7f34fcb17110, free > space=1020k > Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native > code) > V [libjvm.so+0x620d86] Compilation::~Compilation()+0x64 > (c1_Compilation.cpp:607) > V [libjvm.so+0x624b92] Compiler::compile_method(ciEnv*, ciMethod*, int, > bool, DirectiveSet*)+0xec (c1_Compiler.cpp:250) > V [libjvm.so+0x8303ef] > CompileBroker::invoke_compiler_on_method(CompileTask*)+0x899 > (compileBroker.cpp:2291) > V [libjvm.so+0x82f067] CompileBroker::compiler_thread_loop()+0x3df > (compileBroker.cpp:1966) > V [libjvm.so+0x84f0d1] CompilerThread::thread_entry(JavaThread*, > JavaThread*)+0x69 (compilerThread.cpp:59) > V [libjvm.so+0x1209329] JavaThread::thread_main_inner()+0x15d > (thread.cpp:1297) > V [libjvm.so+0x12091c9] JavaThread::run()+0x167 (thread.cpp:1280) > V [libjvm.so+0x1206ada] Thread::call_run()+0x180 (thread.cpp:358) > V [libjvm.so+0x1012e55] thread_native_entry(Thread*)+0x18f > (os_linux.cpp:705) > > For Linux, we need to parse the debug symbols which are generated by GCC in > DWARF - a standardized debugging format. This patch adds support for DWARF 4, > the default of GCC 10.x, for 32 and 64 bit architectures (tested with x86_32, > x86_64 and AArch64). DWARF 5 is not supported as it was still experimental > and not generated for HotSpot. However, newer GCC version may soon generate > DWARF 5 by default in which case this parser either needs to be extended or > the build of HotSpot configured to only emit DWARF 4. > > The code follows the parsing steps described in the official DWARF 4 spec: > https://dwarfstd.org/doc/DWARF4.pdf > I added references to the corresponding sections throughout the code. > However, I tried to explain the steps from the DWARF spec directly in the > code (method names, comments etc.). This allows to follow the code without > the need to actually deep dive into the spec. > > The comments at the `Dwarf` class in the `elf.hpp` file explain in more > detail how a DWARF file is structured and how the parsing algorithm works to > get to the filename and line number information. There are more class > comments throughout the `elf.hpp` file about how different DWARF sections are > structured and how the parsing algorithm needs to fetch the required > information. Therefore, I will not repeat the exact workings of the algorithm > here but refer to the code comments. I've tried to add as much information as > possible to improve the readability. > > Generally, I've tried to stay away from adding any assertions as this code is > almost always executed when already processing a VM error. Instead, the DWARF > parser aims to just exit gracefully and possibly omit source information for > a stack frame instead of risking to stop writing the hs_err file when an > assertion would have failed. To debug failures, `-Xlog:dwarf` can be used > with `info`, `debug` or `trace` which provides logging messages throughout > parsing. > > **Testing:** > Apart from manual testing, I've added two kinds of tests: > - A JTreg test: Spawns new VMs to let them crash in various ways. The test > reads the created hs_err files to check if the DWARF parsing could correctly > find the filename and line number. For normal HotSpot files, I could not > check against hardcoded filenames and line numbers as they are subject to > change (especially line number can quickly become different). I therefore > just added some sanity check
Re: RFR: 8242181: [Linux] Show source information when printing native stack traces in hs_err files [v2]
On Wed, 26 Jan 2022 14:23:14 GMT, Erik Joelsson wrote: >> Christian Hagedorn has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Update test/hotspot/jtreg/runtime/ErrorHandling/TestDwarf.java >> >>Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com> >> - Update test/hotspot/jtreg/runtime/ErrorHandling/TestDwarf.java >> >>Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com> > > Build changes look good. I've pushed an update to fix the builds, add some assertions and to add the `gdwarf-4` GCC flag for now to exclude DWARF 5 and let the tests pass again (@erikj79 is that the right place? Do I need to add it at other places as well?). I think this `gdwarf-4` flag is currently the only option to eventually move forward with this RFE without extending the parser to support DWARF 5. I could still do that as a follow-up task and then remove the `gdwarf-4` flag again. But also adding DWARF 5 support in this patch is probably too much. What do you think about that? As mentioned above, I'm going to be away for the rest of the month. I will get back to this PR at the start of May again. Cheers, Christian - PR: https://git.openjdk.java.net/jdk/pull/7126
Re: RFR: 8242181: [Linux] Show source information when printing native stack traces in hs_err files [v7]
> When printing the native stack trace on Linux (mostly done for hs_err files), > it only prints the method with its parameters and a relative offset in the > method: > > Stack: [0x7f6e01739000,0x7f6e0183a000], sp=0x7f6e01838110, free > space=1020k > Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native > code) > V [libjvm.so+0x620d86] Compilation::~Compilation()+0x64 > V [libjvm.so+0x624b92] Compiler::compile_method(ciEnv*, ciMethod*, int, > bool, DirectiveSet*)+0xec > V [libjvm.so+0x8303ef] > CompileBroker::invoke_compiler_on_method(CompileTask*)+0x899 > V [libjvm.so+0x82f067] CompileBroker::compiler_thread_loop()+0x3df > V [libjvm.so+0x84f0d1] CompilerThread::thread_entry(JavaThread*, > JavaThread*)+0x69 > V [libjvm.so+0x1209329] JavaThread::thread_main_inner()+0x15d > V [libjvm.so+0x12091c9] JavaThread::run()+0x167 > V [libjvm.so+0x1206ada] Thread::call_run()+0x180 > V [libjvm.so+0x1012e55] thread_native_entry(Thread*)+0x18f > > This makes it sometimes difficult to see where exactly the methods were > called from and sometimes almost impossible when there are multiple > invocations of the same method within one method. > > This patch improves this by providing source information (filename + line > number) to the native stack traces on Linux similar to what's already done on > Windows (see [JDK-8185712](https://bugs.openjdk.java.net/browse/JDK-8185712)): > > Stack: [0x7f34fca18000,0x7f34fcb19000], sp=0x7f34fcb17110, free > space=1020k > Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native > code) > V [libjvm.so+0x620d86] Compilation::~Compilation()+0x64 > (c1_Compilation.cpp:607) > V [libjvm.so+0x624b92] Compiler::compile_method(ciEnv*, ciMethod*, int, > bool, DirectiveSet*)+0xec (c1_Compiler.cpp:250) > V [libjvm.so+0x8303ef] > CompileBroker::invoke_compiler_on_method(CompileTask*)+0x899 > (compileBroker.cpp:2291) > V [libjvm.so+0x82f067] CompileBroker::compiler_thread_loop()+0x3df > (compileBroker.cpp:1966) > V [libjvm.so+0x84f0d1] CompilerThread::thread_entry(JavaThread*, > JavaThread*)+0x69 (compilerThread.cpp:59) > V [libjvm.so+0x1209329] JavaThread::thread_main_inner()+0x15d > (thread.cpp:1297) > V [libjvm.so+0x12091c9] JavaThread::run()+0x167 (thread.cpp:1280) > V [libjvm.so+0x1206ada] Thread::call_run()+0x180 (thread.cpp:358) > V [libjvm.so+0x1012e55] thread_native_entry(Thread*)+0x18f > (os_linux.cpp:705) > > For Linux, we need to parse the debug symbols which are generated by GCC in > DWARF - a standardized debugging format. This patch adds support for DWARF 4, > the default of GCC 10.x, for 32 and 64 bit architectures (tested with x86_32, > x86_64 and AArch64). DWARF 5 is not supported as it was still experimental > and not generated for HotSpot. However, newer GCC version may soon generate > DWARF 5 by default in which case this parser either needs to be extended or > the build of HotSpot configured to only emit DWARF 4. > > The code follows the parsing steps described in the official DWARF 4 spec: > https://dwarfstd.org/doc/DWARF4.pdf > I added references to the corresponding sections throughout the code. > However, I tried to explain the steps from the DWARF spec directly in the > code (method names, comments etc.). This allows to follow the code without > the need to actually deep dive into the spec. > > The comments at the `Dwarf` class in the `elf.hpp` file explain in more > detail how a DWARF file is structured and how the parsing algorithm works to > get to the filename and line number information. There are more class > comments throughout the `elf.hpp` file about how different DWARF sections are > structured and how the parsing algorithm needs to fetch the required > information. Therefore, I will not repeat the exact workings of the algorithm > here but refer to the code comments. I've tried to add as much information as > possible to improve the readability. > > Generally, I've tried to stay away from adding any assertions as this code is > almost always executed when already processing a VM error. Instead, the DWARF > parser aims to just exit gracefully and possibly omit source information for > a stack frame instead of risking to stop writing the hs_err file when an > assertion would have failed. To debug failures, `-Xlog:dwarf` can be used > with `info`, `debug` or `trace` which provides logging messages throughout > parsing. > > **Testing:** > Apart from manual testing, I've added two kinds of tests: > - A JTreg test: Spawns new VMs to let them crash in various ways. The test > reads the created hs_err files to check if the DWARF parsing could correctly > find the filename and line number. For normal HotSpot files, I could not > check against hardcoded filenames and line numbers as they are subject to > change (especially line number can quickly become different). I therefore > just added some sanity check
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
@magicus thank you for looking at this, in hindsight I should have pinged you to review this as well. Technically Erik's approval is enough, but I do appreciate if I can get at least 24h to have a look at build changes that touches delicate build logic. It usually takes more than one of us to spot all potential issues with such changes. So a few thoughts, first one thing that crossed my mind which is the ifeq logic here should instead test ALLOW_ABSOLUTE_PATHS_IN_OUTPUT, exactly like the setting of -fdebug-prefix-map and -ffile-macro-prefix already do. In fact just changing to using that maybe sufficient? ie.only use relative paths here when ALLOW_ABSOLUTE_PATHS_IN_OUTPUT is false, ie. it is a "release" type build and not a developer/debug build, where CWD cmdlines are preferable. What's your thought on that? That sounds like even more of a step backwards, making even more command lines relative that could have been absolute. We want to avoid relative paths as far as at all possible. In fact, we did not have relative paths at all in the build for quite a long time, but was forced to introduce it to handle some specific shortcomings in certain combinations of build tools and circumstances. I'm still hoping these issues will someday be resolved upstream, and that we can remove those exceptions. I did search google & bugzilla for any gcc bug, but didn't find anything. However, i've just found this issue: https://github.com/gcc-mirror/gcc/commit/d12153046816f955e74943af7089d30de6a00e19 which maybe related, although looks to do the opposite of what we want, although that maybe just because I don't understand it! However, it is the .file dwarf value for the assembly stripped debuginfo that has the full object path that isn't getting stripped, and I think we do use -flto. This fix is in gcc 11.2, i'm testing using gcc 10.3. I'm going to do two things, recreate the problem again, double check the assembly file compile options, with gcc 10.3, then try again with gcc 11.2.. @magicus just saw your above post, interesting, i'll add I think gcc -v to my above testing, to see what gcc is passing through to the actual compile, apparently using verbose removes anything that gets removed for assembly, ie.anything that is not relavent I think we're calling the GNU assembler using the gcc frontend. It might behave badly about passing arguments properly. I found one bug related to this (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93371) but it seems to claim -fdebug-prefix-map should work (and only -ffile-prefix-map, which we do not use, should be broken). We might need to reconsider that to call as directly, if it works when we pass --debug-prefix-map directly. I have no complete overview of what such a change might bring along, though. /Magnus
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
On Fri, 8 Apr 2022 09:30:51 GMT, Andrew Leonard wrote: >> Actually, I think that the GNU assembler supports debug prefix mapping. See >> e.g. [this bug report](https://github.com/ocaml/ocaml/issues/10770), >> stating: "With GNU tools, to enable debug prefix map on C source we pass >> -fdebug-prefix-map to cc and --debug-prefix-map to as." >> >> @andrew-m-leonard Can you check if we can pass `--debug-prefix-map` to the >> assembler if on the gcc toolchain, and thus get rid of the relative linking? >> (And oh, speaking of that, the if statement here checks for target OS. It >> should, most likely, have checked for toolchain.) > > @magicus just saw your above post, interesting, i'll add I think gcc -v to my > above testing, to see what gcc is passing through to the actual compile, > apparently using verbose removes anything that gets removed for assembly, > ie.anything that is not relavent I did test with --debug-prefix-map on the assembly cmdline, it just seemed to be ignored, but I will try again - PR: https://git.openjdk.java.net/jdk/pull/8124
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
On Fri, 8 Apr 2022 08:31:45 GMT, Magnus Ihse Bursie wrote: >> One way would be to check if we have either assembly source files, or >> anything in EXTRA_OBJ, and if so, do the relative linking. That would at >> least bring down collateral damage significantly, since the majority of libs >> have neither. >> >> It will still hurt the most important libs (like hotspot) but unless we >> can't find a way to get this working in some other way, it seems we'll have >> to accept that. >> >> @andrew-m-leonard Do you know if there is any bug on gcc tracking their >> inability to set debug prefix mapping for assembly files? > > Actually, I think that the GNU assembler supports debug prefix mapping. See > e.g. [this bug report](https://github.com/ocaml/ocaml/issues/10770), stating: > "With GNU tools, to enable debug prefix map on C source we pass > -fdebug-prefix-map to cc and --debug-prefix-map to as." > > @andrew-m-leonard Can you check if we can pass `--debug-prefix-map` to the > assembler if on the gcc toolchain, and thus get rid of the relative linking? > (And oh, speaking of that, the if statement here checks for target OS. It > should, most likely, have checked for toolchain.) @magicus thank you for looking at this, in hindsight I should have pinged you to review this as well. So a few thoughts, first one thing that crossed my mind which is the ifeq logic here should instead test ALLOW_ABSOLUTE_PATHS_IN_OUTPUT, exactly like the setting of -fdebug-prefix-map and -ffile-macro-prefix already do. In fact just changing to using that maybe sufficient? ie.only use relative paths here when ALLOW_ABSOLUTE_PATHS_IN_OUTPUT is false, ie. it is a "release" type build and not a developer/debug build, where CWD cmdlines are preferable. What's your thought on that? I did search google & bugzilla for any gcc bug, but didn't find anything. However, i've just found this issue: https://github.com/gcc-mirror/gcc/commit/d12153046816f955e74943af7089d30de6a00e19 which maybe related, although looks to do the opposite of what we want, although that maybe just because I don't understand it! However, it is the .file dwarf value for the assembly stripped debuginfo that has the full object path that isn't getting stripped, and I think we do use -flto. This fix is in gcc 11.2, i'm testing using gcc 10.3. I'm going to do two things, recreate the problem again, double check the assembly file compile options, with gcc 10.3, then try again with gcc 11.2.. @magicus just saw your above post, interesting, i'll add I think gcc -v to my above testing, to see what gcc is passing through to the actual compile, apparently using verbose removes anything that gets removed for assembly, ie.anything that is not relavent - PR: https://git.openjdk.java.net/jdk/pull/8124
Re: RFR: 8265315: Support for CLDR version 41
On Thu, 7 Apr 2022 21:20:20 GMT, Naoto Sato wrote: > This is to upgrade the CLDR data from version 39 to version 41 which was > released yesterday. The vast majority of the changes are basically replacing > the CLDR data, along with tools/testcase alignments. Here is the link to CLDR > v41's release notes: https://cldr.unicode.org/index/downloads/cldr-41 Have you checked if this update has any impact on the runtime of the CLDRConverter tool? - PR: https://git.openjdk.java.net/jdk/pull/8150
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
On Fri, 8 Apr 2022 08:14:18 GMT, Magnus Ihse Bursie wrote: >> It would certainly be possible to identify the objects created from assembly >> source files based on file extensions, but it wouldn't help. The command >> line would still be dependent on the current working directory (correctly >> guessed!) if just one of the argument files is relative. >> >> The property Magnus is trying to preserve is to be able to copy a command >> line from the build log and run it in whichever directory you happen to be >> in, without having to figure out what the CWD was for that particular >> command. We worked hard to try to figure out ways of preserving that as much >> as possible, but as you say, if we want true reproducibility, then we have >> to give it up for link command lines where assembly source files are >> included. >> >> Now I realize that you aren't even checking if the link unit contains any >> objects from assembly files. I think we should be able to deduce that by >> filtering on the extension in the source file list. That would miss assembly >> input in EXTRA_OBJS, so perhaps we need an explicit parameter to >> SetupNativeCompilation for cases where this would happen. I think it applies >> to gtest, where we add all the object files from libjvm. > > One way would be to check if we have either assembly source files, or > anything in EXTRA_OBJ, and if so, do the relative linking. That would at > least bring down collateral damage significantly, since the majority of libs > have neither. > > It will still hurt the most important libs (like hotspot) but unless we can't > find a way to get this working in some other way, it seems we'll have to > accept that. > > @andrew-m-leonard Do you know if there is any bug on gcc tracking their > inability to set debug prefix mapping for assembly files? Actually, I think that the GNU assembler supports debug prefix mapping. See e.g. [this bug report](https://github.com/ocaml/ocaml/issues/10770), stating: "With GNU tools, to enable debug prefix map on C source we pass -fdebug-prefix-map to cc and --debug-prefix-map to as." @andrew-m-leonard Can you check if we can pass `--debug-prefix-map` to the assembler if on the gcc toolchain, and thus get rid of the relative linking? (And oh, speaking of that, the if statement here checks for target OS. It should, most likely, have checked for toolchain.) - PR: https://git.openjdk.java.net/jdk/pull/8124
Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]
On Thu, 7 Apr 2022 20:15:12 GMT, Erik Joelsson wrote: >> @magicus >> So I did think about this, and the alternative I think would be to hard code >> a list of which object files are the output from assembly files, of which >> there is about a dozen. Unless we could capture somehow the list of >> assembled files as we go ...? >> As @erikj79 points out this is just for the "linking". As the problem is the >> assembly file objects don't have the equivalent of debug prefix mapping, and >> so when stripped the full file path of the object file ends up in the >> debuginfo file, and results in the libjvm.so non-determinism due to >> different debug CRC. > > It would certainly be possible to identify the objects created from assembly > source files based on file extensions, but it wouldn't help. The command line > would still be dependent on the current working directory (correctly > guessed!) if just one of the argument files is relative. > > The property Magnus is trying to preserve is to be able to copy a command > line from the build log and run it in whichever directory you happen to be > in, without having to figure out what the CWD was for that particular > command. We worked hard to try to figure out ways of preserving that as much > as possible, but as you say, if we want true reproducibility, then we have to > give it up for link command lines where assembly source files are included. > > Now I realize that you aren't even checking if the link unit contains any > objects from assembly files. I think we should be able to deduce that by > filtering on the extension in the source file list. That would miss assembly > input in EXTRA_OBJS, so perhaps we need an explicit parameter to > SetupNativeCompilation for cases where this would happen. I think it applies > to gtest, where we add all the object files from libjvm. One way would be to check if we have either assembly source files, or anything in EXTRA_OBJ, and if so, do the relative linking. That would at least bring down collateral damage significantly, since the majority of libs have neither. It will still hurt the most important libs (like hotspot) but unless we can't find a way to get this working in some other way, it seems we'll have to accept that. @andrew-m-leonard Do you know if there is any bug on gcc tracking their inability to set debug prefix mapping for assembly files? - PR: https://git.openjdk.java.net/jdk/pull/8124
