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

[Severin Gehwolf]

On Mon, 13 Jun 2022 07:14:15 GMT, Magnus Ihse Bursie  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?
>
> What do you mean by "build time information"?

@magicus I think it's `-Xinternalversion` which has different output between 
Windows and Linux of the same build. But to me that's a feature not a bug. From 
the PR description:

> With default configuration shows `-Xinternalversion` output same as Windows, 
> [...]

-

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

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

[Severin Gehwolf]

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?

It's not clear **why** you don't want `SOURCE_DATE_EPOCH` set by default. Could 
you explain? It would be good to make the OpenJDK build reproducible by 
default. The way it works currently, is a first step towards that goal.

-

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

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

[Severin Gehwolf]

On Mon, 9 May 2022 18:39:28 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?

This pull request has now been integrated.

Changeset: 63a1ec6e
Author:Severin Gehwolf 
URL:   
https://git.openjdk.java.net/jdk/commit/63a1ec6e7c08fc21d5cded734637eeb80147079f
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod

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

Reviewed-by: ihse, erikj

-

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

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

[Severin Gehwolf]

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'

Thanks for the review, Magnus. I'll wait until tomorrow for integration.

-

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

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

[Severin Gehwolf]

> `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'

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8605/files
  - new: https://git.openjdk.java.net/jdk/pull/8605/files/54f90f4e..97dccef9

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

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

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

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

[Severin Gehwolf]

On Mon, 9 May 2022 18:39:28 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?

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.

-

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

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

[Severin Gehwolf]

`make test TEST="gtest:https://git.openjdk.java.net/jdk/pull/8605/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8605=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8286430
  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8605.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8605/head:pull/8605

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

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

[Severin Gehwolf]

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 .

LGTM

-

Marked as reviewed by sgehwolf (Reviewer).

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

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

[Severin Gehwolf]

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

Looks fine to me.

-

Marked as reviewed by sgehwolf (Reviewer).

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

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

[Severin Gehwolf]

On Fri, 4 Mar 2022 23:43:12 GMT, Magnus Ihse Bursie  wrote:

> As for RPM builds (by whom?)

We, Red Hat, build OpenJDK in Fedora and RHEL via `rpmbuild` :)

> suddenly being build with --enable-reproducible-build, I would not think it 
> is a matter of concern. For linux, the only real difference 
> --enable-reproducible-build does is add the --date flag to jmod (and to the 
> bootjdk jar, if it supports it), and runs a special tool on src.zip to make 
> it reproducible. The only real reason this flag exists (and not just is the 
> default behavior) is out of an abundance of caution. On Windows, it triggers 
> the `-experimental:deterministic` flag, which has the scary sounding word 
> "experimental" in it. But we have been running with this option enabled for 
> all Oracle Windows builds for years by now, I think, without seeing any 
> problems.
> 

Fair enough.

> So I'm actually contemplating removing the --enable-reproducible-build flag, 
> and just make that behavior default. There's very little reason not to have 
> this turned on always, and many good reasons for having it.

+1. It would be more explicit than by sneaking this change in by starting to 
respect `SOURCE_DATE_EPOCH` (which it didn't before - at least consistently)

-

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

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

[Severin Gehwolf]

On Wed, 2 Mar 2022 16:54:21 GMT, Magnus Ihse Bursie  wrote:

> To clarify, the end effect of these changes is that building OpenJDK will 
> basically be compliant with the method of just setting SOURCE_DATE_EPOCH. 
> (The caveat is that it must be set at configure time, not build time.) So
> 
> ```
> $ export SOURCE_DATE_EPOCH=123
> $ bash configure
> $ make
> ```

Do you have an example configure output when used like that post-patch?

> will cause the build to default to building in reproducible mode, with the 
> date given by SOURCE_DATE_EPOCH.

Hmm, when building via RPM, `SOURCE_DATE_EPOCH` is usually set. If I understand 
this correctly, prior to this, the RPM build would be non-reproducible. After 
it it would be. That may be confusing to some. If post-patch this makes the 
build reproducible, does it mention that via a WARNING/INFO or anything like 
that after this patch? I don't see it. Am I missing it? It would be good to 
have something to that effect in the configure output.

-

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

Re: [Ping2?] [8u] RFR: 8210283: Support git as an SCM alternative in the build

[Severin Gehwolf]

On Tue, 2022-02-15 at 02:53 +, Andrew Hughes wrote:
> On 14:09 Thu 10 Feb , Severin Gehwolf wrote:
> > 
> > Latest webrev:
> > https://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210283/02/webrev/
> > 
> > OK?
> > 
> > Thanks,
> > Severin
> > 
> 
> This looks fine. Please flag for approval.

Thanks for the review. Done.

Cheers,
Severin

Re: [Ping2?] [8u] RFR: 8210283: Support git as an SCM alternative in the build

[Severin Gehwolf]

Hi Andrew,

On Wed, 2022-02-09 at 03:54 +, Andrew Hughes wrote:
> On 20:33 Thu 03 Feb , Severin Gehwolf wrote:
> > On Wed, 2021-12-22 at 11:14 +0100, Severin Gehwolf wrote:
> > > On Fri, 2021-12-10 at 15:11 +0100, Severin Gehwolf wrote:
> > > > Hi,
> > > > 
> > > > Please review this adaptation of the corresponding JDK 11 patch. The
> > > > JDK 11u patch didn't apply because the build system is widely different
> > > > between these two releases.
> > > > 
> > > > The main difference is make/common/MakeBase.gmk (JDK 8) vs
> > > > make/SourceRevision.gmk (JDK 11). I've basically rewritten those parts
> > > > of the patch. All the SCM handling in JDK 8 is in MakeBase.
> > > > FindAllReposAbs isn't present in JDK 8u.
> > > > 
> > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8210283
> > > > webrev: 
> > > > https://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210283/01/webrev/
> > > > 
> > > > Testing: "make --trace source-tips" on mercurial tree as well as
> > > >  the git mirror. $IMAGE_DIR/release file contains the SHA of
> > > >  the sources it was built from with 'git:' or 'hg:' prefixes.
> > > > 
> > > > Thoughts?
> > > 
> > > Anyone? When building from the git read-only mirror the "release" file
> > > no longer includes the git sha it was built from without this fix.
> > 
> 
> Well, surely it's never contained a git SHA? :)
> It doesn't have the source IDs because there is no Mercurial repository
> information, so it's as if it was built from a source tarball or some such.
> 
> > Anyone willing to review this?
> > 
> > Thanks,
> > Severin
> > 
> 
> This looks ok to me. I would omit the line "Called from
> jdk/make/closed/bundles.gmk" because this file doesn't exist in our
> repository and we have no idea what it does or doesn't contain.

Removed that part of the comment.

> I think it's also worth noting in the summary text that this removes
> the forest handling, which wasn't part of the original change for
> obvious reasons.

Done.

> JDK-8031567 was the change that introduced make/SourceRevision.gmk,
> but that was at a time when Mercurial forests were still in use, and
> it seems that the use of `hg id` has been adopted in 8u separately
> form that bug anyway.
> 
> I think this change is simple enough as is for what we need, though
> I'll admit I've never made use of this functionality myself.

Latest webrev:
https://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210283/02/webrev/

OK?

Thanks,
Severin

Re: [Ping2?] [8u] RFR: 8210283: Support git as an SCM alternative in the build

[Severin Gehwolf]

On Wed, 2021-12-22 at 11:14 +0100, Severin Gehwolf wrote:
> On Fri, 2021-12-10 at 15:11 +0100, Severin Gehwolf wrote:
> > Hi,
> > 
> > Please review this adaptation of the corresponding JDK 11 patch. The
> > JDK 11u patch didn't apply because the build system is widely different
> > between these two releases.
> > 
> > The main difference is make/common/MakeBase.gmk (JDK 8) vs
> > make/SourceRevision.gmk (JDK 11). I've basically rewritten those parts
> > of the patch. All the SCM handling in JDK 8 is in MakeBase.
> > FindAllReposAbs isn't present in JDK 8u.
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8210283
> > webrev: https://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210283/01/webrev/
> > 
> > Testing: "make --trace source-tips" on mercurial tree as well as
> >  the git mirror. $IMAGE_DIR/release file contains the SHA of
> >  the sources it was built from with 'git:' or 'hg:' prefixes.
> > 
> > Thoughts?
> 
> Anyone? When building from the git read-only mirror the "release" file
> no longer includes the git sha it was built from without this fix.

Anyone willing to review this?

Thanks,
Severin

Re: Heads up: planned Harfbuzz update in jdk11u-dev

[Severin Gehwolf]

On Fri, 2022-01-14 at 09:19 +, Baesken, Matthias wrote:
> For one of the next jdk11 updates, an update to a more recent harfbuzz 
> version is planned.
> This has been done already in jdk/jdk some time ago, and was backported 
> recently to jdk13,
> please see the harfbuzz 2.7.2 / 2.8.0 related changes done in jdk13 :
> 
> https://github.com/openjdk/jdk13u-dev/commit/e183f2d3650b6275a59d0cdd7303ca22f2ae088a
> https://github.com/openjdk/jdk13u-dev/commit/64627b5061fdd15ed52e3858cb52bb6b3b2b020b
> https://github.com/openjdk/jdk13u-dev/commit/c42c231e1dabd37ec09362291dc8141117ce0bbd
> 
> However the new harfbuzz 2.7.2 / 2.8.0 updates need C++11 support (they are 
> built with option -std=c++11).
> So please check that
> 
> a) your toolchain supports -std=c++11 or you have the option to update your 
> toolchain to a more recent version
> 
> 
> or as a workaround
> 
> b) check the -with-harfbuzz=system option to use another precompiled harfbuzz 
> version from your system

Thanks for the heads-up. This will break the JDK 11u vanilla builds
which we still build on RHEL 6 which doesn't have a new enough system
compiler to support c++11. We'll look into a work-around for this.

Cheers,
Severin

Re: [Ping?] [8u] RFR: 8210283: Support git as an SCM alternative in the build

[Severin Gehwolf]

On Fri, 2021-12-10 at 15:11 +0100, Severin Gehwolf wrote:
> Hi,
> 
> Please review this adaptation of the corresponding JDK 11 patch. The
> JDK 11u patch didn't apply because the build system is widely different
> between these two releases.
> 
> The main difference is make/common/MakeBase.gmk (JDK 8) vs
> make/SourceRevision.gmk (JDK 11). I've basically rewritten those parts
> of the patch. All the SCM handling in JDK 8 is in MakeBase.
> FindAllReposAbs isn't present in JDK 8u.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8210283
> webrev: https://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210283/01/webrev/
> 
> Testing: "make --trace source-tips" on mercurial tree as well as
>  the git mirror. $IMAGE_DIR/release file contains the SHA of
>  the sources it was built from with 'git:' or 'hg:' prefixes.
> 
> Thoughts?

Anyone? When building from the git read-only mirror the "release" file
no longer includes the git sha it was built from without this fix.

Thanks,
Severin

Re: RFR: 8276746: Add section on reproducible builds in building.md

[Severin Gehwolf]

On Fri, 5 Nov 2021 16:30:26 GMT, Magnus Ihse Bursie  wrote:

> Reproducible builds are all the vogue. The JDK has been making great strides 
> in getting there, but still has some way to go. However, to get as close as 
> possible, some special configuration is needed. 
> 
> This has been "tribal knowledge" of a few persons in the build team. It needs 
> to be properly documented to help other users wanting to create reproducible 
> builds of the JDK.

LGTM. Thanks for documenting it!

-

Marked as reviewed by sgehwolf (Reviewer).

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

Withdrawn: 8271148: static-libs-image target --with-native-debug-symbols=external doesn't produce debug info

[Severin Gehwolf]

On Thu, 22 Jul 2021 16:43:26 GMT, Severin Gehwolf  wrote:

> Hi!
> 
> Please review this tiny patch which removes the special casing of 
> `--with-native-debug-symbols=external` for the static libs build. I don't see 
> why this is needed. If no debug symbols are wanted 
> `--with-native-debug-symbols=none` can be used to achieve the same effect. 
> Therefore, I propose to remove this hunk.
> 
> Testing: Inspecting of the log files and seeing that `-g` switch is there. 
> Run the reproducer test on the resulting static libraries.
> 
> Thoughts?

This pull request has been closed without being integrated.

-

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

Re: RFR: 8271148: static-libs-image target --with-native-debug-symbols=external doesn't produce debug info

[Severin Gehwolf]

On Thu, 22 Jul 2021 16:43:26 GMT, Severin Gehwolf  wrote:

> Hi!
> 
> Please review this tiny patch which removes the special casing of 
> `--with-native-debug-symbols=external` for the static libs build. I don't see 
> why this is needed. If no debug symbols are wanted 
> `--with-native-debug-symbols=none` can be used to achieve the same effect. 
> Therefore, I propose to remove this hunk.
> 
> Testing: Inspecting of the log files and seeing that `-g` switch is there. 
> Run the reproducer test on the resulting static libraries.
> 
> Thoughts?

The use case we'd be needing for this is to have debug info in the static 
libraries, but not in the dynamic variants. The reason for this is that in 
order for somebody to get debug symbols for a binary that includes some OpenJDK 
static libs **and** want relevant debug info for the OpenJDK libs, stripping 
needs to happen after the final binary has been linked. As such, external debug 
symbols for static libs aren't as useful. Therefore, implementing stripping for 
static libs has fairly low priority for me. For the time being I'll withdraw 
this PR.

-

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

Re: RFR: 8273494: Zero: Put libjvm.so into "zero" folder, not "server"

[Severin Gehwolf]

On Thu, 9 Sep 2021 10:17:02 GMT, Aleksey Shipilev  wrote:

> Currently, the build system defaults the libjvm.so location to "server".  
> This makes looking for `libjvm.so` awkward, see JDK-8273487 for example. We 
> need to see if moving the libjvm.so to a proper location breaks anything. 
> 
> Additional testing:
>  - [x] Linux x86_64 Zero build
>  - [x] Linux x86_64 Zero bootcycle-images
>  - [ ] Linux x86_64 Zero `tier1`

Looks fine to me.

-

Marked as reviewed by sgehwolf (Reviewer).

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

Integrated: 8272332: --with-harfbuzz=system doesn't add -lharfbuzz after JDK-8255790

[Severin Gehwolf]

On Wed, 11 Aug 2021 17:58:04 GMT, Severin Gehwolf  wrote:

> Please review this simple build fix to correct the typo done in JDK-8255790. 
> After the patch correct external library is added to the `libfontmanager.so` 
> link command when building with `--with-harfbuzz=system`.
> 
> Thoughts?

This pull request has now been integrated.

Changeset: d38b3143
Author:Severin Gehwolf 
URL:   
https://git.openjdk.java.net/jdk/commit/d38b31438dd4730ee2149c02277d60c35b9d7d81
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod

8272332: --with-harfbuzz=system doesn't add -lharfbuzz after JDK-8255790

Reviewed-by: prr

-

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

Re: RFR: 8272332: --with-harfbuzz=system doesn't add -lharfbuzz after JDK-8255790

[Severin Gehwolf]

On Wed, 11 Aug 2021 18:21:14 GMT, Phil Race  wrote:

>> Please review this simple build fix to correct the typo done in JDK-8255790. 
>> After the patch correct external library is added to the `libfontmanager.so` 
>> link command when building with `--with-harfbuzz=system`.
>> 
>> Thoughts?
>
> Marked as reviewed by prr (Reviewer).

Thanks for the review @prrace!

-

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

RFR: 8272332: --with-harfbuzz=system doesn't add -lharfbuzz after JDK-8255790

[Severin Gehwolf]

Please review this simple build fix to correct the type done in JDK-8255790. 
After the patch correct external library is added to the `libfontmanager.so` 
link command when building with `--with-harfbuzz=system`.

Thoughts?

-

Commit messages:
 - 8272332: --with-harfbuzz=system doesn't add -lharfbuzz after JDK-8255790

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

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

Re: RFR: 8271148: static-libs-image target --with-native-debug-symbols=external doesn't produce debug info

[Severin Gehwolf]

On Thu, 22 Jul 2021 17:38:28 GMT, Mikael Vidstedt  wrote:

>> Hi!
>> 
>> Please review this tiny patch which removes the special casing of 
>> `--with-native-debug-symbols=external` for the static libs build. I don't 
>> see why this is needed. If no debug symbols are wanted 
>> `--with-native-debug-symbols=none` can be used to achieve the same effect. 
>> Therefore, I propose to remove this hunk.
>> 
>> Testing: Inspecting of the log files and seeing that `-g` switch is there. 
>> Run the reproducer test on the resulting static libraries.
>> 
>> Thoughts?
>
> If I understand things correctly (and I may well be misunderstanding 
> something), with this change the debug symbols are included in release 
> versions of the static libraries when --with-native-debug-symbols=external is 
> specified. That's a significant change - people may be depending on debug 
> symbols *not* being included in the resulting release binaries.
> 
> Doesn't --with-native-debug-symbols=none turn off debug symbols completely 
> for all native code? What if one wants external debug symbols for other 
> (non-static) libraries?

@vidmik Yes, correct. It's equally incorrect to **not** produce any debuginfo 
for `--with-native-debug-symbols=external`, no? With this change we have a 
situation where the external case for static libraries would have them include 
like the 'internal' case does. What's missing is the stripping part of `*.a` 
files, which if desired can get added to this patch too (for Linux). But to me 
this special casing, is not very intuitive either. Thoughts?

> Doesn't --with-native-debug-symbols=none turn off debug symbols completely 
> for all native code? What if one wants external debug symbols for other 
> (non-static) libraries?

Yes. Are you suggesting that somebody is relying on these exact semantics? 
Configure **once** with `--with-native-debug-symbols=external` and expect 
static libs to have *no* debuginfo (neither inline nor in an external file) 
while shared bits should have them in external files? That use case would still 
be possible by using two configurations. One with 
`--with-native-debug-symbols=external` and one with 
`--with-native-debug-symbols=none` and building only the needed targets each.

-

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

RFR: 8271148: static-libs-image target --with-native-debug-symbols=external doesn't produce debug info

[Severin Gehwolf]

Hi!

Please review this tiny patch which removes the special casing of 
`--with-native-debug-symbols=external` for the static libs build. I don't see 
why this is needed. If no debug symbols are wanted 
`--with-native-debug-symbols=none` can be used to achieve the same effect. 
Therefore, I propose to remove this hunk.

