Re: RFR: 8288396: Always create reproducible builds [v2]

[Erik Joelsson]

On Tue, 14 Jun 2022 10:09:37 GMT, Magnus Ihse Bursie  wrote:

>> When we started introducing some possibly more intrusive compiler flags and 
>> functionality for reproducible builds, we also introduced a flag to turn 
>> this off  out of an abundance of caution. But we have been been using this 
>> configuration for a year or so internally within Oracle, with no issues. So 
>> there's really no reason to be able to turn this off. (If you were to ask 
>> me, the fact that compilers and build tools ever started to produce 
>> non-deterministic output has been a bug from day one.)
>> 
>> With this fix, all randomness should be gone from our builds, at least on 
>> linux and windows. There are no more `__DATE__` and `__TIME__` macros in the 
>> source code.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix exitTransportWithError signature

make/autoconf/flags-ldflags.m4 line 132:

> 130: 
> 131:   if test "x$TOOLCHAIN_TYPE" = xmicrosoft; then
> 132: REPRODUCIBLE_LDFLAGS="-experimental:deterministic"

For the cflag, we check that the compiler supports it, but for the linker flag 
you are just setting it without a check. Before this patch, if we got here and 
ENABLE_REPRODUCIBLE_BUILD was true, it meant that the test had passed for the 
compiler, from which we could assume it would also work for the linker, but 
that is no longer the case.

-

PR: https://git.openjdk.org/jdk/pull/9152

Re: RFR: 8288001: SOURCE_DATE_EPOCH should not be set by default

[Erik Joelsson]

On Wed, 8 Jun 2022 07:47:24 GMT, KIRIYAMA Takuya  wrote:

> At default configuration, SOURCE_DATE_EPOCH is exported as environment 
> variable in SetupReproducibleBuild. Then, gcc is affected of 
> SOURCE_DATE_EPOCH environment variable. This value is used only to set 
> SOURCE_DATE_ISO_8601 (except below), so I removed "export" for 
> SOURCE_DATE_EPOCH in SetupReproducibleBuild. And, at building ct.sym, 
> SOURCE_DATE_EPOCH environment variable is needed. So I added setting routine 
> if SOURCE_DATE_EPOCH is not set.
> 
> This fix works fine. With default configuration shows -Xinternalversion 
> output same as Windows, and with --with-source-date configuration shows 
> -Xinternalversion output specified timestamp. Would you please review this 
> fix?

Is the problem that on Windows we get -Xinternalversion in local timezone but 
on Linux we get UTC? That's a difference that may warrant fixing, but I don't 
think this is the way to do it. 

I don't think it makes a practical difference if the timestamp is taken at the 
start of the build or when a certain src file is compiled.

-

PR: https://git.openjdk.org/jdk/pull/9081

Re: RFR: 8287906: Rewrite of GitHub Actions (GHA) sanity tests [v12]

[Erik Joelsson]

On Mon, 13 Jun 2022 06:45:49 GMT, Magnus Ihse Bursie  wrote:

>> With project Skara, the ability to run a set of sanity build and test jobs 
>> on selected platforms was added. This functionality was driven by 
>> `.github/workflows/submit.yml`. This file unfortunately lacks any real 
>> structure, and contains a lot of code duplication and redundancy. This has 
>> made it hard to add functionality, and new platforms to test, and it has 
>> made it even harder to debug issues. (This is hard enough as it is, since we 
>> have no direct access to the platforms that GHA runs on.)
>> 
>> Since the GHA tests are important for a large subset of the community, we 
>> need to do better. 
>> 
>> ## GitHub Actions framework rewrite
>>  
>> This is a complete overhaul of the GHA testing framework. I started out 
>> trying to just tease the old `submit.yml` apart, trying to de-duplicate 
>> code, but I soon realized a much more thorough rework was needed.
>> 
>> ### Design description
>> 
>> The principle for the new design was to avoid code duplication, and to 
>> improve readability of the code. The latter is extra important since the GHA 
>> "language" is very limited, needs a lot of quirks and workarounds, and is 
>> probably not well known by many OpenJDK developers. I've strived to find 
>> useful layers of abstraction to make the expressions as clear as possible.
>> 
>> Unfortunately, the Workflow/Action YAML language is quite limited. There are 
>> two ways to avoid duplication, "local composite actions" and "callable 
>> workflows". They both have several limitations:
>> 
>>  * "Callable workflows" can only be used in a single redirection. They are 
>> (apparently) inlined into the "calling workflow" at run time, and as such, 
>> they are present without having to check out the source code. (Which is a 
>> lengthy process.)
>> 
>>  * "Local composite actions" can use other actions, but you must start by 
>> checking out the repo.
>> 
>> To use the strength of both kinds of sub-modules, I'm using "callable 
>> workflows" from `main.yml` to call `build-.yml` and `test.yml`. It 
>> is not allowed to mix "strategies" (that is, the method of automatically 
>> creating a test matrix) when calling callable workflows, so I needed to have 
>> some amount of duplication in `main.yml` that could have been avoided 
>> otherwise.
>> 
>> All the callable workflows need to check out the source code anyway, so 
>> there is no real additional cost of using "local composite actions" for 
>> abstraction of these workflows. (A bit of a lucky break.) I've created "high 
>> level" actions, corresponding to something like a function call. The goal 
>> here was both to avoid duplication, and to improve readability of the 
>> workflows.
>> 
>> The four `build-.yml` files are very similar. But in the end of 
>> the day, only like 50% of the source code is shared, and the platform 
>> specific changes permeate the files. So I decided to keep them separately, 
>> since mixing them all into one would have made a mess, due to the lack of 
>> proper abstraction mechanisms. But that also mean that if we change platform 
>> independent code in building, we need to remember to update it in all four 
>> places.
>> 
>> In the strictest sense, this is a "refactoring" in that the functionality 
>> should be equal to the old `submit.yml`. The same platforms should build, 
>> with the same arguments, and the same tests should run. When I look at the 
>> code now, I see lots of potential for improvement here, by rethinking what 
>> we do run. But let's save that discussion for the next PR.
>> 
>> There is one major change, though. Windows is no longer running on Cygwin, 
>> but on MSYS2. This was not really triggered by the recurring build issues on 
>> Cygwin (though that certainly did help me in thinking I made the right 
>> choice), but the sheer impossibility of getting Cygwin to behave as a normal 
>> unix shell on GHA Windows hosts. I spent countless hours trying to work out 
>> limitations, by setting `SHELLOPTS=igncr`, by running `set +x posix` to turn 
>> of the POSIX compliance mode that kept turning on by itself and made bash 
>> choke on several of our scripts, by playing tricks with the `PATH`, but in 
>> the end to no avail. There were no single combination of hacks and 
>> workarounds that could get us past the entire chain from configure, to 
>> build, to testing. (The old solution user PowerShell instead to get around 
>> these limitations.) I'm happy to report that I have had absolutely zero 
>> issues with MSYS2 since I made the switch (and understood how to set the 
>> PATH properly), and I'm seriously c
 onsidering switching stance to recommend using MSYS2 instead of Cygwin as the 
