RE: RFR: 8237192: Generate stripped/public pdbs on Windows for jdk images
Hi Erik, thanks for your review. While I’ve addressed all your points (thanks for the great hints regarding usage of SetupCopyFiles), I also enhanced the configure option a little bit. What you can now pass is --with-external-symbols-in-bundles=. Default setting is none. It’ll require --with-native-debug-symbols=external obviously and is currently only implemented for Windows. This is checked in configure. Depending on the value used, the bundles and jmods will contain either no pdbs, public pdbs or the full pdbs. This configure option could easily be enhanced to work for Linux/Unix/Mac as well – but I didn’t want to go too far with my change now. What also comes with my change is debug files for executables (launchers, cmds) in images/ which weren’t copied so far in Images.gmk. It’s however needed because bundles are created from these images and with the current bundle logic, stripped pdb files need to exist in images/… to get bundled for option ‘public’. I also removed the special handling of the pdb file for java.exe in Launcher-java.base.gmk, as the filtering to make sure it’s not overwriting the pdb for java.dll has to be done in CreateJmods.gmk and Images.gmk anyway. Eventually, when this is all refactored, one should probably generate the pdb files to ship into support/modules_libs and support/modules_cmds and only in the case of public pdbs redirect generation of the full pdbs to something like support/modules_libs_full_debug_info or something like that. Then, from support/modules_libs and support/modules_cmds, one should generate the base image via jlink which can simply be packed into the shipment bundle. And the other image with all debug/demos can be created by copying the base image and then applying the stuff from support/ modules_libs_full_debug_info etc. Here's the new webrev: http://cr.openjdk.java.net/~clanger/webrevs/8237192.1/ Best regards Christoph From: Erik Joelsson Sent: Mittwoch, 12. Februar 2020 23:17 To: Langer, Christoph ; Magnus Ihse Bursie ; 'build-dev@openjdk.java.net' Cc: 'hotspot-...@openjdk.java.net' ; Baesken, Matthias Subject: Re: RFR: 8237192: Generate stripped/public pdbs on Windows for jdk images Hello Christoph, This patch certainly looks better to me, though I agree it's a bit hackish to have to filter and rename the stripped.pdb files twice, once for jmods and again for bundles. I think I'm ok with it for now though. The future improvement I would like to make would be to create two sets of jdk images, one that contains debug symbols and demos, which we continue to use for testing, and another which only contains exactly what we bundle up, including the correctly named top dir. The latter would be created first and used as input for the former. I think lots of things would be cleaner then, especially Bundles.gmk. But, that can wait for later. With your current proposal, my proposed future change will apply seamlessly in regard to the stripped pdb files and our distribution bundles. The clash between launchers and libs is annoying, and we also have it for java.exe and java.dll already, though the build is explicitly handling that one somewhere else. Now to review, there are some details I will nitpick about. CreateJmods.gmk: 174, 185: Rule declarations should be indented like any other line inside conditionals. But, I have a problem with these rules as the target is the directory. This will not work well with incremental builds. I would recommend doing a SetupCopyFiles construct instead so you get individual rules for each file. It would look something like this: rename-stripped-pdb = \ $(patsubst %.stripped.pdb,%.pdb,$1) $(eval $(call SetupCopyFiles, COPY_STRIPPED_LIBS, \ SRC := $(LIBS_DIR), \ DEST := $(LIBS_DIR_STRIPPED), \ FILES := $(call FindFiles, $(LIBS_DIR)), \ NAME_MACRO := rename-stripped-pdb, \ )) DEPS += $(COPY_STRIPPED_LIBS) For the corresponding CMD_DIR, the removal of jimage and friends can be done with $(filter ) around The FindFiles call. GenerateLinkOptData.gmk: Please indent inside ifeq block. I would prefer having the TARGETS += inside the conditional block. Seems you also left a commented out endif there. NativeCompilation.gmk 919: You changed the continuation indent from 4 to 2, please revert. /Erik On 2020-02-12 05:26, Langer, Christoph wrote: Hi Magnus, Erik, I started off with Matthias’ patch and tried to address your concerns. Especially I explored adding the stripped pdbs to the jmods as well. Here is what I came up with: http://cr.openjdk.java.net/~clanger/webrevs/8237192.0/ It’s a bit hacky in that it’ll copy support/modules_libs to support/modules_libs_stripped and fix the pdbs to ship in there. The same goes for modules_cmds. The problem with that approach is that probably not all dependencies are placed correctly and also there’s a bit more of duplication of binaries in the build directories. I think, it could be repaired eventually by
Re: RFR: 8199138: Add RISC-V support to Zero
Hi Aleksey! On 2/17/20 9:07 AM, Aleksey Shipilev wrote: > On 2/12/20 6:13 PM, Aleksey Shipilev wrote: >> On 2/12/20 6:00 PM, John Paul Adrian Glaubitz wrote: >>> On 2/12/20 5:59 PM, Aleksey Shipilev wrote: On 2/12/20 5:54 PM, John Paul Adrian Glaubitz wrote: > I assume I can push with those changes and mark it as Reviewed-by: erikj, > shade? Mark it, yes. I believe non-trivial (yet exceedingly simple) things like these require waiting for 24 hours to anyone else to chime in with comments. There seem to be no rush to get it in, right? >>> >>> No rush, no. Just wanted to make sure that it's fine to push then :). >> >> Yes, I believe it would be fine to push then. > > I think it is fine to push now ;) I have come up with a better and cleaner approach now which I will post later this week. But I want to get the fix for JDK-8239001 out first. Adrian -- .''`. John Paul Adrian Glaubitz : :' : Debian Developer - glaub...@debian.org `. `' Freie Universitaet Berlin - glaub...@physik.fu-berlin.de `-GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
Re: RFR: JDK-8239139 testmake fail with warning about strncpy using gcc version 8
On 17/02/2020 07:48, linzang(臧琳) wrote: Hi, May I ask help to review one tiny fix of build failure described at https://bugs.openjdk.java.net/browse/JDK-8239139 Root cause is gcc version 8 prints warning for strncpy. The fix simply replace strncpy with snprintf. Thanks! Bug: https://bugs.openjdk.java.net/browse/JDK-8239139 webrev: http://cr.openjdk.java.net/~lzang/8239139/webrev/ I've moved this issue to core-libs/java.nio as this relates to a recent added test for JDK-8231187. Can you follow you on nio-dev? -Alan.
RE: RFR [jdk11]: 8234525: enable link-time section-gc for linux s390x to remove unused code
Hi Matthias, Backport looks good to me, too. But please move the new section before TOOLCHAIN_CFLAGS_JDK_CONLY in make/autoconf/flags-cflags.m4, as suggested by Andrew. No need for new webrev. Cheers Christoph > -Original Message- > From: build-dev On Behalf Of > Andrew John Hughes > Sent: Donnerstag, 13. Februar 2020 23:18 > To: Baesken, Matthias ; jdk-updates- > d...@openjdk.java.net; 'build-dev@openjdk.java.net' d...@openjdk.java.net> > Subject: Re: RFR [jdk11]: 8234525: enable link-time section-gc for linux s390x > to remove unused code > > > > On 13/02/2020 11:48, Baesken, Matthias wrote: > > Ping - any reviews ? > > > > Thanks, Matthias > > > > From: Baesken, Matthias > > Sent: Dienstag, 11. Februar 2020 10:24 > > To: jdk-updates-...@openjdk.java.net; 'build-dev@openjdk.java.net' > > > Subject: RFR [jdk11]: 8234525: enable link-time section-gc for linux s390x > > to > remove unused code > > > > Hello , please review the downport of "8234525: enable link-time section- > gc for linux s390x to remove unused code" to jdk11 . > > > > My change from jdk/jdk did not apply directly and I had to adjust it > > slightly . > > > > > > > > > > Bug and jdk/jdk change : > > > > https://bugs.openjdk.java.net/browse/JDK-8234525 > > https://hg.openjdk.java.net/jdk/jdk/rev/c04fa10636fd > > > > > > Adjusted change for jdk11u-dev : > > > > http://cr.openjdk.java.net/~mbaesken/webrevs/8234525.0_jdk11/ > > > > > > Original review thread : > > > > https://mail.openjdk.java.net/pipermail/build-dev/2019- > November/026326.html > > > > > > Thanks, Matthias > > > > In the original patch, the lines are added after > TOOLCHAIN_CFLAGS_JDK_CONLY. In the 11u version, they seem to have > needlessly moved above it. > > Otherwise, looks good. > > Regarding the patch itself, are these flags only useful when the s390x > port is present or would they be advantageous on older versions where > s390x is still using the Zero assembler port? > > Thanks, > -- > Andrew :) > > Senior Free Java Software Engineer > Red Hat, Inc. (http://www.redhat.com) > > PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net) > Fingerprint = 5132 579D D154 0ED2 3E04 C5A0 CFDA 0F9B 3596 4222 > https://keybase.io/gnu_andrew
Re: RFR: 8199138: Add RISC-V support to Zero
On 2/12/20 6:13 PM, Aleksey Shipilev wrote: > On 2/12/20 6:00 PM, John Paul Adrian Glaubitz wrote: >> On 2/12/20 5:59 PM, Aleksey Shipilev wrote: >>> On 2/12/20 5:54 PM, John Paul Adrian Glaubitz wrote: I assume I can push with those changes and mark it as Reviewed-by: erikj, shade? >>> >>> Mark it, yes. I believe non-trivial (yet exceedingly simple) things like >>> these require waiting for >>> 24 hours to anyone else to chime in with comments. There seem to be no rush >>> to get it in, right? >> >> No rush, no. Just wanted to make sure that it's fine to push then :). > > Yes, I believe it would be fine to push then. I think it is fine to push now ;) -- Thanks, -Aleksey
Re: Re: RFR: JDK-8239139 testmake fail with warning about strncpy using gcc version 8(Internet mail)
Thanks a lot ! -- Lin >On 2020-02-17 08:48, linzang(臧琳) wrote: >Hi, > May I ask help to review one tiny fix of build failure described at > https://bugs.openjdk.java.net/browse/JDK-8239139 > Root cause is gcc version 8 prints warning for strncpy. > The fix simply replace strncpy with snprintf. > Thanks! > Bug: https://bugs.openjdk.java.net/browse/JDK-8239139 > webrev: http://cr.openjdk.java.net/~lzang/8239139/webrev/ > >BRs, >Lin > >Hi Lin, > >This is not a build issue. I'm redirecting this to core-libs. > >/Magnus