Testing: Inspecting of the log files and seeing that `-g` switch is there. Run 
the reproducer test on the resulting static libraries.

Thoughts?

-

Commit messages:
 - 8271148: static-libs-image target --with-native-debug-symbols=external 
doesn't produce debug info

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

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

Re: RFR: 8270117: Broken jtreg link in "Building the JDK" page [v2]

[Severin Gehwolf]

On Mon, 12 Jul 2021 17:41:27 GMT, Magnus Ihse Bursie  wrote:

>> Paraphrased description from the bug report:
>> 
>> The "Building the JDK" page has a jtreg download link pointing to 
>> 
>>  but that gets a 404. I believe the correct link is 
>> 
>>  , which is not all that easy to find.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Use link suggested by Severin instead

LGTM.

-

Marked as reviewed by sgehwolf (Reviewer).

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

Re: RFR: 8270117: Broken jtreg link in "Building the JDK" page

[Severin Gehwolf]

On Mon, 12 Jul 2021 13:17:18 GMT, Magnus Ihse Bursie  wrote:

> Paraphrased description from the bug report:
> 
> The "Building the JDK" page has a jtreg download link pointing to 
> 
>  but that gets a 404. I believe the correct link is 
> 
>  , which is not all that easy to find.

Changes requested by sgehwolf (Reviewer).

doc/building.md line 851:

> 849: The [Adoption Group](https://wiki.openjdk.java.net/display/Adoption) 
> provides
> 850: recent builds of jtreg [here](
> 851: 
> https://ci.adoptopenjdk.net/view/work-in-progress/job/GetNode/lastSuccessfulBuild/artifact/).

The right link is: 
https://ci.adoptopenjdk.net/view/Dependencies/job/dependency_pipeline/lastSuccessfulBuild/artifact/jtreg/

-

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

Re: RFR: 8257056: Submit workflow should apt-get update to avoid package installation errors

[Severin Gehwolf]

On Wed, 25 Nov 2020 08:56:33 GMT, Aleksey Shipilev  wrote:

> For example, current jobs fail with:
> 
> Get:13 http://azure.archive.ubuntu.com/ubuntu focal/main amd64 libxtst-dev 
> amd64 2:1.2.3-1 [15.2 kB]
> E: Failed to fetch 
> http://azure.archive.ubuntu.com/ubuntu/pool/main/a/alsa-lib/libasound2-dev_1.2.2-2.1ubuntu2.1_amd64.deb
>  404 Not Found [IP: 52.147.219.192 80]
> Fetched 760 kB in 0s (3154 kB/s)
> E: Unable to fetch some archives, maybe run apt-get update or try with 
> --fix-missing?

Ship it!

-

Marked as reviewed by sgehwolf (Reviewer).

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

Re: RFR: 8252998: ModuleWrapper.gmk doesn't consult include path

[Severin Gehwolf]

On Fri, 18 Sep 2020 12:44:13 GMT, Erik Joelsson  wrote:

>> A change made to ModuleWrapper.gmk as part of JDK-8244044 means that an 
>> included makefile is found in the current
>> directory, so Make doesn't check the include dirs for overriding gmk files.
>> Recommend a minor, partial reversion of this changeset to ensure any 
>> override files are included instead.
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8252998
>
> Looks good.

@adamfarley Please see 
https://github.com/openjdk/jdk/pull/250#issuecomment-694845694 for getting this 
integrated. For
non-committers it's `/integrate` and then `/sponsor` by somebody with commit 
rights.

-

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

Re: [8u] RFR: 8252975: [8u] JDK-8252395 breaks the build for --with-native-debug-symbols=internal

[Severin Gehwolf]

Hi Andrew,

On Fri, 2020-09-18 at 06:40 +0100, Andrew Hughes wrote:
> On 20:16 Wed 09 Sep , Severin Gehwolf wrote:
> > Hi,
> > 
> > Please review this 8u (jdk8u/jdk8u-dev tree) fix for JDK-8252395 that
> > I've pushed today. Thanks for Zhengyu Gu for noticing it. The pushed
> > fix added the java.debuginfo and unpack.debuginfo make targets on the
> > condition of ENABLE_DEBUG_SYMBOLS=true, which is insufficient. It needs
> > another check on POST_STRIP_CMD which is set to non-empty for --with-
> > native-debug-symbols={external,zipped}, and is indeed empty for --with-
> > native-debug-symbols=internal. For the --with-native-debug-symbols=none 
> > case we have ENABLE_DEBUG_SYMBOLS=false.
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8252975
> > webrev: https://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8252975/01/webrev/
> > 
> > Testing: Builds with 
> > --with-native-debug-symbols={none,internal,external,zipped} on Linux x86_64
> > 
> > OK?
> > 
> > Thanks,
> > Severin
> > 
> 
> Build is still broken for me with this patch:
> 
> /usr/bin/cp /home/ahughes/builder/8u-dev/jdk/objs/java_objs/java.diz 
> /home/ahughes/builder/8u-dev/jdk/bin/java.diz
> /usr/bin/cp: cannot stat 
> '/home/ahughes/builder/8u-dev/jdk/objs/java_objs/java.diz': No such file or 
> directory
> 
> where --with-native-debug-symbols is not set (pre-JDK-8207324)

Hmm, I'm not sure how you are building. I've checked builds with:

$ bash configure \
 --with-boot-jdk="/some/boot/jdk" \
 --with-extra-cflags=-Wno-error \
 --disable-zip-debug-info
$ make images

and

$ bash configure \
 --with-boot-jdk="/some/boot/jdk" \
 --with-extra-cflags=-Wno-error
$ make images

and

$ bash configure \
 --with-boot-jdk="/some/boot/jdk" \
 --with-extra-cflags=-Wno-error \
 --disable-debug-symbols
$ make images

All seem to work fine for me. Are you sure you are not
setting POST_STRIP_CMD or the like explicitly somewhere else. Please
post the exact configure/make invocations you are using.

FWIW, that bug you've referenced seems wrong:
8207324: aarch64 jtreg: assert in TestOptionsWithRanges.jtr

You probably meant JDK-8036003:
8036003: Add --with-native-debug-symbols=[none|internal|external|zipped]

> The use of these two conditionals seems odd to me. What we want to know
> is whether zipped debuginfo is in use.

No. We want to know whether or not external debug symbols (zipped or
otherwise) are in use.

> This is not the same as debug symbols
> in general being enabled.

Yes. Correctly so.

> Also, DEBUGINFO_EXT is only being set when
> ZIP_DEBUGINFO_FILES is true!

How so?

ifeq ($(ZIP_DEBUGINFO_FILES), true)
  DEBUGINFO_EXT := .diz
else ifeq ($(OPENJDK_TARGET_OS), macosx)
  DEBUGINFO_EXT := .dSYM
else ifeq ($(OPENJDK_TARGET_OS), windows)
  DEBUGINFO_EXT := .pdb
else
  DEBUGINFO_EXT := .debuginfo
endif

On Linux with --with-native-debug-symbols=external this ends up with:

DEBUGINFO_EXT := .debuginfo

> POST_STRIP_CMD seems to only be used in image creation, so I don't
> think checking this is appropriate either.

Seems debatable.

> Instead, we should mirror what is done in make/common/NativeCompilation.gmk 
> when
> creating the .diz files:
> 
> -ifeq ($(ENABLE_DEBUG_SYMBOLS), true)
> -  ifneq ($(POST_STRIP_CMD), )
> +ifeq ($(ZIP_DEBUGINFO_FILES), true)
> +  ifneq ($(STRIP_POLICY), no_strip)
> 
> The second check is necessary because ZIP_DEBUGINFO_FILES is set by default,
> and so may be true even if STRIP_POLICY has been set to no_strip.

Your patch won't work for --with-native-debug-symbols=external. In that
case no *.debuginfo files for the java and unpack200 launchers would be
created.

With your patch:
$ find build/linux-x86_64-normal-server-release/images/j2sdk-image/bin/ | grep 
-E 'java\.debuginfo|unpack200\.debuginfo'


Expected:
$ find build/linux-x86_64-normal-server-release/images/j2sdk-image/bin/ | grep 
-E 'java\.debuginfo|unpack200\.debuginfo'
build/linux-x86_64-normal-server-release/images/j2sdk-image/bin/java.debuginfo
build/linux-x86_64-normal-server-release/images/j2sdk-image/bin/unpack200.debuginfo

> The attached patch fixed the build for me.

Sorry, I don't seem to be able to reproduce your build failure.

Thanks,
Severin

[8u] RFR: 8252975: [8u] JDK-8252395 breaks the build for --with-native-debug-symbols=internal

[Severin Gehwolf]

Hi,

Please review this 8u (jdk8u/jdk8u-dev tree) fix for JDK-8252395 that
I've pushed today. Thanks for Zhengyu Gu for noticing it. The pushed
fix added the java.debuginfo and unpack.debuginfo make targets on the
condition of ENABLE_DEBUG_SYMBOLS=true, which is insufficient. It needs
another check on POST_STRIP_CMD which is set to non-empty for --with-
native-debug-symbols={external,zipped}, and is indeed empty for --with-
native-debug-symbols=internal. For the --with-native-debug-symbols=none 
case we have ENABLE_DEBUG_SYMBOLS=false.

Bug: https://bugs.openjdk.java.net/browse/JDK-8252975
webrev: https://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8252975/01/webrev/

Testing: Builds with 
--with-native-debug-symbols={none,internal,external,zipped} on Linux x86_64

OK?

Thanks,
Severin

Re: [8u] RFR: 8252395: [8u] --with-native-debug-symbols=external doesn't include debuginfo files for binaries

[Severin Gehwolf]

Hi,

On Tue, 2020-09-08 at 17:05 +0100, Andrew Hughes wrote:
> On 13:44 Mon 31 Aug , Severin Gehwolf wrote:
> > Sorry, wrong webrev. Now corrected.
> > 
> > On Mon, 2020-08-31 at 10:02 +0200, Severin Gehwolf wrote:
> > > Hi,
> > > 
> > > Could I get a reivew of this 8u specific bug please? When configured
> > > --with-native-debug-symbols=external,zipped the resulting external
> > > debuginfo files for binaries (in images/bin folder) aren't there.
> > > 
> > > In OpenJDK 8u there is some special casing involved for bin/java and
> > > bin/unpack200. Thus, I had to special case them for the debuginfo
> > > variant files too otherwise those files would be missing in the images
> > > folders (j2sdk-image, j2re-image).
> > > 
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8252395
> > 
> > webrev: https://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8252395/02/webrev/
> > 
> > > Testing: Build in configs 
> > > --with-native-debugsymbols={zipped,external,internal,none}
> > >  on Linux x86 and manually debugging some launcher code. Works now
> > >  with symbols. Symbols were missing before this patch.
> > > 
> > 
> > Thanks,
> > Severin
> > 
> 
> Patch looks ok

Thanks for the review!

> Is any follow-on change for Windows necessary?

I don't know as I don't have a way to test/develop it. It shouldn't
make anything worse on the Windows side, though.

Thanks,
Severin

Re: jdk14 build failed cause "The header is deprecated and will be removed."

[Severin Gehwolf]

On Wed, 2020-08-05 at 15:46 +0800, 451541695 wrote:
> Hello, Thanks for providing java for developers.
>  I am trying to build jdk-14 to use, but failed to build.
>  The error is following:
> 
>  My environment:
>Linux:Ubuntu 20.04.1 LTS
>autoconfig:
> 
> I don't know what happened to  too high?
> Long for your replying.

Could this be:
https://bugs.openjdk.java.net/browse/JDK-8235833

Looks like it's fixed in 14.0.2. Are you building from the updates
tree?

http://hg.openjdk.java.net/jdk-updates/jdk14u/

Thanks,
Severin

Re: RFR: JDK-8251193 bin/idea.sh generating wrong source folders for JVMCI modules

[Severin Gehwolf]

Hi,

On Thu, 2020-09-03 at 16:48 +0100, Maurizio Cimadamore wrote:
> Sorry for the delay,
> overall this looks like a good improvement, so I'll approve.
> 
> Is there some colleague that can help you do the push?
> 
> Let me know if you need help.

I'll sponsor the patch for Galder. Yet, sponsoring with git works
differently now.

Maurizio: I guess approving this PR would be sufficient?
https://github.com/openjdk/jdk/pull/63

Thanks,
Severin



> Thanks
> Maurizio
> 
> On 06/08/2020 15:28, Galder Zamarreno wrote:
> > Hi,
> > 
> > `bin/idea.sh` is not generating the right source folders for
> > jdk.internal.vm.ci and jdk.internal.vm.compiler modules.
> > 
> > These modules have different directory structures to the rest and so should
> > be handled exceptionally so that the IDE has the right source folder
> > definitions.
> > 
> > The fix takes these two modules, inspects all the subdirectories within
> > each module root and generates a source folder definition for each. As
> > example, for jdk.internal.vm.ci, there should be a source folder for:
> > 
> > * src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.aarch64/src
> > * src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.amd64/src
> > * ...
> > 
> > I've tested this in the patch on IDEA 2020.2.
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8251193
> > WebRev:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/galder/JDK-8251193/01/webrev/
> > 
> > Galder

Re: [8u] RFR: 8252395: [8u] --with-native-debug-symbols=external doesn't include debuginfo files for binaries

[Severin Gehwolf]

Sorry, wrong webrev. Now corrected.

On Mon, 2020-08-31 at 10:02 +0200, Severin Gehwolf wrote:
> Hi,
> 
> Could I get a reivew of this 8u specific bug please? When configured
> --with-native-debug-symbols=external,zipped the resulting external
> debuginfo files for binaries (in images/bin folder) aren't there.
> 
> In OpenJDK 8u there is some special casing involved for bin/java and
> bin/unpack200. Thus, I had to special case them for the debuginfo
> variant files too otherwise those files would be missing in the images
> folders (j2sdk-image, j2re-image).
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8252395

webrev: https://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8252395/02/webrev/

> 
> Testing: Build in configs 
> --with-native-debugsymbols={zipped,external,internal,none}
>  on Linux x86 and manually debugging some launcher code. Works now
>  with symbols. Symbols were missing before this patch.
> 

Thanks,
Severin

[8u] RFR: 8252395: [8u] --with-native-debug-symbols=external doesn't include debuginfo files for binaries

[Severin Gehwolf]

Hi,

Could I get a reivew of this 8u specific bug please? When configured
--with-native-debug-symbols=external,zipped the resulting external
debuginfo files for binaries (in images/bin folder) aren't there.

In OpenJDK 8u there is some special casing involved for bin/java and
bin/unpack200. Thus, I had to special case them for the debuginfo
variant files too otherwise those files would be missing in the images
folders (j2sdk-image, j2re-image).

Bug: https://bugs.openjdk.java.net/browse/JDK-8252395
webrev: https://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8252395/01/webrev/

Testing: Build in configs 
--with-native-debugsymbols={zipped,external,internal,none}
 on Linux x86 and manually debugging some launcher code. Works now
 with symbols. Symbols were missing before this patch.

Thanks,
Severin

Re: RFR: 8252233: Enable debug-image target to support producing a pure debug image package

[Severin Gehwolf]

Hi Andrew,

On Thu, 2020-08-27 at 16:55 +0100, Andrew Leonard wrote:
> Hi,
> Please may I request a sponsor and review for this build enhancement to 
> provide a pure debug "image", for those developers that want to accompany 
> a straight jdk image with a debug-image when needed:
> https://bugs.openjdk.java.net/browse/JDK-8252233
> webrev: http://cr.openjdk.java.net/~aleonard/8252233/webrev.00/

Not really a review...

If I understand this correctly it's an image of only *debugsymbols* for
native files, right? debug-image as a name seems a bit confusing to me.
There is already a concept of 'release', 'fastdebug' and 'slowdebug'
builds for HotSpot. Perhaps calling it debugsymbols-image would make
more sense?

Thanks,
Severin

Re: [ping] [11u] RFR 8234535: Cross compilation fails due to missing CFLAGS for the BUILD_CC

[Severin Gehwolf]

On Wed, 2020-08-26 at 11:29 +0200, Christoph Göttschkes wrote:
> > Webrev: https://cr.openjdk.java.net/~cgo/8234535/webrev-11u.00/

This looks fine to me.

Thanks,
Severin

Re: RFR: JDK-8251193 bin/idea.sh generating wrong source folders for JVMCI modules

[Severin Gehwolf]

Hi,

On Thu, 2020-08-06 at 16:01 +0100, Maurizio Cimadamore wrote:
> The basic fix looks good, I'm wondering, however, if we would only want 
> to enable sources which match the current configuration? E.g. not add 
> the sources for aarch64 if building on linux? But if we build all these 
> modules regardless, then it's ok to add them all.

Looks like on an x86_64 Linux build aarch64 classes get compiled into
the module(s) as well:

$ find build/linux-x86_64-server-release/jdk/modules/jdk.internal.vm.ci | grep 
aarch64
build/linux-x86_64-server-release/jdk/modules/jdk.internal.vm.ci/jdk/vm/ci/aarch64
build/linux-x86_64-server-release/jdk/modules/jdk.internal.vm.ci/jdk/vm/ci/aarch64/AArch64$Flag.class
build/linux-x86_64-server-release/jdk/modules/jdk.internal.vm.ci/jdk/vm/ci/aarch64/AArch64Kind$1.class
build/linux-x86_64-server-release/jdk/modules/jdk.internal.vm.ci/jdk/vm/ci/aarch64/AArch64.class
build/linux-x86_64-server-release/jdk/modules/jdk.internal.vm.ci/jdk/vm/ci/aarch64/AArch64$CPUFeature.class
build/linux-x86_64-server-release/jdk/modules/jdk.internal.vm.ci/jdk/vm/ci/aarch64/AArch64$1.class
build/linux-x86_64-server-release/jdk/modules/jdk.internal.vm.ci/jdk/vm/ci/aarch64/AArch64Kind.class
build/linux-x86_64-server-release/jdk/modules/jdk.internal.vm.ci/jdk/vm/ci/hotspot/aarch64
build/linux-x86_64-server-release/jdk/modules/jdk.internal.vm.ci/jdk/vm/ci/hotspot/aarch64/AArch64HotSpotRegisterConfig.class
build/linux-x86_64-server-release/jdk/modules/jdk.internal.vm.ci/jdk/vm/ci/hotspot/aarch64/AArch64HotSpotRegisterConfig$1.class
build/linux-x86_64-server-release/jdk/modules/jdk.internal.vm.ci/jdk/vm/ci/hotspot/aarch64/AArch64HotSpotVMConfig.class
build/linux-x86_64-server-release/jdk/modules/jdk.internal.vm.ci/jdk/vm/ci/hotspot/aarch64/AArch64HotSpotJVMCIBackendFactory.class

Thanks,
Severin

> Maurizio
> 
> On 06/08/2020 15:28, Galder Zamarreno wrote:
> > Hi,
> > 
> > `bin/idea.sh` is not generating the right source folders for
> > jdk.internal.vm.ci and jdk.internal.vm.compiler modules.
> > 
> > These modules have different directory structures to the rest and so should
> > be handled exceptionally so that the IDE has the right source folder
> > definitions.
> > 
> > The fix takes these two modules, inspects all the subdirectories within
> > each module root and generates a source folder definition for each. As
> > example, for jdk.internal.vm.ci, there should be a source folder for:
> > 
> > * src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.aarch64/src
> > * src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.amd64/src
> > * ...
> > 
> > I've tested this in the patch on IDEA 2020.2.
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8251193
> > WebRev:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/galder/JDK-8251193/01/webrev/
> > 
> > Galder

[11u] RFR(xs): 8247874: Replacement in VersionProps.java.template not working when --with-vendor-bug-url contains '&'

[Severin Gehwolf]

Hi,

Could I get a review of this OpenJDK 11 specific patch? This same issue
has been solved in OpenJDK 13 and better with JDK-8223319[1] which
seems an unrelated issue to the fix of this bug. Also, I have tried
applying the JDK 13 patch and it doesn't apply well and would mean some
form of rewrite. I'm not sure this is worth it in this case.

For this reason, I propose to only backport the small part of the
(larger) JDK-8223319 fix which fixes the --with-*vendor-bug-url
functionality when those URLs contain '&'. Since this will be pushed
(if accepted) under a separate bug there shouldn't be any confusion.

Bug: https://bugs.openjdk.java.net/browse/JDK-8247874
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8247874/01/webrev/

Testing: Manual inspection of generated VersionProps.java and manual
testing of the crash URL.

Thoughts?

Thanks,
Severin

[1] Add copyright footer to specs and man pages
https://bugs.openjdk.java.net/browse/JDK-8223319

Re: [8u] RFR(XS): 8233880: Support compilers with multi-digit major version numbers

[Severin Gehwolf]

Hi Florian,

On Mon, May 11, 2020 at 9:58 AM Florian Weimer  wrote:
>
> * Severin Gehwolf:
>
> > Thanks for the review! Yes, generated-configure.sh changes are due to
> > version skew of autoconf being used. I'll try to generate configure on
> > an older machine so as to avoid this before pushing. Does that sound
> > ok?
>
> THe runstatedir changes come and go (even in the jdk8u-dev history),
> depending on whether Debian's autoconf is used, which patches its
> autoconf 2.69 build to handle runstatedir.  There hasn't been an
> autoconf upstream release in many, many years, so going back to older
> versions will not solve this.  If you want to avoid this change, you
> either have to use Debian's autoconf, or back it out manually.

OK. Thanks for the heads-up.

Cheers,
Severin

Re: [8u] RFR(XS): 8233880: Support compilers with multi-digit major version numbers

[Severin Gehwolf]

Hi Andrew,

On Sun, May 10, 2020 at 4:36 PM Andrew Haley  wrote:
>
> On 5/8/20 2:17 PM, Severin Gehwolf wrote:
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8233880
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8233880/01/webrev/
> >
> > Testing: Build prior this patch on F32 with -fcommon (crashes on java
> >  -version). Passes after.
> >
> > Thoughts?
>
> That looks fine; I presume the apparently spurious diffs are because
> you used a different version of autoconf to generate generated-configure.sh.

Thanks for the review! Yes, generated-configure.sh changes are due to
version skew
of autoconf being used. I'll try to generate configure on an older
machine so as to
avoid this before pushing. Does that sound ok?

Cheers,
Severin

[8u] RFR(XS): 8233880: Support compilers with multi-digit major version numbers

[Severin Gehwolf]

Hi,

Please review this OpenJDK 8u backport of JDK-8233880. It's a
one-liner change which updates the toolchain.m4 code so as to
recognize multi-digit GCC versions. For example Fedora 32 comes
with GCC 10 and falls afoul this check. As a result, a configure
warning is being produced and crucial flags won't get added for
compilation. This results in a broken build on such systems.

Details are in:
https://bugs.openjdk.java.net/browse/JDK-8244538

The original JDK 14 patch doesn't apply cleanly post path-
unshuffeling, because even though JDK-8151841[1] is in 8u,
JDK-8034788[2] later regressed the
COMPILER_VERSION_NUMBER sed expression to its prior
JDK-8151841 form. After this patch, this will be corrected again
and will include JDK-8233880 changes.

Bug: https://bugs.openjdk.java.net/browse/JDK-8233880
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8233880/01/webrev/

Testing: Build prior this patch on F32 with -fcommon (crashes on java
 -version). Passes after.

Thoughts?

Thanks,
Severin

[1] https://bugs.openjdk.java.net/browse/JDK-8151841
[2] https://bugs.openjdk.java.net/browse/JDK-8034788

[8u] RFR(XS): 8243059: Build fails when --with-vendor-name contains a comma

[Severin Gehwolf]

Hi,

Could I please get a review of this OpenJDK 8u backport for JDK-
8243059? The build system is wildly different to JDK 11 and later, thus
is the patch. In turns out on JDK 8, SetupLauncher isn't using eval()
so the evaluation of the comma too early isn't an issue there. However,
on JDK 8u, the same issue is present when COMPANY_NAME is being
evaluated too early in SetupArchive. COMPANY_NAME may contain a comma
an that breaks the embedded rule for creating the jars.

Bug: https://bugs.openjdk.java.net/browse/JDK-8243059
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8243059/jdk8/01/webrev/

Testing: building with --with-vendor-name="foo, bar". Fails before,
passes after patch.

Thoughts?

Thanks,
Severin

Re: RFR: 8243656: Shell built-in test in configure depends on help

[Severin Gehwolf]

On Wed, 2020-05-06 at 10:16 +0200, Magnus Ihse Bursie wrote:
> On 2020-05-05 17:15, Severin Gehwolf wrote:
> > Hi,
> > 
> > Could I please get a review of this trivial change? Apparently using
> > the help builtin for determining whether or not a builtin is available
> > is not a good idea. A more portable way to do this is to use "command
> > -v" or "type". Thanks to Michael Zucchi for contributing this fix.
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8243656
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8243656/02/webrev/
> Hi Severin,
> 
> I missed commenting on this before you pushed it, but does this really 
> work?! Did you try it on your system?

I believe so.

> I just tested the following:
> $ command -V ps
> ps is /bin/ps
> $ command -V fg
> fg is a shell builtin
> $ command -v ps
> /bin/ps
> $ echo  $?
> 0
> $ command -v fg
> fg
> $ echo  $?
> 0
> 
> That is, it does not seem like "command -v" is making any difference in 
> return value between builtins and external commands!

Sure, but is that a problem? The configure check is there as a fallback
if the tool is not found via AC_PATH_PROGS it tries this fallback (i.e.
it must be a built-in if "command -v " works, no?).

$ command -v help
help
$ echo $?
0
$ command -v foo
$ echo $?
1

> Also, even if it should do, this is not documented and I don't think it 
> should be relied on -- as evident, it does not work on my system.

I'm confused, what's not documented? Everything you said seems to be
documented.

Also from 'man bash' for 'command' I see:

"""
If the -V or -v option is  supplied, the  exit  status is 0 if command
was found, and 1 if not.
"""

> In contrast, the (builtin) command "type" seems to work fine, and is 
> documented to work:
> 
> $ type -t ps
> file
> $ type -t fg
> builtin
> 
> I recommend that use $(type -t) = builtin instead.

I agree type would work too. If you insist, I can change the fallback
from 'command -v' to 'type -t'.

Thoughts?

Thanks,
Severin

Re: RFR: 8243656: Shell built-in test in configure depends on help

[Severin Gehwolf]

On Tue, 2020-05-05 at 08:24 -0700, Erik Joelsson wrote:
> Looks good to me.

Thanks for the review, Erik!

Cheers,
Severin

> /Erik
> 
> On 2020-05-05 08:15, Severin Gehwolf wrote:
> > Hi,
> > 
> > Could I please get a review of this trivial change? Apparently using
> > the help builtin for determining whether or not a builtin is available
> > is not a good idea. A more portable way to do this is to use "command
> > -v" or "type". Thanks to Michael Zucchi for contributing this fix.
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8243656
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8243656/02/webrev/
> > 
> > Testing: Local builds. jdk/submit.
> > 
> > Thoughts?
> > 
> > Thanks,
> > Severin
> > 

RFR: 8243656: Shell built-in test in configure depends on help

[Severin Gehwolf]

Hi,

Could I please get a review of this trivial change? Apparently using
the help builtin for determining whether or not a builtin is available
is not a good idea. A more portable way to do this is to use "command
-v" or "type". Thanks to Michael Zucchi for contributing this fix.

Bug: https://bugs.openjdk.java.net/browse/JDK-8243656
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8243656/02/webrev/

Testing: Local builds. jdk/submit.

Thoughts?

Thanks,
Severin

Re: openjdk fails to configure due to shell builtin test

[Severin Gehwolf]

On Tue, 2020-04-28 at 14:58 +1000, David Holmes wrote:
> On 28/04/2020 12:02 pm, Michael Zucchi wrote:
> > Hi Severin,
> > 
> > On 27/4/20 6:17 pm, Severin Gehwolf wrote:
> > > Hi Michael,
> > > 
> > > On Sat, 2020-04-25 at 09:39 +0930, Michael Zucchi wrote:
> > > > Morning all,
> > > > 
> > > > A patch from last year [1] discussed on this list adds an autoconf
> > > > fallback test for a shell builtin command using the bash command 'help
> > > > ' and invokes it for ulimit.  It's probably not very portable
> > > > to start with but bash can be compiled specifically without the help
> > > > command by passing --disable-help-builtin to configure.  guix uses this
> > > > option for it's build environment shell [4], hence openjdk fails to
> > > > configure there without a patch such as the one below.
> > > > 
> > > > For openjdk 14[3], is it possible to use a more portable sequence in the
> > > > BASIC_REQUIRE_BUILTIN_PROGS macro?   The internet [2] suggests using
> > > > "command -v " or "type  " for this purpose.
> > > > 
> > > > Although I have not tried, the same appears to apply to openjdk-15
> > > > although the macro has been moved to UTIL_REQUIRE_BUILTIN_PROGS in
> > > > make/autoconf/util.m4
> > > Thanks for the report. I've filed this bug to track it:
> > > https://bugs.openjdk.java.net/browse/JDK-8243656
> > > 
> > > I can sponsor the patch for you. Do you have the OCA signed?
> > 
> > Ok cheers.  No on the OCA.  I don't have direct access to a printer or 
> > scanner so I will have to run around to sort it out but I have plenty of 
> > spare time so will chase it up (assuming covid hasn't closed everything).
> 
> Simple changes like this do not require the OCA.

Thanks for the clarification, David!

> A Participant is an individual who has subscribed to one or more OpenJDK 
> mailing lists. A Participant may post messages to a list, submit simple 
> patches, and make other kinds of small contributions.
> 
> A Contributor is a Participant who has signed the Oracle Contributor 
> Agreement (OCA), ...
> 
> http://openjdk.java.net/bylaws#participant

OK then, I'll proceed with getting this integrated.

Cheers,
Severin

> Cheers,
> David
> 
> > I can't see how this particular change includes any copyrightable 
> > creative input but I suppose that's up to the project to decide.
> > 
> > Regards,
> >   Michael
> > 
> > 

Re: openjdk fails to configure due to shell builtin test

[Severin Gehwolf]

Hi Michael,

On Sat, 2020-04-25 at 09:39 +0930, Michael Zucchi wrote:
> Morning all,
> 
> A patch from last year [1] discussed on this list adds an autoconf 
> fallback test for a shell builtin command using the bash command 'help 
> ' and invokes it for ulimit.  It's probably not very portable 
> to start with but bash can be compiled specifically without the help 
> command by passing --disable-help-builtin to configure.  guix uses this 
> option for it's build environment shell [4], hence openjdk fails to 
> configure there without a patch such as the one below.
> 
> For openjdk 14[3], is it possible to use a more portable sequence in the 
> BASIC_REQUIRE_BUILTIN_PROGS macro?   The internet [2] suggests using 
> "command -v " or "type  " for this purpose.
> 
> Although I have not tried, the same appears to apply to openjdk-15 
> although the macro has been moved to UTIL_REQUIRE_BUILTIN_PROGS in 
> make/autoconf/util.m4

Thanks for the report. I've filed this bug to track it:
https://bugs.openjdk.java.net/browse/JDK-8243656

I can sponsor the patch for you. Do you have the OCA signed?

Thanks,
Severin

> Thanks,
>   Z
> 
> [1] 
> https://mail.openjdk.java.net/pipermail/build-dev/2019-November/026307.html
> [2] 
> https://stackoverflow.com/questions/592620/how-can-i-check-if-a-program-exists-from-a-bash-script
> [3] http://hg.openjdk.java.net/jdk/jdk14/rev/bc54620a3848
> [4] 
> https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/bash.scm#n202
> 
> $ help command
> command: command [-pVv] command [arg ...]
>  Execute a simple command or display information about commands.
> 
>  Runs COMMAND with ARGS suppressing  shell function lookup, or display
>  information about the specified COMMANDs.  Can be used to invoke 
> commands
>  on disk when a function with the same name exists.
> 
>  Options:
>-puse a default value for PATH that is guaranteed to find all of
>  the standard utilities
>-vprint a description of COMMAND similar to the `type' builtin
>-Vprint a more verbose description of each COMMAND
> 
>  Exit Status:
>  Returns exit status of COMMAND, or failure if COMMAND is not found.
> 
> 
> --- jdk14-bc54620a3848/make/autoconf/basics.m4  2020-02-07 
> 04:40:54.0 +1030
> +++ jdk14-bc54620a3848-new/make/autoconf/basics.m4  2020-04-24 
> 10:59:33.056098506 +0930
> @@ -583,7 +583,7 @@
> BASIC_SETUP_TOOL($1, [AC_PATH_PROGS($1, $2, , $3)])
> if test "x[$]$1" = x; then
>   AC_MSG_NOTICE([Required tool $2 not found in PATH, checking built-in])
> -if help $2 > /dev/null 2>&1; then
> +if command -v $2 > /dev/null 2>&1; then
> AC_MSG_NOTICE([Found $2 as shell built-in. Using it])
> $1="$2"
>   else
> 

[11u] RFR: 8243059: Build fails when --with-vendor-name contains a comma

[Severin Gehwolf]

Hi,

Could I please get a review of this 11u backport of a build fix? The
jdk/jdk patch does not apply cleanly since JDK-8222510[1] isn't in
OpenJDK 11u. I.e. this makes the context different and the patch
failing to apply. I've manually ported the patch. The gist is:

$(VERSION_CFLAGS) => $$(VERSION_CFLAGS)

Bug: https://bugs.openjdk.java.net/browse/JDK-8243059
webrev: 
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8243059/jdk11/01/webrev/
original change: https://hg.openjdk.java.net/jdk/jdk/rev/dff3aabdc2f3

Testing: Build fails before, passes after patch. java.vendor property
is as expected when it includes commas.

Thoughts?

Thanks,
Severin

[1] https://bugs.openjdk.java.net/browse/JDK-8222510

Re: RFR: 8243059: Build fails when --with-vendor-name contains a comma

[Severin Gehwolf]

On Fri, 2020-04-17 at 07:32 -0700, Erik Joelsson wrote:
> This version looks good to me.

Thanks for the review, Erik!

Cheers,
Severin

> /Erik
> 
> On 2020-04-17 05:15, Severin Gehwolf wrote:
> > Hi Magnus,
> > 
> > On Fri, 2020-04-17 at 13:44 +0200, Magnus Ihse Bursie wrote:
> > > On 2020-04-17 12:18, Severin Gehwolf wrote:
> > > > Hi,
> > > > 
> > > > Could I please get a review of this build fix? When --with-vendor-name
> > > > contains a comma, like 'foo, bar, Inc.' the build fails. As it turns
> > > > out SetupBuildLauncherBody calls SetupJdkExecutable with some
> > > > parameters. If $(VERSION_CFLAGS) contain a comma, the comma is being
> > > > treated as a prameter delimiter and the build fails.
> > > > 
> > > > I propose to substitute ',' with $(DOLLAR)COMMA in VERSION_CFLAGS so as
> > > > to avoid inadvertent expansion of the comma and, thus, it being treated
> > > > as a parameter to the SetupJdkExecutable macro.
> > > > 
> > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8243059
> > > > webrev: 
> > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8243059/02/webrev
> > > > 
> > > > NOTE: Magnus pointed out that there were two pre-existing bugs too.
> > > > LAUNCHER_NAME and LAUNCHER_CFLAGS should have $$ in front of them, not
> > > > just a single one. Fixed this too.
> > > Hi Severin,
> > > 
> > > I was a bit unclear in my comment in the bug. :-)
> > > 
> > > What I meant was that you *only* need to use $$(VERSION_CFLAGS). The
> > > subst thing should not be necessary. The problem you are seeing is due
> > > to VERSION_CFLAGS being evaluated too early without the $$ in front.
> > Aah, I see. Makes sense. Updated now:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8243059/03/webrev/
> > 
> > Thanks,
> > Severin
> > 

Re: RFR: 8243059: Build fails when --with-vendor-name contains a comma

[Severin Gehwolf]

On Fri, 2020-04-17 at 14:28 +0200, Magnus Ihse Bursie wrote:
> On 2020-04-17 14:15, Severin Gehwolf wrote:
> > Hi Magnus,
> > 
> > On Fri, 2020-04-17 at 13:44 +0200, Magnus Ihse Bursie wrote:
> > > On 2020-04-17 12:18, Severin Gehwolf wrote:
> > > > Hi,
> > > > 
> > > > Could I please get a review of this build fix? When --with-vendor-name
> > > > contains a comma, like 'foo, bar, Inc.' the build fails. As it turns
> > > > out SetupBuildLauncherBody calls SetupJdkExecutable with some
> > > > parameters. If $(VERSION_CFLAGS) contain a comma, the comma is being
> > > > treated as a prameter delimiter and the build fails.
> > > > 
> > > > I propose to substitute ',' with $(DOLLAR)COMMA in VERSION_CFLAGS so as
> > > > to avoid inadvertent expansion of the comma and, thus, it being treated
> > > > as a parameter to the SetupJdkExecutable macro.
> > > > 
> > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8243059
> > > > webrev: 
> > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8243059/02/webrev
> > > > 
> > > > NOTE: Magnus pointed out that there were two pre-existing bugs too.
> > > > LAUNCHER_NAME and LAUNCHER_CFLAGS should have $$ in front of them, not
> > > > just a single one. Fixed this too.
> > > Hi Severin,
> > > 
> > > I was a bit unclear in my comment in the bug. :-)
> > > 
> > > What I meant was that you *only* need to use $$(VERSION_CFLAGS). The
> > > subst thing should not be necessary. The problem you are seeing is due
> > > to VERSION_CFLAGS being evaluated too early without the $$ in front.
> > Aah, I see. Makes sense. Updated now:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8243059/03/webrev/
> 
> Looks good to me.

Thanks for the review!

> (But please verify that it really solves your problem, 
> so there's not a second layer of macro parsing that messes things up in 
> this case.)

I did. It seems to work. Thanks for the reminder :)

Cheers,
Severin

Re: RFR: 8243059: Build fails when --with-vendor-name contains a comma

[Severin Gehwolf]

Hi Magnus,

On Fri, 2020-04-17 at 13:44 +0200, Magnus Ihse Bursie wrote:
> On 2020-04-17 12:18, Severin Gehwolf wrote:
> > Hi,
> > 
> > Could I please get a review of this build fix? When --with-vendor-name
> > contains a comma, like 'foo, bar, Inc.' the build fails. As it turns
> > out SetupBuildLauncherBody calls SetupJdkExecutable with some
> > parameters. If $(VERSION_CFLAGS) contain a comma, the comma is being
> > treated as a prameter delimiter and the build fails.
> > 
> > I propose to substitute ',' with $(DOLLAR)COMMA in VERSION_CFLAGS so as
> > to avoid inadvertent expansion of the comma and, thus, it being treated
> > as a parameter to the SetupJdkExecutable macro.
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8243059
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8243059/02/webrev
> > 
> > NOTE: Magnus pointed out that there were two pre-existing bugs too.
> > LAUNCHER_NAME and LAUNCHER_CFLAGS should have $$ in front of them, not
> > just a single one. Fixed this too.
> Hi Severin,
> 
> I was a bit unclear in my comment in the bug. :-)
> 
> What I meant was that you *only* need to use $$(VERSION_CFLAGS). The 
> subst thing should not be necessary. The problem you are seeing is due 
> to VERSION_CFLAGS being evaluated too early without the $$ in front.

Aah, I see. Makes sense. Updated now:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8243059/03/webrev/

Thanks,
Severin

RFR: 8243059: Build fails when --with-vendor-name contains a comma

[Severin Gehwolf]

Hi,

Could I please get a review of this build fix? When --with-vendor-name
contains a comma, like 'foo, bar, Inc.' the build fails. As it turns
out SetupBuildLauncherBody calls SetupJdkExecutable with some
parameters. If $(VERSION_CFLAGS) contain a comma, the comma is being
treated as a prameter delimiter and the build fails.

I propose to substitute ',' with $(DOLLAR)COMMA in VERSION_CFLAGS so as
to avoid inadvertent expansion of the comma and, thus, it being treated
as a parameter to the SetupJdkExecutable macro.

Bug: https://bugs.openjdk.java.net/browse/JDK-8243059
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8243059/02/webrev

NOTE: Magnus pointed out that there were two pre-existing bugs too.
LAUNCHER_NAME and LAUNCHER_CFLAGS should have $$ in front of them, not
just a single one. Fixed this too.

Testing: Setting --with-vendor-name='foo, bar' and build passes post-
patch. Also manually inspecting VENDOR is properly passed on the
command line when java/main.o is being compiled. I'll run this through
jdk-submit before I push.

Thoughts?

Thanks,
Severin

Re: [RFR] [11u] JDK-8232748: "Build static versions of certain JDK libraries"

[Severin Gehwolf]

Hi Andrew,

On Thu, 2020-02-27 at 15:43 +0100, Severin Gehwolf wrote:
> On Thu, 2020-02-27 at 04:52 +, Andrew Hughes wrote:
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8232748
> > Webrev: https://cr.openjdk.java.net/~andrew/openjdk11/8232748/webrev.01/
> > 
> > This patch adds targets to the build so that static versions of the JDK
> > native libraries can be built, using static-libs-image. Such static
> > versions of the libraries are required for consumption by Graal.
> > 
> > With JDK-8210459 now in 11u, this is a largely clean backport, aside
> > from some context changes, due to additional targets (JCov, JMH) being
> > present in trunk:
> > 
> > * In make/Bundles.gmk, 11u does not have jcov-bundles
> > * In make/Main.gmk, 11u does not have jcov-image or jcov-bundles
> > * In make/autoconf/spec.gmk.in, 11u does not have JMH_CORE_JAR, etc or
> > JCOV_BUNDLE_NAME.
> > * In make/conf/jib-profiles.js, the dependencies list in 11u doesn't
> > include jmh and jcov.

Right, those changes look fine to me.

Reviewed. Please add jdk11u-fix-request label to the bug.

> > Building the new target, static-libs-image, succeeded. This should have
> > no effect on other targets.
> 
> Unfortunately this patch doesn't work. While a build of static-libs-
> image succeeds, it doesn't create the image in build/ name>/images. Expected is this:
> 
> $ find build/linux-x86_64-normal-server-release/images/static-libs/
> build/linux-x86_64-normal-server-release/images/static-libs/
> build/linux-x86_64-normal-server-release/images/static-libs/lib
> build/linux-x86_64-normal-server-release/images/static-libs/lib/libj2pkcs11.a
> build/linux-x86_64-normal-server-release/images/static-libs/lib/libj2pcsc.a
> build/linux-x86_64-normal-server-release/images/static-libs/lib/libnio.a
> build/linux-x86_64-normal-server-release/images/static-libs/lib/libprefs.a
> build/linux-x86_64-normal-server-release/images/static-libs/lib/libjava.a
> build/linux-x86_64-normal-server-release/images/static-libs/lib/libjli.a
> build/linux-x86_64-normal-server-release/images/static-libs/lib/libnet.a
> build/linux-x86_64-normal-server-release/images/static-libs/lib/libjimage.a
> build/linux-x86_64-normal-server-release/images/static-libs/lib/libjaas.a
> build/linux-x86_64-normal-server-release/images/static-libs/lib/libfdlibm.a
> build/linux-x86_64-normal-server-release/images/static-libs/lib/libj2gss.a
> build/linux-x86_64-normal-server-release/images/static-libs/lib/libsunec.a
> build/linux-x86_64-normal-server-release/images/static-libs/lib/libjsig.a
> build/linux-x86_64-normal-server-release/images/static-libs/lib/libextnet.a
> build/linux-x86_64-normal-server-release/images/static-libs/lib/libverify.a
> build/linux-x86_64-normal-server-release/images/static-libs/lib/libzip.a
> 
> The reason for this is that FindFiles isn't available in JDK 11u.

Since Christoph backported JDK-8189861 to jdk11u-dev, the patch now
works as expected with FindFiles.

Thanks,
Severin

Re: [11u] RFR: 8189861: Refactor CacheFind

[Severin Gehwolf]

On Wed, 2020-03-11 at 12:47 +, Langer, Christoph wrote:
> OK, I guess you're right. Here is the new webrev: 
> http://cr.openjdk.java.net/~clanger/webrevs/8189861.11u.1/

Looks good.

Thanks,
Severin

Re: [11u] RFR: 8189861: Refactor CacheFind

[Severin Gehwolf]

Hi,

On Wed, 2020-03-11 at 09:58 +, Langer, Christoph wrote:
> Hi Severin,
> 
> thanks for the review.
> 
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8189861
> > > Original Change: http://hg.openjdk.java.net/jdk/jdk/rev/e297c7bb6469
> > > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8189861.11u/
> > 
> > I've compared it to the original change and it looks good except for
> > the hunk in make/hotspot/lib/CompileJvm.gmk. Andrew's backport then
> > also works for getting the static-libs image built.
> 
> So you think we should have the hunk in
> make/hotspot/lib/CompileJvm.gmk or leave it out?

Leave it out, please. If anything it should be part of a future 
JDK-8220383 backport.

> You should probably also make up your mind on whether it's 11.0.7 or
> 11.0.8. It applies to both repos and I test it with both. I guess, in
> the end it depends on how urgent the backport of "JDK-8232748 Build
> static versions of certain JDK libraries" is.

11.0.8 is fine.

Thanks,
Severin

Re: [11u] RFR: 8189861: Refactor CacheFind

[Severin Gehwolf]

Hi Christoph,

Thanks for doing this!

On Wed, 2020-03-11 at 08:58 +, Langer, Christoph wrote:
> Hi,
> 
> for the backports of "JDK-8223678 Add Visual Studio Code workspace
> generation support (for native code)" and "JDK-8232748 Build static
> versions of certain JDK libraries" it seems useful to first backport
> the refactoring of CacheFind. Furthermore it can improve performance
> in the Windows Build and will probably also be a nice base for future
> backports in the build system area.
> 
> I had to resolve several rejects because there is some code
> divergence between jdk11u-dev and the state of jdk13 at the time of
> the original change. But in fact, it was quite straightforward and I
> replaced all occurences of CacheFind with the new FindFiles function.
> Lots of the rejects were also just copyright years.
> 
> In make/hotspot/lib/CompileJvm.gmk, I effectively added the "$(call
> FillFindCache, $(JVM_SRC_DIRS))" statement which was only modified in
> the original change.

I believe we should omit this hunk in the 11u backport, because that
logic was added by JDK-8220383[1] which is not in OpenJDK 11u.

> Please let me know what you think.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8189861
> Original Change: http://hg.openjdk.java.net/jdk/jdk/rev/e297c7bb6469
> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8189861.11u/

I've compared it to the original change and it looks good except for
the hunk in make/hotspot/lib/CompileJvm.gmk. Andrew's backport then
also works for getting the static-libs image built.

Thanks,
Severin

> Thanks & Best regards
> Christoph

[1] https://bugs.openjdk.java.net/browse/JDK-8220383

Re: 11u RFR: 8232569: Use test image from different jib profile for testing

[Severin Gehwolf]

On Mon, 2020-03-09 at 15:14 +, Langer, Christoph wrote:
> Is this file only used in internal Oracle builds or does it have use
> cases outside? (I guess this question unmasks my ignorance in that
> area )

I'd be curious about this too. There is little point in us backporting 
an Oracle-only build change if so.

Thanks,
Severin

Re: [RFR] [11u] JDK-8232748: "Build static versions of certain JDK libraries"

[Severin Gehwolf]

Hi Andrew,

On Thu, 2020-02-27 at 04:52 +, Andrew Hughes wrote:
> Bug: https://bugs.openjdk.java.net/browse/JDK-8232748
> Webrev: https://cr.openjdk.java.net/~andrew/openjdk11/8232748/webrev.01/
> 
> This patch adds targets to the build so that static versions of the JDK
> native libraries can be built, using static-libs-image. Such static
> versions of the libraries are required for consumption by Graal.
> 
> With JDK-8210459 now in 11u, this is a largely clean backport, aside
> from some context changes, due to additional targets (JCov, JMH) being
> present in trunk:
> 
> * In make/Bundles.gmk, 11u does not have jcov-bundles
> * In make/Main.gmk, 11u does not have jcov-image or jcov-bundles
> * In make/autoconf/spec.gmk.in, 11u does not have JMH_CORE_JAR, etc or
> JCOV_BUNDLE_NAME.
> * In make/conf/jib-profiles.js, the dependencies list in 11u doesn't
> include jmh and jcov.
> 
> Building the new target, static-libs-image, succeeded. This should have
> no effect on other targets.

Unfortunately this patch doesn't work. While a build of static-libs-
image succeeds, it doesn't create the image in build//images. Expected is this:

$ find build/linux-x86_64-normal-server-release/images/static-libs/
build/linux-x86_64-normal-server-release/images/static-libs/
build/linux-x86_64-normal-server-release/images/static-libs/lib
build/linux-x86_64-normal-server-release/images/static-libs/lib/libj2pkcs11.a
build/linux-x86_64-normal-server-release/images/static-libs/lib/libj2pcsc.a
build/linux-x86_64-normal-server-release/images/static-libs/lib/libnio.a
build/linux-x86_64-normal-server-release/images/static-libs/lib/libprefs.a
build/linux-x86_64-normal-server-release/images/static-libs/lib/libjava.a
build/linux-x86_64-normal-server-release/images/static-libs/lib/libjli.a
build/linux-x86_64-normal-server-release/images/static-libs/lib/libnet.a
build/linux-x86_64-normal-server-release/images/static-libs/lib/libjimage.a
build/linux-x86_64-normal-server-release/images/static-libs/lib/libjaas.a
build/linux-x86_64-normal-server-release/images/static-libs/lib/libfdlibm.a
build/linux-x86_64-normal-server-release/images/static-libs/lib/libj2gss.a
build/linux-x86_64-normal-server-release/images/static-libs/lib/libsunec.a
build/linux-x86_64-normal-server-release/images/static-libs/lib/libjsig.a
build/linux-x86_64-normal-server-release/images/static-libs/lib/libextnet.a
build/linux-x86_64-normal-server-release/images/static-libs/lib/libverify.a
build/linux-x86_64-normal-server-release/images/static-libs/lib/libzip.a

The reason for this is that FindFiles isn't available in JDK 11u. I had
to do these modifications to your patch to make it work:

diff --git a/make/Bundles.gmk b/make/Bundles.gmk
--- a/make/Bundles.gmk
+++ b/make/Bundles.gmk
@@ -367,7 +367,7 @@
 

 
 ifneq ($(filter static-libs-bundles, $(MAKECMDGOALS)), )
-  STATIC_LIBS_BUNDLE_FILES := $(call FindFiles, $(STATIC_LIBS_IMAGE_DIR))
+  STATIC_LIBS_BUNDLE_FILES := $(shell $(FIND) $(STATIC_LIBS_IMAGE_DIR))
 
   ifeq ($(OPENJDK_TARGET_OS)-$(DEBUG_LEVEL), macosx-release)
 STATIC_LIBS_BUNDLE_SUBDIR := $(JDK_MACOSX_CONTENTS_SUBDIR)/Home
diff --git a/make/StaticLibsImage.gmk b/make/StaticLibsImage.gmk
--- a/make/StaticLibsImage.gmk
+++ b/make/StaticLibsImage.gmk
@@ -42,7 +42,7 @@
   SRC := $(SUPPORT_OUTPUTDIR)/native/$m, \
   DEST := $(STATIC_LIBS_IMAGE_DIR)/lib, \
   FILES := $(filter %$(STATIC_LIBRARY_SUFFIX), \
-  $(call FindFiles, $(SUPPORT_OUTPUTDIR)/native/$m/*/static)), \
+  $(shell $(FIND) $(SUPPORT_OUTPUTDIR)/native/$m/*/static)), \
   )) \
   $(eval TARGETS += $$(COPY_STATIC_LIBS_$m)) \
 )

This uses FIND directly, but there ought to be a (convoluted?) way to
use CacheFind instead of FindFiles in 11u. Maybe build-dev folks could
help there?

Thanks,
Severin

Re: [8u] RFR: 8227397: Add --with-extra-asflags configure option

[Severin Gehwolf]

Hi,

Could I get a second review from an JDK 8u Reviewer, please?

Thanks,
Severin

On Mon, 2019-09-30 at 11:36 +0200, Magnus Ihse Bursie wrote:
> On 2019-09-27 17:48, Severin Gehwolf wrote:
> > Hi,
> > 
> > Could I please get a review of this 8u build change backport which
> > adds
> > --with-extra-asflags to OpenJDK 8u. At Red Hat, we need to pass
> > certain
> > assembler only flags for some builds. For example "-Wa,--generate-
> > missing-build-notes=yes", to assembly files only. As the build
> > system
> > is different in 8u over 11u I've re-done the patch.
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8227397
> > webrev: 
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227397/jdk8/01/
>  Looks good to me.
> 
> /Magnus
> > Testing: Built with --with-extra-asflags=-Wa,--generate-missing-
> > build-
> > notes=yes on Linux x86_64, confirmed linux_x86_64.s gets assembled
> > with
> > the flag and only that file.
> > 
> > I've omitted the windows portion of passing as flags to the hotspot
> > build as I've no idea how. Contributions welcome if that should get
> > included.
> > 
> > Thoughts?
> > 
> > Thanks,
> > Severin
> > 
>  

Re: RFR: 8236921: Add build target to produce a JDK image suitable for a Graal/SVM build

[Severin Gehwolf]

On Fri, 2020-01-10 at 13:19 -0800, Erik Joelsson wrote:
> Hello Severin,
> 
> Looks good.

Thanks for the review, Erik!

> /Erik
> 
> On 2020-01-10 09:28, Bob Vandette wrote:
> > I like it!
> > 
> > I have to manually copy the libs each time.
> > 
> > BTW:  I have a PR in our internal queue to update the graal repo so
> > it can build with the latest JDK 14.

I'd love to see that patch pushed to graal master! :)

Thanks,
Severin

> > Bob.
> > 
> > 
> > > On Jan 10, 2020, at 10:45 AM, Severin Gehwolf <
> > > sgehw...@redhat.com> wrote:
> > > 
> > > Hi,
> > > 
> > > Currently there is no easy way to produce an OpenJDK build which
> > > could
> > > subsequently be used to build Substrate VM (part of Graal CE).
> > > Basic
> > > building blocks are there, but no actual JDK image is getting
> > > produced
> > > with the static libs along side their dynamic counterparts. This
> > > patch
> > > addresses this by adding a new target called 'graal-builder-
> > > image'
> > > which produces a jdk image in $IMAGES/graal-builder-jdk. It
> > > basically
> > > copies over libs from static-libs folder and the jdk folder to
> > > assemble
> > > the desired result.
> > > 
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8236921
> > > webrev: 
> > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8236921/02/webrev/
> > > 
> > > I've tested this works for some snapshot of JDK 14 (rev
> > > 3009b27660be)
> > > and graal at revision d9fb0b7dc35a9a7eb559a5e468bcc4c99e918449.
> > > 
> > > Thoughts?
> > > 
> > > Thanks,
> > > Severin
> > > 

RFR: 8236921: Add build target to produce a JDK image suitable for a Graal/SVM build

[Severin Gehwolf]

Hi,

Currently there is no easy way to produce an OpenJDK build which could
subsequently be used to build Substrate VM (part of Graal CE). Basic
building blocks are there, but no actual JDK image is getting produced
with the static libs along side their dynamic counterparts. This patch
addresses this by adding a new target called 'graal-builder-image'
which produces a jdk image in $IMAGES/graal-builder-jdk. It basically
copies over libs from static-libs folder and the jdk folder to assemble
the desired result.

Bug: https://bugs.openjdk.java.net/browse/JDK-8236921
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8236921/02/webrev/

I've tested this works for some snapshot of JDK 14 (rev 3009b27660be)
and graal at revision d9fb0b7dc35a9a7eb559a5e468bcc4c99e918449.

Thoughts?

Thanks,
Severin

Re: RFR: 8233712: Limit default tests jobs based on ulimit -u setting

[Severin Gehwolf]

On Fri, 2019-11-15 at 08:27 -0800, Erik Joelsson wrote:
> On 2019-11-15 04:23, Severin Gehwolf wrote:
> > Hi Erik, Magnus,
> > 
> > On Fri, 2019-11-08 at 08:57 -0800, Erik Joelsson wrote:
> > > Hello Severin,
> > > 
> > > On 2019-11-08 07:48, Severin Gehwolf wrote:
> > > > Right. I believe webrev 03 does this?
> > > Yes, that was mostly a reply to Magnus.
> > > > Thanks! Tried it, which doesn't seem to work[1]:
> > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8233712/04/webrev/
> > > Looking at your job, it's failing already in configure on our Linux
> > > hosts. At least on Oracle Linux, it seems ulimit is a bash builtin and
> > > not found in path. Configure needs to fallback on that.
> > Thanks for the info. This updated webrev passes jdk/submit now:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8233712/05/webrev/
> > 
> > Thoughts?
> 
> That is an interesting approach. I think it's good to go. Magnus may 
> want to tweak it, but as he is on vacation, it may take a while to get 
> feedback from him.

I see.

> Perhaps we could just put the builtin logic in the default 
> BASIC_REQUIRE_PROG, but we could also wait and do that later. No need to 
> stall this fix more I think.

Thanks, Erik. I'll get this pushed on Monday if I hear no objections by
then.

Cheers,
Severin

> /Erik
> 
> > Thanks,
> > Severin
> > 
> > > > Is there some way to reproduce locally?
> > > > 
> > > > Thanks,
> > > > Severin
> > > > 
> > > > [1] [Mach5] mach5-one-sgehwolf-JDK-8233712-4-20191108-1454-6533841:
> > > > FAILED
> > > > 
> > > > > /Erik
> > > > > 
> > > > > > Thanks,
> > > > > > Severin
> > > > > > 

Re: RFR: 8233712: Limit default tests jobs based on ulimit -u setting

[Severin Gehwolf]

Hi Erik, Magnus,

On Fri, 2019-11-08 at 08:57 -0800, Erik Joelsson wrote:
> Hello Severin,
> 
> On 2019-11-08 07:48, Severin Gehwolf wrote:
> > Right. I believe webrev 03 does this?
> Yes, that was mostly a reply to Magnus.
> > Thanks! Tried it, which doesn't seem to work[1]:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8233712/04/webrev/
> 
> Looking at your job, it's failing already in configure on our Linux 
> hosts. At least on Oracle Linux, it seems ulimit is a bash builtin and 
> not found in path. Configure needs to fallback on that.

Thanks for the info. This updated webrev passes jdk/submit now:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8233712/05/webrev/

Thoughts?

Thanks,
Severin

> > Is there some way to reproduce locally?
> > 
> > Thanks,
> > Severin
> > 
> > [1] [Mach5] mach5-one-sgehwolf-JDK-8233712-4-20191108-1454-6533841: 
> > FAILED
> > 
> > > /Erik
> > > 
> > > > Thanks,
> > > > Severin
> > > > 

Re: RFR: 8233712: Limit default tests jobs based on ulimit -u setting

[Severin Gehwolf]

On Fri, 2019-11-08 at 06:06 -0800, Erik Joelsson wrote:
> Hello Severin,
> 
> On 2019-11-08 05:08, Severin Gehwolf wrote:
> > On Fri, 2019-11-08 at 11:26 +0100, Severin Gehwolf wrote:
> > > On Fri, 2019-11-08 at 09:58 +0100, Magnus Ihse Bursie wrote:
> > > > On 2019-11-07 20:48, Severin Gehwolf wrote:
> > > > > Hi Erik,
> > > > > 
> > > > > On Thu, 2019-11-07 at 10:01 -0800, Erik Joelsson wrote:
> > > > > > Hello Severin,
> > > > > > 
> > > > > > Taking ulimit -u into account does seem like a good idea. I don't 
> > > > > > see
> > > > > > why it needs to be limited to just aarch64 though. It should 
> > > > > > however be
> > > > > > limited to OSes that use ulimit (i.e. not windows).
> > > > > > 
> > > > > > I would suggest removing the arbitrary thresholds of 16 and 4096 
> > > > > > and try
> > > > > > to come up with a plain number based on the ulimit value. You are 
> > > > > > saying
> > > > > > 14 is good for 4096, so the formula for the ULIMIT_DIVIDER is 4096 
> > > > > > / 14,
> > > > > > which you can just write as:
> > > > > > 
> > > > > > ULIMIT_DIVIER := (4096 / 14)
> > > > > > 
> > > > > > And let the awk script calculate it if needed. In the awk script, 
> > > > > > you
> > > > > > need to put line 275 as conditional of the if statement on line 274.
> > > > > > Otherwise ULIMIT=-1 will cause concurrency -1.
> > > > > Thanks for the review! Updated webrev:
> > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8233712/02/webrev/
> > > > I know I'm coming here and making a simple patch more complicated, but
> > > > I'm not entirely comfortable with the explicit call to ulimit. Our
> > > > principle is that all executables that we call from the makefiles should
> > > > be confirmed existing by configure, so the call should be "$(shell
> > > > $(ULIMIT) -u)".
> > > > 
> > > > It should be relatively trivial to add this as a required prog in
> > > > basics.m4 and spec.gmk.in. Make sure you only add it as required for
> > > > non-Windows platforms. If you do this, you can change the check in the
> > > > makefile to if $(ULIMIT) has a value, rather than checking on platform.
> Unfortunately ulimit exists in Cygwin but I it's unclear what it does. 
> On my 32 threads Windows machine, it currently has value 256 for -u, 
> which would severely limit testing for no good reason. I think we need 
> to keep explicitly not doing this on Windows.

Right. I believe webrev 03 does this?

> > > Here you go:
> > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8233712/03/webrev/
> > Hmm, this failed jdk/submit. Could anybody provide me with some
> > details, please?
> > 
> > [Mach5] mach5-one-sgehwolf-JDK-8233712-3-20191108-1036-6526715: FAILED
> 
> The problem is that when we run tests in our distributed system, we 
> don't run configure on the test node, but instead use the 
> "run-test-prebuilt" make target. For any tool configure needs to 
> discover, that is also used in RunTest.gmk, there needs to be a backup 
> discovery method in RunTestPrebuiltSpec.gmk. It's currently just a list 
> of hardcoded values. So add "ULIMIT := ulimit" there and it should work.

Thanks! Tried it, which doesn't seem to work[1]:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8233712/04/webrev/

Is there some way to reproduce locally?

Thanks,
Severin

[1] [Mach5] mach5-one-sgehwolf-JDK-8233712-4-20191108-1454-6533841: FAILED

> /Erik
> 
> > Thanks,
> > Severin
> > 

Re: RFR: 8233712: Limit default tests jobs based on ulimit -u setting

[Severin Gehwolf]

On Fri, 2019-11-08 at 11:26 +0100, Severin Gehwolf wrote:
> On Fri, 2019-11-08 at 09:58 +0100, Magnus Ihse Bursie wrote:
> > On 2019-11-07 20:48, Severin Gehwolf wrote:
> > > Hi Erik,
> > > 
> > > On Thu, 2019-11-07 at 10:01 -0800, Erik Joelsson wrote:
> > > > Hello Severin,
> > > > 
> > > > Taking ulimit -u into account does seem like a good idea. I don't see
> > > > why it needs to be limited to just aarch64 though. It should however be
> > > > limited to OSes that use ulimit (i.e. not windows).
> > > > 
> > > > I would suggest removing the arbitrary thresholds of 16 and 4096 and try
> > > > to come up with a plain number based on the ulimit value. You are saying
> > > > 14 is good for 4096, so the formula for the ULIMIT_DIVIDER is 4096 / 14,
> > > > which you can just write as:
> > > > 
> > > > ULIMIT_DIVIER := (4096 / 14)
> > > > 
> > > > And let the awk script calculate it if needed. In the awk script, you
> > > > need to put line 275 as conditional of the if statement on line 274.
> > > > Otherwise ULIMIT=-1 will cause concurrency -1.
> > > Thanks for the review! Updated webrev:
> > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8233712/02/webrev/
> > I know I'm coming here and making a simple patch more complicated, but 
> > I'm not entirely comfortable with the explicit call to ulimit. Our 
> > principle is that all executables that we call from the makefiles should 
> > be confirmed existing by configure, so the call should be "$(shell 
> > $(ULIMIT) -u)".
> > 
> > It should be relatively trivial to add this as a required prog in 
> > basics.m4 and spec.gmk.in. Make sure you only add it as required for 
> > non-Windows platforms. If you do this, you can change the check in the 
> > makefile to if $(ULIMIT) has a value, rather than checking on platform.
> 
> Here you go:
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8233712/03/webrev/

Hmm, this failed jdk/submit. Could anybody provide me with some
details, please?

[Mach5] mach5-one-sgehwolf-JDK-8233712-3-20191108-1036-6526715: FAILED

Thanks,
Severin

Re: RFR: 8233712: Limit default tests jobs based on ulimit -u setting

[Severin Gehwolf]

On Fri, 2019-11-08 at 09:58 +0100, Magnus Ihse Bursie wrote:
> On 2019-11-07 20:48, Severin Gehwolf wrote:
> > Hi Erik,
> > 
> > On Thu, 2019-11-07 at 10:01 -0800, Erik Joelsson wrote:
> > > Hello Severin,
> > > 
> > > Taking ulimit -u into account does seem like a good idea. I don't see
> > > why it needs to be limited to just aarch64 though. It should however be
> > > limited to OSes that use ulimit (i.e. not windows).
> > > 
> > > I would suggest removing the arbitrary thresholds of 16 and 4096 and try
> > > to come up with a plain number based on the ulimit value. You are saying
> > > 14 is good for 4096, so the formula for the ULIMIT_DIVIDER is 4096 / 14,
> > > which you can just write as:
> > > 
> > > ULIMIT_DIVIER := (4096 / 14)
> > > 
> > > And let the awk script calculate it if needed. In the awk script, you
> > > need to put line 275 as conditional of the if statement on line 274.
> > > Otherwise ULIMIT=-1 will cause concurrency -1.
> > Thanks for the review! Updated webrev:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8233712/02/webrev/
> I know I'm coming here and making a simple patch more complicated, but 
> I'm not entirely comfortable with the explicit call to ulimit. Our 
> principle is that all executables that we call from the makefiles should 
> be confirmed existing by configure, so the call should be "$(shell 
> $(ULIMIT) -u)".
> 
> It should be relatively trivial to add this as a required prog in 
> basics.m4 and spec.gmk.in. Make sure you only add it as required for 
> non-Windows platforms. If you do this, you can change the check in the 
> makefile to if $(ULIMIT) has a value, rather than checking on platform.

Here you go:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8233712/03/webrev/

Thanks,
Severin

> /Magnus
> > What do you think?
> > 
> > Thanks,
> > Severin
> > 
> > > /Erik
> > > 
> > > On 2019-11-07 06:59, Severin Gehwolf wrote:
> > > > Hi,
> > > > 
> > > > Could I please get a review of this change for running tests on big
> > > > Aarch64 boxes? Currently, only memory and number of cores are taken
> > > > into account for the -concurrency setting of jtreg. After this patch
> > > > ulimit -u settings are taken into account as well on big Aarch64 boxes
> > > > with > 16 cores, yet low ulimit -u setting (<= 4096). This is to avoid
> > > > running into the max allowed threads limit causing random test
> > > > failures.
> > > > 
> > > > Thoughts?
> > > > 
> > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8233712
> > > > webrev: 
> > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8233712/01/webrev/
> > > > 
> > > > Testing: Ran tier1 tests on a large Aarch64 box and low ulimit -u
> > > > value. Tests run stable. Did the same for a user with high ulimit -u
> > > > value, which resulted in concurrency setting as before this patch.
> > > > 
> > > > Thanks,
> > > > Severin
> > > > 

Re: RFR: 8233712: Limit default tests jobs based on ulimit -u setting

[Severin Gehwolf]

On Fri, 2019-11-08 at 09:58 +0100, Magnus Ihse Bursie wrote:
> On 2019-11-07 20:48, Severin Gehwolf wrote:
> > Hi Erik,
> > 
> > On Thu, 2019-11-07 at 10:01 -0800, Erik Joelsson wrote:
> > > Hello Severin,
> > > 
> > > Taking ulimit -u into account does seem like a good idea. I don't see
> > > why it needs to be limited to just aarch64 though. It should however be
> > > limited to OSes that use ulimit (i.e. not windows).
> > > 
> > > I would suggest removing the arbitrary thresholds of 16 and 4096 and try
> > > to come up with a plain number based on the ulimit value. You are saying
> > > 14 is good for 4096, so the formula for the ULIMIT_DIVIDER is 4096 / 14,
> > > which you can just write as:
> > > 
> > > ULIMIT_DIVIER := (4096 / 14)
> > > 
> > > And let the awk script calculate it if needed. In the awk script, you
> > > need to put line 275 as conditional of the if statement on line 274.
> > > Otherwise ULIMIT=-1 will cause concurrency -1.
> > Thanks for the review! Updated webrev:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8233712/02/webrev/
> I know I'm coming here and making a simple patch more complicated, but 
> I'm not entirely comfortable with the explicit call to ulimit. Our 
> principle is that all executables that we call from the makefiles should 
> be confirmed existing by configure, so the call should be "$(shell 
> $(ULIMIT) -u)".
> 
> It should be relatively trivial to add this as a required prog in 
> basics.m4 and spec.gmk.in. Make sure you only add it as required for 
> non-Windows platforms. If you do this, you can change the check in the 
> makefile to if $(ULIMIT) has a value, rather than checking on platform.

Sure, will do. Thanks Magnus!

Cheers,
Severin

> /Magnus
> > What do you think?
> > 
> > Thanks,
> > Severin
> > 
> > > /Erik
> > > 
> > > On 2019-11-07 06:59, Severin Gehwolf wrote:
> > > > Hi,
> > > > 
> > > > Could I please get a review of this change for running tests on big
> > > > Aarch64 boxes? Currently, only memory and number of cores are taken
> > > > into account for the -concurrency setting of jtreg. After this patch
> > > > ulimit -u settings are taken into account as well on big Aarch64 boxes
> > > > with > 16 cores, yet low ulimit -u setting (<= 4096). This is to avoid
> > > > running into the max allowed threads limit causing random test
> > > > failures.
> > > > 
> > > > Thoughts?
> > > > 
> > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8233712
> > > > webrev: 
> > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8233712/01/webrev/
> > > > 
> > > > Testing: Ran tier1 tests on a large Aarch64 box and low ulimit -u
> > > > value. Tests run stable. Did the same for a user with high ulimit -u
> > > > value, which resulted in concurrency setting as before this patch.
> > > > 
> > > > Thanks,
> > > > Severin
> > > > 

Re: RFR: 8233712: Limit default tests jobs based on ulimit -u setting

[Severin Gehwolf]

Hi Erik,

On Thu, 2019-11-07 at 10:01 -0800, Erik Joelsson wrote:
> Hello Severin,
> 
> Taking ulimit -u into account does seem like a good idea. I don't see 
> why it needs to be limited to just aarch64 though. It should however be 
> limited to OSes that use ulimit (i.e. not windows).
> 
> I would suggest removing the arbitrary thresholds of 16 and 4096 and try 
> to come up with a plain number based on the ulimit value. You are saying 
> 14 is good for 4096, so the formula for the ULIMIT_DIVIDER is 4096 / 14, 
> which you can just write as:
> 
> ULIMIT_DIVIER := (4096 / 14)
> 
> And let the awk script calculate it if needed. In the awk script, you 
> need to put line 275 as conditional of the if statement on line 274. 
> Otherwise ULIMIT=-1 will cause concurrency -1.

Thanks for the review! Updated webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8233712/02/webrev/

What do you think?

Thanks,
Severin

> /Erik
> 
> On 2019-11-07 06:59, Severin Gehwolf wrote:
> > Hi,
> > 
> > Could I please get a review of this change for running tests on big
> > Aarch64 boxes? Currently, only memory and number of cores are taken
> > into account for the -concurrency setting of jtreg. After this patch
> > ulimit -u settings are taken into account as well on big Aarch64 boxes
> > with > 16 cores, yet low ulimit -u setting (<= 4096). This is to avoid
> > running into the max allowed threads limit causing random test
> > failures.
> > 
> > Thoughts?
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8233712
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8233712/01/webrev/
> > 
> > Testing: Ran tier1 tests on a large Aarch64 box and low ulimit -u
> > value. Tests run stable. Did the same for a user with high ulimit -u
> > value, which resulted in concurrency setting as before this patch.
> > 
> > Thanks,
> > Severin
> > 

RFR: 8233712: Limit default tests jobs based on ulimit -u setting

[Severin Gehwolf]

Hi,

Could I please get a review of this change for running tests on big
Aarch64 boxes? Currently, only memory and number of cores are taken
into account for the -concurrency setting of jtreg. After this patch
ulimit -u settings are taken into account as well on big Aarch64 boxes
with > 16 cores, yet low ulimit -u setting (<= 4096). This is to avoid
running into the max allowed threads limit causing random test
failures.

Thoughts?

Bug: https://bugs.openjdk.java.net/browse/JDK-8233712
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8233712/01/webrev/

Testing: Ran tier1 tests on a large Aarch64 box and low ulimit -u
value. Tests run stable. Did the same for a user with high ulimit -u
value, which resulted in concurrency setting as before this patch.

Thanks,
Severin

Re: [11u] RFR: 8214311: dtrace gensrc has missing dependencies

[Severin Gehwolf]

Hi Christoph,

On Wed, 2019-10-30 at 05:48 +, Langer, Christoph wrote:
> Hi Severin,
> 
> backport looks good. Thanks for doing it.

Thanks for the review!

Cheers,
Severin

> Cheers
> Christoph
> 
> > -Original Message-
> > From: build-dev  On Behalf Of
> > Severin Gehwolf
> > Sent: Dienstag, 29. Oktober 2019 19:19
> > To: jdk-updates-dev 
> > Cc: build-dev 
> > Subject: [11u] RFR: 8214311: dtrace gensrc has missing dependencies
> > 
> > Hi,
> > 
> > Please review this OpenJDK 11u backport of JDK-8214311. It's an
> > Oracle
> > JDK 11.0.6 feature parity patch. The jdk/jdk patch didn't apply
> > cleanly
> > due to context differences in make/hotspot/gensrc/GensrcDtrace.gmk.
> > That is due to JDK-8211029 not being present in OpenJDK 11u code-
> > base.
> > I have fixed the patch manually, which was quite trivial.
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8214311
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-
> > 8214311/jdk11/01/webrev/
> > Original changeset: 
> > http://hg.openjdk.java.net/jdk/jdk/rev/7b757120a053
> > 
> > Testing: Building on Linux x86_64 with dtrace enabled. Still works.
> > Unfortunately I don't have a Solaris machine available.
> > 
> > Thoughts?
> > 
> > Thanks,
> > Severin

[11u] RFR: 8214311: dtrace gensrc has missing dependencies

[Severin Gehwolf]

Hi,

Please review this OpenJDK 11u backport of JDK-8214311. It's an Oracle
JDK 11.0.6 feature parity patch. The jdk/jdk patch didn't apply cleanly
due to context differences in make/hotspot/gensrc/GensrcDtrace.gmk.
That is due to JDK-8211029 not being present in OpenJDK 11u code-base.
I have fixed the patch manually, which was quite trivial.

Bug: https://bugs.openjdk.java.net/browse/JDK-8214311
webrev: 
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214311/jdk11/01/webrev/
Original changeset: http://hg.openjdk.java.net/jdk/jdk/rev/7b757120a053

Testing: Building on Linux x86_64 with dtrace enabled. Still works.
Unfortunately I don't have a Solaris machine available.

Thoughts?

Thanks,
Severin

Re: [11u] RFR: 8212028: Use run-test makefile framework for testing in Oracle's Mach5

[Severin Gehwolf]

On Thu, 2019-10-10 at 13:47 +, Langer, Christoph wrote:
> Adjusted webrev: 
> http://cr.openjdk.java.net/~clanger/webrevs/8212028.11u-dev.1/

Seems OK to me.

Thanks,
Severin

Re: [11u] RFR: 8212028: Use run-test makefile framework for testing in Oracle's Mach5

[Severin Gehwolf]

Hi Christoph,

On Thu, 2019-10-10 at 12:16 +, Langer, Christoph wrote:
> Hi,
> 
> please help reviewing this backport of a build infrastructure change to 
> jdk11u.
> 
> One reason for doing this is parity with Oracle's 11.0.6 but the patch also 
> contains some test improvements which will help stabilizing 11u testing. This 
> mainly means increasing some test timeouts for a few tests that tend to 
> timeout on slow or busy test hardware.
> 
> The patch mostly applies, apart from these two rejects:
> make/RunTests.gmk: 
> http://cr.openjdk.java.net/~clanger/webrevs/8212028.11u-dev.0/8212028-RunTests.gmk.rej
> make/RunTestsPrebuilt.gmk: 
> http://cr.openjdk.java.net/~clanger/webrevs/8212028.11u-dev.0/8212028-RunTestsPrebuilt.gmk.rej
> So, when reviewing, please have the most thorough look at these places.
> 
> I ran the fix through our nightly tests and didn't find regressions. I'm 
> planning to push backport JDK-8213005 together with this change, as it fixes 
> a regression for it.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8212028
> Original Change: http://hg.openjdk.java.net/jdk/jdk/rev/28375a1de254
> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8212028.11u-dev.0/

 222 langtools_JTREG_MAX_MEM := 768m
 223 
 224 langtools_JTREG_MAX_MEM := 768m

Lines 222 and 224 in RunTests.gmk are duplicates.

Otherwise seems fine.

Thanks,
Severin

Re: [8u] RFR: 8227397: Add --with-extra-asflags configure option

[Severin Gehwolf]

On Mon, 2019-09-30 at 11:36 +0200, Magnus Ihse Bursie wrote:
> On 2019-09-27 17:48, Severin Gehwolf wrote:
> > Hi,
> > 
> > Could I please get a review of this 8u build change backport which adds
> > --with-extra-asflags to OpenJDK 8u. At Red Hat, we need to pass certain
> > assembler only flags for some builds. For example "-Wa,--generate-
> > missing-build-notes=yes", to assembly files only. As the build system
> > is different in 8u over 11u I've re-done the patch.
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8227397
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227397/jdk8/01/
>  Looks good to me.
> 
> /Magnus

Thanks for the review, Magnus!

Cheers,
Severin

> > Testing: Built with --with-extra-asflags=-Wa,--generate-missing-build-
> > notes=yes on Linux x86_64, confirmed linux_x86_64.s gets assembled with
> > the flag and only that file.
> > 
> > I've omitted the windows portion of passing as flags to the hotspot
> > build as I've no idea how. Contributions welcome if that should get
> > included.
> > 
> > Thoughts?
> > 
> > Thanks,
> > Severin
> > 
>  

[8u] RFR: 8227397: Add --with-extra-asflags configure option

[Severin Gehwolf]

Hi,

Could I please get a review of this 8u build change backport which adds
--with-extra-asflags to OpenJDK 8u. At Red Hat, we need to pass certain
assembler only flags for some builds. For example "-Wa,--generate-
missing-build-notes=yes", to assembly files only. As the build system
is different in 8u over 11u I've re-done the patch.

Bug: https://bugs.openjdk.java.net/browse/JDK-8227397
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227397/jdk8/01/

Testing: Built with --with-extra-asflags=-Wa,--generate-missing-build-
notes=yes on Linux x86_64, confirmed linux_x86_64.s gets assembled with
the flag and only that file.

I've omitted the windows portion of passing as flags to the hotspot
build as I've no idea how. Contributions welcome if that should get
included.

Thoughts?

Thanks,
Severin

Re: How to get a specific tag from Opne jdk source control.

[Severin Gehwolf]

On Sun, 2019-09-22 at 16:06 +0300, Moshe Zuisman wrote:
> Hi.
> I need to build open JDK jdk8u 222
> 
> .
> I know that I can download precompiled binary distro of this version. But I
> need to compile it at our site.
> Question is - how can I get source of it?
> I am going to :
> https://hg.openjdk.java.net/jdk8u/jdk8u/tags then to jdk8u222-ga
> 
> and download  zip file  with source directory.
> Then perform "get_sources.sh" and build it...
> When I perform java -version
> I get::
> [jdkbuild@vl-tlv-ctm-dv7j jdk8u-eeeabadc6bf0]$
> jdk8u/build/linux-x86_64-normal-server-release/images/j2re-image/bin/java
> -version
> openjdk version "1.8.0-internal"
> OpenJDK Runtime Environment (build
> 1.8.0-internal-jdkbuild_2019_09_22_17_42-b00)
> OpenJDK 64-Bit Server VM (build 25.71-b00, mixed mode)
> 
> and not something like:  openjdk version "1.8.0-b222"
> that I get if I download binary distro from AdoptOpenJDK site...
> What am I doing wrong?

Like David said, you'd need to configure the build properly so as to
pass the right flags. For inspiration consider these:

https://github.com/AdoptOpenJDK/openjdk8-upstream-binaries/blob/master/build-openjdk8.sh#L45

Thanks,
Severin

Re: [11u] RFR: 8214003: Limit default test jobs based on memory size

[Severin Gehwolf]

On Fri, 2019-08-23 at 08:33 -0700, Erik Joelsson wrote:
> Looks good to me.

Thanks, Erik. Appreciate the review!

Cheers,
Severin

Re: [11u] RFR: 8211727: Adjust default concurrency settings for running tests on Sparc

[Severin Gehwolf]

On Fri, 2019-08-23 at 08:33 -0700, Erik Joelsson wrote:
> Looks good to me.

Thank you, Erik!

Cheers,
Severin

[11u] RFR: 8214003: Limit default test jobs based on memory size

[Severin Gehwolf]

Hi,

Please review this backport of a reliability fix for running tests.
Currently on certain machines where num_cores/2 > mem_in_gb/2 there is
a risk of tests failing due to memory issues. JTREG's -concurrency
setting will currently be num_cures/2 and the available memory for this
might not be sufficient to run the tests.

The JDK 12 patch didn't apply cleanly due to some whitespace/context
differences and a difference in argument's numbers in
RunTestsPrebuilt.gmk (due to jcov work not present in JDK 11. i.e. JDK-
8217761 and friends). The patch had to be done manually.

This patch depends on the backport for JDK-8211727 here:
http://mail.openjdk.java.net/pipermail/jdk-updates-dev/2019-August/001747.html

Bug: https://bugs.openjdk.java.net/browse/JDK-8214003
webrev: 
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214003/jdk11/01/webrev/
original changeset: http://hg.openjdk.java.net/jdk/jdk/rev/8c887dcd5d90

Testing: tier1 on Linux x86_64. Smoke tested "make run-test-prebuilt".
I didn't have access to a specific system which matches the conditions
for this bug to surface, though. Appreciate any testers.

Thoughts?

Thanks,
Severin

[11u] RFR: 8211727: Adjust default concurrency settings for running tests on Sparc

[Severin Gehwolf]

Hi,

Please review this dependency patch of a bug I'd like to backport (JDK-
8214003). It does not apply cleanly - due to JDK-8212028 missing in
jdk11u[1], but the changes needed are quite trivial:

1)
JTREG_TIMEOUT_FACTOR => JTREG_TIMEOUT

2)
test/hotspot/jtreg/compiler/jsr292/ContinuousCallSiteTargetChange.java
change omitted since the test in JDK 11u does not have /timeout=300.

Bug: https://bugs.openjdk.java.net/browse/JDK-8211727
webrev: 
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8211727/jdk11/01/webrev/
Original change: http://hg.openjdk.java.net/jdk/jdk/rev/6b05634db714

Tested this on Linux x86_64 tier1 and verified timeout continues to be
set at 4. Tests on sparc would be welcome.

Thoughts?

Thanks,
Severin

[1] 8212028: Use run-test makefile framework for testing in
Oracle's Mach5. Quite a big change. Does not seem suitable for
JDK 11u.

Re: RFR: [8u] 8141570: Fix Zero interpreter build for --disable-precompiled-headers

[Severin Gehwolf]

On Thu, 2019-08-22 at 10:15 +0100, Andrew Hughes wrote:
> Here it is: https://cr.openjdk.java.net/~andrew/openjdk8/8141570/

Looks good.

Thanks,
Severin

Re: RFR: [8u] 8141570: Fix Zero interpreter build for --disable-precompiled-headers

[Severin Gehwolf]

Hi Andrew,

On Wed, 2019-08-21 at 20:33 +0100, Andrew John Hughes wrote:
> This is the first of a series of four changes to support -Wreturn-type
> in OpenJDK 8u. The -Wreturn-type warning catches instances where control
> flow exits a non-void function without returning a value. This can
> combine with compiler optimisations in some cases to cause runtime
> crashes. The warning is turned on by default in GCC 8 [0], causing the
> build to fail if it is not explicitly disabled.
> 
> The subsequent changes are JDK-8062808 to apply the warning and fix
> issues in the default build, JDK-8143245 to fix issues in the Zero build
> and JDK-8197981 to fix one remaining Zero issue on 32-bit platforms only.
> 
> This change ensures the Zero build is not broken by 8062808 by disabling
> -Wreturn-type in the Zero makefile.
> 
> Changes in the backport process:
> 
> * The G1 changes are dropped as 8u does not contain JDK-8073013, which
> introduces g1EvacStats.*
> * I've changed the ifeq ($(USE_CLANG), true) to ifeq
> ($(JVM_VARIANT_ZEROSHARK), true). There seems to have been a mistake in
> the original patch which equated LLVM with clang. What is actually
> referred to by "some version of llvm do not like -Wundef" is the LLVM
> headers that Shark is built against, not the clang compiler which also
> uses LLVM. This made its way through to the new HotSpot build in OpenJDK
> 9 & 10, but is gone as of JDK-8198724 in OpenJDK 11 and later.
> 
> This backport has been in IcedTea since 3.8.0 (2018-05-29) [1] and our
> RHEL RPMs since 2018-06-16 without issue. Builds of current 8u-dev on
> x86_64 with Zero release and Zero slowdebug succeeded.
> 
> [0] https://gcc.gnu.org/gcc-8/changes.html
> [1] https://icedtea.classpath.org/bugzilla/show_bug.cgi?id=3548

I don't see any webrev. Did you forget to link to it?

Thanks,
Severin

Re: [8u] [PING?] RFR: 8222737: [TESTBUG] Allow for tier 1 like testing in OpenJDK 8u

[Severin Gehwolf]

On Mon, 2019-07-29 at 16:32 +0100, Andrew John Hughes wrote:
> 
> On 29/07/2019 11:10, Severin Gehwolf wrote:
> > On Mon, 2019-07-29 at 10:19 +0100, Andrew Dinn wrote:
> > > On 26/07/2019 18:32, Andrew John Hughes wrote:
> > > > On 26/07/2019 16:53, Severin Gehwolf wrote:
> > [...]
> > > > > > What exactly is being pushed
> > > > > > here?
> > > > > 
> > > > > The following 4 patches:
> > > > > 
> > > > > jdk:   
> > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/04/jdk/webrev/
> > > > > hotspot:   
> > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/03/hotspot/webrev/
> > > > > langtools: 
> > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/03/langtools/webrev/
> > > > > top:   
> > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/02/top/webrev/
> > > > > 
> > > > > Each of them has a reference to a plain patch file if you prefer to
> > > > > review that.
> > > > > 
> > > > > Thanks,
> > > > > Severin
> > > > > 
> > > > 
> > > > Ok. HotSpot, JDK & top look fine, but there seem to be a lot of changes
> > > > in langtools. Are these original changes to this patch or are they
> > > > backports? Is there a reason langtools needs so much more work than the
> > > > others?
> > > The langtools Makefile changes appear to be larger because they required
> > > adding a load of definitions/macros that are already present in the
> > > Makefiles in the other trees (i.e. AWK, CAT etc, ZIP_UP_RESULTS,
> > > BUNDLE_UP_AND_EXIT etc). These changes look ok to me.
> > 
> > Yes, exactly. The langools Makefile looks *very* different from
> > jdk/hotspot. In order to get some unified status of test results across
> > all three, these changes were needed. I've opted to make the langools
> > Makefile look more like the ones from jdk and hotspot.
> > 
> > Old "make test" from top-level continues to work with these changes
> > (modulo -verbose:nopass=> summary change).
> > 
> > Anyway, I had another look at this and made 8075546[1] another pre-
> > requisite which makes the langtools changes slightly smaller. Updated
> > webrevs set, thus, is:
> > 
> > jdk:   
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/04/jdk/webrev/
> > hotspot:   
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/03/hotspot/webrev/
> > langtools: 
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/05/langtools/webrev/
> > top:   
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/02/top/webrev/
> > 
> > Thanks,
> > Severin
> > 
> > [1] https://bugs.openjdk.java.net/browse/JDK-8075546
> > 
> 
> Right. That doesn't answer the question as to the origin of the changes.
> If I look at the langtools Makefile in OpenJDK 11, it doesn't have these
> changes. Should they not go there first if they are needed?

It looks like test/langtools/Makefile in JDK 11 is not used. At least
make run-test-tier1 doesn't use it in JDK 11. I've just tried the
following which works fine:

$ rm test/langtools/Makefile
$ hg status
R test/langtools/Makefile
$ make  JAVAC_FLAGS=-g  DISABLE_INTREE_EC=true  LOG=debug  run-test-tier1

Old make files have been removed with JDK-8217638 in JDK 13.

To answer your question: I don't think these changes are needed for
OpenJDK 11 as it has the "run-tests" facility (JDK-8176084) which
deprecates the old way of running tests. Having said that, it seems
more intrusive to try to get that facility ported to JDK 8u, than
proceeding with the proposed change.

JDK-8217638 suggests that old make files were kept for preserving some
old workflows. Due to deprecation of "make test" from JDK 8 in JDK 9+,
most of it is different from JDK 9+ onwards. For some time, both
approaches seem to have kept working. I'm not sure in what way "make
run-test" depends on the new build system in JDK 9. It may do. Briefly
looking at the run-test patches it seems to open a can of worms.

Incidentally, JDK-8170629, as one of the changes to the build/test
infrastructure which happened in JDK 9 timeframe, seems to confirm that
langtools is the "odd-man-out" in terms of make files and code
duplication :)

> Update releases are primarily for backports, so I'm dubious of anything
> that brings in a bunch of apparently new changes without making it clear
> why there are needed and why a ba

Re: [8u] [PING?] RFR: 8222737: [TESTBUG] Allow for tier 1 like testing in OpenJDK 8u

[Severin Gehwolf]

On Mon, 2019-07-29 at 10:19 +0100, Andrew Dinn wrote:
> On 26/07/2019 18:32, Andrew John Hughes wrote:
> > On 26/07/2019 16:53, Severin Gehwolf wrote:
[...]
> > > 
> > > > What exactly is being pushed
> > > > here?
> > > 
> > > The following 4 patches:
> > > 
> > > jdk:   
> > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/04/jdk/webrev/
> > > hotspot:   
> > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/03/hotspot/webrev/
> > > langtools: 
> > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/03/langtools/webrev/
> > > top:   
> > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/02/top/webrev/
> > > 
> > > Each of them has a reference to a plain patch file if you prefer to
> > > review that.
> > > 
> > > Thanks,
> > > Severin
> > > 
> > 
> > Ok. HotSpot, JDK & top look fine, but there seem to be a lot of changes
> > in langtools. Are these original changes to this patch or are they
> > backports? Is there a reason langtools needs so much more work than the
> > others?
> The langtools Makefile changes appear to be larger because they required
> adding a load of definitions/macros that are already present in the
> Makefiles in the other trees (i.e. AWK, CAT etc, ZIP_UP_RESULTS,
> BUNDLE_UP_AND_EXIT etc). These changes look ok to me.

Yes, exactly. The langools Makefile looks *very* different from
jdk/hotspot. In order to get some unified status of test results across
all three, these changes were needed. I've opted to make the langools
Makefile look more like the ones from jdk and hotspot.

Old "make test" from top-level continues to work with these changes
(modulo -verbose:nopass=> summary change).

Anyway, I had another look at this and made 8075546[1] another pre-
requisite which makes the langtools changes slightly smaller. Updated
webrevs set, thus, is:

jdk:   
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/04/jdk/webrev/
hotspot:   
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/03/hotspot/webrev/
langtools: 
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/05/langtools/webrev/
top:   
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/02/top/webrev/

Thanks,
Severin

[1] https://bugs.openjdk.java.net/browse/JDK-8075546

Re: [8u] [PING?] RFR: 8222737: [TESTBUG] Allow for tier 1 like testing in OpenJDK 8u

[Severin Gehwolf]

Hi Andrew,

On Fri, 2019-07-26 at 14:46 +0100, Andrew John Hughes wrote:
> On 25/07/2019 16:02, Severin Gehwolf wrote:
> 
> snip...
> 
> > > Done now. I've added fix-request comments/labels to the above bugs and
> > > rebased on top of them. New jdk changeset:
> > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/04/jdk/webrev/
> > > 
> > > Test groups definition is the JDK 9 set plus :jdk_jdi test set (part of
> > > JDK-8198551 in later JDKs). This seems a reasonable test set. Modulo
> > > added intrinsics testing only relevant for 9+, see JDK-8132855 and JDK-
> > > 8132854.
> > > 
> > > OK to push?
> > 
> > Any more thoughts?
> > 
> > Thanks,
> > Severin
> > 
> 
> This webrev lists three different bug IDs.

It beats me why webrev lists other bugs. The other two, 8075573 and
8075544, were clean backports and were pushed after jdk8u-fix-yes got
added.

> What exactly is being pushed
> here?

The following 4 patches:

jdk:   
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/04/jdk/webrev/
hotspot:   
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/03/hotspot/webrev/
langtools: 
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/03/langtools/webrev/
top:   
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/02/top/webrev/

Each of them has a reference to a plain patch file if you prefer to
review that.

Thanks,
Severin

Re: [8u] [PING?] RFR: 8222737: [TESTBUG] Allow for tier 1 like testing in OpenJDK 8u

[Severin Gehwolf]

On Fri, 2019-07-26 at 10:44 +0200, Aleksey Shipilev wrote:
> On 7/25/19 5:02 PM, Severin Gehwolf wrote:
> > Any more thoughts?
> 
> I think we should go on with it. The lack of tier1 in jdk8u is a serious 
> impediment for backports. I
> have applied all the recent patches (04/jdk, 03/hotspot, 03/langtools/ 
> 02/top), and tier1 works for
> me. That is, it produces test failures that we really need to get on fixing.

Coming back to this I've realized the fix request comment has been
added a while back. Waiting for approval and then I can push it :)

Thanks,
Severin

Re: [8u] [PING?] RFR: 8222737: [TESTBUG] Allow for tier 1 like testing in OpenJDK 8u

[Severin Gehwolf]

On Fri, 2019-07-26 at 10:44 +0200, Aleksey Shipilev wrote:
> On 7/25/19 5:02 PM, Severin Gehwolf wrote:
> > Any more thoughts?
> 
> I think we should go on with it. The lack of tier1 in jdk8u is a serious 
> impediment for backports. I
> have applied all the recent patches (04/jdk, 03/hotspot, 03/langtools/ 
> 02/top), and tier1 works for
> me. That is, it produces test failures that we really need to get on fixing.

Agreed. Adding jdk8u-fix-request to the bug.

Thanks,
Severin

Re: [8u] [PING?] RFR: 8222737: [TESTBUG] Allow for tier 1 like testing in OpenJDK 8u

[Severin Gehwolf]

On Mon, 2019-07-08 at 14:35 +0200, Severin Gehwolf wrote:
> Hi Andrew,
> 
> On Fri, 2019-06-28 at 14:45 +0100, Andrew John Hughes wrote:
> > On 28/06/2019 10:52, Severin Gehwolf wrote:
> > > Hi Andrew,
> > > 
> > > On Thu, 2019-06-27 at 17:36 +0100, Andrew John Hughes wrote:
> > > > On 22/05/2019 17:34, Severin Gehwolf wrote:
> > > > > Hi,
> > > > > 
> > > > > Could I please get reviews for this minimal implementation of a tier1-
> > > > > like test set for JDK 8u? The implementation is rather barebones as I
> > > > > don't think it's worth rewriting the build system just for a command
> > > > > that runs a certain set of tests across a select set of repositories.
> > > > > I've re-used existing work in Makefiles as much as possible. After 
> > > > > this
> > > > > patch one can do:
> > > > > 
> > > > > $ make test TEST="tier1"
> > > > > 
> > > > > Inspiration came from JDK 11u's tier1. As for prior art to this, I've
> > > > > only found "make test" to be working for JDK 8u from the top level.
> > > > > Yet, it doesn't run any hotspot tests, exits with a zero code on test
> > > > > failures and doesn't present a summary at the end. Overall not a nice
> > > > > developer experience.
> > > > > 
> > > > > This patch makes it easier for a developers tests. It presents a
> > > > > summary at the end, returns non-zero on test failures so this can get
> > > > > used in CI and runs hotspot tests.
> > > > > 
> > > > > As a follow-on we can work on fixing/excluding tests so that we always
> > > > > have a passing set of tests for developers to run before a checkin.
> > > > > 
> > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8222737
> > > > > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/02/
> > > > > (includes changes to top/hotspot/jdk/langtools repos)
> > > > > 
> > > > > Example excerpt from a run:
> > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/02/example_output.txt
> > > > > 
> > > > > Thoughts?
> > > > > 
> > > > > Thanks,
> > > > > Severin
> > > > > 
> > > > 
> > > > Is there a reason for creating new tier definitions here rather than
> > > > backporting the existing ones?
> > > 
> > > Yes. The tests in JDK 8 are significantly different from JDK 11. Most
> > > notably the number of tests available and defined test groups.
> > > Backporting the JDK 11 changes seems like fitting a square peg into a
> > > round hole.
> > > 
> > > The JDK 8u "tier1" could only be a rough approximation of the JDK 11
> > > "tier1" at best. There is also no intention to make the test output
> > > 100% identical to JDK 11 as bringing back those changes would be too
> > > invasive.
> > > 
> > > I believe the proposed patch is a reasonable minimal patch which aids
> > > 8u developer's testing.
> > > 
> > > > https://bugs.openjdk.java.net/browse/JDK-8075543
> > > > 
> > > > The subtasks also cover nashorn & jaxp which are missed here. jaxp would
> > > > need JDK-8065673, JDK-8051540 and friends to convert its tests to jtreg.
> > > 
> > > Sure. We can consider adding later tiers in upcoming work. That's not
> > > the priority, though. Note that tier1 tests in JDK 11 only run
> > > jdk/lantools/hotspot tests. The intention of this initial patch would
> > > be to have some testing "baseline" covering a reasonable set of repos
> > > and tests.
> > > 
> > > Thanks,
> > > Severin
> > > 
> > 
> > I'm not talking about JDK 11 changes. These are changes in JDK 9.
> > 
> > For example,
> > 
> > https://hg.openjdk.java.net/jdk9/dev/jdk/rev/cd4aea326e89
> > 
> > adds groups to two tiers, all of which exist in 8u as well. It's not
> > clear where the list in your patch comes from and it appears to add a
> > bunch of arbitrary other tests beyond the tier 1 defined in the JDK 9
> > change.
> 
> Other changes to the JDK tier1 definition were taken from current JDK
> 11u.
> 
> > It would make more sense to me to backport JDK-8075544 & JDK-8075573 as
> > pre-requisites rather than creating completely new definitions.
> 
> Done now. I've added fix-request comments/labels to the above bugs and
> rebased on top of them. New jdk changeset:
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/04/jdk/webrev/
> 
> Test groups definition is the JDK 9 set plus :jdk_jdi test set (part of
> JDK-8198551 in later JDKs). This seems a reasonable test set. Modulo
> added intrinsics testing only relevant for 9+, see JDK-8132855 and JDK-
> 8132854.
> 
> OK to push?

Any more thoughts?

Thanks,
Severin

Re: [Ping?] RFR: 8227397: Add --with-extra-asflags configure option

[Severin Gehwolf]

Hi Paul,

Thanks for the review!

On Mon, 2019-07-15 at 18:02 +, Hohensee, Paul wrote:
> In CompileJvm.gmk, I'd replace
> 
> -ASFLAGS := $(JVM_ASFLAGS), \
> +ASFLAGS := $(JVM_ASFLAGS) $(EXTRA_ASFLAGS), \
> 
> with adding EXTRA_ASFLAGS to JVM_ASFLAGS right after where JVM_LDFLAGS is 
> finalized, vis
> 
> JVM_LDFLAGS += \
> $(SHARED_LIBRARY_FLAGS) \
> $(JVM_LDFLAGS_FEATURES) \
> $(EXTRA_LDFLAGS) \
> #
> + 
> + JVM_ASFLAGS += ${EXTRA_ASFLAGS)
> 
> That way, uses of EXTRA_xxFLAGS are all in one place and possible future 
> file-specific assembler flags don't have to duplicate adding EXTRA_ASFLAGS.

Fixed as suggested:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227397/02/webrev/

Thanks,
Severin

> Otherwise, good.
> 
> Paul
> 
> On 7/15/19, 7:27 AM, "build-dev on behalf of Severin Gehwolf" 
>  wrote:
> 
> Anyone?
> 
> On Mon, 2019-07-08 at 17:56 +0200, Severin Gehwolf wrote:
> > Hi,
> > 
> > Could I please get a review for this patch which adds a new configure
> > option --with-extra-asflags? The issue at hand is that we, Red Hat,
> > need to pass certain extra flags to the assembler when OpenJDK is being
> > compiled. -Wa,--generate-missing-build-notes=yes in our case. That's
> > currently not possible and extra C/C++ flags would need to be used,
> > which seems not nice.
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8227397
> > webrev: 
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227397/01/webrev/
> > 
> > After this patch extra assembler flags get added to *.s/.S files for
> > libjvm.so:
> > 
> > $ grep -n generate-missing-build-notes=yes 
> build/linux-x86_64-server-release/build.log 
> > 1049: [18] ASFLAGS := -m64 -Wa,--generate-missing-build-notes=yes  
> > 15005:( /usr/bin/rm -f 
> /disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o.log
>  && /usr/bin/gcc -c -m64 -Wa,--generate-missing-build-notes=yes -g -o 
> /disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o
>  
> /disk/openjdk/upstream-sources/openjdk-head/src/hotspot/os_cpu/linux_x86/linux_x86_64.s
>  > >(/usr/bin/tee -a 
> /disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o.log)
>  2> >(/usr/bin/tee -a 
> /disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o.log
>  >&2) || ( exitcode=$? && /usr/bin/cp 
> /disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o.log
>  
> /disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/make-support/failure-logs/hotspot_variant-server_libjvm_objs_linux_x86_64.o.log
>  && /usr/bin/cp 
> /disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o.cmdline
>  
> /disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/make-support/failure-logs/hotspot_variant-server_libjvm_objs_linux_x86_64.o.cmdline
>  && exit $exitcode ) )
> > 
> > I'll run this through jdk/submit before I push.
> > 
> > Thoughts?
> > 
> > Thanks,
> > Severin
> 
> 
> 

Re: [Ping?] RFR: 8227397: Add --with-extra-asflags configure option

[Severin Gehwolf]

Anyone?

On Mon, 2019-07-08 at 17:56 +0200, Severin Gehwolf wrote:
> Hi,
> 
> Could I please get a review for this patch which adds a new configure
> option --with-extra-asflags? The issue at hand is that we, Red Hat,
> need to pass certain extra flags to the assembler when OpenJDK is being
> compiled. -Wa,--generate-missing-build-notes=yes in our case. That's
> currently not possible and extra C/C++ flags would need to be used,
> which seems not nice.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8227397
> webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227397/01/webrev/
> 
> After this patch extra assembler flags get added to *.s/.S files for
> libjvm.so:
> 
> $ grep -n generate-missing-build-notes=yes 
> build/linux-x86_64-server-release/build.log 
> 1049: [18] ASFLAGS := -m64 -Wa,--generate-missing-build-notes=yes  
> 15005:( /usr/bin/rm -f 
> /disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o.log
>  && /usr/bin/gcc -c -m64 -Wa,--generate-missing-build-notes=yes -g -o 
> /disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o
>  
> /disk/openjdk/upstream-sources/openjdk-head/src/hotspot/os_cpu/linux_x86/linux_x86_64.s
>  > >(/usr/bin/tee -a 
> /disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o.log)
>  2> >(/usr/bin/tee -a 
> /disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o.log
>  >&2) || ( exitcode=$? && /usr/bin/cp 
> /disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o.log
>  
> /disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/make-support/failure-logs/hotspot_variant-server_libjvm_objs_linux_x86_64.o.log
>  && /usr/bin/cp 
> /disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o.cmdline
>  
> /disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/make-support/failure-logs/hotspot_variant-server_libjvm_objs_linux_x86_64.o.cmdline
>  && exit $exitcode ) )
> 
> I'll run this through jdk/submit before I push.
> 
> Thoughts?
> 
> Thanks,
> Severin

Re: [8u] RFR: 8210761: libjsig is being compiled without optimization

[Severin Gehwolf]

Hi Christoph,

On Wed, 2019-07-10 at 09:08 +, Langer, Christoph wrote:
> Hi Severin,
> 
> You made a little mistake. It must be "-xO4" instead of "-x04" in the
> Solaris build file (It's the letter O instead of the number 0) 

Sigh. Take 5:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210761/jdk8/05/webrev/

Thanks again for your help!

Cheers,
Severin

> 
> Best regards
> Christoph
> 
> > -----Original Message-
> > From: Severin Gehwolf 
> > Sent: Dienstag, 9. Juli 2019 12:09
> > To: Langer, Christoph 
> > Cc: build-dev ; Andrew John Hughes
> > ; jdk8u-dev 
> > Subject: Re: [8u] RFR: 8210761: libjsig is being compiled without
> > optimization
> > 
> > Hi Christoph,
> > 
> > On Mon, 2019-07-08 at 21:42 +, Langer, Christoph wrote:
> > > Hi Severin,
> > > 
> > > I have a solution for Solaris.
> > > 
> > > In your webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-
> > 8210761/jdk8/02/webrev/, make/solaris/makefiles/jsig.make, Line 52:
> > JSIG_OPT_FLAGS = $(OPT_CFLAGS)
> > > Should be: JSIG_OPT_FLAGS = -xO4 -g
> > > 
> > > OPT_CFLAGS are the opt flags for the C++ compiler, but libjsig.o
> > > is compiled
> > with the C compiler. And the C compiler does not like -g0 but needs
> > just -g.
> > 
> > Thanks! webrev #04 is here:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-
> > 8210761/jdk8/04/webrev/
> > 
> > Good to go?
> > 
> > Thanks,
> > Severin
> > > -Original Message-
> > From: Langer, Christoph
> > Sent: Donnerstag, 4. Juli 2019 14:21
> > To: Severin Gehwolf ; Andrew John Hughes
> > ; jdk8u-dev 
> > Cc: build-dev 
> > Subject: RE: [8u] RFR: 8210761: libjsig is being compiled without
> > optimization
> > 
> > Hi Severin,
> > 
> > as we have the Solaris infrastructure in-house, let me try to
> > produce
> > something for Solaris. I'll get back to you soon...
> > 
> > Cheers
> > Christoph
> > 
> > -Original Message-
> > From: Severin Gehwolf 
> > Sent: Donnerstag, 4. Juli 2019 14:18
> > To: Langer, Christoph ; Andrew John
> > Hughes
> > ; jdk8u-dev 
> > Cc: build-dev 
> > Subject: Re: [8u] RFR: 8210761: libjsig is being compiled without
> > optimization
> > Hi Christoph,
> > 
> > On Thu, 2019-07-04 at 10:24 +, Langer, Christoph wrote:
> > Hi Severin,
> > 
> > sorry, this item got drained down in my pile of work.
> > 
> > AIX looks good. For Solaris, however, there is a problem.
> > 
> > This is an excerpt of the logs:
> > SS12u1/SUNWspro/bin/cc -g -xs -m64 -xarch=sparc -G -KPIC \
> >  -M
> > jdk8/hotspot/make/solaris/makefiles/mapfile-vers-jsig -
> > mt
> > -xnolib -xO4  -g0 -xs -o libjsig.so
> > sun_64/nightly/jdk8/hotspot/src/os/solaris/vm/jsig.c -ldl
> > cc: Warning: Option -0 passed to ld, if ld is invoked, ignored
> > otherwise
> > ld: fatal: unrecognized option '-0'
> > ld: fatal: use the '-z help' option for usage information
> > 
> > Thanks for this info!
> > 
> > Seems like the options don't work for Oracle Studio (12 u1) when
> > compiling and linking in one go. A fix would be to split
> > compilation
> > and linking of the lib into 2 steps, I guess.
> > 
> > As I don't have access to such a system and test a potential patch,
> > I've removed the solaris changes:
> > 
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-
> > 8210761/jdk8/03/webrev/
> > 
> > If somebody is able to provide me with a solaris patch, I can
> > happily
> > include it. Otherwise, solaris will stay as-is.
> > 
> > Thoughts? OK to push?
> > 
> > Thanks,
> > Severin
> > 
> > Best regards
> > Christoph
> > 
> > 
> > -Original Message-
> > From: Severin Gehwolf 
> > Sent: Donnerstag, 4. Juli 2019 11:39
> > To: Langer, Christoph ; Andrew John
> > Hughes
> > ; jdk8u-dev  > d...@openjdk.java.net>
> > Cc: build-dev 
> > Subject: Re: [8u] RFR: 8210761: libjsig is being compiled without
> > optimization
> > Hi Christoph,
> > 
> > On Wed, 2019-06-26 at 13:11 +, Langer, Christoph wrote:
> > Here you go:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-
> > 8210761/jdk8/02/webrev/
> > 
> > I cannot really test on bsd, solaris or aix, though :( Appreciate
> > any
> > testers for those platforms.
> > 
> > I pulled the patch into our test environment. It will be run for
> > AIX
> > and solaris there. Will let you know the results...
> > 
> > Any update?
> > 
> > Thanks,
> > Severin

Re: [8u] RFR: 8210761: libjsig is being compiled without optimization

[Severin Gehwolf]

Hi Christoph,

On Mon, 2019-07-08 at 21:42 +, Langer, Christoph wrote:
> Hi Severin,
> 
> I have a solution for Solaris.
> 
> In your webrev: 
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210761/jdk8/02/webrev/, 
> make/solaris/makefiles/jsig.make, Line 52:JSIG_OPT_FLAGS = $(OPT_CFLAGS)
> 
> Should be: JSIG_OPT_FLAGS = -xO4 -g
> 
> OPT_CFLAGS are the opt flags for the C++ compiler, but libjsig.o is compiled 
> with the C compiler. And the C compiler does not like -g0 but needs just -g.

Thanks! webrev #04 is here:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210761/jdk8/04/webrev/

Good to go?

Thanks,
Severin
> -Original Message-
From: Langer, Christoph
Sent: Donnerstag, 4. Juli 2019 14:21
To: Severin Gehwolf ; Andrew John Hughes
; jdk8u-dev 
Cc: build-dev 
Subject: RE: [8u] RFR: 8210761: libjsig is being compiled without optimization

Hi Severin,

as we have the Solaris infrastructure in-house, let me try to produce
something for Solaris. I'll get back to you soon...

Cheers
Christoph

-----Original Message-
From: Severin Gehwolf 
Sent: Donnerstag, 4. Juli 2019 14:18
To: Langer, Christoph ; Andrew John Hughes
; jdk8u-dev 
Cc: build-dev 
Subject: Re: [8u] RFR: 8210761: libjsig is being compiled without
optimization
Hi Christoph,

On Thu, 2019-07-04 at 10:24 +, Langer, Christoph wrote:
Hi Severin,

sorry, this item got drained down in my pile of work.

AIX looks good. For Solaris, however, there is a problem.

This is an excerpt of the logs:
SS12u1/SUNWspro/bin/cc -g -xs -m64 -xarch=sparc -G -KPIC \
 -M 
jdk8/hotspot/make/solaris/makefiles/mapfile-vers-jsig -
mt
-xnolib -xO4  -g0 -xs -o libjsig.so
sun_64/nightly/jdk8/hotspot/src/os/solaris/vm/jsig.c -ldl
cc: Warning: Option -0 passed to ld, if ld is invoked, ignored otherwise
ld: fatal: unrecognized option '-0'
ld: fatal: use the '-z help' option for usage information

Thanks for this info!

Seems like the options don't work for Oracle Studio (12 u1) when
compiling and linking in one go. A fix would be to split compilation
and linking of the lib into 2 steps, I guess.

As I don't have access to such a system and test a potential patch,
I've removed the solaris changes:

http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-
8210761/jdk8/03/webrev/

If somebody is able to provide me with a solaris patch, I can happily
include it. Otherwise, solaris will stay as-is.

Thoughts? OK to push?

Thanks,
Severin

Best regards
Christoph


-----Original Message-
From: Severin Gehwolf 
Sent: Donnerstag, 4. Juli 2019 11:39
To: Langer, Christoph ; Andrew John
Hughes
; jdk8u-dev 
Cc: build-dev 
Subject: Re: [8u] RFR: 8210761: libjsig is being compiled without
optimization
Hi Christoph,

On Wed, 2019-06-26 at 13:11 +, Langer, Christoph wrote:
Here you go:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-
8210761/jdk8/02/webrev/

I cannot really test on bsd, solaris or aix, though :( Appreciate any
testers for those platforms.

I pulled the patch into our test environment. It will be run for AIX
and solaris there. Will let you know the results...

Any update?

Thanks,
Severin

Re: RFR: 8227397: Add --with-extra-asflags configure option

[Severin Gehwolf]

Hi Martin,

On Mon, 2019-07-08 at 10:42 -0700, Martin Buchholz wrote:
> (not really a review ...)
> 
> I'm confused because the assembler is invoked when compiling any sort of 
> source file - C, C++, or .s/.S.
> Wouldn't we want assembler flags passed uniformly, no matter when the 
> assembler is invoked?

Good point. I guess that's debatable. The intention is to only affect
assembling of, well, assembly files (.s/.S). Right now, --with-extra-
cflags would work, but that's more an accident I'd think:

http://hg.openjdk.java.net/jdk/jdk/file/377e49b3014c/make/autoconf/flags-other.m4#l119

In JDK 8 this is a real issue. See:
https://bugs.openjdk.java.net/browse/JDK-8219772

During review this suggestion (with-extra-asflags) came up:
http://mail.openjdk.java.net/pipermail/jdk8u-dev/2019-March/008861.html

> It looks like this patch only affects compilation of .s/.S files in hotspot?

Yes. I've only found assemly files in hotspot. Happy to add it for core
libs, too, but not sure where.

Thanks,
Severin

> On Mon, Jul 8, 2019 at 8:57 AM Severin Gehwolf  wrote:
> > Hi,
> > 
> > Could I please get a review for this patch which adds a new configure
> > option --with-extra-asflags? The issue at hand is that we, Red Hat,
> > need to pass certain extra flags to the assembler when OpenJDK is being
> > compiled. -Wa,--generate-missing-build-notes=yes in our case. That's
> > currently not possible and extra C/C++ flags would need to be used,
> > which seems not nice.
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8227397
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227397/01/webrev/
> > 
> > After this patch extra assembler flags get added to *.s/.S files for
> > libjvm.so:
> > 
> > $ grep -n generate-missing-build-notes=yes 
> > build/linux-x86_64-server-release/build.log 
> > 1049: [18] ASFLAGS := -m64 -Wa,--generate-missing-build-notes=yes  
> > 15005:( /usr/bin/rm -f 
> > /disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o.log
> >  && /usr/bin/gcc -c -m64 -Wa,--generate-missing-build-notes=yes -g -o 
> > /disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o
> >  
> > /disk/openjdk/upstream-sources/openjdk-head/src/hotspot/os_cpu/linux_x86/linux_x86_64.s
> >  > >(/usr/bin/tee -a 
> > /disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o.log)
> >  2> >(/usr/bin/tee -a 
> > /disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o.log
> >  >&2) || ( exitcode=$? && /usr/bin/cp 
> > /disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o.log
> >  
> > /disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/make-support/failure-logs/hotspot_variant-server_libjvm_objs_linux_x86_64.o.log
> >  && /usr/bin/cp 
> > /disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o.cmdline
> >  
> > /disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/make-support/failure-logs/hotspot_variant-server_libjvm_objs_linux_x86_64.o.cmdline
> >  && exit $exitcode ) )
> > 
> > I'll run this through jdk/submit before I push.
> > 
> > Thoughts?
> > 
> > Thanks,
> > Severin
> > 

RFR: 8227397: Add --with-extra-asflags configure option

[Severin Gehwolf]

Hi,

Could I please get a review for this patch which adds a new configure
option --with-extra-asflags? The issue at hand is that we, Red Hat,
need to pass certain extra flags to the assembler when OpenJDK is being
compiled. -Wa,--generate-missing-build-notes=yes in our case. That's
currently not possible and extra C/C++ flags would need to be used,
which seems not nice.

Bug: https://bugs.openjdk.java.net/browse/JDK-8227397
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227397/01/webrev/

After this patch extra assembler flags get added to *.s/.S files for
libjvm.so:

$ grep -n generate-missing-build-notes=yes 
build/linux-x86_64-server-release/build.log 
1049: [18] ASFLAGS := -m64 -Wa,--generate-missing-build-notes=yes  
15005:( /usr/bin/rm -f 
/disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o.log
 && /usr/bin/gcc -c -m64 -Wa,--generate-missing-build-notes=yes -g -o 
/disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o
 
/disk/openjdk/upstream-sources/openjdk-head/src/hotspot/os_cpu/linux_x86/linux_x86_64.s
 > >(/usr/bin/tee -a 
/disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o.log)
 2> >(/usr/bin/tee -a 
/disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o.log
 >&2) || ( exitcode=$? && /usr/bin/cp 
/disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o.log
 
/disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/make-support/failure-logs/hotspot_variant-server_libjvm_objs_linux_x86_64.o.log
 && /usr/bin/cp 
/disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/objs/linux_x86_64.o.cmdline
 
/disk/openjdk/upstream-sources/openjdk-head/build/linux-x86_64-server-release/make-support/failure-logs/hotspot_variant-server_libjvm_objs_linux_x86_64.o.cmdline
 && exit $exitcode ) )

I'll run this through jdk/submit before I push.

Thoughts?

Thanks,
Severin

Re: [8u] RFR: 8222737: [TESTBUG] Allow for tier 1 like testing in OpenJDK 8u

[Severin Gehwolf]

Hi Andrew,

On Fri, 2019-06-28 at 14:45 +0100, Andrew John Hughes wrote:
> On 28/06/2019 10:52, Severin Gehwolf wrote:
> > Hi Andrew,
> > 
> > On Thu, 2019-06-27 at 17:36 +0100, Andrew John Hughes wrote:
> > > On 22/05/2019 17:34, Severin Gehwolf wrote:
> > > > Hi,
> > > > 
> > > > Could I please get reviews for this minimal implementation of a tier1-
> > > > like test set for JDK 8u? The implementation is rather barebones as I
> > > > don't think it's worth rewriting the build system just for a command
> > > > that runs a certain set of tests across a select set of repositories.
> > > > I've re-used existing work in Makefiles as much as possible. After this
> > > > patch one can do:
> > > > 
> > > > $ make test TEST="tier1"
> > > > 
> > > > Inspiration came from JDK 11u's tier1. As for prior art to this, I've
> > > > only found "make test" to be working for JDK 8u from the top level.
> > > > Yet, it doesn't run any hotspot tests, exits with a zero code on test
> > > > failures and doesn't present a summary at the end. Overall not a nice
> > > > developer experience.
> > > > 
> > > > This patch makes it easier for a developers tests. It presents a
> > > > summary at the end, returns non-zero on test failures so this can get
> > > > used in CI and runs hotspot tests.
> > > > 
> > > > As a follow-on we can work on fixing/excluding tests so that we always
> > > > have a passing set of tests for developers to run before a checkin.
> > > > 
> > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8222737
> > > > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/02/
> > > > (includes changes to top/hotspot/jdk/langtools repos)
> > > > 
> > > > Example excerpt from a run:
> > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/02/example_output.txt
> > > > 
> > > > Thoughts?
> > > > 
> > > > Thanks,
> > > > Severin
> > > > 
> > > 
> > > Is there a reason for creating new tier definitions here rather than
> > > backporting the existing ones?
> > 
> > Yes. The tests in JDK 8 are significantly different from JDK 11. Most
> > notably the number of tests available and defined test groups.
> > Backporting the JDK 11 changes seems like fitting a square peg into a
> > round hole.
> > 
> > The JDK 8u "tier1" could only be a rough approximation of the JDK 11
> > "tier1" at best. There is also no intention to make the test output
> > 100% identical to JDK 11 as bringing back those changes would be too
> > invasive.
> > 
> > I believe the proposed patch is a reasonable minimal patch which aids
> > 8u developer's testing.
> > 
> > > https://bugs.openjdk.java.net/browse/JDK-8075543
> > > 
> > > The subtasks also cover nashorn & jaxp which are missed here. jaxp would
> > > need JDK-8065673, JDK-8051540 and friends to convert its tests to jtreg.
> > 
> > Sure. We can consider adding later tiers in upcoming work. That's not
> > the priority, though. Note that tier1 tests in JDK 11 only run
> > jdk/lantools/hotspot tests. The intention of this initial patch would
> > be to have some testing "baseline" covering a reasonable set of repos
> > and tests.
> > 
> > Thanks,
> > Severin
> > 
> 
> I'm not talking about JDK 11 changes. These are changes in JDK 9.
> 
> For example,
> 
> https://hg.openjdk.java.net/jdk9/dev/jdk/rev/cd4aea326e89
> 
> adds groups to two tiers, all of which exist in 8u as well. It's not
> clear where the list in your patch comes from and it appears to add a
> bunch of arbitrary other tests beyond the tier 1 defined in the JDK 9
> change.

Other changes to the JDK tier1 definition were taken from current JDK
11u.

> It would make more sense to me to backport JDK-8075544 & JDK-8075573 as
> pre-requisites rather than creating completely new definitions.

Done now. I've added fix-request comments/labels to the above bugs and
rebased on top of them. New jdk changeset:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/04/jdk/webrev/

Test groups definition is the JDK 9 set plus :jdk_jdi test set (part of
JDK-8198551 in later JDKs). This seems a reasonable test set. Modulo
added intrinsics testing only relevant for 9+, see JDK-8132855 and JDK-
8132854.

OK to push?

Thanks,
Severin

Re: [8u] RFR: 8210761: libjsig is being compiled without optimization

[Severin Gehwolf]

Hi Christoph,

On Thu, 2019-07-04 at 10:24 +, Langer, Christoph wrote:
> Hi Severin,
> 
> sorry, this item got drained down in my pile of work.
> 
> AIX looks good. For Solaris, however, there is a problem.
> 
> This is an excerpt of the logs:
> SS12u1/SUNWspro/bin/cc -g -xs -m64 -xarch=sparc -G -KPIC \
>  -M 
> jdk8/hotspot/make/solaris/makefiles/mapfile-vers-jsig -mt -xnolib -xO4  -g0 
> -xs -o libjsig.so sun_64/nightly/jdk8/hotspot/src/os/solaris/vm/jsig.c -ldl
> cc: Warning: Option -0 passed to ld, if ld is invoked, ignored otherwise
> ld: fatal: unrecognized option '-0'
> ld: fatal: use the '-z help' option for usage information

Thanks for this info!

> Seems like the options don't work for Oracle Studio (12 u1) when
> compiling and linking in one go. A fix would be to split compilation
> and linking of the lib into 2 steps, I guess.

As I don't have access to such a system and test a potential patch,
I've removed the solaris changes:

http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210761/jdk8/03/webrev/

If somebody is able to provide me with a solaris patch, I can happily
include it. Otherwise, solaris will stay as-is.

Thoughts? OK to push?

Thanks,
Severin

> Best regards
> Christoph
> 
> 
> > -Original Message-
> > From: Severin Gehwolf 
> > Sent: Donnerstag, 4. Juli 2019 11:39
> > To: Langer, Christoph ; Andrew John Hughes
> > ; jdk8u-dev 
> > Cc: build-dev 
> > Subject: Re: [8u] RFR: 8210761: libjsig is being compiled without 
> > optimization
> > 
> > Hi Christoph,
> > 
> > On Wed, 2019-06-26 at 13:11 +, Langer, Christoph wrote:
> > > > Here you go:
> > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-
> > > > 8210761/jdk8/02/webrev/
> > > > 
> > > > I cannot really test on bsd, solaris or aix, though :( Appreciate any
> > > > testers for those platforms.
> > > 
> > > I pulled the patch into our test environment. It will be run for AIX
> > > and solaris there. Will let you know the results...
> > 
> > Any update?
> > 
> > Thanks,
> > Severin

Re: [8u] RFR: 8210761: libjsig is being compiled without optimization

[Severin Gehwolf]

Hi Christoph,

On Wed, 2019-06-26 at 13:11 +, Langer, Christoph wrote:
> > Here you go:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-
> > 8210761/jdk8/02/webrev/
> > 
> > I cannot really test on bsd, solaris or aix, though :( Appreciate any
> > testers for those platforms.
> 
> I pulled the patch into our test environment. It will be run for AIX
> and solaris there. Will let you know the results...

Any update?

Thanks,
Severin

Re: [8u] RFR: 8222737: [TESTBUG] Allow for tier 1 like testing in OpenJDK 8u

[Severin Gehwolf]

Hi Andrew,

On Thu, 2019-06-27 at 17:36 +0100, Andrew John Hughes wrote:
> 
> On 22/05/2019 17:34, Severin Gehwolf wrote:
> > Hi,
> > 
> > Could I please get reviews for this minimal implementation of a tier1-
> > like test set for JDK 8u? The implementation is rather barebones as I
> > don't think it's worth rewriting the build system just for a command
> > that runs a certain set of tests across a select set of repositories.
> > I've re-used existing work in Makefiles as much as possible. After this
> > patch one can do:
> > 
> > $ make test TEST="tier1"
> > 
> > Inspiration came from JDK 11u's tier1. As for prior art to this, I've
> > only found "make test" to be working for JDK 8u from the top level.
> > Yet, it doesn't run any hotspot tests, exits with a zero code on test
> > failures and doesn't present a summary at the end. Overall not a nice
> > developer experience.
> > 
> > This patch makes it easier for a developers tests. It presents a
> > summary at the end, returns non-zero on test failures so this can get
> > used in CI and runs hotspot tests.
> > 
> > As a follow-on we can work on fixing/excluding tests so that we always
> > have a passing set of tests for developers to run before a checkin.
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8222737
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/02/
> > (includes changes to top/hotspot/jdk/langtools repos)
> > 
> > Example excerpt from a run:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8222737/02/example_output.txt
> > 
> > Thoughts?
> > 
> > Thanks,
> > Severin
> > 
> 
> Is there a reason for creating new tier definitions here rather than
> backporting the existing ones?

Yes. The tests in JDK 8 are significantly different from JDK 11. Most
notably the number of tests available and defined test groups.
Backporting the JDK 11 changes seems like fitting a square peg into a
round hole.

The JDK 8u "tier1" could only be a rough approximation of the JDK 11
"tier1" at best. There is also no intention to make the test output
100% identical to JDK 11 as bringing back those changes would be too
invasive.

I believe the proposed patch is a reasonable minimal patch which aids
8u developer's testing.

> https://bugs.openjdk.java.net/browse/JDK-8075543
> 
> The subtasks also cover nashorn & jaxp which are missed here. jaxp would
> need JDK-8065673, JDK-8051540 and friends to convert its tests to jtreg.

Sure. We can consider adding later tiers in upcoming work. That's not
the priority, though. Note that tier1 tests in JDK 11 only run
jdk/lantools/hotspot tests. The intention of this initial patch would
be to have some testing "baseline" covering a reasonable set of repos
and tests.

Thanks,
Severin