primary winenv for building the JDK.
>> 
>> ### Example run
>> 
>> A good example on how a run looks like with the new GHA system is [the run 
>> for this PR](https://github.com/magicus/jdk/actions/runs/2454577164).
>> 
>> ### New features
>> 
>> 

Re: RFR: 8288114: Update JIRA link in vcs.xml

[Erik Joelsson]

On Fri, 10 Jun 2022 17:11:41 GMT, Alexey Ivanov  wrote:

> Update the link to JBS in `vcs.xml` template to https://bugs.openjdk.org/
> 
> It will affect newly generated project files only.  
> Edit `vcs.xml` manually or in UI to update in existing projects.

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.org/jdk/pull/9130

Re: RFR: 8288195: Prepare build system for GHA changes

[Erik Joelsson]

On Fri, 10 Jun 2022 09:54:36 GMT, Magnus Ihse Bursie  wrote:

> A few changes to the build system is needed for the GHA rewrite 
> ([JDK-8287906](https://bugs.openjdk.org/browse/JDK-8287906)).

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.org/jdk/pull/9122

Re: RFR: 8283724: Incorrect description for jtreg-failure-handler option

[Erik Joelsson]

On Thu, 9 Jun 2022 06:49:15 GMT, KIRIYAMA Takuya  wrote:

> The description for the jtreg-failure-handler option is incorrect, so I fixed 
> it to describe jtreg-failure-handler option.  
> Would you please review this fix?

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/9099

Re: RFR: 8287894: Use fixed timestamp as an alternative of __DATE__ macro in jdk.jdi to make Windows build reproducible

[Erik Joelsson]

On Tue, 7 Jun 2022 19:57:48 GMT, Alexey Pavlyutkin  
wrote:

> Hi!
> 
> Here is a fix to jdk.jdi that fixes reproducible build for Windows. The idea 
> of the fix is to re-use value of --with-hotspot-build-time option to generate 
> deterministic timestamp exactly like it's done to hotspot component.
> 
> Verification (Windows-10/MSVS-2019): ```bash ./configure 
> --with-boot-jdk=c:/work/boot-jdk/jdk-18 --with-debug-level=fastdebug 
> --with-hotspot-build-time="6/7/2022 2:35pm" 
> --with-extra-cflags="/experimental:deterministic" 
> --with-extra-cxxflags="/experimental:deterministic"```
> 
> Regression (Windows-10/MSVS-2019): ```bash ./configure 
> --with-boot-jdk=c:/work/boot-jdk/jdk-18 --with-debug-level=fastdebug```

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/9070

Re: RFR: 8287828: Fix so that one can select jtreg test case by ID from make [v5]

[Erik Joelsson]

On Tue, 7 Jun 2022 18:15:30 GMT, Leo Korinth  wrote:

>> One can select a testcase by ID when running a jtreg test case directly from 
>> jtreg (using the testcase.java#testID syntax). However, this has not been 
>> possible to do when launching jtreg indirectly from make.
>> 
>> This fix attempts to address this issue. I have not tested this thoroughly 
>> yet, I wanted to show the code to get feedback first. The idea is to *not* 
>> emulated destructive imperative assignments through the use of eval. I 
>> instead try to handle it in a functional style without reassigning 
>> variables. I have tried to make the change as small as possible.
>> 
>> I am not used to change the build system, so I would appreciate thorough 
>> review.
>> 
>> `IfAppend` and `IfPrepend` are general purpose functions. If similar 
>> functions exists elsewhere inside the code base or in make itself I should 
>> probably use those instead. If they do not exist, they might be useful 
>> elsewhere and maybe should be placed in a common make file instead of the 
>> RunTests.gmk file.
>
> Leo Korinth has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   use test id in comments

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/9028

Re: RFR: 8287828: Fix so that one can select jtreg test case by ID from make [v3]

[Erik Joelsson]

On Tue, 7 Jun 2022 13:45:47 GMT, Leo Korinth  wrote:

>> One can select a testcase by ID when running a jtreg test case directly from 
>> jtreg (using the testcase.java#testID syntax). However, this has not been 
>> possible to do when launching jtreg indirectly from make.
>> 
>> This fix attempts to address this issue. I have not tested this thoroughly 
>> yet, I wanted to show the code to get feedback first. The idea is to *not* 
>> emulated destructive imperative assignments through the use of eval. I 
>> instead try to handle it in a functional style without reassigning 
>> variables. I have tried to make the change as small as possible.
>> 
>> I am not used to change the build system, so I would appreciate thorough 
>> review.
>> 
>> `IfAppend` and `IfPrepend` are general purpose functions. If similar 
>> functions exists elsewhere inside the code base or in make itself I should 
>> probably use those instead. If they do not exist, they might be useful 
>> elsewhere and maybe should be placed in a common make file instead of the 
>> RunTests.gmk file.
>
> Leo Korinth has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   wip

I think this looks good, but Magnus should still take a look.

-

Marked as reviewed by erikj (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/9028

Re: RFR: 8287828: Fix so that one can select jtreg test case by ID from make

[Erik Joelsson]

On Sat, 4 Jun 2022 01:51:20 GMT, Leo Korinth  wrote:

> One can select a testcase by ID when running a jtreg test case directly from 
> jtreg (using the testcase.java#testID syntax). However, this has not been 
> possible to do when launching jtreg indirectly from make.
> 
> This fix attempts to address this issue. I have not tested this thoroughly 
> yet, I wanted to show the code to get feedback first. The idea is to *not* 
> emulated destructive imperative assignments through the use of eval. I 
> instead try to handle it in a functional style without reassigning variables. 
> I have tried to make the change as small as possible.
> 
> I am not used to change the build system, so I would appreciate thorough 
> review.
> 
> `IfAppend` and `IfPrepend` are general purpose functions. If similar 
> functions exists elsewhere inside the code base or in make itself I should 
> probably use those instead. If they do not exist, they might be useful 
> elsewhere and maybe should be placed in a common make file instead of the 
> RunTests.gmk file.

I think this is a nice fix. I mostly have style opinions. I would also like to 
have Magnus look at this as he wrote this originally.

As for testing, it would be good to verify a decently sized job in Mach5, as 
well as trying to use the TestID selection on multiple platforms.

make/RunTests.gmk line 39:

> 37: 
> 
> 38: 
> 39: HASH:=\#

HASH is already defined in MakeBase.gmk so should be available here.

make/RunTests.gmk line 47:

> 45: define IfPrepend
> 46: $(if $(strip $1),$(strip $2)$(strip $1),)
> 47: endef

These two probably fits better in make/common/Utils.gmk. 

Also please have a look at the [Code Conventions for the Build 
System](http://openjdk.java.net/groups/build/doc/code-conventions.html). One 
liner macros like this we like to format like this:


IfPrepend = \
  $(if $(strip $1),$(strip $2)$(strip $1),)

make/RunTests.gmk line 51:

> 49: define TestID
> 50: $(subst .java,,$(subst .java$(HASH),,$(suffix $(notdir $1
> 51: endef

This macro deserves a comment explaining what it's trying to do.

make/RunTests.gmk line 53:

> 51: endef
> 52: 
> 53: define HashTestID

This one too.

make/RunTests.gmk line 377:

> 375: # either absolute or relative to any of the TEST_BASEDIRS or test roots.
> 376: define ParseJtregTestSelection
> 377: $(call IfAppend, \

Please adjust indentation of the whole body here.

make/RunTests.gmk line 444:

> 442: $(strip $(foreach parser,ParseCustomTestSelection 
> ParseGtestTestSelection ParseMicroTestSelection ParseJtregTestSelection 
> ParseSpecialTestSelection \
> 443:  ,$(call $(parser),$1)))
> 444: endef

As this is logically a one-liner macro, please format according to guidelines. 
Also try to keep lines shortish. Something like this:

Suggestion:

ParseTestSelection = \
  $(strip $(foreach parser, ParseCustomTestSelection ParseGtestTestSelection 
ParseMicroTestSelection \ 
  ParseJtregTestSelection ParseSpecialTestSelection, \
$(call $(parser), $1) \
  ))

make/RunTests.gmk line 448:

> 446: # Now intelligently convert the test selection given by the user in TEST
> 447: # into a list of fully qualified test descriptors of the tests to run.
> 448: TESTS_TO_RUN:=$(strip $(foreach test,$(TEST),$(call 
> ParseTestSelection,$(test

I think this rewrite is more elegant, but logically there is a difference as it 
no longer breaks early. I would like to have Magnus' opinion on this as he 
wrote this originally.

Style wise, please leave spaces around operators and space after comma when 
possible. The top level strip should handle any whitespace issues caused by 
this.

-

PR: https://git.openjdk.java.net/jdk/pull/9028

Re: RFR: 8287745 jni/nullCaller/NullCallerTest.java fails with "exitValue = 1"

[Erik Joelsson]

On Fri, 3 Jun 2022 07:56:38 GMT, Tim Prinzing  wrote:

> Fixed JtregNativeJdk.gmk to include c++ libs for NullCallerTest

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/9010

Re: RFR: JDK-8252717: Integrate/merge legacy standard doclet diagnostics and doclint

[Erik Joelsson]

On Thu, 2 Jun 2022 20:59:26 GMT, Jonathan Gibbons  wrote:

> Please review a moderate change to enable (most) doclet warnings even if 
> doclint is enabled.
> 
> Since the introduction of doclint, there was some (small) overlap between the 
> small set of warnings generated by the doclet and the new larger set of 
> diagnostics that could be generated by doclint.  The solution, until now, has 
> been to disable doclet warnings when doclint is enabled. But, the sets do not 
> overlap, and the policy has inappropriately suppressed some warnings and 
> inhibited the generation of new warnings by the doclet that could only be 
> done by the doclet, and not doclint.
> 
> One notable group of warnings that has been inappropriately suppressed is the 
> warnings generated by using the `-serial warn` option.
> 
> The fundamental core of the change is to remove the conditional checks in the 
> doclet `Messages.java`, which would suppress messages when doclint was 
> enabled.  A consequence is that some specific messages have to disabled if 
> they are duplicate checks if doclint is enabled. (The messages cannot be 
> removed altogether because doclint might _not_ be enabled.)   A test is added 
> to verify that either one message or the other is generated, but never both.
> 
> As previously noted, an issue with the earlier policy is that warnings 
> generated by using the `-serial warn` option were inappropriately suppressed, 
> and this change fixes that ... revealing a number of latent warnings in the 
> JDK API that need to be addressed.  The short term fix here is to temporarily 
> remove the use of the `-serialwarn` option. See 
> [JDK-8287749](https://bugs.openjdk.java.net/browse/JDK-8287749).

Build change looks ok.

-

Marked as reviewed by erikj (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/9006

Re: RFR: 8287724: Fix various issues with msys2

[Erik Joelsson]

On Thu, 2 Jun 2022 09:17:59 GMT, Magnus Ihse Bursie  wrote:

> I encountered a bunch of issues when running with msys2 on Windows (but one 
> of them could have happened on cygwin as well).
> 
> * fixpath must set MSYS2_ARG_CONV_EXCL="*" before running cmd.exe to figure 
> out the temp directory, or msys might interfere with the command line to cmd.
> 
> * Paths like "/c/s/source/jdk", meaning to point to "c:\s\source\jdk", would 
> be interpreted by fixpath as "/cs:\source\jdk", since the leading "/c" would 
> be considered a prefix. This is not a problem on cygwin, where the /cygpath 
> prefix makes paths unambiguous. I countered this by checking if the file 
> exists (as written, or just the basepath, or the first 3 parts of the path). 
> If so, I treat it as a filename, rather than a prefix.
> 
> * Configure is supposed to handle windows-style input paths, but 
> `--with-bootjdk=c:\java\jdk-17` or similar would break, since we started to 
> look for files in that directory without having to normalized the path first.
> 
> * Finally, configure.guess sometimes reports msys as `mingw` and sometimes as 
> `msys`, depending on the value of MSYSTEM. (And for some values, the old 
> autoconf-configure.guess breaks -- I did not bother fixing this.)

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8989

Re: RFR: 8233269: Improve handling of JAVA_ARGS

[Erik Joelsson]

On Wed, 1 Jun 2022 13:52:46 GMT, Adam Sotona  wrote:

> LauncherCommon.gmk is unfortunately defining JAVA_ARGS with `-J-ms8m` option 
> for all JDK launchers, including java launcher.
> JAVA_ARGS  should not be defined for java launcher (in contrast to the other 
> JDK launchers), and the command line option `-J-ms8m` is obsolete for java 
> launcher.
> 
> Proposed patch removes JAVA_ARGS from java launcher, while keeping status quo 
> for all other JDK launchers.
> The patch of LauncherCommon.gmk identifies java launcher by undefined 
> MAIN_CLASS.
> 
> Thanks for review,
> Adam

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8972

Re: RFR: JDK-8284858: Start of release updates for JDK 20 [v2]

[Erik Joelsson]

On Thu, 26 May 2022 23:05:32 GMT, Joe Darcy  wrote:

>> Time to start getting ready for JDK 20...
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Respond to review feedback.

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8236

Re: RFR: 8287254: Clean up Xcode sysroot logic

[Erik Joelsson]

On Tue, 24 May 2022 17:09:10 GMT, Magnus Ihse Bursie  wrote:

> The logic in BASIC_SETUP_DEVKIT for setting a correct sysroot for Xcode is 
> hard to follow. This should be straightened out. We also expose variables 
> that are no longer used. So there's a bit of related cleanup. 
> 
> The new code is more or less functionally equivalent to the old. There were 
> some corner cases which the old code did not handle well; this has now been 
> improved. I've also added yet another method of trying to get the SDK root 
> before falling back to the system library, by using `xcrun --show-sdk-path`.
> 
> In an ideal world, the sysroot flags to `mig` should be set in configure, 
> e.g. as `MIG_FLAGS` or `MIG_SYSROOT_FLAGS`. I can't be bothered to do that 
> for a single call to `mig`, though.
> 
> As always, changes like this that depend on the environment is tricky to 
> test. I've tried running it on a couple of different macs, with and without a 
> devkit.

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8872

Re: RFR: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe

[erik . joelsson]

On 2022-05-12 04:58, Magnus Ihse Bursie wrote:

On 2022-05-12 13:17, Michael Hall wrote:
A solution like including a bundle identifier something like 
net.java.openjdk.MYAPP.java would be possible at packaging time but 
not at build time.
To fix this at build time you would need to generate a name unique to 
each installed jdk. Including release 
net.java.openjdk.JDK_RELEASE.java might avoid some conflicts but 
would still be open to conflict for two applications at the same 
release. So it can’t be addressed ‘before the fact’ either. The only 
thing I am currently thinking of that might work would be include a 
replaceable part in the identifier. So something like 
net.java.openjdk.java.XX
Where jpackage could include something to change the X…. to a 
unique application name. If you don’t change the string size you 
could probably avoid some of the resizing issues Apple DTS mentions. 
Whether there is a standard Apple tool to do this I don’t know.


As you say, we're a bit in a bind since the java executable needs to 
be created when the JDK is built, but the bundle ID needs to be 
determined when jpackage is run (and a suitable, per-application ID 
can be created), and there is no standard tooling to update the bundle 
ID after build time.


I believe what you are describing is exactly solution #2 suggested by 
the Apple engineer. I don't think that would be horribly difficult to 
achieve, though. Sure, it's a bit of a hack, but not the ugliest I've 
seen in my days. If we go down this route, I suppose we do something 
like this:


1) When building the JDK, we create an additional copy of the java 
executable, and store it with the jdk.jpackage resources. Let's call 
it java.template, for the sake of it. This is identical to the real 
bin/java except for the fact that it contains a different bundle ID, 
with a large enough padding field, like X...  This way, we don't 
have to mess around with the real java executable for the JDK.


Aren't we embedding this bundle ID in every launcher, so you would need 
a .template for each possible launcher that could be included 
in an app?


What I think we need to do is first evaluate if we actually need to 
embed this bundle ID in the executables at all, or rather, what would 
the consequences be if we didn't. My understanding is that we only do 
this to be able to run them outside of a bundle directory structure, but 
this would need some pretty thorough testing of course. If we are to 
generate a special set of launchers for jpackage, maybe all we need to 
do is not embed any bundle ID in them, as they are meant to only be used 
within a jpackaged app, so they should be covered by Info.plist files 
anyway.


/Erik

Re: RFR: 8286623: Bundle zlib by default with JDK on macos aarch64

[Erik Joelsson]

On Thu, 12 May 2022 08:43:56 GMT, Jaikiran Pai  wrote:

> Can I please get a review of this change to the build system which now makes 
> bundled zlib as the default for macos aarch64 systems? 
> 
> We have been running into various intermittent failures on macos aarch64 
> systems for several months now. After investigation we have been able to 
> ascertain that the root cause of the issue lies within the zlib that's 
> shipped on macos aarch64 systems. The complete details about that issue is 
> available at https://bugs.openjdk.java.net/browse/JDK-8282954. 
> We have filed a bug with Apple for their inputs on what we can do to fix it 
> in that shipped library. Given the nature of that issue, we don't have a 
> timeline on when/if the solution for that will be available. Until that time, 
> at least, the proposal is to use bundled zlib (which doesn't reproduce those 
> failures) by default on macos aarch64.

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8675

Re: RFR: 8286601: Mac Aarch: Excessive warnings to be ignored for build jdk

[Erik Joelsson]

On Wed, 11 May 2022 20:55:29 GMT, Adam Farley  wrote:

> These warnings are ignored while building the build+full jdks with gcc,
> but only ignored while building the full JDK if using clang. This
> produces tons of warnings we normally ignore, e.g. unused parameter.
> 
> I suggest that if we're ignoring this type of error while using gcc,
> we should also ignore them while using clang.
> 
> Signed-off-by: Adam Farley 

Looks good. This looks like an oversight.

-

Marked as reviewed by erikj (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8665

Integrated: 8286429: jpackageapplauncher build fails intermittently in Tier[45]

[Erik Joelsson]

On Mon, 9 May 2022 23:18:47 GMT, Erik Joelsson  wrote:

> The way LauncherCommon.gmk is currently written, it's only meant to be 
> included from "make/module//Launcher.gmk", or at least only from one 
> single place for each module. This is because the man page generation that 
> happens in LauncherCommon.gmk is module global. For the jdk.jpackage module, 
> LauncherCommon.gmk is now called from two separate sub makefiles, both 
> make/module/jdk.jpackage/Lib.gmk and make/module/jdk.jpackage/Launcher.gmk. 
> These files are called from the top level targets jdk.jpackage-libs and 
> jdk.jpackage-launchers respectively. These top level targets are assumed to 
> be able to run independently of each other, which is normally fine, but now 
> they define the same rules for the same files. This creates a race where one 
> may be deleting files that the other one is creating, causing directories to 
> disappear while files are being written to them. This can fail the build, but 
> also risks silently corrupting the build.
> 
> This patch fixes this by adding a conditional around the man page generation, 
> which helps guarantee that man pages are only processed when called from 
> make/module//Launcher.gmk. It's a bit of a hack, but it's building on 
> top of the existing design of piggybacking man page generation on the 
> launchers build.
> 
> Also fixing broken whitespace further down in the file (tabs->space).

This pull request has now been integrated.

Changeset: 65f50678
Author:Erik Joelsson 
URL:   
https://git.openjdk.java.net/jdk/commit/65f50678f2fc9b129db57181f227ba0da53ecd38
Stats: 7 lines in 1 file changed: 2 ins; 0 del; 5 mod

8286429: jpackageapplauncher build fails intermittently in Tier[45]

Reviewed-by: asemenyuk, ihse

-

PR: https://git.openjdk.java.net/jdk/pull/8618

Re: RFR: 8286430: make test TEST="gtest:" exits with error when it shouldn't [v2]

[Erik Joelsson]

On Tue, 10 May 2022 08:46:44 GMT, Severin Gehwolf  wrote:

>> `make test TEST="gtest:> `1.10.0`. It expects `suites` to be present in the google test output 
>> whereas the OpenJDK build infra code expects `cases`. I'm not sure when/if 
>> that changed, but here is a proposed fix.
>> 
>> Thoughts?
>
> Severin Gehwolf has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Handle both 'cases' and 'suites'

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8605

Re: RFR: 8286430: make test TEST="gtest:" exits with error when it shouldn't

[Erik Joelsson]

On Tue, 10 May 2022 08:42:10 GMT, Severin Gehwolf  wrote:

> Apparently gtest 1.8.1 uses `cases`. I've updated the patch to handle both: 
> `suites` and `cases`. If somebody could test it with gtest 1.8.1 I'd 
> appreciate it.

Verified that it works with 1.8.1, which does indeed use `cases`.

-

PR: https://git.openjdk.java.net/jdk/pull/8605

Re: VS2017 build errors jdk/jdk

[erik . joelsson]

On 2022-05-10 04:52, Baesken, Matthias wrote:

So maybe it time to look at it, it might be easier to have a smaller set
of VS releases to support.


Because of how much specific logic we need in configure for each version 
of Visual Studio, we have generally kept configure support for older 
versions of VS for a long time, to not unnecessarily limit people. We 
removed 2015 and older when we moved to C++11 in Hotspot as 2017 or 
later was a hard requirement.


Which compiler versions actually work depends on if anyone wants to keep 
up with the maintenance to keep them working. If VS2017 becomes unusable 
and nobody wants or is able to fix it, then I agree that we should 
remove support from configure.



On the other hand,  having VS2017 for JDK11 - head  is rather nice for  
backporting .


Visual Studio allows side by side installations of multiple major 
versions. The OpenJDK configure script will pick one by default based on 
what's available, but you can also direct it with the parameter 
--with-toolchain-version=[2019|2017|2022].


/Erik


Best regards, Matthias



-Original Message-
From: Alan Bateman 
Sent: Tuesday, 10 May 2022 13:16
To: Baesken, Matthias ; 'build-dev@openjdk.java.net' 

Cc: Zeller, Arno 
Subject: Re: VS2017 build errors jdk/jdk

On 10/05/2022 09:29, Baesken, Matthias wrote:

Hello, it seems jdk/jdk does not build any more with VS2017.
Should we still support this compiler ?

For the error see :

https://bugs.openjdk.java.net/browse/JDK-8286459
8286459: compile error with VS2017 in continuationFreezeThaw.cpp


In Oracle, we moved from using VS2019 16.9.3 to VS2022 17.1.0 a few
months ago.

I checked the Microsoft site to get the status of VS2017. It says that
the "Mainstream End Date" for that release was April 2022 so I guess
it's not easy to get updates without extended support now.

So maybe it time to look at it, it might be easier to have a smaller set
of VS releases to support.

-Alan

RFR: 8286429: jpackageapplauncher build fails intermittently in Tier[45]

[Erik Joelsson]

The way LauncherCommon.gmk is currently written, it's only meant to be included 
from "make/module//Launcher.gmk", or at least only from one single 
place for each module. This is because the man page generation that happens in 
LauncherCommon.gmk is module global. For the jdk.jpackage module, 
LauncherCommon.gmk is now called from two separate sub makefiles, both 
make/module/jdk.jpackage/Lib.gmk and make/module/jdk.jpackage/Launcher.gmk. 
These files are called from the top level targets jdk.jpackage-libs and 
jdk.jpackage-launchers respectively. These top level targets are assumed to be 
able to run independently of each other, which is normally fine, but now they 
define the same rules for the same files. This creates a race where one may be 
deleting files that the other one is creating, causing directories to disappear 
while files are being written to them. This can fail the build, but also risks 
silently corrupting the build.

This patch fixes this by adding a conditional around the man page generation, 
which helps guarantee that man pages are only processed when called from 
make/module//Launcher.gmk. It's a bit of a hack, but it's building on 
top of the existing design of piggybacking man page generation on the launchers 
build.

Also fixing broken whitespace further down in the file (tabs->space).

-

Commit messages:
 - JDK-8286429

Changes: https://git.openjdk.java.net/jdk/pull/8618/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8618=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8286429
  Stats: 7 lines in 1 file changed: 2 ins; 0 del; 5 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8618.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8618/head:pull/8618

PR: https://git.openjdk.java.net/jdk/pull/8618

Re: RFR: 8244681: Add a warning for possibly lossy conversion in compound assignments

[Erik Joelsson]

On Mon, 9 May 2022 15:56:35 GMT, Adam Sotona  wrote:

> Please review this patch adding new lint option, **lossy-conversions**, to 
> javac to warn about type casts in compound assignments with possible lossy 
> conversions.
> 
> The new lint warning is shown if the type of the right-hand operand of a 
> compound assignment is not assignment compatible with the type of the 
> variable.
> 
> The implementation of the warning is based on similar check performed to emit 
> "possible lossy conversion" compilation error for simple assignments. 
> 
> Proposed patch also include complex matrix-style test with positive and 
> negative test cases of lossy conversions in compound assignments.
> 
> Proposed patch also disables this new lint option in all affected JDK modules 
> and libraries to allow smooth JDK build. Individual cases to address possibly 
> lossy conversions warnings in JDK are already addressed in a separate 
> umbrella issue and its sub-tasks.
> 
> Thanks for your review,
> Adam

Build changes look good.

-

Marked as reviewed by erikj (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8599

Re: RFR: JDK-8285730: unify _WIN32_WINNT settings [v4]

[Erik Joelsson]

On Wed, 4 May 2022 08:00:08 GMT, Matthias Baesken  wrote:

>> Currently we set _WIN32_WINNT at various places in the codebase; this is 
>> used to target a minimum Windows version we want to support. See also for 
>> more detailled information :
>> https://docs.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers?redirectedfrom=MSDN#setting-winver-or-_win32_winnt
>> Macros for Conditional Declarations
>> "Certain functions that depend on a particular version of Windows are 
>> declared using conditional code. This enables you to use the compiler to 
>> detect whether your application uses functions that are not supported on its 
>> target version(s) of Windows."
>> 
>> However currently we have quite a lot of differing settings of _WIN32_WINNT 
>> in the codebase ; setting _WIN32_WINNT to 0x0601 (Windows 7) where possible 
>> would make sense because we have this setting already at   java_props_md.c  
>> (so targeting older Windows versions at other places is most likely not 
>> useful).
>
> Matthias Baesken has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   adjust API level to Windows 8 for security.cpp and do some cleanup

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8428

Re: RFR: 8284675: "jpackage.exe" creates application launcher without Windows Application Manfiest

[Erik Joelsson]

On Thu, 5 May 2022 19:02:51 GMT, Alexey Semenyuk  wrote:

> Copy missing manifest-related parameters to `SetupJdkExecutable` calls 
> building app launchers from `SetupBuildLauncher` 
> function:https://github.com/openjdk/jdk/blob/0c3094c8186b4d53e8bad80e2369fc7b9ae9e201/make/common/modules/LauncherCommon.gmk#L174-L175

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8561

Re: RFR: 8282060: RemoteRuntimeImageTest is not actually testing on JDK 8

[Erik Joelsson]

On Thu, 5 May 2022 09:00:47 GMT, Athijegannathan Sundararajan 
 wrote:

> This test requires jdk8 to be available while running jdk tests. But 
> JDK8_HOME is defined to be BOOT_JDK and so version check fails in the test. 
> The test vacuously passes just printing a message. There are already tests 
> that exercise jrt-fs.jar on the same JDK being tested and so basic jrt-fs.jar 
> is covered for same target JDK case.

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8547

Re: RFR: 8286105: SourceRevision.gmk should respect GIT variable

[Erik Joelsson]

On Wed, 4 May 2022 03:06:44 GMT, Yasumasa Suenaga  wrote:

> We can specify `git` binary via `GIT` in configure script, but it does not 
> affect in SourceRevision.gmk .

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8526

Re: RFR: JDK-8285730: unify _WIN32_WINNT settings [v3]

[Erik Joelsson]

On Tue, 3 May 2022 07:10:58 GMT, Matthias Baesken  wrote:

>> Currently we set _WIN32_WINNT at various places in the codebase; this is 
>> used to target a minimum Windows version we want to support. See also for 
>> more detailled information :
>> https://docs.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers?redirectedfrom=MSDN#setting-winver-or-_win32_winnt
>> Macros for Conditional Declarations
>> "Certain functions that depend on a particular version of Windows are 
>> declared using conditional code. This enables you to use the compiler to 
>> detect whether your application uses functions that are not supported on its 
>> target version(s) of Windows."
>> 
>> However currently we have quite a lot of differing settings of _WIN32_WINNT 
>> in the codebase ; setting _WIN32_WINNT to 0x0601 (Windows 7) where possible 
>> would make sense because we have this setting already at   java_props_md.c  
>> (so targeting older Windows versions at other places is most likely not 
>> useful).
>
> Matthias Baesken has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Adjust Copyright year in wepoll.c

If we define this centrally using compiler flags, then we should not also 
update each location in the source. Those need to either be removed or left 
alone. In the cases where the source definition is guarded with an ifndef and 
there is a comment explaining why a particular version of windows is required, 
keeping it around could make sense. But on the other hand, since those defines 
are overridden using flags, they are likely to bit rot over time so we might as 
well just remove all of them.

-

PR: https://git.openjdk.java.net/jdk/pull/8428

Re: RFR: 8285915: failure_handler: gather the contents of /etc/hosts file [v2]

[Erik Joelsson]

On Mon, 2 May 2022 06:25:28 GMT, Jaikiran Pai  wrote:

>> Can I please get a review of this change which addresses 
>> https://bugs.openjdk.java.net/browse/JDK-8285915?
>> 
>> With this change, the environment details collected by the failure handler 
>> will now include the contents of the `/etc/hosts/` file, which can be useful 
>> in certain cases when debugging network tests that fail.
>> 
>> Testing done (on macOS):
>> 
>> 
>> cd test/failure_handler 
>> make test
>> 
>> Then verified that the generated environment.html had the `/etc/hosts` file 
>> content
>
> Jaikiran Pai has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   print contents of hosts file in windows failure handler

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8466

Re: RFR: 8282828: CDS uncompressed oops archive is not deterministic

[Erik Joelsson]

On Fri, 29 Apr 2022 22:50:45 GMT, Ioi Lam  wrote:

> During `java -Xshare:dump -XX:-UseCompressedOops`, the location of the Java 
> heap is chosen by the OS. Due to Address Space Layout Randomization, the heap 
> will always start at a different location. This causes the archive for 
> uncompressed oops ($JAVA_HOME/lib/server/classes_nocoops.jsa) to be 
> non-deterministic.
> 
> The fix is to patch the archived object pointers to make it look like the 
> heap starts at a fixed address -- I chose 0x1000, but the exact value 
> doesn't really matter.
> 
> At runtime, the object pointers will be patched again according to the real 
> location of the heap.
> 
> Tested with tiers 1-5. I am running builds-tier5 several times to test the 
> xxx-cmp-baseline builds.

Change to compare.sh looks good. Thanks for fixing this!

-

Marked as reviewed by erikj (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8478

Re: RFR: 8285919: Remove debug printout from JDK-8285093

[Erik Joelsson]

On Fri, 29 Apr 2022 12:43:02 GMT, Magnus Ihse Bursie  wrote:

> A debug printout in configure was introduced in JDK-8285093. It should be 
> removed.

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8467

Re: RFR: 8285915: failure_handler: gather the contents of /etc/hosts file

[Erik Joelsson]

On Fri, 29 Apr 2022 12:51:21 GMT, Jaikiran Pai  wrote:

> Quick question - the path you note, is that even applicable for x64? I see 
> that it has a "System32" so just curious. 

Yes, System32 is not related to 32 vs 64 bit. As I understand it, that name was 
introduced when moving from 16 to 32 bit.

-

PR: https://git.openjdk.java.net/jdk/pull/8466

Re: RFR: 8285915: failure_handler: gather the contents of /etc/hosts file

[Erik Joelsson]

On Fri, 29 Apr 2022 11:28:32 GMT, Jaikiran Pai  wrote:

> Can I please get a review of this change which addresses 
> https://bugs.openjdk.java.net/browse/JDK-8285915?
> 
> With this change, the environment details collected by the failure handler 
> will now include the contents of the `/etc/hosts/` file, which can be useful 
> in certain cases when debugging network tests that fail.
> 
> Testing done (on macOS):
> 
> 
> cd test/failure_handler 
> make test
> 
> Then verified that the generated environment.html had the `/etc/hosts` file 
> content

Marked as reviewed by erikj (Reviewer).

Not sure if it's relevant, but did you consider doing this for Windows as well? 
The file is located at `"$WINDIR/System32/drivers/etc/hosts"`.

-

PR: https://git.openjdk.java.net/jdk/pull/8466

Integrated: 8285755: JDK-8285093 changed the default for --with-output-sync

[Erik Joelsson]

On Wed, 27 Apr 2022 19:16:48 GMT, Erik Joelsson  wrote:

> The make option '--output-sync recurse' can be useful in certain situations, 
> especially when dealing with very verbose output from makefiles and you want 
> to parse them after the fact. However, when running make interactively on the 
> command line, it certainly gets in the way, as output from each sub make call 
> is buffered until that make process terminates.
> 
> In [JDK-8285093](https://bugs.openjdk.java.net/browse/JDK-8285093), the 
> configure logic for this configuration was changed and, probably by mistake, 
> the default was changes to default "recurse" if available in the supplied GNU 
> make. This is a regression for most users of the build system and needs to be 
> changed back.

This pull request has now been integrated.

Changeset: ccf0e8bf
Author:Erik Joelsson 
URL:   
https://git.openjdk.java.net/jdk/commit/ccf0e8bf9060bca010541b9851f4f39efe9cf375
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod

8285755: JDK-8285093 changed the default for --with-output-sync

Reviewed-by: mikael, mcimadamore

-

PR: https://git.openjdk.java.net/jdk/pull/8431

Re: RFR: JDK-8285728: Alpine Linux build fails with busybox tar

[Erik Joelsson]

On Thu, 28 Apr 2022 10:24:37 GMT, Matthias Baesken  wrote:

> Currently , in target "product-bundles" , the Alpine Linux build fails when 
> BusyBox tar is used.
> Error is :

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8449

Re: RFR: 8285630: Fix a configure error in RISC-V cross build [v2]

[Erik Joelsson]

On Thu, 28 Apr 2022 07:27:36 GMT, Pengfei Li  wrote:

>> We are trying to cross build a RISC-V version of OpenJDK. We specified
>> `--openjdk-target=riscv64-linux-gnu` after `bash configure` but got an
>> error message.
>> 
>> 
>> configure: error: /usr/bin/bash 
>> /home/ent-user/jdk_src/make/autoconf/build-aux/config.sub riscv64-linux-gnu 
>> failed
>> configure exiting with result code 1
>> 
>> 
>> It shows the processing of `riscv64-linux-gnu` in script `config.sub`
>> fails. We see `config.sub` calls another script `autoconf-config.sub`
>> to validate and canonicalize a configuration type. The validation fails
>> here because `autoconf-config.sub` is a quite old version and has no
>> RISC-V support inside. Comments in those scripts tell us patching the
>> autoconf script is not a good idea so we add a fix in `config.sub` in
>> this patch.
>> 
>> We have verified RISC-V cross build succeeds after this change. As we
>> are not quite familiar with the build system, please let us know if this
>> is the best way to fix.
>
> Pengfei Li has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Address comments

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8420

Re: RFR: 8285630: Fix a configure error in RISC-V cross build [v2]

[Erik Joelsson]

On Thu, 28 Apr 2022 08:14:08 GMT, Nick Gasson  wrote:

> Is there a reason we can't import the latest upstream config.sub? That 
> already handles RISC-V and AArch64 correctly.
> 
> https://git.savannah.gnu.org/cgit/config.git/plain/config.sub

That would indeed be convenient, but unfortunately there is a license issue as 
those files changed from GPL v2 to v3. We currently have one of the last 
versions with v2.

-

PR: https://git.openjdk.java.net/jdk/pull/8420

Re: RFR: JDK-8285730: unify _WIN32_WINNT settings

[Erik Joelsson]

On Thu, 28 Apr 2022 07:16:30 GMT, Matthias Baesken  wrote:

> Hi Erik/David/Phil, we already have a good central place where we set the 
> definition of WIN32_LEAN_AND_MEAN
> 
> autoconf/flags-cflags.m4:460: ALWAYS_DEFINES_JDK="-DWIN32_LEAN_AND_MEAN 
> -D_CRT_SECURE_NO_DEPRECATE autoconf/flags-cflags.m4:462: 
> ALWAYS_DEFINES_JVM="-DNOMINMAX -DWIN32_LEAN_AND_MEAN"
> 
> should we add there the _WIN32_WINNT=0x0601 define (and remove the differing 
> defines instead at the other places) ? (defines of _WIN32_WINNT in 3rd party 
> code would stay like wepoll.c and harfbuzz).

Seems reasonable to me at least.

-

PR: https://git.openjdk.java.net/jdk/pull/8428

RFR: 8285755: JDK-8285093 changed the default for --with-output-sync

[Erik Joelsson]

The make option '--output-sync recurse' can be useful in certain situations, 
especially when dealing with very verbose output from makefiles and you want to 
parse them after the fact. However, when running make interactively on the 
command line, it certainly gets in the way, as output from each sub make call 
is buffered until that make process terminates.

In [JDK-8285093](https://bugs.openjdk.java.net/browse/JDK-8285093), the 
configure logic for this configuration was changed and, probably by mistake, 
the default was changes to default "recurse" if available in the supplied GNU 
make. This is a regression for most users of the build system and needs to be 
changed back.

-

Commit messages:
 - JDK-8285755

Changes: https://git.openjdk.java.net/jdk/pull/8431/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8431=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8285755
  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8431.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8431/head:pull/8431

PR: https://git.openjdk.java.net/jdk/pull/8431

Re: RFR: JDK-8285730: unify _WIN32_WINNT settings

[Erik Joelsson]

On Wed, 27 Apr 2022 14:57:41 GMT, Matthias Baesken  wrote:

> Currently we set _WIN32_WINNT at various places in the codebase; this is used 
> to target a minimum Windows version we want to support. See also for more 
> detailled information :
> https://docs.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers?redirectedfrom=MSDN#setting-winver-or-_win32_winnt
> Macros for Conditional Declarations
> "Certain functions that depend on a particular version of Windows are 
> declared using conditional code. This enables you to use the compiler to 
> detect whether your application uses functions that are not supported on its 
> target version(s) of Windows."
> 
> However currently we have quite a lot of differing settings of _WIN32_WINNT 
> in the codebase ; setting _WIN32_WINNT to 0x0601 (Windows 7) where possible 
> would make sense because we have this setting already at   java_props_md.c  
> (so targeting older Windows versions at other places is most likely not 
> useful).

Requiring different windows versions in different parts of the product doesn't 
make much sense, but I think it's even worse trying to keep all these different 
locations in sync. At least most of these have a comment explaining why that 
particular Windows version is required there. Changing these values invalidates 
those comments. 

If we are to do something about this, then we need to define this macro in a 
central location, and verify the value in configure. Then we would provide it 
either on compile command lines, or in a globally included config.h.

-

PR: https://git.openjdk.java.net/jdk/pull/8428

Re: build issues with BusyBox tar on Alpine

[erik . joelsson]

On 2022-04-27 06:55, Baesken, Matthias wrote:

Hi,  today I was running  into build problems with  busybox tar  on Alpine 
Linux .
When building “product-bundles”   I get :

/bin/tar: unrecognized option: I
BusyBox v1.34.1 (2022-04-04 10:19:27 UTC) multi-call binary
Usage: tar c|x|t [-ZzJjahmvokO] [-f TARFILE] [-C DIR] [-T FILE] [-X FILE] 
[LONGOPT]... [FILE]...

Looks like the   TAR_INCLUDE_PARAM   is  not  correctly  detected .
We handle already some different tar implementations  in  
autoconf/basic_tools.m4  but so far not BusyBox   :

   if test "x$TAR_TYPE" = "xgnu"; then
 TAR_INCLUDE_PARAM="T"
 TAR_SUPPORTS_TRANSFORM="true"
   elif test "x$TAR_TYPE" = "aix"; then
 # -L InputList of aix tar: name of file listing the files and directories
 # that need to be   archived or extracted
 TAR_INCLUDE_PARAM="L"
 TAR_SUPPORTS_TRANSFORM="false"
   else
 TAR_INCLUDE_PARAM="I"
 TAR_SUPPORTS_TRANSFORM="false"
   Fi

So  this leads us to  I  for   TAR_INCLUDE_PARAM  but seems  Busybox tar  has 
also  T   like gnutar :

bash-5.1# tar -v
BusyBox v1.34.1 (2022-04-04 10:19:27 UTC) multi-call binary.

Usage: tar c|x|t [-ZzJjahmvokO] [-f TARFILE] [-C DIR] [-T FILE] [-X FILE] 
[LONGOPT]... [FILE]...

Create, extract, or list files from a tar file
  ……….
 -T FILE   File with names to include

Should we add logic  to  autoconf/basic_tools.m4 to   handle  Busybox tar  
and set  TAR_INCLUDE_PARAM   correctly ?


Seems like a reasonable suggestion.

/Erik


My  “workaround” was to additionally  install   gnu tar  on my Alpine (this is 
available too ).

(another option might be to add some kind of check , currently  configure  runs 
successfully but still the build fails which is not really nice  ) !

Best regards, Matthias

Re: RFR: 8285630: Fix a configure error in RISC-V cross build

[Erik Joelsson]

On Wed, 27 Apr 2022 09:50:35 GMT, Pengfei Li  wrote:

> We are trying to cross build a RISC-V version of OpenJDK. We specified
> `--openjdk-target=riscv64-linux-gnu` after `bash configure` but got an
> error message.
> 
> 
> configure: error: /usr/bin/bash 
> /home/ent-user/jdk_src/make/autoconf/build-aux/config.sub riscv64-linux-gnu 
> failed
> configure exiting with result code 1
> 
> 
> It shows the processing of `riscv64-linux-gnu` in script `config.sub`
> fails. We see `config.sub` calls another script `autoconf-config.sub`
> to validate and canonicalize a configuration type. The validation fails
> here because `autoconf-config.sub` is a quite old version and has no
> RISC-V support inside. Comments in those scripts tell us patching the
> autoconf script is not a good idea so we add a fix in `config.sub` in
> this patch.
> 
> We have verified RISC-V cross build succeeds after this change. As we
> are not quite familiar with the build system, please let us know if this
> is the best way to fix.

Looks good. This is indeed how we resolve such issues.

-

Marked as reviewed by erikj (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8420

Re: RFR: 8285093: Introduce UTIL_ARG_WITH

[Erik Joelsson]

On Tue, 19 Apr 2022 20:38:31 GMT, Magnus Ihse Bursie  wrote:

> Analogous to `UTIL_ARG_ENABLE`, we need a `UTIL_ARG_WITH` which wraps 
> ´AC_ARG_WITH`, provides a declarative rather than programmatic way of 
> handling configure arguments. It can also make sure that all edge cases are 
> covered, and that we treat arguments consistently. 
> 
> This PR contains the implementation of `UTIL_ARG_WITH`, and an example 
> conversation of the calls to `AC_ARG_WITH` in basic_tools.m4. There are some 
> 120-odd more places where `AC_ARG_WITH` is called that need to be converted, 
> but that is out of scope for this PR. Getting the m4 logic for `AC_ARG_WITH` 
> was hard enough for this PR, and verifying a whole bunch of configure 
> arguments is mind-numbingly boring. So I intend to attack those piecewise, 
> fixing them in large enough batches at a time, later on.
> 
> I also fixed a bug and a documentation issue in `UTIL_ARG_ENABLE`.

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8306

Re: RFR: 8284999: Remove remaining files in src/samples

[Erik Joelsson]

On Tue, 19 Apr 2022 10:41:07 GMT, Magnus Ihse Bursie  wrote:

> JEP 298 was about removing demos and samples. Unfortunately, 
> [JDK-8173801](https://bugs.openjdk.java.net/browse/JDK-8173801) which should 
> have removed all files in src/samples, left a few non-source files (key 
> stores and images). These should be removed.

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8294

Re: RFR: 8284949: riscv: Add Zero support for the 32-bit RISC-V architecture

[Erik Joelsson]

On Mon, 18 Apr 2022 09:07:03 GMT, Feilong Jiang  wrote:

> This patch adds Zero support for the 32-bit RISC-V architecture.
> 
> Additional tests:
> 
> - [x] Linux zero RISCV32 cross-compilation
> - [x] Resulting binaries run on QEMU User mode without problems

Build change looks good.

-

Marked as reviewed by erikj (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8284

Re: RFR: 8284890: Support for Do not fragment IP socket options [v2]

[Erik Joelsson]

On Fri, 15 Apr 2022 11:49:29 GMT, Michael McMahon  wrote:

>> Hi,
>> 
>> Could I get the following PR review please? It adds a new JDK specific 
>> extended socket option
>> called IP_DONTFRAGMENT, which disables IP packet fragmentation in both IPv4 
>> and IPv6
>> UDP sockets (NIO DatagramChannels). For IPv4 in particular, it sets the DF 
>> (Dont Fragment) bit
>> in the IP header. There is no equivalent in the IPv6 packet header as 
>> fragmentation is implemented
>> exclusively by the sending and receiving nodes.
>> 
>> Thanks,
>> Michael
>
> Michael McMahon has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   builds in github action now

Build change looks good.

-

Marked as reviewed by erikj (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8245

Re: RFR: 8284539: Configure --with-source-date=version fails on MacOS

[Erik Joelsson]

On Thu, 14 Apr 2022 16:13:59 GMT, Andrew Leonard  wrote:

> JDK-8282769 added support for more ISO-8601 formats, but remove handling of 
> just a date "-MM-DD" being present, which is the case for a configure 
> using --with-source-date=version which uses the date string from 
> version-numbers.conf.
> Also, the first date parse had an invalid format string "%FZ %TZ", with too 
> many Zs.
> This PR corrects the first date parse to parse a standard ISO-8601 Zulu 
> date: "%FT%TZ"
> Then it adds the final check for no time being specified.
> 
> Signed-off-by: Andrew Leonard 

Marked as reviewed by erikj (Reviewer).

make/autoconf/util.m4 line 243:

> 241: # BSD date
> 242: # ISO-8601 date in Zulu 'date'T'time'Z
> 243: timestamp=$($DATE -u -j -f "%FT%TZ" "$2" "+%s" 2> /dev/null)

You are removing the space between FT and TZ, I'm just curious why and if that 
is significant.
EDIT: Never mind me, this looks good.

-

PR: https://git.openjdk.java.net/jdk/pull/8247

Re: RFR: 8284891: Fix typos in build system files

[Erik Joelsson]

On Thu, 14 Apr 2022 16:05:48 GMT, Magnus Ihse Bursie  wrote:

> I ran `codespell` on the `make` directory, and accepted those changes where 
> it indeed discovered real typos.
> 
> (Due to false positives this can unfortunately not be run automatically) 
> 
> Most of the fixes are in comments. A few are in messages aimed at the user.

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8246

Re: RFR: 8284880: Re-examine sun.invoke.util.Wrapper hash tables

[Erik Joelsson]

On Thu, 14 Apr 2022 11:19:04 GMT, Claes Redestad  wrote:

> This patch examines and optimizes `Wrapper` lookups.
> 
> First wrote a few simple microbenchmarks to verify there are actual speedups 
> from using perfect hash tables in `sun.invoke.util.Wrapper` compared to 
> simpler lookup mechanisms (such as if-else or switch). Turns out there _is_ a 
> speed-up for the case of `char` -> `Wrapper`, but not when mapping from 
> `Class` -> `Wrapper`, so let's drop those. The `forPrimitiveType` case 
> didn't use the `FROM_CHAR` table for some reason, which is remedied.  
> 
> Micros show benefits across the board for warmed up case:
> 
> 
> Baseline, OOTB
> Benchmark  Mode  Cnt   Score   Error  Units
> Wrappers.forBasicType  avgt5  14.387 ? 0.127  ns/op
> Wrappers.forPrimitive  avgt5  38.818 ? 0.592  ns/op
> Wrappers.forPrimitiveType  avgt5  26.085 ? 2.291  ns/op
> Wrappers.forWrapperavgt5  44.459 ? 1.635  ns/op
> 
> Patch, OOTB
> Benchmark  Mode  Cnt   Score   Error  Units
> Wrappers.forBasicType  avgt5  14.357 ? 0.133  ns/op
> Wrappers.forPrimitive  avgt5  23.930 ? 0.071  ns/op
> Wrappers.forPrimitiveType  avgt5  14.343 ? 0.017  ns/op
> Wrappers.forWrapperavgt5  27.622 ? 0.022  ns/op
> 
> 
> For `-Xint` case (`Wrapper` use is prominent during warmup, e.g., when 
> spinning up of MHs) there are decent or even great wins in all cases but 
> `forPrimitiveType` - which was changed from a simple switch to use the hash 
> lookup. Since the interpreter penalty is small in absolute terms and the win 
> on JITed code is significant this seems like a reasonable trade-off:
> 
> 
> Baseline, -Xint
> Benchmark  Mode  Cnt Score Error  Units
> Wrappers.forBasicType  avgt5  1246.144 ? 149.933  ns/op
> Wrappers.forPrimitive  avgt5  4955.297 ? 329.869  ns/op
> Wrappers.forPrimitiveType  avgt5   716.840 ?  62.568  ns/op
> Wrappers.forWrapperavgt5  5774.700 ? 367.627  ns/op
> 
> Patch, -Xint
> Benchmark  Mode  Cnt Score Error  Units
> Wrappers.forBasicType  avgt5  1068.096 ? 101.728  ns/op
> Wrappers.forPrimitive  avgt5  1146.670 ?  59.142  ns/op
> Wrappers.forPrimitiveType  avgt5   998.037 ? 118.144  ns/op
> Wrappers.forWrapperavgt5  3581.226 ?  20.167  ns/op

Build change looks ok.

make/test/BuildMicrobenchmark.gmk line 97:

> 95: SRC := $(MICROBENCHMARK_SRC), \
> 96: BIN := $(MICROBENCHMARK_CLASSES), \
> 97: JAVAC_FLAGS := --add-exports java.base/sun.security.util=ALL-UNNAMED 
> --add-exports java.base/sun.invoke.util=ALL-UNNAMED, \

Suggestion:

JAVAC_FLAGS := --add-exports java.base/sun.security.util=ALL-UNNAMED \
--add-exports java.base/sun.invoke.util=ALL-UNNAMED, \

-

Marked as reviewed by erikj (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8242

Re: RFR: 8284661: Reproducible assembly builds without relative linking [v3]

[Erik Joelsson]

On Mon, 11 Apr 2022 17:26:12 GMT, Andrew Leonard  wrote:

>> test/jdk/build/AbsPathsInImage.java line 167:
>> 
>>> 165: if (Files.isSymbolicLink(file)) {
>>> 166: return super.visitFile(file, attrs);
>>> 167: } else if (fileName.endsWith(".pdb")) {
>> 
>> The .debuginfo suffix has at least historically been used on more OSes than 
>> just Linux. I think we should only include .debuginfo files in this test if 
>> the OS is also Linux.
>
> @erikj79 As far as I can see only AIX is the exception of not being Linux and 
> using .debuginfo suffix, are there others?
> I am currently testing AIX, so I will see if it passes, as you say, I would 
> have my doubts it will!

Solaris used that extension as well. It's no longer supported in mainline, but 
if you backport this, which I assume you will want to, then someone may hit it.

-

PR: https://git.openjdk.java.net/jdk/pull/8177

Re: RFR: 8284661: Reproducible assembly builds without relative linking

[Erik Joelsson]

On Mon, 11 Apr 2022 12:55:16 GMT, Erik Joelsson  wrote:

>> This PR removes the need for relative object file linking introduced by 
>> JDK-8284437 for linux libraries, by specifying
>> .file  directives in the linux .S source files. The 
>> source files specify a .file ASSEMBLY_SRC_FILE
>> where ASSEMBLY_SRC_FILE is defined by the NativeCompliation.gmk.
>> 
>> Signed-off-by: Andrew Leonard 
>
> Maybe some short explanation should be added as comment in the src files?
> 
> If this makes the debuginfo files free from absolute paths on Linux, could 
> you also adjust `open/test/jdk/build/AbsPathsInImage.java` to include those 
> files on Linux? Would be good to be able to catch any regression to this 
> behavior in the future.

> thanks @erikj79 Not familiar with AbsPathsInImage, could you give some 
> background please? wondering why it's not found these paths before, maybe 
> it's not running on debugimages? What tier does it get run in?

I wrote this test when introducing the --disable-absolute-paths-in-output 
functionality, so we could verify that no absolute paths made it into any part 
of the output that we eventually ship to customers. I excluded all kinds of 
debuginfo files because I didn't know if they could be made free of absolute 
paths, and it was out of scope for what I was doing at the time. 

I believe we run this test as part of our tier2 internally at Oracle. I think 
generally it's an optional test as it relies on a specific build configuration 
that all builders of OpenJDK may not want to use.

-

PR: https://git.openjdk.java.net/jdk/pull/8177

Re: RFR: 8284661: Reproducible assembly builds without relative linking [v3]

[Erik Joelsson]

On Mon, 11 Apr 2022 15:41:20 GMT, Andrew Leonard  wrote:

>> This PR removes the need for relative object file linking introduced by 
>> JDK-8284437 for linux libraries, by specifying
>> .file  directives in the linux .S source files. The 
>> source files specify a .file ASSEMBLY_SRC_FILE
>> where ASSEMBLY_SRC_FILE is defined by the NativeCompliation.gmk.
>> 
>> Signed-off-by: Andrew Leonard 
>
> Andrew Leonard has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8284661: Reproducible assembly builds without relative linking
>   
>   Signed-off-by: Andrew Leonard 

test/jdk/build/AbsPathsInImage.java line 167:

> 165: if (Files.isSymbolicLink(file)) {
> 166: return super.visitFile(file, attrs);
> 167: } else if (fileName.endsWith(".pdb")) {

The .debuginfo suffix has at least historically been used on more OSes than 
just Linux. I think we should only include .debuginfo files in this test if the 
OS is also Linux.

-

PR: https://git.openjdk.java.net/jdk/pull/8177

Re: RFR: 8284661: Reproducible assembly builds without relative linking

[Erik Joelsson]

On Mon, 11 Apr 2022 09:43:37 GMT, Andrew Leonard  wrote:

> This PR removes the need for relative object file linking introduced by 
> JDK-8284437 for linux libraries, by specifying
> .file  directives in the linux .S source files. The 
> source files specify a .file ASSEMBLY_SRC_FILE
> where ASSEMBLY_SRC_FILE is defined by the NativeCompliation.gmk.
> 
> Signed-off-by: Andrew Leonard 

Maybe some short explanation should be added as comment in the src files?

If this makes the debuginfo files free from absolute paths on Linux, could you 
also adjust `open/test/jdk/build/AbsPathsInImage.java` to include those files 
on Linux? Would be good to be able to catch any regression to this behavior in 
the future.

-

PR: https://git.openjdk.java.net/jdk/pull/8177

Re: RFR: 8284588: Remove GensrcCommonLangtools.gmk

[Erik Joelsson]

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

Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]

[Erik Joelsson]

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]

[Erik Joelsson]

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 

Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]

[Erik Joelsson]

On Thu, 7 Apr 2022 18:17:57 GMT, Andrew Leonard  wrote:

>> make/common/NativeCompilation.gmk line 1159:
>> 
>>> 1157:   ifeq ($(call isTargetOs, linux), true)
>>> 1158: ifeq ($$($1_COMPILE_WITH_DEBUG_SYMBOLS), true)
>>> 1159:   $1_LINK_OBJS_RELATIVE := true
>> 
>> I realize this PR has already been integrated, but I have some questions 
>> about this. The comment talk about "ASM" objects (I assume that means output 
>> of assembly files), but the code change is not restricted to assembly files. 
>> 
>> Afaict, this change means that all builds on linux with debug symbols and 
>> reproducible builds will use relative paths! This is perhaps the most common 
>> compilation scenario, and it would mean that our efforts to keep a CWD 
>> neutral command line is basically in vain. :-(
>> 
>> Or can anyone (@erikj79, @andrew-m-leonard) explain to me why this would not 
>> be the case?
>
> @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.

-

PR: https://git.openjdk.java.net/jdk/pull/8124

Re: RFR: 8284437: Building from different users/workspace is not always deterministic [v2]

[Erik Joelsson]

On Thu, 7 Apr 2022 15:51:30 GMT, Magnus Ihse Bursie  wrote:

>> Andrew Leonard has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Trigger checks
>
> make/common/NativeCompilation.gmk line 1159:
> 
>> 1157:   ifeq ($(call isTargetOs, linux), true)
>> 1158: ifeq ($$($1_COMPILE_WITH_DEBUG_SYMBOLS), true)
>> 1159:   $1_LINK_OBJS_RELATIVE := true
> 
> I realize this PR has already been integrated, but I have some questions 
> about this. The comment talk about "ASM" objects (I assume that means output 
> of assembly files), but the code change is not restricted to assembly files. 
> 
> Afaict, this change means that all builds on linux with debug symbols and 
> reproducible builds will use relative paths! This is perhaps the most common 
> compilation scenario, and it would mean that our efforts to keep a CWD 
> neutral command line is basically in vain. :-(
> 
> Or can anyone (@erikj79, @andrew-m-leonard) explain to me why this would not 
> be the case?

You are correct for the linking command line. All the compilation command lines 
are still handled with flags instead.

-

PR: https://git.openjdk.java.net/jdk/pull/8124

Re: RFR: JDK-8281006 Module::getResourceAsStream should check if the resource is open unconditionally when caller is null [v2]

[Erik Joelsson]

On Thu, 7 Apr 2022 00:56:42 GMT, Tim Prinzing  wrote:

>> Created a test called NullCallerGetResource to test 
>> Module::getResourceAsStream and Class::getResourceAsStream from the native 
>> level.
>> 
>> At the java level the test builds a test module called 'n' which opens the 
>> package 'open' to everyone. There is also a package 'closed' which is 
>> neither opened or exported. Both packages have a text file called 
>> 'test.txt'. The open package has a class called OpenResources and the closed 
>> package has a class called ClosedResources. The native test is launched with 
>> the test module n added.
>> 
>> At the native level the test tries to read both the open and closed resource 
>> from both the classes and the module. It performs the equivalent of the 
>> following java operations:
>> 
>> Class c = open.OpenResources.fetchClass();
>> InputStream in = c.getResourceAsStream("test.txt");
>> assert(in != null); in.close();
>> 
>> Module n = c.getModule();
>> in = n.getResourceAsStream("open/test.txt");
>> assert(in != null); in.close();
>> 
>> Class closed = closed.ClosedResources.fetchClass();
>> assert(closedsStream("test.txt") == null);
>> assert(n.getResourceAsStream("closed/test.txt") == null);
>> 
>> The test initially threw an NPE when trying to fetch the open resource. The 
>> Module class was fixed by removing the fragment with the (caller == null) 
>> test in getResourceAsStream, and changing the call to isOpen(String,Module) 
>> to use EVERYONE_MODULE if the caller module is null.
>
> Tim Prinzing has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   update copyright date

Build change looks good.

-

Marked as reviewed by erikj (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8134

Re: RFR: 8284437: Building from different users/workspace is not always deterministic

[Erik Joelsson]

On Wed, 6 Apr 2022 10:27:40 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 

Marked as reviewed by erikj (Reviewer).

make/autoconf/flags-cflags.m4 line 109:

> 107:   workspace_root_trailing_slash="${WORKSPACE_ROOT%/}/"
> 108:   
> DEBUG_PREFIX_CFLAGS="-fdebug-prefix-map=${workspace_root_trailing_slash}="
> 109:   FLAGS_COMPILER_CHECK_ARGUMENTS(ARGUMENT: [${DEBUG_PREFIX_CFLAGS}],

Ideally capability checks for the compiler should be done separately for the 
BUILD and TARGET compiler. If cross compiling, and using different versions of 
GCC for the BUILD and TARGET, we may get into trouble. That is certainly a rare 
case, but still something to be aware of at least. Not sure if it's worth 
trying to fix.

-

PR: https://git.openjdk.java.net/jdk/pull/8124

Re: RFR: 8283999: Update JMH devkit to 1.35

[Erik Joelsson]

On Wed, 30 Mar 2022 12:25:52 GMT, Aleksey Shipilev  wrote:

> JMH 1.35 is released, so we can bump the devkit too.
> 
> Additional testing:
>  - [x] JMH devkit creation
>  - [x] Sample benchmarks

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8040

Re: RFR: 8283901: Introduce "make doctor" to diagnose build environment problems

[Erik Joelsson]

On Wed, 30 Mar 2022 01:05:08 GMT, Magnus Ihse Bursie  wrote:

> When a user has problems with their build environment that trips up their 
> builds, the cause is often one of just a few "popular" gotchas. For us 
> working with the build system, we've seen them all before, but for the 
> individual user, it's often the first time, and it can be really frustrating 
> to debug.
> 
> The Doctor will help users detect if any of these well-known problems is 
> present. Not all issues can be tested automatically, but enough of the most 
> common problems *can* actually be checked, which makes this a worthwhile 
> exercise.
> 
> (This was inspired by the macOS Homebrew "brew doctor".) 
> 
> This is an example output:
> 
> $ make doctor
> Building target 'doctor' in configuration 'linux-x86_64-server-release'
> 
> "make doctor" will help you analyze your build environment. It can highlight
> certain well-known problems, but it can never find all possible errors.
> 
> * Verifying that configure has picked up git...
> 
> * Checking for warnings from configure...
>  ---
> WARNING: The result of this configuration has overridden an older
> configuration. You *should* run 'make clean' to make sure you get a
> proper build. Failure to do so might result in strange build problems.
>  ---
> ! Inspect the warnings, fix any problems, and re-run configure
> 
> * Checking for left-over core files...
> 
> * Checking for untracked files with illegal names...
> 
> * If all else fails, try removing the entire build directory and re-creating
> the same configuration using:
>  ---
> configure_command_line=$(make print-configuration)
> make dist-clean
> bash configure $configure_command_line
>  ---
> 
> * The build README (doc/building.md) is a great source of information,
> especially the chapter "Fixing Unexpected Build Failures". Check it out!
> 
> * If you still need assistance please contact build-dev@openjdk.java.net.
> 
> Finished building target 'doctor' in configuration 
> 'linux-x86_64-server-release'

Nice feature!

Did you consider using `.NOTPARALLEL` in Doctor.gmk instead of explicitly 
declaring prereqs between every target?

-

Marked as reviewed by erikj (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8027

Re: RFR: 8283723: Update Visual Studio 2022 to version 17.1.0 for Oracle builds on Windows [v3]

[Erik Joelsson]

On Mon, 28 Mar 2022 16:40:31 GMT, Mikael Vidstedt  wrote:

>> Oracle is updating the version of Visual Studio for building the JDK on 
>> Windows to Visual Studio 2022 17.1.0.
>> 
>> This change adds support for building devkits based on VS 2022. Instead of 
>> creating a new script file for that I decided to combine it with the logic 
>> in createWindowsDevkit2019.sh and rename the resulting file. I decided to 
>> dropped support for VS 2017 since that version is pretty old now, let me 
>> know if you'd prefer to see it kept around.
>> 
>> Testing: tier1-5 + additional testing.
>
> Mikael Vidstedt has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Convert PROGRAMFILES to unix path

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7974

Integrated: 8283575: Check for GNU time fails for version >1.7

[Erik Joelsson]

On Wed, 23 Mar 2022 15:35:44 GMT, Erik Joelsson  wrote:

> The version output of GNU time changed from "GNU time" to "GNU Time" in 
> version 1.8. We need to update our check for identifying GNU time to handle 
> this.

This pull request has now been integrated.

Changeset: 1c4f5fcb
Author:Erik Joelsson 
URL:   
https://git.openjdk.java.net/jdk/commit/1c4f5fcb88892e6c76074eac87b63d81d53647b2
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod

8283575: Check for GNU time fails for version >1.7

Reviewed-by: shade, ihse

-

PR: https://git.openjdk.java.net/jdk/pull/7925

Re: RFR: 8283575: Check for GNU time fails for version >1.7 [v2]

[Erik Joelsson]

> The version output of GNU time changed from "GNU time" to "GNU Time" in 
> version 1.8. We need to update our check for identifying GNU time to handle 
> this.

Erik Joelsson has updated the pull request incrementally with one additional 
commit since the last revision:

  Update make/autoconf/basic_tools.m4
  
  Co-authored-by: Magnus Ihse Bursie 

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7925/files
  - new: https://git.openjdk.java.net/jdk/pull/7925/files/2cf533f0..04d6bf49

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=7925=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7925=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7925.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7925/head:pull/7925

PR: https://git.openjdk.java.net/jdk/pull/7925

Re: RFR: 8283323: libharfbuzz optimization level results in extreme build times

[Erik Joelsson]

On Wed, 23 Mar 2022 12:25:08 GMT, Magnus Ihse Bursie  wrote:

> [JDK-8247872](https://bugs.openjdk.java.net/browse/JDK-8247872) (upgrade 
> HarfBuzz to 2.7.2) caused build time to go up with 24 seconds on my reference 
> linux machine. This was one of the four culprits that caused a 25-30% build 
> time regression over the last two years.
> 
> The problem here was that the new HarfBuzz code caught really bad behaviour 
> from gcc when compiling with optimizations. The official HarfBuzz build does 
> not use any -O flags at all for gcc, so presumably the HarfBuzz team is:
> 
> a) not thinking compiler optimization is important for the performance of 
> this library, and
> b) unaware that their code causes such a headache for gcc.
> 
> (Other compilers fare much better: visual studio makes no difference at all, 
> and for clang just a small regression was observed.)
> 
> The current optimization level was introduced by 
> [JDK-8255790](https://bugs.openjdk.java.net/browse/JDK-8255790), which were 
> really about moving libharfbuzz compilation back into libfontmanager. I could 
> find no comments/discussion relating to the change of optimization level, so 
> I assume it was incidental, and just seemed good at the time.
> 
> This patch changes the optimization level to `SIZE` (which is the closest 
> thing we have to no optimization level) on gcc.

> I have a question, though, because I suppose font rendering engine is 
> performance sensitive. AFAICS, the `LIBFONTMANAGER_OPTIMIZATION` was changed 
> from `HIGH` to `HIGHEST` here: 
> [05fe06a6#diff-c75e2d581b3730cf900075cedb66ac72eaba40fbbd92eaac5710161830b572b4L534-R491](https://github.com/openjdk/jdk/commit/05fe06a6#diff-c75e2d581b3730cf900075cedb66ac72eaba40fbbd92eaac5710161830b572b4L534-R491)
> 
> Does it mean we should instead downgrade to `HIGH`, not `SIZE`? Would that 
> recover the build times?

For GCC on Linux, that doesn't make a difference as both HIGH and HIGHEST 
translate to -O3.

I've been looking a bit at this too, and there are a handful of files that 
stick out, hb-subset.cc is the worst one. Here are the top three on my Linux 
host (using 31 threads), first with -O3 and then with Os:

[TIME:0:18.36] hb-subset-plan.cc
[TIME:0:21.53] hb-ot-layout.cc
[TIME:0:35.63] hb-subset.cc

[TIME:0:12.51] hb-subset-plan.cc
[TIME:0:14.81] hb-ot-layout.cc
[TIME:0:21.40] hb-subset.cc

On my mac (6 threads, but single core perf is higher), the corresponding 
numbers are:

[TIME:0:14.46] hb-subset-plan.cc
[TIME:0:19.64] hb-ot-layout.cc
[TIME:0:27.45] hb-subset.cc

[TIME:0:11.69] hb-subset-plan.cc
[TIME:0:16.00] hb-ot-layout.cc
[TIME:0:20.68] hb-subset.cc

This is certainly not quite as bad, but still a clear difference.

The reason this has such a high impact on total compilation time is due to how 
java.desktop ends up being one of the last top level targets on the critical 
path of possible concurrency. So a single compilation unit here will stall the 
whole process on a machine with many threads (32 in my case). On a smaller 
machine, this impact will be much less pronounced. Here are the total 
compilation times for "make exploded-image" with a few different settings:

all libfontmanager HIGHEST
linux: 4m14.174s
macosx: 7m8.736s

all libfontmanager SIZE
linux: 3m56.134s
macosx: 7m5.966s

just the above 3 files SIZE
linux: 3m57.009s
macosx: (skipped, no point)

The difference on the smaller macosx machine is negligible. Most of the 
compilation time gain on the big machine is gained by just dropping the 
optimization level on a few files. 

One could certainly argue that dropping the optimization level for 
libfontmanager is only helping a rather niche usecase. There could certainly be 
other compilation units that would have a similar effect on total build time 
due to how they end up on the critical path, I would be surprised if there 
aren't. The question is which ones we can safely drop the optimization level 
for without adversely affect performance of the finished product.

For this particular change it's probably better to at least just drop it for 
one or a few files.

-

PR: https://git.openjdk.java.net/jdk/pull/7919

Re: RFR: 8283575: Check for GNU time fails for version >1.7

[Erik Joelsson]

On Wed, 23 Mar 2022 16:22:55 GMT, Aleksey Shipilev  wrote:

> Looks okay, provided you tested with some version of GNU [Tt]ime.

Thanks! I hit this while trying to use GNU time on my mac to get LOG=profile to 
work, to profile compilation times of individual compilation units. I built 
latest (1.9) from source and it didn't detect it (same with 1.8). With this 
patch it works as expected.

> Is double square bracket some sort of escaping? Haven't seen it before.

Yes, it's escaping needed to use brackets in autoconf/m4. Sometimes we put the 
escape sequence around the complete expression, sometimes we just double up 
like I did here. You should be able to find several occurrences through out our 
autoconf scripts.

-

PR: https://git.openjdk.java.net/jdk/pull/7925

RFR: 8283575: Check for GNU time fails for version >1.7

[Erik Joelsson]

The version output of GNU time changed from "GNU time" to "GNU Time" in version 
1.8. We need to update our check for identifying GNU time to handle this.

-

Commit messages:
 - JDK-8283575

Changes: https://git.openjdk.java.net/jdk/pull/7925/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7925=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8283575
  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7925.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7925/head:pull/7925

PR: https://git.openjdk.java.net/jdk/pull/7925

Re: RFR: 8283323: libharfbuzz optimization level results in extreme build times

[Erik Joelsson]

On Wed, 23 Mar 2022 12:25:08 GMT, Magnus Ihse Bursie  wrote:

> [JDK-8247872](https://bugs.openjdk.java.net/browse/JDK-8247872) (upgrade 
> HarfBuzz to 2.7.2) caused build time to go up with 24 seconds on my reference 
> linux machine. This was one of the four culprits that caused a 25-30% build 
> time regression over the last two years.
> 
> The problem here was that the new HarfBuzz code caught really bad behaviour 
> from gcc when compiling with optimizations. The official HarfBuzz build does 
> not use any -O flags at all for gcc, so presumably the HarfBuzz team is:
> 
> a) not thinking compiler optimization is important for the performance of 
> this library, and
> b) unaware that their code causes such a headache for gcc.
> 
> (Other compilers fare much better: visual studio makes no difference at all, 
> and for clang just a small regression was observed.)
> 
> The current optimization level was introduced by 
> [JDK-8255790](https://bugs.openjdk.java.net/browse/JDK-8255790), which were 
> really about moving libharfbuzz compilation back into libfontmanager. I could 
> find no comments/discussion relating to the change of optimization level, so 
> I assume it was incidental, and just seemed good at the time.
> 
> This patch changes the optimization level to `SIZE` (which is the closest 
> thing we have to no optimization level) on gcc.

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7919

Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v4]

[Erik Joelsson]

On Tue, 22 Mar 2022 19:07:12 GMT, Maurizio Cimadamore  
wrote:

>> This PR contains the API and implementation changes for JEP-424 [1]. A more 
>> detailed description of such changes, to avoid repetitions during the review 
>> process, is included as a separate comment.
>> 
>> [1] - https://openjdk.java.net/jeps/424
>
> Maurizio Cimadamore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix indentation in Lib.gmk

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7888

Re: RFR: 8283457: [macos] libpng build failures with Xcode13.3

[Erik Joelsson]

On Tue, 22 Mar 2022 18:53:20 GMT, Phil Race  wrote:

> Disable a warning from the Xcode 13.3 clang compiler when compiling libpng, 
> which is used by libsplashscreen.
> 
> Policy is not to change the upstream code locally unless it is also changed 
> in the same way upstream.
> We could consider reporting it upstream but libpng releases are very 
> infrequent.

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7911

Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v3]

[Erik Joelsson]

On Tue, 22 Mar 2022 14:04:07 GMT, Maurizio Cimadamore  
wrote:

>> This PR contains the API and implementation changes for JEP-424 [1]. A more 
>> detailed description of such changes, to avoid repetitions during the review 
>> process, is included as a separate comment.
>> 
>> [1] - https://openjdk.java.net/jeps/424
>
> Maurizio Cimadamore has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - rename syslookup lib on Windows
>  - Add missing LIBS flag
>  - Simplify syslookup build changes

make/modules/java.base/Lib.gmk line 217:

> 215:   LDFLAGS_linux := -Wl$(COMMA)--no-as-needed, \
> 216:   LIBS := $(LIBCXX), \
> 217:   LIBS_linux := -lc -lm -ldl, \

This looks much better, thanks! Now if you could just fix the indentation of 
the parameters to 4 spaces, it would be perfect.

-

PR: https://git.openjdk.java.net/jdk/pull/7888

Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview)

[Erik Joelsson]

On Mon, 21 Mar 2022 10:45:27 GMT, Maurizio Cimadamore  
wrote:

> This PR contains the API and implementation changes for JEP-424 [1]. A more 
> detailed description of such changes, to avoid repetitions during the review 
> process, is included as a separate comment.
> 
> [1] - https://openjdk.java.net/jeps/424

Build changes look ok.

make/modules/java.base/Lib.gmk line 217:

> 215:   CXXFLAGS := $(CXXFLAGS_JDKLIB), \
> 216:   LDFLAGS := $(LDFLAGS_JDKLIB) -Wl$(COMMA)--no-as-needed, \
> 217:   LIBS := $(LIBCXX) -lc -lm -ldl, \

Instead of repeating this whole macro call for both Linux and non Linux, you 
can use parameters of the form LDFLAGS_linux and LIBS_linux to add the Linux 
specific flags. Something like this:


LDFLAGS := $(LDFLAGS_JDKLIB), \
LDFLAGS_linux := -Wl$(COMMA)--no-as-needed, \


For the NAME field, there is no such support, so the way we usually do that is 
through a variable and conditionals before the macro call. What's the reason to 
have a different lib name on Windows? If they were the same, and the source 
file in windows/native/... had the same name, it would just automatically 
override in the build.

I realize now that this is just moved code from jdk.incubator.foreign, and this 
patch is probably big enough as it is so no need to refactor the build logic at 
the same time.

-

Marked as reviewed by erikj (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7888

Re: RFR: 8257733: Move module-specific data from make to respective module [v13]

[Erik Joelsson]

On Mon, 21 Mar 2022 16:29:25 GMT, Magnus Ihse Bursie  wrote:

>> A lot (but not all) of the data in make/data is tied to a specific module. 
>> For instance, the publicsuffixlist is used by java.base, and fontconfig by 
>> java.desktop. (A few directories, like mainmanifest, is *actually* used by 
>> make for the whole build.) 
>> 
>> These data files should move to the module they belong to. The are, after 
>> all, "source code" for that module that is "compiler" into resulting 
>> deliverables, for that module. (But the "source code" language is not Java 
>> or C, but typically a highly domain specific language or data format, and 
>> the "compilation" is, often, a specialized transformation.) 
>> 
>> This misplacement of the data directory is most visible at code review time. 
>> When such data is changed, most of the time build-dev (or the new build 
>> label) is involved, even though this has nothing to do with the build. While 
>> this is annoying, a worse problem is if the actual team that needs to review 
>> the patch (i.e., the team owning the module) is missed in the review.
>> 
>> ### Modules reviewed
>> 
>> - [x] java.base
>> - [x] java.desktop
>> - [x] jdk.compiler
>> - [x] java.se
>
> Magnus Ihse Bursie has updated the pull request with a new target base due to 
> a merge or a rebase. The pull request now contains 20 commits:
> 
>  - Merge branch 'master' into shuffle-data
>  - Correct name for bfc files
>  - Merge tag 'jdk-19+14' into shuffle-data
>
>Added tag jdk-19+14 for changeset 08cadb47
>  - Move x11wrappergen from share/data to unix/data
>  - Fix fontconfig according to review request
>  - Fix typos
>  - Restore charsetmapping and cldr to make/data
>  - Update copyright year
>  - Merge branch 'master' into shuffle-data-reborn
>  - Fix merge
>  - ... and 10 more: 
> https://git.openjdk.java.net/jdk/compare/19d34bdf...1c47d82d

OS specific X11 and fontconfig looks good.

make/modules/java.desktop/gendata/GendataFontConfig.gmk line 57:

> 55:   TARGETS += $(FONTCONFIG_OUT_BIN_FILE)
> 56: 
> 57: endif

It says no newline at end of file, maybe add that if it's true.

-

Marked as reviewed by erikj (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1611

Re: RFR: 8283057: Update GCC to version 11.2 for Oracle builds on Linux

[Erik Joelsson]

On Fri, 18 Mar 2022 20:33:54 GMT, Mikael Vidstedt  wrote:

> Oracle is updating the version of GCC for building the JDK on Linux to 11.2. 
> 
> Testing: tier1-5 + additional significant testing. Re-running 
> tier1,builds-tier{2,3,4,5} now for good luck.

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7871

Re: RFR: JDK-8283320: Error message for Windows libraries always points to --with-msvcr-dll no matter the actual file name

[Erik Joelsson]

On Thu, 17 Mar 2022 13:11:19 GMT, Julian Waters  wrote:

> TOOLCHAIN_SETUP_MSVC_DLL always points to --with-msvcr-dll if it couldn't 
> find the requested file, even if the dll being searched for was msvcp.dll for 
> instance. This small patch fixes the potentially confusing advice to point to 
> the correct options

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7853

Re: RFR: 8257733: Move module-specific data from make to respective module [v9]

[Erik Joelsson]

On Thu, 17 Mar 2022 00:12:38 GMT, Magnus Ihse Bursie  wrote:

>> A lot (but not all) of the data in make/data is tied to a specific module. 
>> For instance, the publicsuffixlist is used by java.base, and fontconfig by 
>> java.desktop. (A few directories, like mainmanifest, is *actually* used by 
>> make for the whole build.) 
>> 
>> These data files should move to the module they belong to. The are, after 
>> all, "source code" for that module that is "compiler" into resulting 
>> deliverables, for that module. (But the "source code" language is not Java 
>> or C, but typically a highly domain specific language or data format, and 
>> the "compilation" is, often, a specialized transformation.) 
>> 
>> This misplacement of the data directory is most visible at code review time. 
>> When such data is changed, most of the time build-dev (or the new build 
>> label) is involved, even though this has nothing to do with the build. While 
>> this is annoying, a worse problem is if the actual team that needs to review 
>> the patch (i.e., the team owning the module) is missed in the review.
>> 
>> ### Modules reviewed
>> 
>> - [x] java.base
>> - [x] java.desktop
>> - [x] jdk.compiler
>> - [x] java.se
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix typos

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/1611

Re: RFR: 8283260: gcc is not supported on mac

[Erik Joelsson]

On Wed, 16 Mar 2022 13:14:39 GMT, Magnus Ihse Bursie  wrote:

> Nowadays only clang is available as compiler for macOS. We should remove 
> remnants of the gcc support.

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7841

Re: RFR: JDK-8236128: Allow jpackage create installers for services

[Erik Joelsson]

On Fri, 11 Mar 2022 23:37:06 GMT, Alexey Semenyuk  wrote:

> Implementation of [JDK-8275062: "Allow jpackage create installers for 
> services"](https://bugs.openjdk.java.net/browse/JDK-8275062)
>  CSR

Build change looks good.

-

PR: https://git.openjdk.java.net/jdk/pull/7793

Re: Supporting alternative toolchains on Windows

[erik . joelsson]

On 2022-03-11 23:17, Julian Waters wrote:

I meant in toolchain.m4, which allows gcc for macOS


Apple used to ship GCC as part of Xcode, and the original Mac port of 
OpenJDK used that GCC. This was eventually removed from Xcode and the 
OpenJDK build was changed to use Clang instead. To make the transition 
smoother, as we can't expect all OpenJDK builders to change Xcode 
versions at the same time, we left dual support in the build, and we 
never got around to removing the GCC support. There is still a GCC 
launcher in Xcode, but it's just a wrapper for Clang. AFAIK, we have 
never supported any other compiler than the one in Xcode on Macos.


So while toolchain.m4 may make it look like we support GCC on Macos, 
that isn't actually the case. We only support Xcode, and I very much 
doubt we can still build with an Xcode old enough to still include an 
actual GCC.


/Erik


best regards,
Julian

On Sat, Mar 12, 2022 at 4:32 AM Magnus Ihse Bursie <
magnus.ihse.bur...@oracle.com> wrote:


On 2022-03-11 20:02, Julian Waters wrote:

I understand the concerns, seems like I grossly underestimated the
complexity such a task would entail. Though I would say the following can
only really be known if it's already been implemented:

 Most likely there will be strange bugs with anything that requires
OS interaction (like dll loading and whatsnot)

To my knowledge the versions of said compilers that link against the
universal CRT utilize exactly the same Windows APIs (And corresponding
dlls) for OS related stuff that Visual C++ itself uses (Minus vcruntime,
which is specific to MSVC), without any POSIX compatibility layers on top.
I've tried rewiring the build system incrementally in the meantime on my
end to see what areas would be of interest and it's now failing when
hitting POSIX specific includes during make, hinting that (At least for
gcc) compatibility has been traded for full native support for Windows APIs
within the compiler, which may mitigate any issues slightly (That's also
why I suggested initially to only allow for versions that link against the
ucrt without any compatibility layers- Cygwin's toolchains which link
against newlib and old MinGW binaries which link against msvcrt would just
be an unnecessary headache). That said, whether aforementioned bugs will
actually surface should the attempt be successful I don't really know yet

 Actually, I would seriously assume that any other compiler than VS
on Windows will give much worse results.
 The VS compiler is battle-hardened. gcc and clang are only used by
a very small enthusiastic hobbyist minority.

The Windows ports of both compilers do keep the same optimizations and
code generation quality as they do on other platforms from what I know
though, it's mainly the linkers that have been modified to generate the PE
format instead of the ELF on Linux. I digress, I don't have any results to
show for this yet

I'm not sure I get the part about macOS strictly mapping to clang. Isn't
there the option to swap to using gcc for macOS?

No, Apple removed that option years ago.

/Magnus


best regards,
Julian

On Sat, Mar 12, 2022 at 1:50 AM Magnus Ihse Bursie <
magnus.ihse.bur...@oracle.com> wrote:



On 2022-03-11 14:34, Julian Waters wrote:

Darn, seems like it'll be much harder than I expected. Since multiple
toolchains are supported for macOS and Linux, I assumed a slight patch
would help get it to work on Windows. Looking through the stuff in make
though, it appears a lot of the build system implicitly expects the
compiler for Windows to always be Visual C++, which doesn't really help
that much (Though the fact that we can exclude many versions of gcc, such
as Cygwin's and old MinGW binaries helps a lot). The build process for the
newer Windows ports of gcc are surprisingly similar to Visual C++ though
(Eg rc can be swapped out for windres) so this might hopefully be something
I can try exploring in the future (Gonna look a bit harder at make and
write what I can find back to this mailing list in the meantime). It'd be
interesting if benchmarks of the JVM compiled with different compilers on
Windows can be compared side by side on the off chance this becomes a
reality though


In theory OS and compiler toolchain are separate things. In practice, in
the JDK, they are not. There is basically a 1-to-1 mapping between
toolchain and OS:
Windows <-> Visual Studio
macOS <-> XCode/clang
linux <-> gcc

(The one possible exception is that clang on linux is probably feasible.)

After years and years on this, all kinds of assumptions has been
hard-coded, even if people try to do the right thing. But sometimes it is
not clear if you are checking for toolchain or os; perhaps when linking
with a specific library, or setting some define.

Any attempt to change this is, as I said, a *huge* undertaking. *And*
there will be tons of negative consequences for the code base as a whole,
when trying to differentiate between toolchain and OS. So this will 

Re: RFR: 8283062: Uninitialized warnings in libgtest with GCC 11.2

[Erik Joelsson]

On Sat, 12 Mar 2022 03:26:29 GMT, Mikael Vidstedt  wrote:

> Background, from JBS:
> 
> In file included from 
> googletest-release-1.8.1/googletest/src/gtest-all.cc:42: 
> googletest-release-1.8.1/googletest/src/gtest-death-test.cc: In function 
> 'bool testing::internal::StackGrowsDown()': 
> googletest-release-1.8.1/googletest/src/gtest-death-test.cc:1224:24: error: 
> 'dummy' may be used uninitialized [-Werror=maybe-uninitialized] 
>  1224 | StackLowerThanAddress(, ); 
>   | ~^ 
> googletest-release-1.8.1/googletest/src/gtest-death-test.cc:1214:13: note: by 
> argument 1 of type 'const void*' to 'void 
> testing::internal::StackLowerThanAddress(const void*, bool*)' declared here 
>  1214 | static void StackLowerThanAddress(const void* ptr, bool* result) { 
>   | ^ 
> googletest-release-1.8.1/googletest/src/gtest-death-test.cc:1222:7: note: 
> 'dummy' declared here 
>  1222 | int dummy;
> 
> 
> Details:
> 
> Since googletest is external code this change disable the relevant warning.
> 
> Testing:
> 
> tier1, builds-tier{2,3,4,5}

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7798

Re: RFR: 8283017: GHA: Workflows break with update release versions

[Erik Joelsson]

On Fri, 11 Mar 2022 09:27:55 GMT, Aleksey Shipilev  wrote:

> Current GHA workflow only takes `VERSION_FEATURE` to deduce the bundle names, 
> which means the test jobs in GHA workflows are unable to run.
> 
> See for example JDK 18u GHA run:
> 
> Build step produce:
> 
> 
> Creating jdk-18.0.1-internal+0_linux-x64_bin.tar.gz
> Creating jdk-18.0.1-internal+0_linux-x64_bin-symbols.tar.gz
> Creating jdk-18.0.1-internal+0_linux-x64_bin-tests-demos.tar.gz
> 
> 
> Persist step fails to find it:
> 
> 
> Warning: No files were found with the provided path: 
> jdk/build/linux-x64/bundles/jdk-18-internal+0_linux-x64_bin.tar.gz
> 
> 
> ...because it looks for "18", not "18.0.1".
> 
> 17u and 11u hacked the GHA workflow to get tests to work (see 
> [JDK-8276130](https://bugs.openjdk.java.net/browse/JDK-8276130)), but this 
> would keep breaking in update releases as 19.0.1, 20.0.1, etc. fork out of 
> the mainline. We should instead fix that in the mainline workflow config.
> 
> Additional testing:
>  - [x] GHA passes with "fake" 19.0.1
>  - [ ] GHA passes with "normal" 19

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7785

Re: RFR: 8282948: JDK-8274980 missed correct handling of MACOSX_BUNDLE_BUILD_VERSION

[Erik Joelsson]

On Thu, 10 Mar 2022 11:49:06 GMT, Magnus Ihse Bursie  wrote:

> If VERSION_BUILD is empty, we must still store a "0" in 
> MACOSX_BUNDLE_BUILD_VERSION.

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7773

Re: RFR: 8253495: CDS generates non-deterministic output

[Erik Joelsson]

On Tue, 8 Mar 2022 19:11:02 GMT, Ioi Lam  wrote:

> This patch makes the result of "java -Xshare:dump" deterministic:
> - Disabled new Java threads from launching. This is harmless. See comments in 
> jvm.cpp
> - Fixed a problem in hashtable ordering in heapShared.cpp
> - BasicHashtableEntry has a gap on 64-bit platforms that may contain random 
> bits. Added code to zero it.
> - Enabled checking of $JAVA_HOME/lib/server/classes.jsa in 
> make/scripts/compare.sh
> 
> Note:  $JAVA_HOME/lib/server/classes_ncoops.jsa is still non-deterministic. 
> This will be fixed in 
> [JDK-8282828](https://bugs.openjdk.java.net/browse/JDK-8282828).
> 
> Testing under way:
> - tier1~tier5
> - Run all *-cmp-baseline jobs 20 times each (linux-aarch64-cmp-baseline, 
> windows-x86-cmp-baseline,  etc).

compare.sh looks good. Can't comment on the rest. Thank you for fixing this!

-

Marked as reviewed by erikj (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7748

Re: RFR: 8282770: Set source date in jib profiles from buildId

[Erik Joelsson]

On Mon, 7 Mar 2022 23:32:33 GMT, Magnus Ihse Bursie  wrote:

> For reproducability, we should set the source date on jib profiles from the 
> buildId.

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7735

Re: RFR: 8282769: BSD date cannot handle all ISO 8601 formats

[Erik Joelsson]

On Mon, 7 Mar 2022 22:47:13 GMT, Magnus Ihse Bursie  wrote:

> The BSD version of the date command that ships with macOS cannot handle all 
> ISO 8601 formats. More specifically, it cannot handle trailing milliseconds.
> 
> This fix will allow it to be more tolerant.

Are we losing the timezone when using this fallback? I'm not sure if that is 
actually better behavior.

-

Marked as reviewed by erikj (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7733

Re: RFR: 8282567: Improve source-date handling in build system [v6]

[Erik Joelsson]

On Mon, 7 Mar 2022 17:23:40 GMT, Magnus Ihse Bursie  wrote:

>> When doing reproducible builds, controlling the build time is imperative. To 
>> make this as easy as possible, some changes are needed in the build system.
>> 
>>  * If source-date is set to anything other than "updated" (meaning that it 
>> should be determined at build time), then the default value of 
>> --with-hotspot-build-time should be the same time.
>> 
>> * If the industry standard SOURCE_DATE_EPOCH is set when running configure, 
>> the default value for --with-source-date should be picked up from this 
>> variable. (If the command line option is given, it will override the 
>> environment variable however.)
>> 
>> * Finally the code can be made a bit clearer that we can set and export 
>> SOURCE_DATE_EPOCH to SOURCE_DATE in spec.gmk, unless we're using the 
>> "updated" (determined at build time) value.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix building.md

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7660

Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines

[Erik Joelsson]

On Sat, 5 Mar 2022 06:49:16 GMT, Julian Waters  wrote:

> Should I change the JBS issue title to match the PR title, or is it preferred 
> for the PR title to change?

They need to match. You can either do it manually, or change the title to just 
the bug number and the bot will change it for you.

-

PR: https://git.openjdk.java.net/jdk/pull/7268

Re: RFR: 8282567: Improve source-date handling in build system [v5]

[Erik Joelsson]

On Sat, 5 Mar 2022 08:57:45 GMT, Magnus Ihse Bursie  wrote:

>> When doing reproducible builds, controlling the build time is imperative. To 
>> make this as easy as possible, some changes are needed in the build system.
>> 
>>  * If source-date is set to anything other than "updated" (meaning that it 
>> should be determined at build time), then the default value of 
>> --with-hotspot-build-time should be the same time.
>> 
>> * If the industry standard SOURCE_DATE_EPOCH is set when running configure, 
>> the default value for --with-source-date should be picked up from this 
>> variable. (If the command line option is given, it will override the 
>> environment variable however.)
>> 
>> * Finally the code can be made a bit clearer that we can set and export 
>> SOURCE_DATE_EPOCH to SOURCE_DATE in spec.gmk, unless we're using the 
>> "updated" (determined at build time) value.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Test for updated, not current

doc/building.md line 1546:

> 1544: You must also make sure your build does not rely on `configure`'s 
> default adhoc
> 1545: version strings. Default adhoc version strings `OPT` segment include 
> user name,
> 1546: source directory and timestamp. You can either override just the `OPT` 
> segment

The timestamp comes from Oracle internal builds using Jib. If you just use 
configure, the default OPT string is `adhoc..`.

As a followup on this, I think we should change jib-profiles.js to extract the 
date from the buildId and set --with-source-date based on it.

doc/building.md line 1570:

> 1568:  * `--with-source-date`
> 1569: 
> 1570: This option controls how the JDK build sets SOURCE_DATE_ EPOCH when

Space in variable name. Should it be backtick quoted?

-

PR: https://git.openjdk.java.net/jdk/pull/7660

Re: RFR: JDK-8282700: Properly handle several --without options during configure [v2]

[Erik Joelsson]

On Mon, 7 Mar 2022 08:07:30 GMT, Julian Waters  wrote:

>> Some of the --without options are not properly handled and will crash when 
>> processed (For example, --without-version-string), in other cases the 
>> --without-* option will actually silently produce incorrect results instead 
>> of actually doing what --without-* implies (For example, 
>> --without-build-user and all the --with-vendor-* options). The most elegant 
>> way to solve this would simply be to handle such cases and display warnings 
>> when they're encountered (or if the option is critical to the build process, 
>> throwing an error)
>> 
>> Even if it doesn't make sense to pass said option however, it should display 
>> a warning instead of letting configure exit with a confusing error when it's 
>> run
>
> Julian Waters has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Handle remaining options and change critical options to error when 
> --without is passed

I think it's good to error out when incorrectly using --without-foo options. I 
don't think we need warnings in cases where --without-foo is equivalent to 
--with-foo="".

make/autoconf/jdk-version.m4 line 113:

> 111: AC_MSG_ERROR([--with-vendor-name must have a value])
> 112:   elif test "x$with_vendor_name" = xno; then
> 113: AC_MSG_WARN([--without-vendor-name is the same as not passing 
> --with-vendor-name to begin with])

I don't think this warrants a warning. The user is just trying to be explicit. 
It's good to handle the "no" case though.

make/autoconf/jdk-version.m4 line 129:

> 127: AC_MSG_ERROR([--with-vendor-url must have a value])
> 128:   elif test "x$with_vendor_url" = xno; then
> 129: AC_MSG_WARN([--without-vendor-url is the same as not passing 
> --with-vendor-url to begin with])

Same as with vendor.

make/autoconf/jdk-version.m4 line 145:

> 143: AC_MSG_ERROR([--with-vendor-bug-url must have a value])
> 144:   elif test "x$with_vendor_bug_url" = xno; then
> 145: AC_MSG_WARN([--without-vendor-bug-url is the same as not passing 
> --with-vendor-bug-url to begin with])

Same as with vendor.

make/autoconf/jdk-version.m4 line 161:

> 159: AC_MSG_ERROR([--with-vendor-vm-bug-url must have a value])
> 160:   elif test "x$with_vendor_vm_bug_url" = xno; then
> 161: AC_MSG_WARN([--without-vendor-vm-bug-url is the same as not passing 
> --with-vendor-vm-bug-url to begin with])

Same as with vendor.

make/autoconf/jdk-version.m4 line 180:

> 178: AC_MSG_ERROR([--with-version-string must have a value])
> 179:   elif test "x$with_version_string" = xno; then
> 180: AC_MSG_WARN([--without-version-string is the same as not passing 
> --with-version-string to begin with])

Same as with vendor.

-

PR: https://git.openjdk.java.net/jdk/pull/7713

Re:

[erik . joelsson]

Hello Julian,

You are misunderstanding Magnus here. What he means is that we are 
compiling the version numbers into Java classes and native libraries 
(through generating Java source files with the version information in 
them and by setting -D flags when compiling C/C++). Because of this, if 
we let the version string dynamically update at make time, we would 
always have to recompile those files on incremental builds, which 
triggers quite a bit of downstream rebuilding (relinking jibjvm, 
recreating jmods and images etc). This makes the incremental build slow 
and unwieldy and would be impractical for most developers.


What Magnus is suggesting is that in the final JDK image, we would have 
a text file with the version information in it, and every native lib or 
Java class in the JDK that currently has this compiled into it, would 
instead know how to reference it from this text file.


It's not clear to me if this is a feasible proposal.

/Erik

On 2022-03-04 07:46, Julian Waters wrote:

Hi Magnus,

While looking through the now resolved JDK-8274980
 one discussed
possibility in particular caught my eye:
"The one thing remaining now to make me fully happy wrt version numbers, is
if we could stop hard-coding them into files during build, and instead have
interested parties read it from a text file."

By hardcoded files, are you referring to the files generated when configure
is run? In my opinion a separate text file wouldn't be needed if a flag is
set somewhere, and we could instead modify the generated spec.gmk to run
other support scripts to determine VERSION_BUILD if it hasn't been set yet
(ie shifting automated version detection to when make is called by changing
spec.gmk.in to an absolute value if --with-version-opt was passed, and
compile time check otherwise). perhaps by escaping bash variables into make
while it's running. What do you think?

Thanks for your time and I'm really sorry if I'm troubling you on this,
have a great day! :)

best regards,
Julian

Re: RFR: 8252769: Warn in configure if git config autocrlf has invalid value

[Erik Joelsson]

On Fri, 4 Mar 2022 12:00:15 GMT, Magnus Ihse Bursie  wrote:

> Over and over again, people are running into issues when their git config has 
> core.autocrlf set to true or input. (This is apparently done by default by 
> some Windows git installations.)
> 
> This will not work with the OpenJDK build system.
> 
> We should check this at configure time and give suitable warning message if 
> this is detected.

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7690

Re: RFR: 8282567: Improve source-date handling in build system

[Erik Joelsson]

On Wed, 2 Mar 2022 16:52:12 GMT, Magnus Ihse Bursie  wrote:

> When doing reproducible builds, controlling the build time is imperative. To 
> make this as easy as possible, some changes are needed in the build system.
> 
>  * If source-date is set to anything other than "updated" (meaning that it 
> should be determined at build time), then the default value of 
> --with-hotspot-build-time should be the same time.
> 
> * If the industry standard SOURCE_DATE_EPOCH is set when running configure, 
> the default value for --with-source-date should be picked up from this 
> variable. (If the command line option is given, it will override the 
> environment variable however.)
> 
> * Finally the code can be made a bit clearer that we can set and export 
> SOURCE_DATE_EPOCH to SOURCE_DATE in spec.gmk, unless we're using the 
> "updated" (determined at build time) value.

I think building.md needs to be updated to reflect these changes, as you no 
longer need to manually propagate the value of SOURCE_DATE_EPOCH through 
--with-source-date as the documentation currently suggests.

-

PR: https://git.openjdk.java.net/jdk/pull/7660

Re: RFR: 8209784: Include hsdis in the JDK [v3]

[Erik Joelsson]

On Wed, 2 Mar 2022 14:30:43 GMT, Magnus Ihse Bursie  wrote:

>> This PR adds a new configure argument, `--enable-hsdis-bundling`. Setting 
>> this, together with a valid backend using `--with-hsdis`, will bundle the 
>> generated hsdis library with the JDK.
>> 
>> The PR also includes a refactoring of the hsdis handling in autoconf, 
>> breaking it out to a separate file `lib-hsdis.gmk`, and breaking the 
>> functionality up in separate functions per backend.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   install-hsdis needs to depend on jdk-image

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7578

Re: RFR: 8282567: Improve source-date handling in build system

[Erik Joelsson]

On Wed, 2 Mar 2022 16:52:12 GMT, Magnus Ihse Bursie  wrote:

> When doing reproducible builds, controlling the build time is imperative. To 
> make this as easy as possible, some changes are needed in the build system.
> 
>  * If source-date is set to anything other than "updated" (meaning that it 
> should be determined at build time), then the default value of 
> --with-hotspot-build-time should be the same time.
> 
> * If the industry standard SOURCE_DATE_EPOCH is set when running configure, 
> the default value for --with-source-date should be picked up from this 
> variable. (If the command line option is given, it will override the 
> environment variable however.)
> 
> * Finally the code can be made a bit clearer that we can set and export 
> SOURCE_DATE_EPOCH to SOURCE_DATE in spec.gmk, unless we're using the 
> "updated" (determined at build time) value.

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7660

Re: RFR: 8209784: Include hsdis in the JDK [v2]

[Erik Joelsson]

On Wed, 2 Mar 2022 13:12:52 GMT, Magnus Ihse Bursie  wrote:

>> This PR adds a new configure argument, `--enable-hsdis-bundling`. Setting 
>> this, together with a valid backend using `--with-hsdis`, will bundle the 
>> generated hsdis library with the JDK.
>> 
>> The PR also includes a refactoring of the hsdis handling in autoconf, 
>> breaking it out to a separate file `lib-hsdis.gmk`, and breaking the 
>> functionality up in separate functions per backend.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Somehow the tabs got converted to spaces

Top level install-hsdis needs to depend on jdk-image.

-

Changes requested by erikj (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7578

Re: RFR: 8209784: Include hsdis in the JDK

[Erik Joelsson]

On Tue, 22 Feb 2022 16:10:22 GMT, Magnus Ihse Bursie  wrote:

> This PR adds a new configure argument, `--enable-hsdis-bundling`. Setting 
> this, together with a valid backend using `--with-hsdis`, will bundle the 
> generated hsdis library with the JDK.
> 
> The PR also includes a refactoring of the hsdis handling in autoconf, 
> breaking it out to a separate file `lib-hsdis.gmk`, and breaking the 
> functionality up in separate functions per backend.

Marked as reviewed by erikj (Reviewer).

make/Hsdis.gmk line 186:

> 184: $(install-file)
> 185: 
> 186:   $(INSTALLED_HSDIS_IMAGE): $(BUILT_HSDIS_LIB)

This slipped past me earlier, but the install-hsdis target at the top level 
does not have any prereq. I believe it needs to depend on jdk-image for 
installation to the image to work. Otherwise it will just get deleted if the 
jdk-image target is built after.

-

PR: https://git.openjdk.java.net/jdk/pull/7578

Re: RFR: 8244593: Clean up GNM/NM after JEP 381

[Erik Joelsson]

On Tue, 22 Feb 2022 15:47:05 GMT, Magnus Ihse Bursie  wrote:

> Long overdue follow-up bug from JEP 381 (removal of Solaris support). We used 
> to make a difference between gnm (GNU nm) and nm (GNU nm on all platforms 
> except Solaris, where it were Solaris nm), but this does not make sense 
> anymore, and the code is just schizophrenic wrt nm/gnm.

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7576