Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators

[Magnus Ihse Bursie]

On Tue, 17 Nov 2020 21:33:13 GMT, Magnus Ihse Bursie  wrote:

>>> my local branch seems to have the right sources for doc
>> 
>> Maybe, but your branch on GitHub does not.
>
> This PR looks seriously messed up:
> 
> JimLaskey added 30 commits on Oct 9
> @JimLaskey
> 8248862: Implement Enhanced Pseudo-Random Number Generators
> 515a5a4
> @JimLaskey
> 8248862: Implement Enhanced Pseudo-Random Number Generators  …
> 7c25f2d
> @JimLaskey
> 8248862: Implement Enhanced Pseudo-Random Number Generators  …
> 642022d
> @JimLaskey
> Merge branch 'master' into 8248862
> b537e0f
> @JimLaskey
> 8248862: Implement Enhanced Pseudo-Random Number Generators  …
> ca90d01
> @JimLaskey
> 8248862: Implement Enhanced Pseudo-Random Number Generators  …
> ec2941a
> @JimLaskey
> Merge branch 'master' into 8248862
> dbe142c
> @JimLaskey
> 8248862: Implement Enhanced Pseudo-Random Number Generators  …
> cca6f0f
> @JimLaskey
> Merge branch 'master' into 8248862
> 93ee16f
> @JimLaskey
> 8248862: Implement Enhanced Pseudo-Random Number Generators  …
> 84d4704
> @JimLaskey
> 8248862: Implement Enhanced Pseudo-Random Number Generators  …
> b50f2ee
> @JimLaskey
> Merge branch 'master' into 8248862
> b1d6abe
> @JimLaskey
> 8248862: Implement Enhanced Pseudo-Random Number Generators  …
> 8afe7a2
> @JimLaskey
> 8248862: Implement Enhanced Pseudo-Random Number Generators  …
> 2f99696
> @JimLaskey
> 8248862: Implement Enhanced Pseudo-Random Number Generators  …
> 7c58819
> @JimLaskey
> Merge branch 'master' into 8248862
> 655ad08
> @JimLaskey
> 8248862: Implement Enhanced Pseudo-Random Number Generators  …
> d5860b9
> @JimLaskey
> 8248862: Implement Enhanced Pseudo-Random Number Generators  …
> 3ac78f8
> @JimLaskey
> Merge branch 'master' into 8248862
> e3b6e64
> @JimLaskey
> 8248862: Implement Enhanced Pseudo-Random Number Generators  …
> 8ec7f87
> @JimLaskey
> Merge branch 'master' into 8248862
> ed959f8
> @JimLaskey
> 8248862: Implement Enhanced Pseudo-Random Number Generators  …
> 72195e8
> @JimLaskey
> 8248862: Implement Enhanced Pseudo-Random Number Generators  …
> 84730c3
> @JimLaskey
> Merge branch 'master' into 8248862
> 05f0477
> @JimLaskey
> 8248862: Implement Enhanced Pseudo-Random Number Generators  …
> 7dd81ac
> @JimLaskey
> 8248862: Implement Enhanced Pseudo-Random Number Generators  …
> 38f534b
> @JimLaskey
> 8248862: Implement Enhanced Pseudo-Random Number Generators  …
> f428b61
> @JimLaskey
> Merge branch 'master' into 8248862
> 5e33872
> @JimLaskey
> 8248862: Implement Enhanced Pseudo-Random Number Generators  …
> 0bc8e3d
> @JimLaskey
> Merge branch 'master' into 8248862
> 679f1b5
> JimLaskey added 7 commits 4 days ago
> @JimLaskey
> 8248862; Implement Enhanced Pseudo-Random Number Generators  …
> 17972a9
> @JimLaskey
> 8248862: Implement Enhanced Pseudo-Random Number Generators  …
> 4f0338f
> @JimLaskey
> Merge branch 'master' into 8248862
> 7c6b9fa
> @JimLaskey
> 8248862: Implement Enhanced Pseudo-Random Number Generators  …
> c67c729
> @JimLaskey
> Merge branch 'master' into 8248862
> 108ea50
> @JimLaskey
> Merge branch 'master' into 8248862
> 1b31205
> @JimLaskey
> 8248862: Implement Enhanced Pseudo-Random Number Generators  …
> 7469ca5
> 
> It might be better if you close this, rebase your actual change on top of a 
> current master, and open a new PR instead.
> 
> The only reason this was flagged with the `build` label was due to the 
> incorrect deletion of doc, so if you intend to go on with this PR, please 
> remove the label after you've fixed the doc issue.

Ah, sorry, I see now that I expanded the commit comments that it was not just 
the same commit applied over and over again, as I assumed, but iterative steps 
from your branch while developing, that just had the same first line... sorry 
for the confusion about that.

-

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

Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators

[Magnus Ihse Bursie]

On Tue, 17 Nov 2020 21:10:48 GMT, Kevin Rushforth  wrote:

>> @erikj79 my local branch seems to have the right sources for doc
>
>> my local branch seems to have the right sources for doc
> 
> Maybe, but your branch on GitHub does not.

This PR looks seriously messed up:

JimLaskey added 30 commits on Oct 9
@JimLaskey
8248862: Implement Enhanced Pseudo-Random Number Generators
515a5a4
@JimLaskey
8248862: Implement Enhanced Pseudo-Random Number Generators  …
7c25f2d
@JimLaskey
8248862: Implement Enhanced Pseudo-Random Number Generators  …
642022d
@JimLaskey
Merge branch 'master' into 8248862
b537e0f
@JimLaskey
8248862: Implement Enhanced Pseudo-Random Number Generators  …
ca90d01
@JimLaskey
8248862: Implement Enhanced Pseudo-Random Number Generators  …
ec2941a
@JimLaskey
Merge branch 'master' into 8248862
dbe142c
@JimLaskey
8248862: Implement Enhanced Pseudo-Random Number Generators  …
cca6f0f
@JimLaskey
Merge branch 'master' into 8248862
93ee16f
@JimLaskey
8248862: Implement Enhanced Pseudo-Random Number Generators  …
84d4704
@JimLaskey
8248862: Implement Enhanced Pseudo-Random Number Generators  …
b50f2ee
@JimLaskey
Merge branch 'master' into 8248862
b1d6abe
@JimLaskey
8248862: Implement Enhanced Pseudo-Random Number Generators  …
8afe7a2
@JimLaskey
8248862: Implement Enhanced Pseudo-Random Number Generators  …
2f99696
@JimLaskey
8248862: Implement Enhanced Pseudo-Random Number Generators  …
7c58819
@JimLaskey
Merge branch 'master' into 8248862
655ad08
@JimLaskey
8248862: Implement Enhanced Pseudo-Random Number Generators  …
d5860b9
@JimLaskey
8248862: Implement Enhanced Pseudo-Random Number Generators  …
3ac78f8
@JimLaskey
Merge branch 'master' into 8248862
e3b6e64
@JimLaskey
8248862: Implement Enhanced Pseudo-Random Number Generators  …
8ec7f87
@JimLaskey
Merge branch 'master' into 8248862
ed959f8
@JimLaskey
8248862: Implement Enhanced Pseudo-Random Number Generators  …
72195e8
@JimLaskey
8248862: Implement Enhanced Pseudo-Random Number Generators  …
84730c3
@JimLaskey
Merge branch 'master' into 8248862
05f0477
@JimLaskey
8248862: Implement Enhanced Pseudo-Random Number Generators  …
7dd81ac
@JimLaskey
8248862: Implement Enhanced Pseudo-Random Number Generators  …
38f534b
@JimLaskey
8248862: Implement Enhanced Pseudo-Random Number Generators  …
f428b61
@JimLaskey
Merge branch 'master' into 8248862
5e33872
@JimLaskey
8248862: Implement Enhanced Pseudo-Random Number Generators  …
0bc8e3d
@JimLaskey
Merge branch 'master' into 8248862
679f1b5
JimLaskey added 7 commits 4 days ago
@JimLaskey
8248862; Implement Enhanced Pseudo-Random Number Generators  …
17972a9
@JimLaskey
8248862: Implement Enhanced Pseudo-Random Number Generators  …
4f0338f
@JimLaskey
Merge branch 'master' into 8248862
7c6b9fa
@JimLaskey
8248862: Implement Enhanced Pseudo-Random Number Generators  …
c67c729
@JimLaskey
Merge branch 'master' into 8248862
108ea50
@JimLaskey
Merge branch 'master' into 8248862
1b31205
@JimLaskey
8248862: Implement Enhanced Pseudo-Random Number Generators  …
7469ca5

It might be better if you close this, rebase your actual change on top of a 
current master, and open a new PR instead.

The only reason this was flagged with the `build` label was due to the 
incorrect deletion of doc, so if you intend to go on with this PR, please 
remove the label after you've fixed the doc issue.

-

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

Integrated: 8253892: Disable misleading-indentation on clang as well as gcc

[Magnus Ihse Bursie]

On Tue, 3 Nov 2020 22:25:08 GMT, Magnus Ihse Bursie  wrote:

> With clang 10.0, the compiler now detects a new class of warnings. The 
> `misleading-indentation` warning has previously been disabled on gcc for 
> hotspot and libfdlibm.  Now we need to disable it for clang as well.

This pull request has now been integrated.

Changeset: 7dcaba63
Author:    Magnus Ihse Bursie 
URL:   https://git.openjdk.java.net/jdk/commit/7dcaba63
Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod

8253892: Disable misleading-indentation on clang as well as gcc

Reviewed-by: erikj

-

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

Re: RFR: 8253892: Disable misleading-indentation on clang as well as gcc

[Magnus Ihse Bursie]

On Tue, 3 Nov 2020 22:36:34 GMT, John Paul Adrian Glaubitz 
 wrote:

>> With clang 10.0, the compiler now detects a new class of warnings. The 
>> `misleading-indentation` warning has previously been disabled on gcc for 
>> hotspot and libfdlibm.  Now we need to disable it for clang as well.
>
> Why do we disable the warning instead of fixing the incorrect indentations?

@glaubitz Good question. :) If you want to start fixing code to get rid of 
disabled warnings, I will not stand in your way!

For example, in hotspot and gcc, we have `parentheses comment unknown-pragmas 
address delete-non-virtual-dtor char-subscripts array-bounds 
int-in-bool-context ignored-qualifiers missing-field-initializers 
implicit-fallthrough empty-body strict-overflow sequence-point 
maybe-uninitialized misleading-indentation cast-function-type 
shift-negative-value`. I believe many of these should be fixed and removed from 
the list of disabled warnings.

But this bug is about disabling a warning in one compiler that we have already 
decided to disable in another.

-

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

RFR: 8253892: Disable misleading-indentation on clang as well as gcc

[Magnus Ihse Bursie]

With clang 10.0, the compiler now detects a new class of warnings. The 
`misleading-indentation` warning has previously been disabled on gcc for 
hotspot and libfdlibm.  Now we need to disable it for clang as well.

-

Commit messages:
 - 8253892: Disable misleading-indentation on clang as well as gcc

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

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

RFR: 8255798: Remove dead headless code in CompileJavaModules.gmk

[Magnus Ihse Bursie]

The variable BUILD_HEADLESS_ONLY is no longer set. And sun/applet does not 
exist anymore.

-

Commit messages:
 - 8255798: Remove dead headless code in CompileJavaModules.gmk

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

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

Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v4]

[Magnus Ihse Bursie]

On Fri, 23 Oct 2020 16:21:53 GMT, Jan Lahoda  wrote:

>> This is an update to javac and javadoc, to introduce support for Preview 
>> APIs, and generally improve javac and javadoc behavior to more closely 
>> adhere to JEP 12.
>> 
>> The notable changes are:
>> 
>>  * adding support for Preview APIs (javac until now supported primarily only 
>> preview language features, and APIs associated with preview language 
>> features). This includes:
>>  * the @PreviewFeature annotation has boolean attribute "reflective", 
>> which should be true for reflective Preview APIs, false otherwise. This 
>> replaces the existing "essentialAPI" attribute with roughly inverted meaning.
>>  * the preview warnings for preview APIs are auto-suppressed as 
>> described in the JEP 12. E.g. the module that declares the preview API is 
>> free to use it without warnings
>>  * improving error/warning messages. Please see [1] for a list of 
>> cases/examples.
>>  * class files are only marked as preview if they are using a preview 
>> feature. [1] also shows if a class file is marked as preview or not.
>>  * the PreviewFeature annotation has been moved to jdk.internal.javac 
>> package (originally was in the jdk.internal package).
>>  * Preview API in JDK's javadoc no longer needs to specify @preview tag in 
>> the source files. javadoc will auto-generate a note for @PreviewFeature 
>> elements, see e.g. [2] and [3] (non-reflective and reflective API, 
>> respectively). A summary of preview elements is also provided [4]. Existing 
>> uses of @preview have been updated/removed.
>>  * non-JDK javadoc is also enhanced to auto-generate preview notes for uses 
>> of Preview elements, and for declaring elements using preview language 
>> features [5].
>>  
>>  Please also see the CSR [6] for more information.
>>  
>>  [1] 
>> http://cr.openjdk.java.net/~jlahoda/8250768/JEP12-errors-warnings-v6.html
>>  [2] 
>> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.base/java/lang/Record.html
>>  [3] 
>> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.compiler/javax/lang/model/element/RecordComponentElement.html
>>  [4] 
>> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/preview-list.html
>>  [5] http://cr.openjdk.java.net/~jlahoda/8250768/test.javadoc.00/
>>  [6] https://bugs.openjdk.java.net/browse/JDK-8250769
>
> Jan Lahoda has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Removing unnecessary cast.

Build changes look good.

-

Marked as reviewed by ihse (Reviewer).

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

Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v12]

[Magnus Ihse Bursie]

On Fri, 23 Oct 2020 14:06:22 GMT, Maurizio Cimadamore  
wrote:

>> Changes requested by ihse (Reviewer).
>
> @magicus the files you commented on are not part of this PR, but they are 
> introduced as part of:
> https://git.openjdk.java.net/jdk/pull/548
> (you seemed to have approved the changes there - but it's also likely that 
> this PR doesn't include the latest changes in that PR). Sorry for the 
> confusion - but please do report any comment you have on the build changes on 
> that PR!

@mcimadamore I'm sorry too for the confusion. :) I must have been a bit in a 
bit of a hurry when approving it on the other PR. I've now moved my comments 
there. I don't think there's any way for me to "un-review" this change, so I'll 
mark it as accepted, even though I don't have anything to say about it (so that 
I'm not blocking a push). I'll ask the Skara guys if there's a better way to 
deal with this.

Also, in the future, if you are creating a PR which Skara believes has changes 
in the build system, but it "really" does not, please remove the `build` label, 
and I won't even see the PR to come bothering you again! ;-)

-

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

Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v12]

[Magnus Ihse Bursie]

On Thu, 22 Oct 2020 17:04:34 GMT, Maurizio Cimadamore  
wrote:

>> This patch contains the changes associated with the first incubation round 
>> of the foreign linker access API incubation
>> (see JEP 389 [1]). This work is meant to sit on top of the foreign memory 
>> access support (see JEP 393 [2] and associated pull request [3]).
>> 
>> The main goal of this API is to provide a way to call native functions from 
>> Java code without the need of intermediate JNI glue code. In order to do 
>> this, native calls are modeled through the MethodHandle API. I suggest 
>> reading the writeup [4] I put together few weeks ago, which illustrates what 
>> the foreign linker support is, and how it should be used by clients.
>> 
>> Disclaimer: the pull request mechanism isn't great at managing *dependent* 
>> reviews. For this reasons, I'm attaching a webrev which contains only the 
>> differences between this PR and the memory access PR. I will be periodically 
>> uploading new webrevs, as new iterations come out, to try and make the life 
>> of reviewers as simple as possible.
>> 
>> A big thank to Jorn Vernee and Vladimir Ivanov - they are the main 
>> architects of all the hotspot changes you see here, and without their help, 
>> the foreign linker support wouldn't be what it is today. As usual, a big 
>> thank to Paul Sandoz, who provided many insights (often by trying the bits 
>> first hand).
>> 
>> Thanks
>> Maurizio
>> 
>> Webrev:
>> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/webrev
>> 
>> Javadoc:
>> 
>> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/javadoc/jdk/incubator/foreign/package-summary.html
>> 
>> Specdiff (relative to [3]):
>> 
>> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/specdiff_delta/overview-summary.html
>> 
>> CSR:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8254232
>> 
>> 
>> 
>> ### API Changes
>> 
>> The API changes are actually rather slim:
>> 
>> * `LibraryLookup`
>>   * This class allows clients to lookup symbols in native libraries; the 
>> interface is fairly simple; you can load a library by name, or absolute 
>> path, and then lookup symbols on that library.
>> * `FunctionDescriptor`
>>   * This is an abstraction that is very similar, in spirit, to `MethodType`; 
>> it is, at its core, an aggregate of memory layouts for the function 
>> arguments/return type. A function descriptor is used to describe the 
>> signature of a native function.
>> * `CLinker`
>>   * This is the real star of the show. A `CLinker` has two main methods: 
>> `downcallHandle` and `upcallStub`; the first takes a native symbol (as 
>> obtained from `LibraryLookup`), a `MethodType` and a `FunctionDescriptor` 
>> and returns a `MethodHandle` instance which can be used to call the target 
>> native symbol. The second takes an existing method handle, and a 
>> `FunctionDescriptor` and returns a new `MemorySegment` corresponding to a 
>> code stub allocated by the VM which acts as a trampoline from native code to 
>> the user-provided method handle. This is very useful for implementing 
>> upcalls.
>>* This class also contains the various layout constants that should be 
>> used by clients when describing native signatures (e.g. `C_LONG` and 
>> friends); these layouts contain additional ABI classfication information (in 
>> the form of layout attributes) which is used by the runtime to *infer* how 
>> Java arguments should be shuffled for the native call to take place.
>>   * Finally, this class provides some helper functions e.g. so that clients 
>> can convert Java strings into C strings and back.
>> * `NativeScope`
>>   * This is an helper class which allows clients to group together logically 
>> related allocations; that is, rather than allocating separate memory 
>> segments using separate *try-with-resource* constructs, a `NativeScope` 
>> allows clients to use a _single_ block, and allocate all the required 
>> segments there. This is not only an usability boost, but also a performance 
>> boost, since not all allocation requests will be turned into `malloc` calls.
>> * `MemorySegment`
>>   * Only one method added here - namely `handoff(NativeScope)` which allows 
>> a segment to be transferred onto an existing native scope.
>> 
>> ### Safety
>> 
>> The foreign linker API is intrinsically unsafe; many things can go wrong 
>> when requesting a native method handle. For instance, the description of the 
>> native signature might be wrong (e.g. have too many arguments) - and the 
>> runtime has, in the general case, no way to detect such mismatches. For 
>> these reasons, obtaining a `CLinker` instance is a *restricted* operation, 
>> which can be enabled by specifying the usual JDK property 
>> `-Dforeign.restricted=permit` (as it's the case for other restricted method 
>> in the foreign memory API).
>> 
>> ### Implementation changes
>> 
>> The Java changes associated with `LibraryLookup` are relative 
>> straightforward; the only interesting thing to note here is that library 

Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v13]

[Magnus Ihse Bursie]

On Mon, 19 Oct 2020 10:34:32 GMT, Maurizio Cimadamore  
wrote:

>> This patch contains the changes associated with the third incubation round 
>> of the foreign memory access API incubation  (see JEP 393 [1]). This 
>> iteration focus on improving the usability of the API in 3 main ways:
>> 
>> * first, by providing a way to obtain truly *shared* segments, which can be 
>> accessed and closed concurrently from multiple threads
>> * second, by providing a way to register a memory segment against a 
>> `Cleaner`, so as to have some (optional) guarantee that the memory will be 
>> deallocated, eventually
>> * third, by not requiring users to dive deep into var handles when they 
>> first pick up the API; a new `MemoryAccess` class has been added, which 
>> defines several useful dereference routines; these are really just thin 
>> wrappers around memory access var handles, but they make the barrier of 
>> entry for using this API somewhat lower.
>> 
>> A big conceptual shift that comes with this API refresh is that the role of 
>> `MemorySegment` and `MemoryAddress` is not the same as it used to be; it 
>> used to be the case that a memory address could (sometimes, not always) have 
>> a back link to the memory segment which originated it; additionally, memory 
>> access var handles used `MemoryAddress` as a basic unit of dereference.
>> 
>> This has all changed as per this API refresh;  now a `MemoryAddress` is just 
>> a dumb carrier which wraps a pair of object/long addressing coordinates; 
>> `MemorySegment` has become the star of the show, as far as dereferencing 
>> memory is concerned. You cannot dereference memory if you don't have a 
>> segment. This improves usability in a number of ways - first, it is a lot 
>> easier to wrap native addresses (`long`, essentially) into a 
>> `MemoryAddress`; secondly, it is crystal clear what a client has to do in 
>> order to dereference memory: if a client has a segment, it can use that; 
>> otherwise, if the client only has an address, it will have to create a 
>> segment *unsafely* (this can be done by calling 
>> `MemoryAddress::asSegmentRestricted`).
>> 
>> A list of the API, implementation and test changes is provided below. If  
>> you have any questions, or need more detailed explanations, I (and the  rest 
>> of the Panama team) will be happy to point at existing discussions,  and/or 
>> to provide the feedback required. 
>> 
>> A big thank to Erik Osterlund, Vladimir Ivanov and David Holmes, without 
>> whom the work on shared memory segment would not have been possible; also 
>> I'd like to thank Paul Sandoz, whose insights on API design have been very 
>> helpful in this journey.
>> 
>> Thanks 
>> Maurizio 
>> 
>> Javadoc: 
>> 
>> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html
>> 
>> Specdiff: 
>> 
>> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html
>> 
>> CSR: 
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8254163
>> 
>> 
>> 
>> ### API Changes
>> 
>> * `MemorySegment`
>>   * drop factory for restricted segment (this has been moved to 
>> `MemoryAddress`, see below)
>>   * added a no-arg factory for a native restricted segment representing 
>> entire native heap
>>   * rename `withOwnerThread` to `handoff`
>>   * add new `share` method, to create shared segments
>>   * add new `registerCleaner` method, to register a segment against a cleaner
>>   * add more helpers to create arrays from a segment e.g. `toIntArray`
>>   * add some `asSlice` overloads (to make up for the fact that now segments 
>> are more frequently used as cursors)
>>   * rename `baseAddress` to `address` (so that `MemorySegment` can implement 
>> `Addressable`)
>> * `MemoryAddress`
>>   * drop `segment` accessor
>>   * drop `rebase` method and replace it with `segmentOffset` which returns 
>> the offset (a `long`) of this address relative to a given segment
>> * `MemoryAccess`
>>   * New class supporting several static dereference helpers; the helpers are 
>> organized by carrier and access mode, where a carrier is one of the usual 
>> suspect (a Java primitive, minus `boolean`); the access mode can be simple 
>> (e.g. access base address of given segment), or indexed, in which case the 
>> accessor takes a segment and either a low-level byte offset,or a high level 
>> logical index. The classification is reflected in the naming scheme (e.g. 
>> `getByte` vs. `getByteAtOffset` vs `getByteAtIndex`).
>> * `MemoryHandles`
>>   * drop `withOffset` combinator
>>   * drop `withStride` combinator
>>   * the basic memory access handle factory now returns a var handle which 
>> takes a `MemorySegment` and a `long` - from which it is easy to derive all 
>> the other handles using plain var handle combinators.
>> * `Addressable`
>>   * This is a new interface which is attached to entities which can be 
>> projected to a `MemoryAddress`. For now, both `MemoryAddress` and 
>> 

Re: RFR: JDK-8252870: Finalize (remove "incubator" from) jpackage [v4]

[Magnus Ihse Bursie]

On Wed, 21 Oct 2020 20:05:31 GMT, Andy Herrick  wrote:

>> JDK-8252870: Finalize (remove "incubator" from) jpackage
>
> Andy Herrick has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains five commits:
> 
>  - Merge branch master into JDK-8252870
>  - Merge branch 'master' into JDK-8252870
>  - JDK-8252870: Finalize (remove "incubator" from) jpackage
>  - reverting two auto-generated files, and changing module-info to 
> "@since 16"
>  - JDK-8252870: Finalize (remove "incubator" from) jpackage
>Merge branch 'finalize' into JDK-8252870
>  - 8252869 Finalize (remove incubator from) jpackage (implementation)

Build changes look good.

-

Marked as reviewed by ihse (Reviewer).

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

Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v12]

[Magnus Ihse Bursie]

On Thu, 22 Oct 2020 17:04:34 GMT, Maurizio Cimadamore  
wrote:

>> This patch contains the changes associated with the first incubation round 
>> of the foreign linker access API incubation
>> (see JEP 389 [1]). This work is meant to sit on top of the foreign memory 
>> access support (see JEP 393 [2] and associated pull request [3]).
>> 
>> The main goal of this API is to provide a way to call native functions from 
>> Java code without the need of intermediate JNI glue code. In order to do 
>> this, native calls are modeled through the MethodHandle API. I suggest 
>> reading the writeup [4] I put together few weeks ago, which illustrates what 
>> the foreign linker support is, and how it should be used by clients.
>> 
>> Disclaimer: the pull request mechanism isn't great at managing *dependent* 
>> reviews. For this reasons, I'm attaching a webrev which contains only the 
>> differences between this PR and the memory access PR. I will be periodically 
>> uploading new webrevs, as new iterations come out, to try and make the life 
>> of reviewers as simple as possible.
>> 
>> A big thank to Jorn Vernee and Vladimir Ivanov - they are the main 
>> architects of all the hotspot changes you see here, and without their help, 
>> the foreign linker support wouldn't be what it is today. As usual, a big 
>> thank to Paul Sandoz, who provided many insights (often by trying the bits 
>> first hand).
>> 
>> Thanks
>> Maurizio
>> 
>> Webrev:
>> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/webrev
>> 
>> Javadoc:
>> 
>> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/javadoc/jdk/incubator/foreign/package-summary.html
>> 
>> Specdiff (relative to [3]):
>> 
>> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/specdiff_delta/overview-summary.html
>> 
>> CSR:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8254232
>> 
>> 
>> 
>> ### API Changes
>> 
>> The API changes are actually rather slim:
>> 
>> * `LibraryLookup`
>>   * This class allows clients to lookup symbols in native libraries; the 
>> interface is fairly simple; you can load a library by name, or absolute 
>> path, and then lookup symbols on that library.
>> * `FunctionDescriptor`
>>   * This is an abstraction that is very similar, in spirit, to `MethodType`; 
>> it is, at its core, an aggregate of memory layouts for the function 
>> arguments/return type. A function descriptor is used to describe the 
>> signature of a native function.
>> * `CLinker`
>>   * This is the real star of the show. A `CLinker` has two main methods: 
>> `downcallHandle` and `upcallStub`; the first takes a native symbol (as 
>> obtained from `LibraryLookup`), a `MethodType` and a `FunctionDescriptor` 
>> and returns a `MethodHandle` instance which can be used to call the target 
>> native symbol. The second takes an existing method handle, and a 
>> `FunctionDescriptor` and returns a new `MemorySegment` corresponding to a 
>> code stub allocated by the VM which acts as a trampoline from native code to 
>> the user-provided method handle. This is very useful for implementing 
>> upcalls.
>>* This class also contains the various layout constants that should be 
>> used by clients when describing native signatures (e.g. `C_LONG` and 
>> friends); these layouts contain additional ABI classfication information (in 
>> the form of layout attributes) which is used by the runtime to *infer* how 
>> Java arguments should be shuffled for the native call to take place.
>>   * Finally, this class provides some helper functions e.g. so that clients 
>> can convert Java strings into C strings and back.
>> * `NativeScope`
>>   * This is an helper class which allows clients to group together logically 
>> related allocations; that is, rather than allocating separate memory 
>> segments using separate *try-with-resource* constructs, a `NativeScope` 
>> allows clients to use a _single_ block, and allocate all the required 
>> segments there. This is not only an usability boost, but also a performance 
>> boost, since not all allocation requests will be turned into `malloc` calls.
>> * `MemorySegment`
>>   * Only one method added here - namely `handoff(NativeScope)` which allows 
>> a segment to be transferred onto an existing native scope.
>> 
>> ### Safety
>> 
>> The foreign linker API is intrinsically unsafe; many things can go wrong 
>> when requesting a native method handle. For instance, the description of the 
>> native signature might be wrong (e.g. have too many arguments) - and the 
>> runtime has, in the general case, no way to detect such mismatches. For 
>> these reasons, obtaining a `CLinker` instance is a *restricted* operation, 
>> which can be enabled by specifying the usual JDK property 
>> `-Dforeign.restricted=permit` (as it's the case for other restricted method 
>> in the foreign memory API).
>> 
>> ### Implementation changes
>> 
>> The Java changes associated with `LibraryLookup` are relative 
>> straightforward; the only interesting thing to note here is that library 

Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v6]

[Magnus Ihse Bursie]

On Wed, 21 Oct 2020 02:28:30 GMT, Kim Barrett  wrote:

>> Finally returning to this review that was started in April 2020.  I've
>> recast it as a github PR.  I think the security concern raised by Gil
>> has been adequately answered.
>> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-April/029203.html
>> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-July/030401.html
>> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-August/030677.html
>> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-September/030793.html
>> 
>> Please review a new function: java.lang.ref.Reference.refersTo.
>> 
>> This function is needed to test the referent of a Reference object without
>> artificially extending the lifetime of the referent object, as may happen
>> when calling Reference.get.  Some garbage collectors require extending the
>> lifetime of a weak referent when accessed, in order to maintain collector
>> invariants.  Lifetime extension may occur with any collector when the
>> Reference is a SoftReference, as calling get indicates recent access.  This
>> new function also allows testing the referent of a PhantomReference, which
>> can't be accessed by calling get.
>> 
>> The new function uses native methods whose implementations are in the VM so
>> they can use the Access API.  It is the intent that these methods will be
>> intrinsified by optimizing compilers like C2 or graal, but that hasn't been
>> implemented yet.  Bear that in mind before rushing off to change existing
>> uses of Reference.get.
>> 
>> There are two native methods involved, one in Reference and an override in
>> PhantomReference, both package private in java.lang.ref. The reason for this
>> split is to simplify the intrinsification. This is a change from the version
>> from April 2020; that version had a single native method in Reference,
>> implemented using the ON_UNKNOWN_OOP_REF Access reference strength category.
>> However, adding support for that category in the compilers adds significant
>> implementation effort and complexity. Splitting avoids that complexity.
>> 
>> Testing:
>> mach5 tier1
>> Locally (linux-x64) verified the new test passes with various garbage 
>> collectors.
>
> Kim Barrett has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   improve wording in refersTo javadoc

Marked as reviewed by ihse (Reviewer).

-

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

Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v6]

[Magnus Ihse Bursie]

On Wed, 21 Oct 2020 02:55:49 GMT, David Holmes  wrote:

>> Kim Barrett has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   improve wording in refersTo javadoc
>
> Update looks good. Need to reflect the change in the CSR.
> 
> Thanks.
> David

Build changes look good.

-

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

Re: RFR(S): 8252407: Build failure with gcc-8+ and asan

[Magnus Ihse Bursie]

On 2020-09-02 09:50, Kim Barrett wrote:

On Sep 2, 2020, at 2:39 AM, Magnus Ihse Bursie  
wrote:

On 2020-09-01 11:46, Kim Barrett wrote:

I really hate -Wstringop-truncation.  It's been a constant source of churn
for us ever since it appeared.  The changes being made to getIndex and
getFlags (NetworkInterface.c) are modifying lines that were changed very
recently to deal with such warnings from gcc10.  I'm worried that these new
changes will re-trigger warnings from gcc10 (though this change isn't a
revert; the gcc10 warning was justifiable).  I think it should be okay, but
there’s some risk here.

Maybe we should have a common library for all native code where we supply our 
own string operation functions? It will then be much easier to make sure the 
implementation passes different compiler versions, and that we provide sane 
semantics (which isn't really the  case with the original C library functions; 
hence all this warning churning).

For the recurring problems with strncpy, there’s already this:
https://bugs.openjdk.java.net/browse/JDK-8232187
Nobody’s picked it up yet.
Yes, but that's hotspot only. The other JDK libraries would not be able 
to use it. (And as I said, there are other, already existing functions, 
that ideally should be shared between hotspot and the rest of the 
libraries).


/Magnus

Re: RFR(S): 8252407: Build failure with gcc-8+ and asan

[Magnus Ihse Bursie]

On 2020-09-01 11:46, Kim Barrett wrote:

On Sep 1, 2020, at 4:01 AM, Eric Liu  wrote:

Hi all,

Please review this simple change to fix some compile warnings.

The newer gcc (gcc-8 or higher) would warn for calls to bounded string
manipulation functions such as 'strncpy' that may either truncate the
copied string or leave the destination unchanged.

This patch fixed stringop-truncation warnings reported by gcc, some of
them only appear when compiled with "--enable-asan".

[TESTS]
Jtreg: hotspot::hotspot_all_no_apps, jdk::jdk_core and langtools::tier1.
No new failure found.

http://cr.openjdk.java.net/~qfeng/ericliu/jdk/stringop_trunc/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8252407

Thanks,
Eric

I really hate -Wstringop-truncation.  It's been a constant source of churn
for us ever since it appeared.  The changes being made to getIndex and
getFlags (NetworkInterface.c) are modifying lines that were changed very
recently to deal with such warnings from gcc10.  I'm worried that these new
changes will re-trigger warnings from gcc10 (though this change isn't a
revert; the gcc10 warning was justifiable).  I think it should be okay, but
there’s some risk here.
Maybe we should have a common library for all native code where we 
supply our own string operation functions? It will then be much easier 
to make sure the implementation passes different compiler versions, and 
that we provide sane semantics (which isn't really the  case with the 
original C library functions; hence all this warning churning).


There have been other problem areas before, where a common library 
(static or dynamic) would have helped. Perhaps it's time to go ahead and 
create one...


/Magnus


Changes look good, subject to that caveat.  I think these changes conform
better to the documented description of the warning than did the recent
NetworkInterface.c change mentioned above, so I’m hopeful that we’re not
in a warning cycle here.  But it would be good to have someone test these
changes against gcc10.x.

Re: RFC: 8229469 JEP 386: Alpine Linux/x64 Port

[Magnus Ihse Bursie]

On 2020-09-01 13:41, Aleksei Voitylov wrote:

Hi,

JEP 386 is now Candidate [1] and as we resolved all outstanding issues
within the Portola project I'd like to ask for comments from HotSpot,
Core Libs (changes in libjli/java_md.c), and Build groups before moving
the JEP to Proposed to Target:

http://cr.openjdk.java.net/~avoitylov/webrev.8247589.01/

Looks good to me.

/Magnus


Testing:

JTReg, VM TestBase on all platforms (Linux x86_64, Linux x86, Linux Arm,
Linux AArch64, Linux PPC, Windows x86_64, Windows x86, Mac) with no
regressions. A downport of these changes to 14 passes JCK 14 on these
platforms.

The port was tested on Alpine Linux 3.8 and 3.11 with JTReg, VM
TestBase. There are no differences with Linux x86_64  with the exception
of SA which is not supported as per the JEP. A downport of these changes
to 14 passes JCK 14 on Alpine Linux.

Thanks,

-Aleksei

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

Re: [OpenJDK 2D-Dev] RFR: 8246032: Implementation of JEP 347: Adopt C++14 Language Features in HotSpot

[Magnus Ihse Bursie]

On 2020-06-18 08:34, Kim Barrett wrote:

On Jun 15, 2020, at 7:41 AM, Kim Barrett  wrote:


On Jun 14, 2020, at 12:45 AM, Philip Race  wrote:

Kim,


Until it says in "the JDK" and not "in HotSpot" you have not addressed my main 
point.
Please rename the JEP.

After some off-list discussions I have a better idea of what Phil was
asking for and why. In response I've changed the JEP, replacing a few
occurrences of "HotSpot" with "the JDK", as described below.  All
other occurrences of "HotSpot" have been left as-is.

Title:
JEP 347: Adopt C++14 Language Features in HotSpot
=>
JEP 347: Adopt C++14 Language Features in the JDK

Summary:
Allow the use of selected C++14 language features in the HotSpot C++ source 
code.
=>
Allow the use of selected C++14 language features in the JDK C++ source code.

Goals:
The purpose of this JEP is to formally allow C++ source code changes within 
HotSpot to take advantage of some C++14 standard language features.
=>
The purpose of this JEP is to formally allow C++ source code changes within the 
JDK to take advantage of some C++14 standard language features.


This all looks good to me.

/Magnus

Re: RFR: 8247532, 8248135: Records deserialization is slow + Build microbenchmarks with --enable-preview

[Magnus Ihse Bursie]

On 2020-06-23 11:17, Peter Levart wrote:

Including build-dev since this patch is adding new issue 8248135:


https://bugs.openjdk.java.net/browse/JDK-8248135


So here's new webrev with a patch for building benchmarks with 
--enable-preview included:



http://cr.openjdk.java.net/~plevart/jdk-dev/RecordsDeserialization/webrev.08/ 


Build changes look good.

/Magnus



Regards, Peter


On 6/23/20 10:23 AM, Claes Redestad wrote:



On 2020-06-23 10:06, Claes Redestad wrote:

Hi,

On 2020-06-23 09:49, Peter Levart wrote:

Hi Chris, Claes,


Ok then, here's with benchmark included:


http://cr.openjdk.java.net/~plevart/jdk-dev/RecordsDeserialization/webrev.07/ 




I haven't been able to run the benchmark with "make test" though. I 
have tried various ways to pass javac options to build like:



make test 
TEST='micro:org.openjdk.bench.java.io.RecordDeserialization' 
TEST_OPTS="VM_OPTIONS=--enable-preview --release=16"



...but javac doesn't seem to get them. Is there some secret option 
to achieve that?


Hmm, we might as well have the microbenchmarks build with
--enable-preview on by default. Try this:


Fixed:

diff -r f2e1cd498381 make/test/BuildMicrobenchmark.gmk
--- a/make/test/BuildMicrobenchmark.gmk Tue Jun 23 10:08:35 2020 +0200
+++ b/make/test/BuildMicrobenchmark.gmk Tue Jun 23 10:33:17 2020 +0200
@@ -90,10 +90,11 @@
 TARGET_RELEASE := $(TARGET_RELEASE_NEWJDK_UPGRADED), \
 SMALL_JAVA := false, \
 CLASSPATH := $(MICROBENCHMARK_CLASSPATH), \
-    DISABLED_WARNINGS := processing rawtypes cast serial, \
+    DISABLED_WARNINGS := processing rawtypes cast serial preview, \
 SRC := $(MICROBENCHMARK_SRC), \
 BIN := $(MICROBENCHMARK_CLASSES), \
 JAVA_FLAGS := --add-modules jdk.unsupported --limit-modules 
java.management, \

+    JAVAC_FLAGS := --enable-preview, \
 ))

 $(BUILD_JDK_MICROBENCHMARK): $(JMH_COMPILE_JARS)

I verified this works with your micro, and doesn't seem to cause
any issues elsewhere:

 make test TEST=micro:RecordDeserialization

I can shepherd this as a separate fix for documentation purposes, but
feel free to include it in your patch and ping build-dev@..

/Claes



diff -r 52741f85bf23 make/test/BuildMicrobenchmark.gmk
--- a/make/test/BuildMicrobenchmark.gmk    Tue Jun 23 09:54:42 2020 
+0200
+++ b/make/test/BuildMicrobenchmark.gmk    Tue Jun 23 09:59:29 2020 
+0200

@@ -93,7 +93,7 @@
  DISABLED_WARNINGS := processing rawtypes cast serial, \
  SRC := $(MICROBENCHMARK_SRC), \
  BIN := $(MICROBENCHMARK_CLASSES), \
-    JAVA_FLAGS := --add-modules jdk.unsupported --limit-modules 
java.management, \
+    JAVA_FLAGS := --enable-preview --add-modules jdk.unsupported 
--limit-modules java.management, \

  ))

  $(BUILD_JDK_MICROBENCHMARK): $(JMH_COMPILE_JARS)




Otherwise, I simulated what would happen when there are more then 
10 ObjectStreamClass layouts for same class rapidly interchanging, 
so that they push each other out of the cache, by temporarily 
setting cache's MAX_SIZE = 0. Note that this is worst case scenario:



Benchmark  (length) Mode Cnt    
Score    Error  Units
RecordDeserializationBench.deserializeClasses    10 avgt   
10    9.393 ±  0.287  us/op
RecordDeserializationBench.deserializeClasses   100 avgt   10   
35.642 ±  0.977  us/op
RecordDeserializationBench.deserializeClasses  1000 avgt   10  
293.769 ±  7.321  us/op
RecordDeserializationBench.deserializeRecords    10 avgt   10   
15.335 ±  0.496  us/op
RecordDeserializationBench.deserializeRecords   100 avgt   10  
211.427 ± 11.908  us/op
RecordDeserializationBench.deserializeRecords  1000 avgt   10  
990.398 ± 26.681  us/op



This is using JMH option '-gc true' to force GC after each 
iteration of benchmark. Without it, I get a big (~4s) full-GC pause 
just in the middle of a run with length=100:



Iteration   1: 528.577 us/op
Iteration   2: 580.404 us/op
Iteration   3: 4438.228 us/op
Iteration   4: 644.532 us/op
Iteration   5: 698.493 us/op
Iteration   6: 800.738 us/op
Iteration   7: 929.791 us/op
Iteration   8: 870.946 us/op
Iteration   9: 863.416 us/op
Iteration  10: 916.508 us/op


...so results are a bit off because of that:


Benchmark  (length) Mode 
Cnt Score  Error  Units
RecordDeserializationBench.deserializeClasses    10 avgt   
10 8.263 ±    0.043  us/op
RecordDeserializationBench.deserializeClasses   100 avgt   
10    33.406 ±    0.160  us/op
RecordDeserializationBench.deserializeClasses  1000 avgt   10   
287.595 ±    0.960  us/op
RecordDeserializationBench.deserializeRecords    10 avgt   
10    15.270 ±    0.080  us/op
RecordDeserializationBench.deserializeRecords   100 avgt   10  
1127.163 ± 1771.892  us/op
RecordDeserializationBench.deserializeRecords  1000 avgt   10  
2003.235 ±  227.159  us/op



Yes, there is quite a bit of GCing going on when cache is 
thrashing. Note that I haven't tuned GC in any way and I'm running 

Re: RFR(S) : 8211977 : move testlibrary tests into one place

[Magnus Ihse Bursie]

On 2020-06-16 15:06, Erik Joelsson wrote:



On 2020-06-15 17:39, Igor Ignatyev wrote:

@David, Erik, Magnus,

please find the answers to your comments at the bottom of this email.

@all,

David's and Erik's comments made me realize that some parts of the 
original patch were stashed away and didn't make it to webrev.00. I'm 
truly sorry for the confusion and inconvenience. I also agree w/ 
David that it's hard to follow/review, and my gaffe just made it 
worse, therefore I'd like to start it over again from a clean sate


http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02


Build changes look good to me now, except for a missing newline in 
Main.gmk. (No need for new review.)




Ditto.

/Magnus


/Erik

833 lines changed: 228 ins; 591 del; 14 mod; 


could you please review the patch which puts all tests for common 
testlibrary classes (which is ones located in /test/lib) into one 
location under /test/lib-test? please note this intentionally doesn't 
move all "testlibrary" tests, e.g. tests for CTW, hotspot-specific 
testlibrary, are left in /test/hotspot/jtreg/testlibrary_tests/ctw .


to simplify review, the patch has been split into several webrevs, 
with each of them accomplishes a more-or-less distinct thing, but is 
not necessary self-contained:


0. trivial move of tests from test/jdk and test/hotspot/jtreg test 
suites to test/lib-test:

http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.00-move/

1. removal of hotspot's AssertsTest, OutputAnalyzerReportingTest and 
"merge" of hotspot's and jdk's OutputAnalyzerTest:

http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.01-merge/

2. simplification of TestNativeProcessBuilder tests: converts Test 
class used by TestNativeProcessBuilder into a static nested class:

http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.02-TestNativeProcessBuilder

3. makefiles changes to build native parts of lib-test tests. in 
past, there were no tests w/ native parts in this test suite, 
TestNativeProcessBuilder is the 1st such test; this part also removes 
now unneeded lines from hotspot test suite makefile:

http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.03-NativeLibTest

4. clean ups in hotspot test suite: adjusted location 
of SimpleClassFileLoadHookTest test, which is a test for 
hotspot-specific test library (located in 
/test/hotspot/jtreg/testlibrary/jvmti) and hence should not be moved 
to /test/lib-test; removed TestMutuallyExclusivePlatformPredicates 
from TEST.groups:

http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.04-hotspot/

5. LingeredAppTest fix (which apparently has never been run before):
http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.05-LingeredAppTest/

6. changes to make test/lib-test a fully supported and working test 
suite, and add in into tier1,  includes adding ProblemList, 
TEST.groups file, and 'randomness' k/w:

http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.06-TESTROOT/

webrev: http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02
JBS: https://bugs.openjdk.java.net/browse/JDK-8211977
testing:
 - make test TEST=tier1 locally on macosx
 - test/lib-test on  {windows,linux,macosx}-x64
 - tier1 job (in progress)

Thanks,
-- Igor


On Jun 14, 2020, at 11:23 PM, David Holmes <mailto:david.hol...@oracle.com>> wrote:

<...>
This doesn't seem to move everything under 
test/hotspot/jtreg/testlibrary_tests/ e.g. ctw directory.
this is intentionally, ctw test-library is hotspot specific, hence 
its tests are to reside in hotspot test suite (until we decide to 
move the library to /test/lib), the same is true 
for SimpleClassFileLoadHookTest.


You did not modify hotspot/jtreg/TEST.groups but it refers to:
testlibrary_tests/TestMutuallyExclusivePlatformPredicates.java \
Plus it should probably refer to the new native_sanity tests somewhere.
the actual version of the patch removed reference to 
TestMutuallyExclusivePlatformPredicates from hotspot's TEST.groups 
and had TestNativeProcessBuilder moved to /test/test-lib, so no 
updates w.r.t. native_sanity are needed


The newly added copyright notice needs to have the correct initial 
copyright year based on when the file was first added to the repo.
 /test/lib-test/TEST.ROOT file was created by JDK-8243010 on 
2020-04-16, hence the added copyright has 2020 as the initial 
copyright year.


You used the wrong license header - makefiles don't use the 
classpath exception.
JtregNativeLibTest.gmk is based on make/test/JtregNativeJdk.gmk which 
also have classpath exception. quick grep showed that make directory 
has 794 files which has '"Classpath" exception', out of 850 which 
contain 'GNU General Public License version 2' string. And although I 
agree that makefiles shouldn't have the classpath exception, I'd 
prefer to keep JtregNativeLibTest.gmk in sync w/ the its siblings and 
would leave it up to build team to decide how it's best to handle.



On Jun 15, 2020, at 8:12 AM, Magnus Ihse 

Re: RFR(S) : 8211977 : move testlibrary tests into one place

[Magnus Ihse Bursie]

A few comments:

This seems like code copied from elsewhere:

  57 # This evaluation is expensive and should only be done if this target was
  58 # explicitly called.
  59 ifneq ($(filter build-test-libtest-jtreg-native, $(MAKECMDGOALS)), )

I don't agree that this is an expensive evaluation. Furthermore, the 
makefile is only called for building the testlib and for making images, 
so in worst case it's just the image part that would get a penalty 
(although I highly doubt there is any).


  82 $(eval $(call SetupCopyFiles,COPY_LIBTEST_JTREG_NATIVE, \

Please use space after comma.

/Magnus

On 2020-06-13 05:38, Igor Ignatyev wrote:

adding build-dev


On Jun 12, 2020, at 8:36 PM, Igor Ignatyev  wrote:

http://cr.openjdk.java.net/~iignatyev//8211977/webrev.00/

796 lines changed: 200 ins; 588 del; 8 mod;

Hi all,

could you please review this small patch which puts all tests for testlibrary 
classes into one location under /test/lib-test?

besides moving tests from test/jdk/lib/testlibrary and 
test/hotspot/jtreg/testlibrary_tests to test/lib-test the patch also
- problem lists HexPrinterTest.java on windows due to JDK-8247521
- introduces make targets to build native parts for the tests in test/lib-test 
(needed b/c one test has a native part)
- adds randomness k/w to test/lib-test (as it's used by 
RandomGeneratorTest.java)
- makes Test class used by TestNativeProcessBuilder a static nested class of 
TestNativeProcessBuilder
- updates LingeredAppTest to use @build instead of @compile and adds necessary 
@library tag
- removes AssertsTest.java, OutputAnalyzerTest.java from 
test/hotspot/jtreg/testlibrary_tests as they are either identical or lesser 
that the same tests from test/jdk/lib/testlibrary/
- merges test/hotspot/jtreg/testlibrary_tests/OutputAnalyzerTest.java and  
test/jdk/lib/testlibrary/OutputAnalyzerTest.java (effectively adds test cases 
for `firstMatch` to the superier copy from test/jdk/lib/testlibrary)

webrev: http://cr.openjdk.java.net/~iignatyev//8211977/webrev.00/
JBS: https://bugs.openjdk.java.net/browse/JDK-8211977
testing: test/lib-test on {windows,linux,macosx}-x64

Thanks,
-- Igor

Re: RFR: 8246032: Implementation of JEP 347: Adopt C++14 Language Features in HotSpot

[Magnus Ihse Bursie]

On 2020-06-05 09:52, Kim Barrett wrote:

[Changes are only to the build system, but since the changes have jdk-wide
effect I’ve cc’d what I think are the relevant dev lists.]

This change is part of JEP 347; the intent is to target JDK 16.

Please review this change to the building of C++ code in the JDK to
enable the use of C++14 language features.  This change does not make
any code changes to use new features provided by C++11 or C++14.

This requires trimming the supported compiler versions, moving the
minimum supported versions forward (significantly, in some cases).
The new minimums are based on compiler documentation rather than
testing.  It may be that more recent versions are actually required.

This change needs to be tested on platforms not supported by Oracle.
The JEP test plan includes additional Oracle testing beyond what I’ve done.

CR:
https://bugs.openjdk.java.net/browse/JDK-8246032

Webrev:
https://cr.openjdk.java.net/~kbarrett/8246032/open.02/

Looks good to me.

/Magnus



Testing:
mach5 tier1-5 on Oracle supported platforms.

Performance testing showed no significant changes in either direction.

Build-only (no tests) for linux-arm32, linux-s390x, linux-ppc64le

Re: RFR: 8246032: Implementation of JEP 347: Adopt C++14 Language Features in HotSpot

[Magnus Ihse Bursie]

On 2020-06-05 13:59, Jim Laskey wrote:

I know there was a discussion about this elsewhere but I would like to take the 
opportunity to correct this now

make//autoconf/flags-cflags.m4:241

   elif test "x$TOOLCHAIN_TYPE" = xclang; then
 if test "x$OPENJDK_TARGET_OS" = xmacosx; then
   # On MacOSX we optimize for size, something
   # we should do for all platforms?
   C_O_FLAG_HIGHEST_JVM="-Os"
   C_O_FLAG_HIGHEST="-Os"
   C_O_FLAG_HI="-Os"
   C_O_FLAG_NORM="-Os"
   C_O_FLAG_DEBUG_JVM=""
 else
   C_O_FLAG_HIGHEST_JVM="-O3"
   C_O_FLAG_HIGHEST="-O3"
   C_O_FLAG_HI="-O3"
   C_O_FLAG_NORM="-O2"
   C_O_FLAG_DEBUG_JVM="-O0"
 fi
 C_O_FLAG_SIZE="-Os"
 C_O_FLAG_DEBUG="-O0"
 C_O_FLAG_NONE="-O0"
   elif test "x$TOOLCHAIN_TYPE" = xxlc; then

should be changed to

   elif test "x$TOOLCHAIN_TYPE" = xclang; then
 C_O_FLAG_HIGHEST_JVM="-O3"
 C_O_FLAG_HIGHEST="-O3"
 C_O_FLAG_HI="-O3"
 C_O_FLAG_NORM="-O2"
 C_O_FLAG_DEBUG_JVM="-O0"
 C_O_FLAG_SIZE="-Os"
 C_O_FLAG_DEBUG="-O0"
 C_O_FLAG_NONE="-O0"
   elif test "x$TOOLCHAIN_TYPE" = xxlc; then

MacOSX has been paying a historic and significant performance penalty for no 
valid reason.
This might be a valid change, but it has nothing to do with C++14, and 
changing it at the same time will increase risk for unrelated strange 
errors. Please open a separate JBS issue for this requested change.


/Magnus


Otherwise +1.

Cheers,

-- Jim




On Jun 5, 2020, at 4:52 AM, Kim Barrett  wrote:

[Changes are only to the build system, but since the changes have jdk-wide
effect I’ve cc’d what I think are the relevant dev lists.]

This change is part of JEP 347; the intent is to target JDK 16.

Please review this change to the building of C++ code in the JDK to
enable the use of C++14 language features.  This change does not make
any code changes to use new features provided by C++11 or C++14.

This requires trimming the supported compiler versions, moving the
minimum supported versions forward (significantly, in some cases).
The new minimums are based on compiler documentation rather than
testing.  It may be that more recent versions are actually required.

This change needs to be tested on platforms not supported by Oracle.
The JEP test plan includes additional Oracle testing beyond what I’ve done.

CR:
https://bugs.openjdk.java.net/browse/JDK-8246032

Webrev:
https://cr.openjdk.java.net/~kbarrett/8246032/open.02/

Testing:
mach5 tier1-5 on Oracle supported platforms.

Performance testing showed no significant changes in either direction.

Build-only (no tests) for linux-arm32, linux-s390x, linux-ppc64le

Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports - (core libraries)

[Magnus Ihse Bursie]

On 2020-05-07 23:27, Mikael Vidstedt wrote:



On May 7, 2020, at 7:52 AM, Kumar Srinivasan  wrote:

Hi Mikael,

I may have created solinux when the macosx port was merged and in an effort to 
reduce the CPP conditionals.
solinux = solaris + linux  ie. Vanilla unix code vs Darwin code IIRC has 
Objective-C, MacOSX specific thread initialization etc.

Thank you for the background - it all makes sense now! :)
If the file is truly only used on linux, it should be moved to 
java.base/linux/native/libjli instead. (And be renamed.) If it's still 
used on other unix platforms (read: AIX) it should probably be renamed 
java_md_unix.h instead, and kept in place.


If this change should be done in a follow-up fix or as part of JEP 381, 
I leave to you to decide.


(edit: *actually looking at the files* ... )

Hm. The include block looks like this:
#if defined(_AIX)
#include "java_md_aix.h"
#endif

#ifdef MACOSX
#include "java_md_macosx.h"
#else  /* !MACOSX */
#include "java_md_solinux.h"
#endif /* MACOSX */
#endif /* JAVA_MD_H */

It would probably make sense to make this a three-pronged include with 
separate files for aix, macosx and linux, but that will probably require 
stuff to migrate from java_md_solinux.h to java_md_aix.h, or at the very 
least, proper testing on AIX. Recommend filing follow-up issue to sort 
this out.


/Magnus



I looked over the launcher related code looks ok to me.

I did notice the \n endings for the messages that Brent pointed out.

Thank you for the review! The line ending should be fixed in webrev.01.

Cheers,
Mikael


On May 6, 2020, at 9:43 PM, Mikael Vidstedt mailto:mikael.vidst...@oracle.com>> wrote:


I have always wondered what “solinux” is supposed to mean - though not enough 
to actually ask anybody :)

I’ll file a follow-up enhancement to cover renaming the files.

Thank you for the review!

Cheers,
Mikael


On May 4, 2020, at 7:59 AM, Roger Riggs mailto:roger.ri...@oracle.com>> wrote:

Hi Michael,

Looks good.

Maybe just a future cleanup to rename files, since the "...so..." is refering 
to solaris.

src/java.base/unix/native/libjli/java_md_solinux.h
src/java.base/unix/native/libjli/java_md_solinux.h

Regards, Roger


On 5/4/20 4:49 AM, Alan Bateman wrote:

On 04/05/2020 06:12, Mikael Vidstedt wrote:

Please review this change which implements part of JEP 381:

JBS: 
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8244224data=02%7C01%7Ckusrinivasan%40vmware.com%7Ce48bd44f0c484e6fd85608d7f243f1ff%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637244245954061434sdata=W5VvD1owIGoUcbcRkcwixXGPkFLjHUFof2v6cxMqchk%3Dreserved=0
 

webrev: 
https://nam04.safelinks.protection.outlook.com/?url=http:%2F%2Fcr.openjdk.java.net%2F~mikael%2Fwebrevs%2F8244224%2Fwebrev.00%2Fcorelibs%2Fopen%2Fwebrev%2Fdata=02%7C01%7Ckusrinivasan%40vmware.com%7Ce48bd44f0c484e6fd85608d7f243f1ff%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637244245954061434sdata=tQZIjuF5cIPs%2FNqTRtY2WtmwAgwa0iv205wQ9vk0vOQ%3Dreserved=0
 

JEP: 
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8241787data=02%7C01%7Ckusrinivasan%40vmware.com%7Ce48bd44f0c484e6fd85608d7f243f1ff%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637244245954061434sdata=rDvBE%2BJ2F1IIFC9fbA92rs0KZNgYPg0JZM2ynqp7mcs%3Dreserved=0
 



Note: When reviewing this, please be aware that this exercise was *extremely* 
mind-numbing, so I appreciate your help reviewing all the individual changes 
carefully. You may want to get that coffee cup filled up (or whatever keeps you 
awake)!


I took a pass over the changes. I agree its a bit tedious. I'm sure there will 
be a few follow up issues as there are opportunities for cleanup in several 
areas. Just a few comments/questions from a first pass.

ExtendedSocketOption.SO_FLOW_SLA is the Solaris specific socket option that was 
terminally deprecated in 14. The patch removes the implementation but leave the 
API (SO_FLOW_SA and 

Re: RFR: JDK-8241602 jlink does not produce reproducible jimage files

[Magnus Ihse Bursie]

On 2020-05-05 21:56, Jim Laskey wrote:

This fix addresses the inconsistent ordering by jimage content by jlink from 
run to run. Bottom line, the implementer was using HashSet without defining 
hashcode/equals for the Set entry classes.

webrev: http://cr.openjdk.java.net/~jlaskey/8241602/webrev-00 


Looks good to me.

/Magnus

jbs: https://bugs.openjdk.java.net/browse/JDK-8241602 


Cheers,

-- Jim

Re: RFR(S) 8241071 Generation of classes.jsa is not deterministic

[Magnus Ihse Bursie]

Hi Ioi,

On 2020-04-27 07:22, Ioi Lam wrote:

https://bugs.openjdk.java.net/browse/JDK-8241071
http://cr.openjdk.java.net/~iklam/jdk15/8241071-deterministic-cds-archive.v02/ 

I can't really comment on the code changes, but I'd like to thank you 
for the effort of getting a deterministic classes.jsa. This is a big 
step towards the goal of getting deterministic, reproducible builds of 
the JDK.


/Magnus


The goal is to for "java -Xshare:dump" to produce deterministic 
contents in
the CDS archive that depend only on the contents of 
$JAVA_HOME/lib/modules.


Problems:
[1] Symbols in the CDS archive may have non-deterministic order because
    Arena allocation is non-deterministic.
[2] The contents of the CDS shared heap region may be randomized due to
    ImmutableCollections.SALT32L.

Fixes:
[1] With -Xshare:dump, allocate Symbols from the class space (64-bit 
only).

    See changes in symbol.cpp for details.
[2] When running the VM with -Xshare:dump, 
ImmutableCollections.SALT32L is
    initialized with a deterministic seed. NOTE: this affects ONLY 
when the
    VM is running with the special flag -Xshare:dump to dump the CDS 
archive.

    It does NOT affect normal execution of Java programs.

---
I also cleaned up the -Xlog:cds output and print out the CRC of each
CDS region, to help diagnose why two CDS archives may have different 
contents.


Thanks
- Ioi

RFR: JDK-8243156 Fix deprecation and unchecked warnings in microbenchmark

[Magnus Ihse Bursie]

This patch addresses the warnings deprecation and unchecked, which are 
impossible to fully suppress.


With this patch, the test-image can be built fully without warnings.

The patch updates non-critical code to use modern solutions for 
deprecated methods, where possible. Tested methods have been annotated 
with SuppressWarnings instead. I'm leaving it for a future update by the 
perf team to reconsider if it's worth testing deprecated methods.


Bug: https://bugs.openjdk.java.net/browse/JDK-8243156
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8243156-fix-warnings-in-microbenchmark/webrev.01


/Magnus

Re: RFR 8241749: Remove the Nashorn JavaScript Engine

[Magnus Ihse Bursie]

On 2020-04-15 17:56, sundararajan.athijegannat...@oracle.com wrote:

Please review.

Nashorn script engine modules (jdk.scripting.nashorn, 
jdk.scripting.nashorn.shell) and jjs tool are removed.


Bug: https://bugs.openjdk.java.net/browse/JDK-8241749

JEP: https://openjdk.java.net/jeps/372

CSR: https://bugs.openjdk.java.net/browse/JDK-8241751

Webrev: http://cr.openjdk.java.net/~sundar/8241749/webrev.00/index.html
Build changes look almost OK. However, in make/RunTests.gmk, you have 
removed this:


-hotspot_JTREG_PROBLEM_LIST += $(TOPDIR)/test/hotspot/jtreg/ProblemList.txt

which should stay.

/Magnus

Few tests that use nashorn are worked around by @ignore or deleting 
relevant nashorn test sections. Separate test bugs have been filed to 
handle these.


https://bugs.openjdk.java.net/browse/JDK-8242860

https://bugs.openjdk.java.net/browse/JDK-8242859

https://bugs.openjdk.java.net/browse/JDK-8242858

https://bugs.openjdk.java.net/browse/JDK-8241982

https://bugs.openjdk.java.net/browse/JDK-8235594

Thanks,

-Sundar

Re: RFR (S): 8241947: Minor comment fixes for system property handling

[Magnus Ihse Bursie]

From a build perspective this looks fine.

But it seems you are changing the interface for java.lang.System. Don't 
you need a CSR for that? Or is your claim that the interface was indeed 
changed by JDK-8197927, and it is a bug that the documentation was not 
updated to match this?


/Magnus

On 2020-03-31 16:56, Langer, Christoph wrote:

Hi,

please review a small fix that updates two comments.

The first one, in make/autoconf/spec.gmk.in, is probably quite old. It talks about handling of a property 
"vm.vendor" in VersionProps.java.template. However, there is no property "vm.vendor", it 
must rather be "java.vendor". I stumbled over that when looking at the change of JDK-4947890 
(Minimize JNI upcalls in system-properties initialization).

The second one is the code comment attached to "private static Properties props;" in 
java.lang.System. After JDK-8197927 (Allow the system property `java.vendor.version` to be 
undefined), "java.vendor.version" can be undefined, so this should be reflected in the 
comment. I also took the liberty to remove an unneeded import statement.

Bug: https://bugs.openjdk.java.net/browse/JDK-8241947
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8241947.0/

Thanks
Christoph

Re: RFR: (T) 8241144 Javadoc is not generated for new module jdk.nio.mapmode

[Magnus Ihse Bursie]

On 2020-03-25 02:22, David Holmes wrote:

On 25/03/2020 3:49 am, Florian Weimer wrote:

* Magnus Ihse Bursie:


On 2020-03-24 09:59, Andrew Dinn wrote:

On 23/03/2020 18:38, Erik Joelsson wrote:

Looks good.

Thanks for the review, Erik.

I'm assuming that also implies it is trivial (because, copyright 
update

a side, it really is a 1-liner :-).


For code in the build system, we do not have the Hotspot rules of
multiple reviewers, waiting period or trtiviality. A single OK 
review is

enough to be allowed to push it.


Where are these rules documented?  I looked for them on
openjdk.java.net, but could not find them unfortunately.


Hotspot rules are buried in here:

https://wiki.openjdk.java.net/display/HotSpot/HotSpot+How+To

Thanks for the link, David.

For build code, we don't have any such set of rules, so the absence of 
rules kind of is the rules. The rule about at least one reviewer is 
enforced by the JDK project (and jcheck), but that's about it.


Hopefully, with Project Skara, many rules such as these can be enforced 
and/or informed about automatically with bots.


/Magnus



"Before pushing"

    You must be a Committer in the JDK project
    You need a non-JEP JBS issue for tracking
    Your change must have been available for review at least 24 hours 
to accommodate for all time zones
    Your change must have been approved by two Committers out of which 
at least one is also a Reviewer
    Your change must have passed through the hs tier 1 testing 
provided by the submit-hs repository with zero failures
    You must run all relevant testing to make sure your actual change 
is working
    You must be available the next few hours, and the next day and 
ready to follow up with any fix needed in case your change causes 
problems in later tiers


There is a notion of trivial changes that can be pushed sooner than 24 
hours. It should be clearly stated in the review mail that the 
intention is to push as a trivial change. How to actually define 
"trivial" is decided on a case-by-case basis but in general it would 
be things like fixing a comment, or moving code without changing it. 
Backing out a change is also considered trivial as the change itself 
in that case is generated by mercurial.



Cheers,
David

Re: RFR: JDK-8241463 Move build tools to respective modules

[Magnus Ihse Bursie]

On 2020-03-23 23:15, naoto.s...@oracle.com wrote:

Hi Magnus,

I looked at i18n related changes:

make/CopyInterimTZDB.gmk
make/ToolsJdk.gmk
make/gendata/Gendata-java.base.gmk
make/gendata/GendataBreakIterator.gmk
make/gendata/GendataTZDB.gmk
make/gensrc/GensrcCharacterData.gmk
make/gensrc/GensrcEmojiData.gmk

They look ok to me.

Thank you!


The *.java changes should have copyright year update.

Ok, I'll update them.


As to charsetmapping and cldrconverter, I believe they can reside in 
java.base, as jdk.charsets and jdk.localedata modules depend on it.

Okay. It's not ideal, but I think you're right. I'll move them as well.

I'll publish an updated webrev with these changes when there's agreement 
on where in the source code tree to move the files.


/Magnus


Naoto

On 3/23/20 12:03 PM, Magnus Ihse Bursie wrote:
The build tools (small java tools that are run during the build to 
generate source code, or data, needed in the JDK) have historically 
been placed in the "make" directory. This maybe made sense long time 
ago, but does not do so anymore.


Instead, the build tools source code should move the the module that 
needs them. For instance, compilefontconfig should move to 
java.desktop, etc.


There are multiple reasons for this:

* Currently we build *all* build tools at once, which mean that we 
cannot compile java.base until e.g. the compilefontconfig tool is 
compiled, even though it is not needed.


* If a build tool, e.g. compilefontconfig is modified, all build 
tools are recompiled, which triggers a rebuild of more or less the 
entire JDK. This makes development of the build tools unnecessary 
tedious.


* When the build tools are modified, the group owning the 
corresponding module is the proper review instance, not the build 
team. But since they reside under "make", the review mails often 
include build-dev, but this is mostly noise for us. With this move, 
the ownership is made clear.


In this patch, I have not modified how and when the build tools are 
compiled, but this shuffle is the prerequisite for continuing with 
that in a follow-up patch.


I have also moved the build tools to the org.openjdk.buildtools.* 
package name space (inspired by Skara), instead of the strangely 
named build.tools.* name space.


A few build tools are not moved in this patch. Two of them, 
charsetmapping and cldrconverter, are shared between two modules. (I 
think they should move to modules nevertheless, but they need some 
more thought to make sure I do this right.) The rest are tools that 
are needed for the build in general, like linking or javadoc support. 
I'll move this to a better location too, but in a separate patch.


Bug: https://bugs.openjdk.java.net/browse/JDK-8241463
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8241463-move-build-tools-to-modules/webrev.01 



/Magnus

Re: RFR: (T) 8241144 Javadoc is not generated for new module jdk.nio.mapmode

[Magnus Ihse Bursie]

On 2020-03-24 09:59, Andrew Dinn wrote:

On 23/03/2020 18:38, Erik Joelsson wrote:

Looks good.

Thanks for the review, Erik.

I'm assuming that also implies it is trivial (because, copyright update
a side, it really is a 1-liner :-).


For code in the build system, we do not have the Hotspot rules of 
multiple reviewers, waiting period or trtiviality. A single OK review is 
enough to be allowed to push it.


(And for the record, you can add me as reviewer as well, if you wish  :))

/Magnus


I will push to the dev tree and request a backport to jdk14u.

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill


On 2020-03-23 10:56, Andrew Dinn wrote:

Could I please have a review for this trivial fix to the module make
file which ensures that javadoc is generated for new module
jdk.nio.mapmode created as part of the implementation of JEP 352. The
original patch added the module to the BOOT_MODULES list but was not to
the DOCS_MODULES list.

JIRA:   https://bugs.openjdk.java.net/browse/JDK-8241144
webrev: http://cr.openjdk.java.net/~adinn/8241144/webrev

Testing:

Built image and compiled + ran Hello World.

Built make target docs-jdk-api-javadoc and checked module
jdk.nio.mapmode was included in output

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill

RFR: JDK-8241463 Move build tools to respective modules

[Magnus Ihse Bursie]

The build tools (small java tools that are run during the build to 
generate source code, or data, needed in the JDK) have historically been 
placed in the "make" directory. This maybe made sense long time ago, but 
does not do so anymore.


Instead, the build tools source code should move the the module that 
needs them. For instance, compilefontconfig should move to java.desktop, 
etc.


There are multiple reasons for this:

* Currently we build *all* build tools at once, which mean that we 
cannot compile java.base until e.g. the compilefontconfig tool is 
compiled, even though it is not needed.


* If a build tool, e.g. compilefontconfig is modified, all build tools 
are recompiled, which triggers a rebuild of more or less the entire JDK. 
This makes development of the build tools unnecessary tedious.


* When the build tools are modified, the group owning the corresponding 
module is the proper review instance, not the build team. But since they 
reside under "make", the review mails often include build-dev, but this 
is mostly noise for us. With this move, the ownership is made clear.


In this patch, I have not modified how and when the build tools are 
compiled, but this shuffle is the prerequisite for continuing with that 
in a follow-up patch.


I have also moved the build tools to the org.openjdk.buildtools.* 
package name space (inspired by Skara), instead of the strangely named 
build.tools.* name space.


A few build tools are not moved in this patch. Two of them, 
charsetmapping and cldrconverter, are shared between two modules. (I 
think they should move to modules nevertheless, but they need some more 
thought to make sure I do this right.) The rest are tools that are 
needed for the build in general, like linking or javadoc support. I'll 
move this to a better location too, but in a separate patch.


Bug: https://bugs.openjdk.java.net/browse/JDK-8241463
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8241463-move-build-tools-to-modules/webrev.01


/Magnus

Re: RFR: JDK-8241310 Fix warnings in jdk buildtools

[Magnus Ihse Bursie]

On 2020-03-19 18:54, Erik Joelsson wrote:

Looks good to me.

Thanks.


I love the WrapperGenerator using Vector and Hashtable!


Yeah. State of the art.

I'm still trying to wrap my head around this piece of beauty:

    assert !(currentContainer instanceof Entry);
    Entry entry = (Entry)currentContainer;

...

/Magnus


/Erik

On 2020-03-19 09:53, Magnus Ihse Bursie wrote:
The buildtools (java tools needed to be run during the build) have 
long been plagued by warnings, includuing deprecations and unchecked 
warnings, which cannot be silenced during the build.


This patch fixes all buildtool warnings. Most of the warnings are 
fixed properly, but a few have had their warnings suppressed locally.


For two tools, cldrconverter and tzdb, I gave up to get them fully 
fixed, and instead suppressed warnings in some places. Common for 
both these tools were that they used collections of some kind, with a 
mixed bag of data types (e.g. a Map from String to either String, 
HashMap, ArrayList and String[]). I'm frankly not sure how this could 
ever have worked :-) but I assume that the data files being parsed 
has a structure that "guarantees" that this works.


Two files in generatecharacter were missing a proper copyright 
header. I noticed this when I were about to update the copyright 
year, and when I checked the other files I noted another one without 
header. While I did not need to change this file, I thought it was 
suitable to fix the missing header for both files.


I have verified that the code generated by the build is identical 
with and without this patch.


Bug: https://bugs.openjdk.java.net/browse/JDK-8241310
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8241310-fix-warnings-in-buildtools/webrev.01


/Magnus

Re: RFR: JDK-8241310 Fix warnings in jdk buildtools

[Magnus Ihse Bursie]

On 2020-03-19 19:12, Remi Forax wrote:

Hi Magnus,

please try not to use @SuppressWarnings("unchecked") on methods, but on local 
variable instead to reduce the scope,
you can introduce a local variable for that.
Aha, I did not know that possibility existed! (My Java skills is 
becoming somewhat rusty after spending too much time in the build 
system.) That's much better, I fully agree.


In Bundle, your patch declare @SuppressWarnings("unchecked") on the method while you 
already have a local variable with a @SuppressWarnings("unchecked").

Also, usually you don't need @SuppressWarnings("rawtype") apart when you use 
Object.getClass, otherwise using a wilcard solve the issue.
By example in PluralsParseHandle,
   instead of
 Entry entry = (Entry)currentContainer;
   the code should be
 Entry entry = (Entry)currentContainer;

Thank you. I'm still struggling with the  capturing mechanics.

BTW, in this method, you have an Arrays.stream(array).forEach() which is an 
antipattern, a simple loop should be used instead.
I'm sure the code is full of ugliness; however it is not "my" code -- 
I'm merely trying to fix the compiler warnings, and I will not do any 
further cleanups. If I started to walk on that path, there will be no end.


Here is an updated webrev. Now I'm down to just four SuppressWarnings, 
all of them localized, and three of them (in Bundle.java) are modeled 
after similar patterns in the same function.


http://cr.openjdk.java.net/~ihse/JDK-8241310-fix-warnings-in-buildtools/webrev.02

/Magnus


regards,
Rémi

- Mail original -

De: "Magnus Ihse Bursie" 
À: "build-dev" , "core-libs-dev" 

Envoyé: Jeudi 19 Mars 2020 17:53:09
Objet: RFR: JDK-8241310 Fix warnings in jdk buildtools
The buildtools (java tools needed to be run during the build) have long
been plagued by warnings, includuing deprecations and unchecked
warnings, which cannot be silenced during the build.

This patch fixes all buildtool warnings. Most of the warnings are fixed
properly, but a few have had their warnings suppressed locally.

For two tools, cldrconverter and tzdb, I gave up to get them fully
fixed, and instead suppressed warnings in some places. Common for both
these tools were that they used collections of some kind, with a mixed
bag of data types (e.g. a Map from String to either String, HashMap,
ArrayList and String[]). I'm frankly not sure how this could ever have
worked :-) but I assume that the data files being parsed has a structure
that "guarantees" that this works.

Two files in generatecharacter were missing a proper copyright header. I
noticed this when I were about to update the copyright year, and when I
checked the other files I noted another one without header. While I did
not need to change this file, I thought it was suitable to fix the
missing header for both files.

I have verified that the code generated by the build is identical with
and without this patch.

Bug: https://bugs.openjdk.java.net/browse/JDK-8241310
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8241310-fix-warnings-in-buildtools/webrev.01

/Magnus

RFR: JDK-8241310 Fix warnings in jdk buildtools

[Magnus Ihse Bursie]

The buildtools (java tools needed to be run during the build) have long 
been plagued by warnings, includuing deprecations and unchecked 
warnings, which cannot be silenced during the build.


This patch fixes all buildtool warnings. Most of the warnings are fixed 
properly, but a few have had their warnings suppressed locally.


For two tools, cldrconverter and tzdb, I gave up to get them fully 
fixed, and instead suppressed warnings in some places. Common for both 
these tools were that they used collections of some kind, with a mixed 
bag of data types (e.g. a Map from String to either String, HashMap, 
ArrayList and String[]). I'm frankly not sure how this could ever have 
worked :-) but I assume that the data files being parsed has a structure 
that "guarantees" that this works.


Two files in generatecharacter were missing a proper copyright header. I 
noticed this when I were about to update the copyright year, and when I 
checked the other files I noted another one without header. While I did 
not need to change this file, I thought it was suitable to fix the 
missing header for both files.


I have verified that the code generated by the build is identical with 
and without this patch.


Bug: https://bugs.openjdk.java.net/browse/JDK-8241310
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8241310-fix-warnings-in-buildtools/webrev.01


/Magnus

Re: RFR 8241073: Pre-generated Stubs for javax.management, Activation, Naming

[Magnus Ihse Bursie]

On 2020-03-18 22:24, Roger Riggs wrote:

Hi,

Some small updates to the source files to minimize the changes to javadoc
of the _Stub classes.

And fixes to the points Magnus raises below.

http://cr.openjdk.java.net/~rriggs/webrev-stubs-classes-8241073-4/


Looks good! Thank you for getting this the whole way.

/Magnus


Thanks, Roger


On 3/18/20 9:01 AM, Magnus Ihse Bursie wrote:



On 2020-03-17 23:07, Roger Riggs wrote:

Hi Magnus, Erik,

Thanks for the pointers, I'm not familiar with those early build 
intricacies.


Updated:

http://cr.openjdk.java.net/~rriggs/webrev-stubs-classes-8241073-2/

Looking much better!

Please remove the reference to "rmic" in Global.gmk as well.

In Docs.gmk, the comment "# On top of the sources that was used to 
compile the JDK, we need some extra sources" is no longer relevant. 
Just remove it.


In ZipSource.gmk, there crept in an extra space between the argument 
and the comma. Please remove it.


In spec.gmk.in:
- # Interim langtools and rmic modules and arguments
+ # Interim langtools and arguments

Should be "Interim langtools modules and arguments"

Apart from this, it looks good. You do not need to re-spin the webrev 
if you fix these minor nits.


/Magnus





More cleanup:

- cleanup of ZipSource.gmk and autoconf/spec.gmk.in and Docs.gmk

- The mystery of ActivationGroup_Stub is resolved.  The class needed 
to be in the spec/javadoc
   but it also needed to be generated by RMIC, the version in 
src/java.rmi/share/doc
   contained the javadoc comments.  I merged the javadoc into the 
generated stub .java class

   and added it to the repo.

- The NetBeans Jmx build script had targets to build the stubs, they 
have been removed.


Thanks, Roger


On 3/17/20 10:06 AM, Magnus Ihse Bursie wrote:

On 2020-03-17 14:17, Erik Joelsson wrote:

Hello,

That looks better, but there are still some more things to remove. 
This whole block:


 


# Targets for running rmic.
$(eval $(call DeclareRecipesForPhase, RMIC, \
    TARGET_SUFFIX := rmic, \
    FILE_PREFIX := Rmic, \
    MAKE_SUBDIR := rmic, \
    CHECK_MODULES := $(ALL_MODULES)))

ALL_TARGETS += $(RMIC_TARGETS)

And all references to $(RMIC_TARGETS) and $(RMIC_MODULES). In most 
cases the whole lines (like the $(foreach) calls that iterate over 
them) can just be removed.


This also means killing the entire "rmic" phase of the build. So 
please also remove the rmic target from Main.gmk line 1015, and 
ALL_TARGETs line 1133. And the reference to "rmic" from the phases 
in the help in Global.gmk.


In ZipSource.gmk, there's special handling to include rmic source 
code, which is no longer needed.


In Docs.gmk, you can remove $(SUPPORT_OUTPUTDIR)/rmic/* from 
MODULES_SOURCE_PATH.


(That actually made me notice another weird rmi thingy. How does 
these new stubs relate to 
src/java.rmi/share/doc/stub/java/rmi/activation/ActivationGroup_Stub.java? 
That file has been present for a long time. It is included when 
generating Javadoc, but not when compiling the class files for the 
JDK.)


 It is probably a good idea to make a case-insensitive search for 
"rmic" in the make directory afterwards, to confirm that all 
remaining cases of rmic should be there (i.e. it relates to 
compiling jdk.rmi, not running rmic during the build.)


And btw, I'm eternally grateful to you for removing this. :-) Now 
the prospect of a fully warning-free build seems closer than ever!


/Magnus








/Erik

On 2020-03-16 15:19, Roger Riggs wrote:

Hi Erik,

Please review a new webrev that adds the change to remove the 
interim build parts.

(Passes Tier 1-3 of CI testing)

http://cr.openjdk.java.net/~rriggs/webrev-stubs-classes-8241073-1/

Thanks, Roger


On 3/16/20 12:22 PM, Erik Joelsson wrote:

Hello Roger,

There is more to be removed in the makefiles.

This file should also be removed:

make/CompileInterimRmic.gmk

In make/Main.gmk, all the targets concerning rmic needs to be 
removed as well as any dependencies declared that involves them. 
Searching for "rmic" should find all relevant lines.


/Erik

On 2020-03-16 09:02, Roger Riggs wrote:
Please review adding pre-generated RMI stub classes to the jdk 
repo

and the removal of make files supporting the specific APIs.

It removes a dependency on build time generation invoking RMIC.
RMIC was  deprecated in JDK 13 [1].

The source files have been edited to remove or suppress 
compilation warnings.


Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-stubs-classes-8241073/

Issue:
 https://bugs.openjdk.java.net/browse/JDK-8241073


Thanks, Roger

p.s. A future change will remove the interim build steps


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

Re: RFR 8241073: Pre-generated Stubs for javax.management, Activation, Naming

[Magnus Ihse Bursie]

On 2020-03-17 23:07, Roger Riggs wrote:

Hi Magnus, Erik,

Thanks for the pointers, I'm not familiar with those early build 
intricacies.


Updated:

http://cr.openjdk.java.net/~rriggs/webrev-stubs-classes-8241073-2/

Looking much better!

Please remove the reference to "rmic" in Global.gmk as well.

In Docs.gmk, the comment "# On top of the sources that was used to 
compile the JDK, we need some extra sources" is no longer relevant. Just 
remove it.


In ZipSource.gmk, there crept in an extra space between the argument and 
the comma. Please remove it.


In spec.gmk.in:
- # Interim langtools and rmic modules and arguments
+ # Interim langtools and arguments

Should be "Interim langtools modules and arguments"

Apart from this, it looks good. You do not need to re-spin the webrev if 
you fix these minor nits.


/Magnus





More cleanup:

- cleanup of ZipSource.gmk and autoconf/spec.gmk.in and Docs.gmk

- The mystery of ActivationGroup_Stub is resolved.  The class needed 
to be in the spec/javadoc
   but it also needed to be generated by RMIC, the version in 
src/java.rmi/share/doc
   contained the javadoc comments.  I merged the javadoc into the 
generated stub .java class

   and added it to the repo.

- The NetBeans Jmx build script had targets to build the stubs, they 
have been removed.


Thanks, Roger


On 3/17/20 10:06 AM, Magnus Ihse Bursie wrote:

On 2020-03-17 14:17, Erik Joelsson wrote:

Hello,

That looks better, but there are still some more things to remove. 
This whole block:


 


# Targets for running rmic.
$(eval $(call DeclareRecipesForPhase, RMIC, \
    TARGET_SUFFIX := rmic, \
    FILE_PREFIX := Rmic, \
    MAKE_SUBDIR := rmic, \
    CHECK_MODULES := $(ALL_MODULES)))

ALL_TARGETS += $(RMIC_TARGETS)

And all references to $(RMIC_TARGETS) and $(RMIC_MODULES). In most 
cases the whole lines (like the $(foreach) calls that iterate over 
them) can just be removed.


This also means killing the entire "rmic" phase of the build. So 
please also remove the rmic target from Main.gmk line 1015, and 
ALL_TARGETs line 1133. And the reference to "rmic" from the phases in 
the help in Global.gmk.


In ZipSource.gmk, there's special handling to include rmic source 
code, which is no longer needed.


In Docs.gmk, you can remove $(SUPPORT_OUTPUTDIR)/rmic/* from 
MODULES_SOURCE_PATH.


(That actually made me notice another weird rmi thingy. How does 
these new stubs relate to 
src/java.rmi/share/doc/stub/java/rmi/activation/ActivationGroup_Stub.java? 
That file has been present for a long time. It is included when 
generating Javadoc, but not when compiling the class files for the JDK.)


 It is probably a good idea to make a case-insensitive search for 
"rmic" in the make directory afterwards, to confirm that all 
remaining cases of rmic should be there (i.e. it relates to compiling 
jdk.rmi, not running rmic during the build.)


And btw, I'm eternally grateful to you for removing this. :-) Now the 
prospect of a fully warning-free build seems closer than ever!


/Magnus








/Erik

On 2020-03-16 15:19, Roger Riggs wrote:

Hi Erik,

Please review a new webrev that adds the change to remove the 
interim build parts.

(Passes Tier 1-3 of CI testing)

http://cr.openjdk.java.net/~rriggs/webrev-stubs-classes-8241073-1/

Thanks, Roger


On 3/16/20 12:22 PM, Erik Joelsson wrote:

Hello Roger,

There is more to be removed in the makefiles.

This file should also be removed:

make/CompileInterimRmic.gmk

In make/Main.gmk, all the targets concerning rmic needs to be 
removed as well as any dependencies declared that involves them. 
Searching for "rmic" should find all relevant lines.


/Erik

On 2020-03-16 09:02, Roger Riggs wrote:

Please review adding pre-generated RMI stub classes to the jdk repo
and the removal of make files supporting the specific APIs.

It removes a dependency on build time generation invoking RMIC.
RMIC was  deprecated in JDK 13 [1].

The source files have been edited to remove or suppress 
compilation warnings.


Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-stubs-classes-8241073/

Issue:
 https://bugs.openjdk.java.net/browse/JDK-8241073


Thanks, Roger

p.s. A future change will remove the interim build steps


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

Re: RFR 8241073: Pre-generated Stubs for javax.management, Activation, Naming

[Magnus Ihse Bursie]

On 2020-03-17 14:17, Erik Joelsson wrote:

Hello,

That looks better, but there are still some more things to remove. 
This whole block:


 


# Targets for running rmic.
$(eval $(call DeclareRecipesForPhase, RMIC, \
    TARGET_SUFFIX := rmic, \
    FILE_PREFIX := Rmic, \
    MAKE_SUBDIR := rmic, \
    CHECK_MODULES := $(ALL_MODULES)))

ALL_TARGETS += $(RMIC_TARGETS)

And all references to $(RMIC_TARGETS) and $(RMIC_MODULES). In most 
cases the whole lines (like the $(foreach) calls that iterate over 
them) can just be removed.


This also means killing the entire "rmic" phase of the build. So please 
also remove the rmic target from Main.gmk line 1015, and ALL_TARGETs 
line 1133. And the reference to "rmic" from the phases in the help in 
Global.gmk.


In ZipSource.gmk, there's special handling to include rmic source code, 
which is no longer needed.


In Docs.gmk, you can remove $(SUPPORT_OUTPUTDIR)/rmic/* from 
MODULES_SOURCE_PATH.


(That actually made me notice another weird rmi thingy. How does these 
new stubs relate to 
src/java.rmi/share/doc/stub/java/rmi/activation/ActivationGroup_Stub.java? 
That file has been present for a long time. It is included when 
generating Javadoc, but not when compiling the class files for the JDK.)


 It is probably a good idea to make a case-insensitive search for 
"rmic" in the make directory afterwards, to confirm that all remaining 
cases of rmic should be there (i.e. it relates to compiling jdk.rmi, not 
running rmic during the build.)


And btw, I'm eternally grateful to you for removing this. :-) Now the 
prospect of a fully warning-free build seems closer than ever!


/Magnus








/Erik

On 2020-03-16 15:19, Roger Riggs wrote:

Hi Erik,

Please review a new webrev that adds the change to remove the interim 
build parts.

(Passes Tier 1-3 of CI testing)

http://cr.openjdk.java.net/~rriggs/webrev-stubs-classes-8241073-1/

Thanks, Roger


On 3/16/20 12:22 PM, Erik Joelsson wrote:

Hello Roger,

There is more to be removed in the makefiles.

This file should also be removed:

make/CompileInterimRmic.gmk

In make/Main.gmk, all the targets concerning rmic needs to be 
removed as well as any dependencies declared that involves them. 
Searching for "rmic" should find all relevant lines.


/Erik

On 2020-03-16 09:02, Roger Riggs wrote:

Please review adding pre-generated RMI stub classes to the jdk repo
and the removal of make files supporting the specific APIs.

It removes a dependency on build time generation invoking RMIC.
RMIC was  deprecated in JDK 13 [1].

The source files have been edited to remove or suppress compilation 
warnings.


Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-stubs-classes-8241073/

Issue:
 https://bugs.openjdk.java.net/browse/JDK-8241073


Thanks, Roger

p.s. A future change will remove the interim build steps


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

Re: RFR: 8239563 - Reduce public exports in dynamic libraries built from static JDK libraries

[Magnus Ihse Bursie]

On 2020-02-27 15:52, Bob Vandette wrote:



On Feb 27, 2020, at 7:15 AM, Magnus Ihse Bursie  
wrote:

On 2020-02-26 22:01, Bob Vandette wrote:

Here’s an updated webrev that implementes the suggestion that allows JNIEXPORT 
in jni.h to be overridden
and the build limits visibility for static libraries.

If this webrev is accepted, I’ll update the CSR solution to match this 
implementation.

http://cr.openjdk.java.net/~bobv/8239563/webrev.01

This looks basically ok, but some remarks:

You have forgotten to update the copyright year in the header files.

Thanks, I’ll update them.


Also, the quoting looks suspicious. I would have guessed, thinking more carefully about 
this than the post yesterday, that the proper syntax would be 
-DJNIEXPORT='__attribute__((visibility("hidden")))' since otherwise the ' will 
make the \ literal. But, I usually end up being wrong about 50% of the time regarding 
this kind of stuff. :-) Have you verified that you get the proper define?

I did verify that the quoting works on Mac and Linux.  I needed to add \” 
before hidden or the quotes would be eliminated causing
the compiler to complain that visibility was expecting a string but didn’t see 
one.
Very good. Just goes to show how much all these years in the build team 
has helped me learn how to spot quoting errors without trying (viz., not 
much :)).


Looks good to me, then. (I can't say if this fix still needs a CSR, though.)

/Magnus



Bob.



/Magnus


Bob.



On Feb 26, 2020, at 10:35 AM, Magnus Ihse Bursie 
 wrote:

On 2020-02-26 15:56, Bob Vandette wrote:

On Feb 26, 2020, at 9:17 AM, Magnus Ihse Bursie  
wrote:

On 2020-02-26 14:31, Bob Vandette wrote:

On Feb 26, 2020, at 7:31 AM, Magnus Ihse Bursie  
wrote:

On 2020-02-26 08:41, David Holmes wrote:

Hi Bob,

Adding build-dev.

Thanks for noticing that we were missing, David!

Sorry, I should have included you folks.


On 26/02/2020 6:37 am, Bob Vandette wrote:

Please review this RFE that alters the visibility of JNI entrypoints to hidden 
when a shared library
is created using static JDK libraries.

RFE:

https://bugs.openjdk.java.net/browse/JDK-8239563

WEBREV:

http://cr.openjdk.java.net/~bobv/8239563/webrev.00/

CSR:

https://bugs.openjdk.java.net/browse/JDK-8239791

All JNI entrypoints that exist in JDK static libraries are declared as exported 
or visible.
If a dynamic library is built from these static libraries, we end up with many 
exported
functions that we don't want to provide access to,

This RFE will change the definition of JNIEXPORT for libraries built when 
JNI_STATIC_BUILD
is defined.  When defined, functions declared with JNIEXPORT will not be 
exported when
linked into shared or dynamic libraries.  This will still allow linking of 
these functions into
dynamic libraries but will not export the JDK symbols outside of the shared 
library.

A CSR has been filed (https://bugs.openjdk.java.net/browse/JDK-8239791) to add 
the JNI_STATIC_BUILD
define support in jni.h.

I have reservations about turning this into something we have to expose and 
support, rather than being something totally handled within the OpenJDK build 
system.

I fully agree. The JNI headers are an exported interface. Our internal build 
mechanisms have nothing to do there.


I'm tempted to suggest the header files be adjusted to have:

 #ifndef JNIEXPORT
 
 #endif

and then we define the modified JNIEXPORT via the build system when doing a 
static build.

Is that feasible?

It's definitely doable, and a far better solution.

Yes, I like this solution.


A patch something akin to this would be needed:

diff --git a/make/autoconf/flags-cflags.m4 b/make/autoconf/flags-cflags.m4
--- a/make/autoconf/flags-cflags.m4
+++ b/make/autoconf/flags-cflags.m4
@@ -709,7 +709,10 @@
# JDK libraries.
STATIC_LIBS_CFLAGS="-DSTATIC_BUILD=1"
if test "x$TOOLCHAIN_TYPE" = xgcc || test "x$TOOLCHAIN_TYPE" = xclang; then
-STATIC_LIBS_CFLAGS="$STATIC_LIBS_CFLAGS -ffunction-sections 
-fdata-sections"
+STATIC_LIBS_CFLAGS="$STATIC_LIBS_CFLAGS -ffunction-sections \
+-fdata-sections -DJNIEXPORT=__attribute__((visibility(\"hidden\")))"
+  else
+STATIC_LIBS_CFLAGS="$STATIC_LIBS_CFLAGS -DJNIEXPORT="
fi
if test "x$TOOLCHAIN_TYPE" = xgcc; then
  # Disable relax-relocation to enable compatibility with older linkers

(With the reservation that I just wrote this on the fly and have not tested it 
-- things like quoting might be off. Also, I'm not sure if the match of
compilers is correct -- it might be the case that all compilers except 
Microsoft defines __GNUC__, so maybe the addition of this -D flag might need
a separate if statement to cover all our compilers correctly.)

Most of the STATIC_BUILD support is done in jni_util.h.  We could define 
JNIEXPORT in that header file after allowing it to be overridden in jni.h.

I'm not sure I understand you correctly here. Do yo

Re: RFR: 8239563 - Reduce public exports in dynamic libraries built from static JDK libraries

[Magnus Ihse Bursie]

On 2020-02-26 22:01, Bob Vandette wrote:
Here’s an updated webrev that implementes the suggestion that allows 
JNIEXPORT in jni.h to be overridden

and the build limits visibility for static libraries.

If this webrev is accepted, I’ll update the CSR solution to match this 
implementation.


http://cr.openjdk.java.net/~bobv/8239563/webrev.01

This looks basically ok, but some remarks:

You have forgotten to update the copyright year in the header files.

Also, the quoting looks suspicious. I would have guessed, thinking more 
carefully about this than the post yesterday, that the proper syntax 
would be -DJNIEXPORT='__attribute__((visibility("hidden")))' since 
otherwise the ' will make the \ literal. But, I usually end up being 
wrong about 50% of the time regarding this kind of stuff. :-) Have you 
verified that you get the proper define?


/Magnus



Bob.


On Feb 26, 2020, at 10:35 AM, Magnus Ihse Bursie 
<mailto:magnus.ihse.bur...@oracle.com>> wrote:


On 2020-02-26 15:56, Bob Vandette wrote:


On Feb 26, 2020, at 9:17 AM, Magnus Ihse Bursie 
<mailto:magnus.ihse.bur...@oracle.com>> wrote:


On 2020-02-26 14:31, Bob Vandette wrote:
On Feb 26, 2020, at 7:31 AM, Magnus Ihse Bursie 
<mailto:magnus.ihse.bur...@oracle.com>> wrote:


On 2020-02-26 08:41, David Holmes wrote:

Hi Bob,

Adding build-dev.

Thanks for noticing that we were missing, David!

Sorry, I should have included you folks.


On 26/02/2020 6:37 am, Bob Vandette wrote:
Please review this RFE that alters the visibility of JNI 
entrypoints to hidden when a shared library

is created using static JDK libraries.

RFE:

https://bugs.openjdk.java.net/browse/JDK-8239563

WEBREV:

http://cr.openjdk.java.net/~bobv/8239563/webrev.00/

CSR:

https://bugs.openjdk.java.net/browse/JDK-8239791

All JNI entrypoints that exist in JDK static libraries are 
declared as exported or visible.
If a dynamic library is built from these static libraries, we 
end up with many exported

functions that we don't want to provide access to,

This RFE will change the definition of JNIEXPORT for libraries 
built when JNI_STATIC_BUILD
is defined.  When defined, functions declared with JNIEXPORT 
will not be exported when
linked into shared or dynamic libraries.  This will still allow 
linking of these functions into
dynamic libraries but will not export the JDK symbols outside 
of the shared library.


A CSR has been filed 
(https://bugs.openjdk.java.net/browse/JDK-8239791) to add the 
JNI_STATIC_BUILD

define support in jni.h.
I have reservations about turning this into something we have to 
expose and support, rather than being something totally handled 
within the OpenJDK build system.
I fully agree. The JNI headers are an exported interface. Our 
internal build mechanisms have nothing to do there.



I'm tempted to suggest the header files be adjusted to have:

#ifndef JNIEXPORT

#endif

and then we define the modified JNIEXPORT via the build system 
when doing a static build.


Is that feasible?

It's definitely doable, and a far better solution.

Yes, I like this solution.


A patch something akin to this would be needed:

diff --git a/make/autoconf/flags-cflags.m4 
b/make/autoconf/flags-cflags.m4

--- a/make/autoconf/flags-cflags.m4
+++ b/make/autoconf/flags-cflags.m4
@@ -709,7 +709,10 @@
   # JDK libraries.
   STATIC_LIBS_CFLAGS="-DSTATIC_BUILD=1"
   if test "x$TOOLCHAIN_TYPE" = xgcc || test "x$TOOLCHAIN_TYPE" = 
xclang; then
-    STATIC_LIBS_CFLAGS="$STATIC_LIBS_CFLAGS -ffunction-sections 
-fdata-sections"

+    STATIC_LIBS_CFLAGS="$STATIC_LIBS_CFLAGS -ffunction-sections \
+    -fdata-sections 
-DJNIEXPORT=__attribute__((visibility(\"hidden\")))"

+  else
+    STATIC_LIBS_CFLAGS="$STATIC_LIBS_CFLAGS -DJNIEXPORT="
   fi
   if test "x$TOOLCHAIN_TYPE" = xgcc; then
 # Disable relax-relocation to enable compatibility with 
older linkers


(With the reservation that I just wrote this on the fly and have 
not tested it -- things like quoting might be off. Also, I'm not 
sure if the match of
compilers is correct -- it might be the case that all compilers 
except Microsoft defines __GNUC__, so maybe the addition of this 
-D flag might need

a separate if statement to cover all our compilers correctly.)
Most of the STATIC_BUILD support is done in jni_util.h.  We could 
define JNIEXPORT in that header file after allowing it to be 
overridden in jni.h.
I'm not sure I understand you correctly here. Do you mean that 
you'd like to re-define JNIEXPORT inside jni_util.h instead of 
using compiler command line flags? I don't think that'd work -- all 
libraries using JNIEXPORT that does not include jni_util.h first 
would then export their symbols just the same. Even if you fixed 
those, the system would be very fragile.
I was just trying to keep all static library building options in one 
place.  The static libraries that we produce need 

Re: RFR: 8239563 - Reduce public exports in dynamic libraries built from static JDK libraries

[Magnus Ihse Bursie]

On 2020-02-27 00:31, David Holmes wrote:

Hi Bob,

On 27/02/2020 7:01 am, Bob Vandette wrote:
Here’s an updated webrev that implementes the suggestion that allows 
JNIEXPORT in jni.h to be overridden

and the build limits visibility for static libraries.


!   #if (defined(__GNUC__) && ((__GNUC__ > 4) || (__GNUC__ == 4) && 
(__GNUC_MINOR__ > 2))) || __has_attribute(visibility)


Don't we have minimum gcc version requirement (>4.2) that negates the 
need for this explicit check now? Magnus?
You are forgetting that this file is not really for us, it's part of our 
exported interface in jni.h. (And in fact, as I have stated before, I 
think we are making a mistake in using the defines from jni.h for our 
internal purposes.) This file is consumed by all Java developers who are 
using JNI. And they might have any kind of compiler.


/Magnus


If this webrev is accepted, I’ll update the CSR solution to match 
this implementation.


I'm not even sure a CSR request is even warranted now.

Thanks,
David


http://cr.openjdk.java.net/~bobv/8239563/webrev.01

Bob.


On Feb 26, 2020, at 10:35 AM, Magnus Ihse Bursie 
<mailto:magnus.ihse.bur...@oracle.com>> wrote:


On 2020-02-26 15:56, Bob Vandette wrote:


On Feb 26, 2020, at 9:17 AM, Magnus Ihse Bursie 
<mailto:magnus.ihse.bur...@oracle.com>> wrote:


On 2020-02-26 14:31, Bob Vandette wrote:
On Feb 26, 2020, at 7:31 AM, Magnus Ihse Bursie 
<mailto:magnus.ihse.bur...@oracle.com>> wrote:


On 2020-02-26 08:41, David Holmes wrote:

Hi Bob,

Adding build-dev.

Thanks for noticing that we were missing, David!

Sorry, I should have included you folks.


On 26/02/2020 6:37 am, Bob Vandette wrote:
Please review this RFE that alters the visibility of JNI 
entrypoints to hidden when a shared library

is created using static JDK libraries.

RFE:

https://bugs.openjdk.java.net/browse/JDK-8239563

WEBREV:

http://cr.openjdk.java.net/~bobv/8239563/webrev.00/

CSR:

https://bugs.openjdk.java.net/browse/JDK-8239791

All JNI entrypoints that exist in JDK static libraries are 
declared as exported or visible.
If a dynamic library is built from these static libraries, we 
end up with many exported

functions that we don't want to provide access to,

This RFE will change the definition of JNIEXPORT for libraries 
built when JNI_STATIC_BUILD
is defined.  When defined, functions declared with JNIEXPORT 
will not be exported when
linked into shared or dynamic libraries.  This will still 
allow linking of these functions into
dynamic libraries but will not export the JDK symbols outside 
of the shared library.


A CSR has been filed 
(https://bugs.openjdk.java.net/browse/JDK-8239791) to add the 
JNI_STATIC_BUILD

define support in jni.h.
I have reservations about turning this into something we have 
to expose and support, rather than being something totally 
handled within the OpenJDK build system.
I fully agree. The JNI headers are an exported interface. Our 
internal build mechanisms have nothing to do there.



I'm tempted to suggest the header files be adjusted to have:

#ifndef JNIEXPORT

#endif

and then we define the modified JNIEXPORT via the build system 
when doing a static build.


Is that feasible?

It's definitely doable, and a far better solution.

Yes, I like this solution.


A patch something akin to this would be needed:

diff --git a/make/autoconf/flags-cflags.m4 
b/make/autoconf/flags-cflags.m4

--- a/make/autoconf/flags-cflags.m4
+++ b/make/autoconf/flags-cflags.m4
@@ -709,7 +709,10 @@
   # JDK libraries.
   STATIC_LIBS_CFLAGS="-DSTATIC_BUILD=1"
   if test "x$TOOLCHAIN_TYPE" = xgcc || test "x$TOOLCHAIN_TYPE" 
= xclang; then
-    STATIC_LIBS_CFLAGS="$STATIC_LIBS_CFLAGS -ffunction-sections 
-fdata-sections"

+    STATIC_LIBS_CFLAGS="$STATIC_LIBS_CFLAGS -ffunction-sections \
+    -fdata-sections 
-DJNIEXPORT=__attribute__((visibility(\"hidden\")))"

+  else
+    STATIC_LIBS_CFLAGS="$STATIC_LIBS_CFLAGS -DJNIEXPORT="
   fi
   if test "x$TOOLCHAIN_TYPE" = xgcc; then
 # Disable relax-relocation to enable compatibility with 
older linkers


(With the reservation that I just wrote this on the fly and have 
not tested it -- things like quoting might be off. Also, I'm not 
sure if the match of
compilers is correct -- it might be the case that all compilers 
except Microsoft defines __GNUC__, so maybe the addition of this 
-D flag might need

a separate if statement to cover all our compilers correctly.)
Most of the STATIC_BUILD support is done in jni_util.h.  We could 
define JNIEXPORT in that header file after allowing it to be 
overridden in jni.h.
I'm not sure I understand you correctly here. Do you mean that 
you'd like to re-define JNIEXPORT inside jni_util.h instead of 
using compiler command line flags? I don't think that'd work -- 
all libraries using JNIEXPORT that does not include jni_util.h 
first would then export t

Re: RFR: 8239563 - Reduce public exports in dynamic libraries built from static JDK libraries

[Magnus Ihse Bursie]

On 2020-02-26 15:56, Bob Vandette wrote:



On Feb 26, 2020, at 9:17 AM, Magnus Ihse Bursie  
wrote:

On 2020-02-26 14:31, Bob Vandette wrote:

On Feb 26, 2020, at 7:31 AM, Magnus Ihse Bursie  
wrote:

On 2020-02-26 08:41, David Holmes wrote:

Hi Bob,

Adding build-dev.

Thanks for noticing that we were missing, David!

Sorry, I should have included you folks.


On 26/02/2020 6:37 am, Bob Vandette wrote:

Please review this RFE that alters the visibility of JNI entrypoints to hidden 
when a shared library
is created using static JDK libraries.

RFE:

https://bugs.openjdk.java.net/browse/JDK-8239563

WEBREV:

http://cr.openjdk.java.net/~bobv/8239563/webrev.00/

CSR:

https://bugs.openjdk.java.net/browse/JDK-8239791

All JNI entrypoints that exist in JDK static libraries are declared as exported 
or visible.
If a dynamic library is built from these static libraries, we end up with many 
exported
functions that we don't want to provide access to,

This RFE will change the definition of JNIEXPORT for libraries built when 
JNI_STATIC_BUILD
is defined.  When defined, functions declared with JNIEXPORT will not be 
exported when
linked into shared or dynamic libraries.  This will still allow linking of 
these functions into
dynamic libraries but will not export the JDK symbols outside of the shared 
library.

A CSR has been filed (https://bugs.openjdk.java.net/browse/JDK-8239791) to add 
the JNI_STATIC_BUILD
define support in jni.h.

I have reservations about turning this into something we have to expose and 
support, rather than being something totally handled within the OpenJDK build 
system.

I fully agree. The JNI headers are an exported interface. Our internal build 
mechanisms have nothing to do there.


I'm tempted to suggest the header files be adjusted to have:

 #ifndef JNIEXPORT
 
 #endif

and then we define the modified JNIEXPORT via the build system when doing a 
static build.

Is that feasible?

It's definitely doable, and a far better solution.

Yes, I like this solution.


A patch something akin to this would be needed:

diff --git a/make/autoconf/flags-cflags.m4 b/make/autoconf/flags-cflags.m4
--- a/make/autoconf/flags-cflags.m4
+++ b/make/autoconf/flags-cflags.m4
@@ -709,7 +709,10 @@
# JDK libraries.
STATIC_LIBS_CFLAGS="-DSTATIC_BUILD=1"
if test "x$TOOLCHAIN_TYPE" = xgcc || test "x$TOOLCHAIN_TYPE" = xclang; then
-STATIC_LIBS_CFLAGS="$STATIC_LIBS_CFLAGS -ffunction-sections 
-fdata-sections"
+STATIC_LIBS_CFLAGS="$STATIC_LIBS_CFLAGS -ffunction-sections \
+-fdata-sections -DJNIEXPORT=__attribute__((visibility(\"hidden\")))"
+  else
+STATIC_LIBS_CFLAGS="$STATIC_LIBS_CFLAGS -DJNIEXPORT="
fi
if test "x$TOOLCHAIN_TYPE" = xgcc; then
  # Disable relax-relocation to enable compatibility with older linkers

(With the reservation that I just wrote this on the fly and have not tested it 
-- things like quoting might be off. Also, I'm not sure if the match of
compilers is correct -- it might be the case that all compilers except 
Microsoft defines __GNUC__, so maybe the addition of this -D flag might need
a separate if statement to cover all our compilers correctly.)

Most of the STATIC_BUILD support is done in jni_util.h.  We could define 
JNIEXPORT in that header file after allowing it to be overridden in jni.h.

I'm not sure I understand you correctly here. Do you mean that you'd like to 
re-define JNIEXPORT inside jni_util.h instead of using compiler command line 
flags? I don't think that'd work -- all libraries using JNIEXPORT that does not 
include jni_util.h first would then export their symbols just the same. Even if 
you fixed those, the system would be very fragile.

I was just trying to keep all static library building options in one place.  
The static libraries that we produce need to include jni_util.h
or the JNI_OnLoad_xxx functions will not be declared properly.  Why not expand 
that dependency to the JNIEXPORT?
Unless *all* libraries that include jni.h also include jni_util.h, then 
the current definition of JNIEXPORT in jni.h will be used -- meaning 
that the so decorated functions will be exported -- which was exactly 
what you wanted to prevent. So I fail to see how this can be a solution.


Do we really have access to all of these compiler defines from within our 
Makefiles?

#if (defined(__GNUC__) && ((__GNUC__ > 4) || (__GNUC__ == 4) && (__GNUC_MINOR__ 
> 2))) || __has_attribute(visibility)
Well, yes and no. I'm not certain which compilers define __GNUC__ just 
to show compatibility with gcc, but otoh that does not really matter. 
All that matters is that we know how we want JNIEXPORT to be defined 
when creating a static build -- and that we know, since we can check 
which toolchain we're using. (This is btw a far better check than to 
look for __GNUC__).


/Magnus



Bob.



/Magnus

Bob.


/Magnus

Thanks,
David


BACKGROUN

Re: RFR: 8239563 - Reduce public exports in dynamic libraries built from static JDK libraries

[Magnus Ihse Bursie]

On 2020-02-26 14:31, Bob Vandette wrote:



On Feb 26, 2020, at 7:31 AM, Magnus Ihse Bursie  
wrote:

On 2020-02-26 08:41, David Holmes wrote:

Hi Bob,

Adding build-dev.

Thanks for noticing that we were missing, David!

Sorry, I should have included you folks.


On 26/02/2020 6:37 am, Bob Vandette wrote:

Please review this RFE that alters the visibility of JNI entrypoints to hidden 
when a shared library
is created using static JDK libraries.

RFE:

https://bugs.openjdk.java.net/browse/JDK-8239563

WEBREV:

http://cr.openjdk.java.net/~bobv/8239563/webrev.00/

CSR:

https://bugs.openjdk.java.net/browse/JDK-8239791

All JNI entrypoints that exist in JDK static libraries are declared as exported 
or visible.
If a dynamic library is built from these static libraries, we end up with many 
exported
functions that we don't want to provide access to,

This RFE will change the definition of JNIEXPORT for libraries built when 
JNI_STATIC_BUILD
is defined.  When defined, functions declared with JNIEXPORT will not be 
exported when
linked into shared or dynamic libraries.  This will still allow linking of 
these functions into
dynamic libraries but will not export the JDK symbols outside of the shared 
library.

A CSR has been filed (https://bugs.openjdk.java.net/browse/JDK-8239791) to add 
the JNI_STATIC_BUILD
define support in jni.h.

I have reservations about turning this into something we have to expose and 
support, rather than being something totally handled within the OpenJDK build 
system.

I fully agree. The JNI headers are an exported interface. Our internal build 
mechanisms have nothing to do there.


I'm tempted to suggest the header files be adjusted to have:

 #ifndef JNIEXPORT
 
 #endif

and then we define the modified JNIEXPORT via the build system when doing a 
static build.

Is that feasible?

It's definitely doable, and a far better solution.

Yes, I like this solution.


A patch something akin to this would be needed:

diff --git a/make/autoconf/flags-cflags.m4 b/make/autoconf/flags-cflags.m4
--- a/make/autoconf/flags-cflags.m4
+++ b/make/autoconf/flags-cflags.m4
@@ -709,7 +709,10 @@
# JDK libraries.
STATIC_LIBS_CFLAGS="-DSTATIC_BUILD=1"
if test "x$TOOLCHAIN_TYPE" = xgcc || test "x$TOOLCHAIN_TYPE" = xclang; then
-STATIC_LIBS_CFLAGS="$STATIC_LIBS_CFLAGS -ffunction-sections 
-fdata-sections"
+STATIC_LIBS_CFLAGS="$STATIC_LIBS_CFLAGS -ffunction-sections \
+-fdata-sections -DJNIEXPORT=__attribute__((visibility(\"hidden\")))"
+  else
+STATIC_LIBS_CFLAGS="$STATIC_LIBS_CFLAGS -DJNIEXPORT="
fi
if test "x$TOOLCHAIN_TYPE" = xgcc; then
  # Disable relax-relocation to enable compatibility with older linkers

(With the reservation that I just wrote this on the fly and have not tested it 
-- things like quoting might be off. Also, I'm not sure if the match of
compilers is correct -- it might be the case that all compilers except 
Microsoft defines __GNUC__, so maybe the addition of this -D flag might need
a separate if statement to cover all our compilers correctly.)

Most of the STATIC_BUILD support is done in jni_util.h.  We could define 
JNIEXPORT in that header file after allowing it to be overridden in jni.h.
I'm not sure I understand you correctly here. Do you mean that you'd 
like to re-define JNIEXPORT inside jni_util.h instead of using compiler 
command line flags? I don't think that'd work -- all libraries using 
JNIEXPORT that does not include jni_util.h first would then export their 
symbols just the same. Even if you fixed those, the system would be very 
fragile.


/Magnus


Bob.


/Magnus

Thanks,
David


BACKGROUND:

In JDK8 the JNI specification and JDK implementation was enhanced to support 
static JNI libraries
but we didn’t consider the issue of exportibility of JNI entrypoint symbols.

https://bugs.openjdk.java.net/browse/JDK-8005716

If developers use these static JDK libraries in order to produce a custom 
shared library, all of the
JNIEXPORTS will be exposed by this library even if the developer did not choose 
to export these.
This is a security issue and a potential problem if this library is mixed with 
other libraries containing
these symbols.

Bob.

Re: RFR: 8239563 - Reduce public exports in dynamic libraries built from static JDK libraries

[Magnus Ihse Bursie]

Also, we've worked hard to get rid of the map files in the build system. 
I'd be hesitant to say the least to re-introduce them.


/Magnus

On 2020-02-26 14:29, Bob Vandette wrote:

Thanks but I don’t like that idea for several reasons.

1. It’s too dramatic of a change for the immediate problem I’m trying to solve.

2. It creates a support issue.  Every time a new native function is added or 
removed, we’d have several files
that would need to be updated (1 per OS type).

3. How do we know which functions to export.  The VM sometimes needs to get 
access to internal native
functions that are not accessible via java native methods.

I prefer the option where we can override the definition of JNIEXPORT.

Bob.



On Feb 26, 2020, at 7:36 AM, Andrew Dinn  wrote:

Hi Bob,

On 25/02/2020 20:37, Bob Vandette wrote:

Please review this RFE that alters the visibility of JNI entrypoints to hidden 
when a shared library
is created using static JDK libraries.

As David Holmes mentions in his ocmment on the JIRA it is possible to
control re-export of the JNI entrypoints in the shared library using a
linker map. Indeed, OpenJDK does this to limit visibility of entrypoints
to libjvm.

Is there a reason why this is not a viable alternative to changing the
definition of the JNIEXPORT macros?

regards,


Andrew Dinn
---

Re: RFR: 8239563 - Reduce public exports in dynamic libraries built from static JDK libraries

[Magnus Ihse Bursie]

On 2020-02-26 08:41, David Holmes wrote:

Hi Bob,

Adding build-dev.

Thanks for noticing that we were missing, David!



On 26/02/2020 6:37 am, Bob Vandette wrote:


Please review this RFE that alters the visibility of JNI entrypoints 
to hidden when a shared library

is created using static JDK libraries.

RFE:

https://bugs.openjdk.java.net/browse/JDK-8239563

WEBREV:

http://cr.openjdk.java.net/~bobv/8239563/webrev.00/

CSR:

https://bugs.openjdk.java.net/browse/JDK-8239791

All JNI entrypoints that exist in JDK static libraries are declared 
as exported or visible.
If a dynamic library is built from these static libraries, we end up 
with many exported

functions that we don't want to provide access to,

This RFE will change the definition of JNIEXPORT for libraries built 
when JNI_STATIC_BUILD
is defined.  When defined, functions declared with JNIEXPORT will not 
be exported when
linked into shared or dynamic libraries.  This will still allow 
linking of these functions into
dynamic libraries but will not export the JDK symbols outside of the 
shared library.


A CSR has been filed 
(https://bugs.openjdk.java.net/browse/JDK-8239791) to add the 
JNI_STATIC_BUILD

define support in jni.h.


I have reservations about turning this into something we have to 
expose and support, rather than being something totally handled within 
the OpenJDK build system.
I fully agree. The JNI headers are an exported interface. Our internal 
build mechanisms have nothing to do there.



I'm tempted to suggest the header files be adjusted to have:

    #ifndef JNIEXPORT
    
    #endif

and then we define the modified JNIEXPORT via the build system when 
doing a static build.


Is that feasible?

It's definitely doable, and a far better solution.

A patch something akin to this would be needed:

diff --git a/make/autoconf/flags-cflags.m4 b/make/autoconf/flags-cflags.m4
--- a/make/autoconf/flags-cflags.m4
+++ b/make/autoconf/flags-cflags.m4
@@ -709,7 +709,10 @@
   # JDK libraries.
   STATIC_LIBS_CFLAGS="-DSTATIC_BUILD=1"
   if test "x$TOOLCHAIN_TYPE" = xgcc || test "x$TOOLCHAIN_TYPE" = 
xclang; then
-    STATIC_LIBS_CFLAGS="$STATIC_LIBS_CFLAGS -ffunction-sections 
-fdata-sections"

+    STATIC_LIBS_CFLAGS="$STATIC_LIBS_CFLAGS -ffunction-sections \
+    -fdata-sections 
-DJNIEXPORT=__attribute__((visibility(\"hidden\")))"

+  else
+    STATIC_LIBS_CFLAGS="$STATIC_LIBS_CFLAGS -DJNIEXPORT="
   fi
   if test "x$TOOLCHAIN_TYPE" = xgcc; then
 # Disable relax-relocation to enable compatibility with older linkers

(With the reservation that I just wrote this on the fly and have not 
tested it -- things like quoting might be off. Also, I'm not sure if the 
match of
compilers is correct -- it might be the case that all compilers except 
Microsoft defines __GNUC__, so maybe the addition of this -D flag might 
need

a separate if statement to cover all our compilers correctly.)

/Magnus


Thanks,
David


BACKGROUND:

In JDK8 the JNI specification and JDK implementation was enhanced to 
support static JNI libraries
but we didn’t consider the issue of exportibility of JNI entrypoint 
symbols.


   https://bugs.openjdk.java.net/browse/JDK-8005716

If developers use these static JDK libraries in order to produce a 
custom shared library, all of the
JNIEXPORTS will be exposed by this library even if the developer did 
not choose to export these.
This is a security issue and a potential problem if this library is 
mixed with other libraries containing

these symbols.

Bob.

Re: RFR: JDK-8239139 testmake fail with warning about strncpy using gcc version 8

[Magnus Ihse Bursie]

On 2020-02-17 08:48, linzang(臧琳) wrote:

Hi,
  May I ask help to review one tiny fix of build failure described at 
https://bugs.openjdk.java.net/browse/JDK-8239139
  Root cause is gcc version 8 prints warning for strncpy.
  The fix simply replace strncpy with snprintf.
  Thanks!
  Bug: https://bugs.openjdk.java.net/browse/JDK-8239139
  webrev: http://cr.openjdk.java.net/~lzang/8239139/webrev/

BRs,
Lin


Hi Lin,

This is not a build issue. I'm redirecting this to core-libs.

/Magnus

Re: RFR: JDK-8238225: Issues reported after replacing symlink at Contents/MacOS/libjli.dylib with binary

[Magnus Ihse Bursie]

On 2020-02-05 16:32, Erik Joelsson wrote:

Hello,

New webrev: http://cr.openjdk.java.net/~erikj/8238225/webrev.02/

Looks good.

/Magnus


On 2020-02-05 02:17, Langer, Christoph wrote:

Hi,

we've tested the patch and all reported failure scenarios we're aware 
of are fixed with that, so basically, ship it 


As for the code review part:
The patch I've tested had removed the "-1" in line 532, so that seems 
to be correct. As Magnus wrote, the pattern seems to be copied from 
the lines above. So I think in line 518, subtracting -1 from 
"sizeOfLastPathComponent" seems to be incorrect as well. So it could 
be corrected in the same fix, I guess.
Yes, I did indeed just copy the pattern, but it also seems you are 
right about the -1, so I fixed it in both locations. I also figured 
reusing the variables wasn't very nice, so add the "Alt" prefix in all 
of them.
And there's one other minor thing: I tried to execute JliLaunchTest 
for the bundle scenario and had to make some adaptions to the example 
call to "make test-only ..." in line 66 of 
test/jdk/tools/launcher/JliLaunchTest.java. I guess the example could 
be more generic if it were changed to:


66 // $ make test-only 
TEST=test/jdk/tools/launcher/JliLaunchTest.java \
67 // 
JDK_IMAGE_DIR=$PWD/images/jdk-bundle/jdk-15.jdk/Contents/Home


(e.g. use relative paths inside the build directory)


Right, the name of the output dir may change. I didn't intend the 
example to be verbatim correct for everyone (hence the "something 
like" wording) but I see your point. Changed it.


/Erik


Thanks
Christoph


-Original Message-
From: Magnus Ihse Bursie 
Sent: Mittwoch, 5. Februar 2020 10:54
To: Erik Joelsson ; core-libs-dev ; build-dev ; Langer,
Christoph 
Subject: Re: RFR: JDK-8238225: Issues reported after replacing 
symlink at

Contents/MacOS/libjli.dylib with binary

On 2020-02-04 18:45, Erik Joelsson wrote:

Does anyone have an opinion on this?

The solution seems sound to me. I'm having a hard time figuring out if
the string manipulations in java_md_macosx.m are correct; as Christoph
is saying, they might not be. I realize you have copied an existing
pattern, and is likely subject to constraints, but that does not 
make it

easier to read.

/Magnus

/Erik

On 2020-01-31 07:31, Erik Joelsson wrote:

In JDK-8235687 the MacOS bundle distribution of the JDK was changed
to conform to Apple requirements by changing
Contents/MacOS/libjli.dylib from a symlink into
../Home/lib/libjli.dylib to a copy of that file. The problem with
having a symlink there is that Contents/MacOS/libjli.dylib is the
declared CFBundleExecutable of the bundle and that executable may not
be a symlink. The history around why that particular library was put
there seems lost in ancient history. All we know is that it was there
when Apple donated the Mac port and according to this bug report,
there are users out there relying on it.

When changing Contents/MacOS/libjli.dylib to a copy, loading that
dylib and using it to launch a JVM no longer works. This patch fixes
that by making libjli.dylib aware of potentially being located there
and if so, finding the JDK home dir in ../Home.

I've also expanded the existing test for launching a JVM through
libjli.dylib directly to also test this location when possible. In
local testing, this will not be covered unless the user explicitly
specifies that the JDK under test should be the bundle image on the
command line like this:

$ make test-only TEST=open/test/jdk/tools/launcher/JliLaunchTest.java
JDK_IMAGE_DIR=$PWD/build/macosx-x64/images/jdk-bundle/jdk-

15.jdk/Contents/Home

But, at least in Oracle's distributed testing, the JDK on MacOS is
distributed in the bundle layout, so there this functionality will be
tested.

Bug: https://bugs.openjdk.java.net/browse/JDK-8238225

Webrev: http://cr.openjdk.java.net/~erikj/8238225/webrev.01/

/Erik

Re: RFR: JDK-8238225: Issues reported after replacing symlink at Contents/MacOS/libjli.dylib with binary

[Magnus Ihse Bursie]

On 2020-02-04 18:45, Erik Joelsson wrote:

Does anyone have an opinion on this?
The solution seems sound to me. I'm having a hard time figuring out if 
the string manipulations in java_md_macosx.m are correct; as Christoph 
is saying, they might not be. I realize you have copied an existing 
pattern, and is likely subject to constraints, but that does not make it 
easier to read.


/Magnus


/Erik

On 2020-01-31 07:31, Erik Joelsson wrote:
In JDK-8235687 the MacOS bundle distribution of the JDK was changed 
to conform to Apple requirements by changing 
Contents/MacOS/libjli.dylib from a symlink into 
../Home/lib/libjli.dylib to a copy of that file. The problem with 
having a symlink there is that Contents/MacOS/libjli.dylib is the 
declared CFBundleExecutable of the bundle and that executable may not 
be a symlink. The history around why that particular library was put 
there seems lost in ancient history. All we know is that it was there 
when Apple donated the Mac port and according to this bug report, 
there are users out there relying on it.


When changing Contents/MacOS/libjli.dylib to a copy, loading that 
dylib and using it to launch a JVM no longer works. This patch fixes 
that by making libjli.dylib aware of potentially being located there 
and if so, finding the JDK home dir in ../Home.


I've also expanded the existing test for launching a JVM through 
libjli.dylib directly to also test this location when possible. In 
local testing, this will not be covered unless the user explicitly 
specifies that the JDK under test should be the bundle image on the 
command line like this:


$ make test-only TEST=open/test/jdk/tools/launcher/JliLaunchTest.java 
JDK_IMAGE_DIR=$PWD/build/macosx-x64/images/jdk-bundle/jdk-15.jdk/Contents/Home


But, at least in Oracle's distributed testing, the JDK on MacOS is 
distributed in the bundle layout, so there this functionality will be 
tested.


Bug: https://bugs.openjdk.java.net/browse/JDK-8238225

Webrev: http://cr.openjdk.java.net/~erikj/8238225/webrev.01/

/Erik

Re: 14 RFR (M) 8232080: jlink plugins for vendor information and run-time options

[Magnus Ihse Bursie]

On 2019-10-22 05:22, mark.reinh...@oracle.com wrote:

RFE: https://bugs.openjdk.java.net/browse/JDK-8232080
CSR: https://bugs.openjdk.java.net/browse/JDK-8232753
Webrev: https://cr.openjdk.java.net/~mr/rev/8232080/

Build changes look good.

/Magnus


This change implements jlink plugins, and associated changes in the VM
and libraries, to support the following new jlink options:

   - --vendor-bug-url= overrides the vendor bug URL baked
 into the build.  The value of the system property "java.vendor.url.bug"
 will be .

   - --vendor-vm-bug-url= overrides the vendor VM bug
 URL baked into the build.  This value will be displayed in VM error
 logs.

   - --vendor-version= overrides the vendor version string
 baked into the build, if any.  The value of the system property
 "java.vendor.version" will be .  This value will be
 displayed in the output of java --version.

   - --add-options= prepends the specified  string,
 which may include whitespace, before any other options when invoking
 the VM in the resulting image.

The vendor-information plugins work by using the JDK’s internal ASM
library to redefine static fields in the java.lang.VersionProps class.
The VM reads the vendor-version and vendor-vm-bug-url strings from that
class during startup.

The add-options plugin works by storing the requested options in an
internal resource, /java.base/jdk/internal/vm/options.  The VM loads that
resource from the lib/modules jimage file during startup and prepends any
options found there to those given on the command line.

Passes tier1-3 and JCK on {linux,macos,windows}-x64.

Thanks,
- Mark

Re: RFR: JDK-8226585: Improve javac messages for using a preview API

[Magnus Ihse Bursie]

On 2019-10-16 14:55, Erik Joelsson wrote:

Build change looks good now.

I agree.

/Magnus


/Erik

On 2019-10-16 05:50, Jan Lahoda wrote:

Hi,

An updated patch is here:
http://cr.openjdk.java.net/~jlahoda/8226585/webrev.02/

Changes in the update:
-added the dependency into the makefiles
-loosened the handling of essential preview APIs when 
--enable-preview and @SuppressWarnings is applied - there is no 
warning for the essential APIs (as there is no warning in such a case 
for non-essential APIs). This is per the discussion in the CSR:

https://bugs.openjdk.java.net/browse/JDK-8231411

Any comments/feedback on this?

Thanks!
    Jan

On 09. 10. 19 17:41, Erik Joelsson wrote:

Oh, you are absolutely correct, the dependency is missing.

We need something like this inside "define SetupInterimModule":

$$(BUILD_$1.interim): $(COPY_PREVIEW_FEATURES)

/Erik

On 2019-10-09 01:42, Magnus Ihse Bursie wrote:
I can’t see how the compilation is dependent on the copy being 
finished. Since Erik contributed this it will probably be correct 
:) but I’d appreciate an explanation on how this dependency is 
guaranteed.


Or maybe I’m misunderstanding what this is supposed to do?

/Magnus


8 okt. 2019 kl. 17:27 skrev Jan Lahoda :

Thanks for the new code Erik!

A new webrev/patch that includes this better way of copying is here:
http://cr.openjdk.java.net/~jlahoda/8226585/webrev.01/

Any feedback is welcome!

Thanks,
    Jan


On 03. 10. 19 18:06, Erik Joelsson wrote:
Hello Jan,
The build change looks ok, but I would recommend this construct 
for copying the file instead:

$(eval $(call SetupCopyFiles, COPY_PREVIEW_FEATURES, \
 FILES := 
$(TOPDIR)/src/java.base/share/classes/jdk/internal/PreviewFeature.java, 
\
 DEST := 
$(BUILDTOOLS_OUTPUTDIR)/gensrc/java.base.interim/jdk/internal/PreviewFeature.java, 
\

))
TARGETS += $(COPY_PREVIEW_FEATURES)
Then you automatically get all the corner case handling we have 
implemented over the years for logging, making directories and 
copying files. Your version is still correct for this case though.

/Erik

On 2019-10-03 02:57, Jan Lahoda wrote:
Hi,

This is a continuation of Joe's patch from here:
https://mail.openjdk.java.net/pipermail/compiler-dev/2019-June/013498.html 



APIs associated with preview features are split into two groups: 
essential and non-essential. These are marked with an 
JDK-internal annotation, PreviewFeature, and a tag in the 
javadoc, @preview. The javac follows the PreviewFeature 
annotation, and produces either warnings or errors for the 
usages of such APIs. For the @preview tag, there is a taglet in 
the JDK build that adds the content of the tag into the 
documentation. The first part of the @preview's text goes into 
the summary, the second part goes into the detailed description.


For build, a tricky problem is that the jdk.compiler module uses 
the PreviewFeature annotation as well, but that is not in the 
bootstrap JDK. So, for the intermediate langtools build, the 
PreviewFeature annotation is copied from java.base.


Proposed webrev:
http://cr.openjdk.java.net/~jlahoda/8226585/webrev.00/

Javadoc with the change:
http://cr.openjdk.java.net/~jlahoda/8226585/docs.00/api/index.html

See for example:
http://cr.openjdk.java.net/~jlahoda/8226585/docs.00/api/java.base/java/lang/String.html 

http://cr.openjdk.java.net/~jlahoda/8226585/docs.00/api/jdk.compiler/com/sun/source/tree/CaseTree.html 



JBS:
https://bugs.openjdk.java.net/browse/JDK-8226585

CSR:
https://bugs.openjdk.java.net/browse/JDK-8231411

Feedback is welcome!

Thanks,
 Jan

Re: RFR: JDK-8226585: Improve javac messages for using a preview API

[Magnus Ihse Bursie]

I can’t see how the compilation is dependent on the copy being finished. Since 
Erik contributed this it will probably be correct :) but I’d appreciate an 
explanation on how this dependency is guaranteed. 

Or maybe I’m misunderstanding what this is supposed to do?

/Magnus

> 8 okt. 2019 kl. 17:27 skrev Jan Lahoda :
> 
> Thanks for the new code Erik!
> 
> A new webrev/patch that includes this better way of copying is here:
> http://cr.openjdk.java.net/~jlahoda/8226585/webrev.01/
> 
> Any feedback is welcome!
> 
> Thanks,
>Jan
> 
>> On 03. 10. 19 18:06, Erik Joelsson wrote:
>> Hello Jan,
>> The build change looks ok, but I would recommend this construct for copying 
>> the file instead:
>> $(eval $(call SetupCopyFiles, COPY_PREVIEW_FEATURES, \
>> FILES := 
>> $(TOPDIR)/src/java.base/share/classes/jdk/internal/PreviewFeature.java, \
>> DEST := 
>> $(BUILDTOOLS_OUTPUTDIR)/gensrc/java.base.interim/jdk/internal/PreviewFeature.java,
>>  \
>> ))
>> TARGETS += $(COPY_PREVIEW_FEATURES)
>> Then you automatically get all the corner case handling we have implemented 
>> over the years for logging, making directories and copying files. Your 
>> version is still correct for this case though.
>> /Erik
>>> On 2019-10-03 02:57, Jan Lahoda wrote:
>>> Hi,
>>> 
>>> This is a continuation of Joe's patch from here:
>>> https://mail.openjdk.java.net/pipermail/compiler-dev/2019-June/013498.html 
>>> 
>>> APIs associated with preview features are split into two groups: essential 
>>> and non-essential. These are marked with an JDK-internal annotation, 
>>> PreviewFeature, and a tag in the javadoc, @preview. The javac follows the 
>>> PreviewFeature annotation, and produces either warnings or errors for the 
>>> usages of such APIs. For the @preview tag, there is a taglet in the JDK 
>>> build that adds the content of the tag into the documentation. The first 
>>> part of the @preview's text goes into the summary, the second part goes 
>>> into the detailed description.
>>> 
>>> For build, a tricky problem is that the jdk.compiler module uses the 
>>> PreviewFeature annotation as well, but that is not in the bootstrap JDK. 
>>> So, for the intermediate langtools build, the PreviewFeature annotation is 
>>> copied from java.base.
>>> 
>>> Proposed webrev:
>>> http://cr.openjdk.java.net/~jlahoda/8226585/webrev.00/
>>> 
>>> Javadoc with the change:
>>> http://cr.openjdk.java.net/~jlahoda/8226585/docs.00/api/index.html
>>> 
>>> See for example:
>>> http://cr.openjdk.java.net/~jlahoda/8226585/docs.00/api/java.base/java/lang/String.html
>>>  
>>> http://cr.openjdk.java.net/~jlahoda/8226585/docs.00/api/jdk.compiler/com/sun/source/tree/CaseTree.html
>>>  
>>> 
>>> JBS:
>>> https://bugs.openjdk.java.net/browse/JDK-8226585
>>> 
>>> CSR:
>>> https://bugs.openjdk.java.net/browse/JDK-8231411
>>> 
>>> Feedback is welcome!
>>> 
>>> Thanks,
>>> Jan

Re: RFR: JDK-8212091 : Move native code under platform specific folders and files

[Magnus Ihse Bursie]

> 16 feb. 2019 kl. 04:03 skrev Alexander Matveev :
> 
> Hi Magnus,
> 
> http://cr.openjdk.java.net/~almatvee/8212091/webrev.01/
> 
> Moved all files from "posix" to "unix" folder and reverted 
> Lib-jdk.jpackage.gmk changes.
> Webrev updated with files moved, instead of add/remove.

Thank you!

This looks good now from a build point of view, but you'll need a review from 
core-libs as well. 

/Magnus

> 
> Thanks,
> Alexander
> 
>> On 2/14/2019 11:44 PM, Magnus Ihse Bursie wrote:
>> 
>> 
>>> On 2019-02-15 04:31, Alexander Matveev wrote:
>>> Please review the jpackage fix for bug [1] at [2].
>>> 
>>> This is a fix for the JDK-8200758-branch branch of the open sandbox 
>>> repository (jpackage).
>>> 
>>> - Moved native code under platform specific folder.
>>> - Removed most usage on #ifdefs for WINDOWS, LINUX, MAC and POSIX.
>>> - MAC define is still used in JavaVirtualMachine.cpp and Package.cpp for 
>>> Mac specific code to filter out some arguments. I decided to keep it as is 
>>> for now, since Mac specific code is small.
>>> - Defines are used in Platform.cpp to initialize platform specific classes.
>>> - Removed all pragma warning and fixed all compilation warnings.
>>> - Removed unused code.
>>> 
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8212091
>>> 
>>> [2] http://cr.openjdk.java.net/~almatvee/8212091/webrev.00/
>> The JDK standard is to use "unix", not "posix", for the shared functionality 
>> between linux/solaris/macosx. You can keep the name "PosixPlatform.*" if you 
>> want, though; the important thing is the directory name.
>> 
>> Also, if you do that, you do not need any changes to 
>> make/lib/Lib-jdk.jpackage.gmk, since that will be automatically understood 
>> by the build system.
>> 
>> It looks from the webrev that you have "moved" the files by doing "hg add" 
>> and "hg remove". Please use "hg move" instead -- this will keep history 
>> intact, and it allows reviewers to see if you have also made changes to the 
>> moved files.
>> 
>> (If you do have modified the moved file, reverting a "hg add+hg remove" 
>> process is a bit more tricky -- you need to do "hg forget" on the new file, 
>> rename it to something else (otherwise "hg move" will complain), "hg revert" 
>> the old file back in place, do a "hg move" from the old to the new, and then 
>> copy the modified, renamed file back over the target new file again.)
>> 
>> /Magnus
>>> 
>>> Thanks,
>>> Alexander
> 

Re: RFR: JDK-8212091 : Move native code under platform specific folders and files

[Magnus Ihse Bursie]

On 2019-02-15 04:31, Alexander Matveev wrote:

Please review the jpackage fix for bug [1] at [2].

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).


- Moved native code under platform specific folder.
- Removed most usage on #ifdefs for WINDOWS, LINUX, MAC and POSIX.
- MAC define is still used in JavaVirtualMachine.cpp and Package.cpp 
for Mac specific code to filter out some arguments. I decided to keep 
it as is for now, since Mac specific code is small.
- Defines are used in Platform.cpp to initialize platform specific 
classes.

- Removed all pragma warning and fixed all compilation warnings.
- Removed unused code.

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

[2] http://cr.openjdk.java.net/~almatvee/8212091/webrev.00/
The JDK standard is to use "unix", not "posix", for the shared 
functionality between linux/solaris/macosx. You can keep the name 
"PosixPlatform.*" if you want, though; the important thing is the 
directory name.


Also, if you do that, you do not need any changes to 
make/lib/Lib-jdk.jpackage.gmk, since that will be automatically 
understood by the build system.


It looks from the webrev that you have "moved" the files by doing "hg 
add" and "hg remove". Please use "hg move" instead -- this will keep 
history intact, and it allows reviewers to see if you have also made 
changes to the moved files.


(If you do have modified the moved file, reverting a "hg add+hg remove" 
process is a bit more tricky -- you need to do "hg forget" on the new 
file, rename it to something else (otherwise "hg move" will complain), 
"hg revert" the old file back in place, do a "hg move" from the old to 
the new, and then copy the modified, renamed file back over the target 
new file again.)


/Magnus


Thanks,
Alexander

Re: RFR: JDK-8218186 Clean up CLDR generation in build

[Magnus Ihse Bursie]

On 2019-02-01 14:43, naoto.s...@oracle.com wrote:

Hi Magnus,

I am assuming that the generated resource bundles are exactly the same 
as before this fix, correct?


Yes. I have verified this using the COMPARE_BUILD functionality.

/Magnus


Naoto

On 2/1/19 2:50 AM, Magnus Ihse Bursie wrote:
The CLDR data is, since Jigsaw, used in two different modules -- 
java.base and jdk.localedata. Unfortunately, the split between these 
two modules were not fully finished as part of the Jigsaw project.


This patch aims to resolving most of this. The CLDRConverter build 
tool is now called from Gensrc-java.base and Gensrc-jdk.localedata, 
for their respective module. The calls have been updated to match 
modern build-infra standards.


Also, the raw CLDR data was located mixed in with the Java source, in 
jdk.localedata. This patch also moves the data to make/data/cldr, to 
align with input data to all other build tools.


Bug: https://bugs.openjdk.java.net/browse/JDK-8218186
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8218186-cleanup-CLDR/webrev.01


/Magnus

Re: RFR: JDK-8217317 : Create jpackage native library for windows

[Magnus Ihse Bursie]

On 2019-02-01 23:38, Alexander Matveev wrote:

Hi Magnus,

http://cr.openjdk.java.net/~almatvee/8217317/webrev.01/

Looks great, thank you!

I can't comment on the actual source code changes, so you'll need a 
thumbs up from someone in core libs as well.


Moved files to libjpackage and remove JPACKAGELIB_SRC.

Old wmain() was in jpackage.cpp line 461.
Aha. :) I only knew about WinMain and main. You learn something every 
day. Thanks.


/Magnus



Thanks,
Alexander

On 2/1/2019 3:39 AM, Magnus Ihse Bursie wrote:

Hi Alexander,

On 2019-02-01 05:22, Alexander Matveev wrote:

Please review the jpackage fix for bug [1] at [2].

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).


- jpackage launcher will now build same as Linux and OS X using 
SetupBuildLauncher.
- jpackage.dll was added based on Windows jpackage.exe launcher 
which will have icon swap and version swap functionality called via 
JNI.
- Some code formatting, clean up and minor improvements where done 
to icon and version swap code. No functional changes.

- Windows registry will be read and enumerated via JNI as well.
- isDirectoryInExclusionPath() will use native path comparison, 
since paths in registry and temp folder returned by Java code can be 
in short or long format, thus simple string comparison will not work.
- Windows Defender workaround warning will be checked for 
--build-root as well if it is set.
- Removed extra escaping from JPackageHelper for Windows, otherwise 
tests fails due to incorrect escaping. Our launcher used 
CreateProcess to launch java.exe by passing args from main() to 
CreateProcess. This is why I think we required extra escaping.


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

[2] http://cr.openjdk.java.net/~almatvee/8217317/webrev.00/
It basically looks good from a build perspective. There is one change 
I'd like to request, however, and that is that you place the source 
code according to the standard layout. This means moving the source 
files from src/jdk.jpackage/windows/native/jpackage to 
src/jdk.jpackage/windows/native/libjpackage. Also, when you do this, 
you don't need JPACKAGELIB_SRC; the location of the files will be 
determined by SetupJdkLibrary based on the name "jpackage" of the 
library.


I'm also surprised to see that I can't find the removal of the old 
WinMain() method?


/Magnus




Thanks,
Alexander

Re: RFR: JDK-8217317 : Create jpackage native library for windows

[Magnus Ihse Bursie]

Hi Alexander,

On 2019-02-01 05:22, Alexander Matveev wrote:

Please review the jpackage fix for bug [1] at [2].

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).


- jpackage launcher will now build same as Linux and OS X using 
SetupBuildLauncher.
- jpackage.dll was added based on Windows jpackage.exe launcher which 
will have icon swap and version swap functionality called via JNI.
- Some code formatting, clean up and minor improvements where done to 
icon and version swap code. No functional changes.

- Windows registry will be read and enumerated via JNI as well.
- isDirectoryInExclusionPath() will use native path comparison, since 
paths in registry and temp folder returned by Java code can be in 
short or long format, thus simple string comparison will not work.
- Windows Defender workaround warning will be checked for --build-root 
as well if it is set.
- Removed extra escaping from JPackageHelper for Windows, otherwise 
tests fails due to incorrect escaping. Our launcher used CreateProcess 
to launch java.exe by passing args from main() to CreateProcess. This 
is why I think we required extra escaping.


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

[2] http://cr.openjdk.java.net/~almatvee/8217317/webrev.00/
It basically looks good from a build perspective. There is one change 
I'd like to request, however, and that is that you place the source code 
according to the standard layout. This means moving the source files 
from src/jdk.jpackage/windows/native/jpackage to 
src/jdk.jpackage/windows/native/libjpackage. Also, when you do this, you 
don't need JPACKAGELIB_SRC; the location of the files will be determined 
by SetupJdkLibrary based on the name "jpackage" of the library.


I'm also surprised to see that I can't find the removal of the old 
WinMain() method?


/Magnus




Thanks,
Alexander

RFR: JDK-8218186 Clean up CLDR generation in build

[Magnus Ihse Bursie]

The CLDR data is, since Jigsaw, used in two different modules -- 
java.base and jdk.localedata. Unfortunately, the split between these two 
modules were not fully finished as part of the Jigsaw project.


This patch aims to resolving most of this. The CLDRConverter build tool 
is now called from Gensrc-java.base and Gensrc-jdk.localedata, for their 
respective module. The calls have been updated to match modern 
build-infra standards.


Also, the raw CLDR data was located mixed in with the Java source, in 
jdk.localedata. This patch also moves the data to make/data/cldr, to 
align with input data to all other build tools.


Bug: https://bugs.openjdk.java.net/browse/JDK-8218186
WebRev: http://cr.openjdk.java.net/~ihse/JDK-8218186-cleanup-CLDR/webrev.01

/Magnus

Re: RFR: JDK-8217880 AIX build issue about JDK-8214533

[Magnus Ihse Bursie]

> 29 jan. 2019 kl. 02:21 skrev Ichiroh Takiguchi :
> 
> Hello.
> 
> Sorry about build issue for JDK-8214533.
> 
> EUC_JP was extra entry on make/data/charsetmapping/stdcs-aix.
> 
> Bug:https://bugs.openjdk.java.net/browse/JDK-8217880
> Change: https://cr.openjdk.java.net/~itakiguchi/8217880/webrev.00/

LGTM. 

/Magnus

> 
> Could you review the fix ?
> 
> Thanks,
> Ichiroh Takiguchi
> IBM Japan, Ltd.
> 
>> On 2019-01-28 22:49, Ichiroh Takiguchi wrote:
>> Hello Goetz.
>> Thank you for your suggestion.
>> I just open JDK-8217880 [1].
>> I just restart clean build.
>> I'll post new fix including testcase for EUC_JP_LINUX and EUC_JP_Open.
>> [1] https://bugs.openjdk.java.net/browse/JDK-8217880
>> Thanks,
>> Ichiroh Takiguchi
>>> On 2019-01-28 22:11, Lindenmaier, Goetz wrote:
>>> Hi Ichiroh,
>>> just open a bug, like "Fix aix build after 8214533" and post a RFR for it.
>>> I assume the fix is quite trivial so we can review it quick.
>>> Best regards,
>>>  Goetz.
>>>> -Original Message-
>>>> From: ppc-aix-port-dev  On
>>>> Behalf Of Ichiroh Takiguchi
>>>> Sent: Montag, 28. Januar 2019 14:13
>>>> To: Baesken, Matthias 
>>>> Cc: build-dev ; ppc-aix-port-dev >>> d...@openjdk.java.net>; core-libs-dev@openjdk.java.net; Alan Bateman
>>>> 
>>>> Subject: RE: RFR: 8214533 IBM-29626C is required for AIX default charset
>>>> Hello.
>>>> I'm very sorry. It's my fault.
>>>> EUC_JP class was moved to java.base module.
>>>> (sun.nio.cs.EUC_JP).
>>>> make/data/charsetmapping/stdcs-aix should have EUC_JP_LINUX and
>>>> EUC_JP_Open.
>>>> Could you suggest me how I should provide new webrev files ?
>>>> Thanks,
>>>> Ichiroh Takiguchi
>>>> On 2019-01-28 17:03, Baesken, Matthias wrote:
>>>> > Hello,   seems  8214533   got pushed  recently  into  jdk/jdk.   Now
>>>> > we see build errors on AIX  ,  are they related to  this change ?
>>>> >
>>>> >
>>>> > /nb/rs6000_64/nightly/output-jdk-
>>>> test/support/gensrc/jdk.charsets/sun/nio/cs/ext/EUC_JP_LINUX.java:63:
>>>> > error: Decoder is not public in EUC_JP; cannot be accessed from
>>>> > outside package
>>>> > private static class Decoder extends EUC_JP.Decoder {
>>>> >^
>>>> > /nb/rs6000_64/nightly/output-jdk-
>>>> test/support/gensrc/jdk.charsets/sun/nio/cs/ext/EUC_JP_LINUX.java:69:
>>>> > error: Encoder is not public in EUC_JP; cannot be accessed from
>>>> > outside package
>>>> > private static class Encoder extends EUC_JP.Encoder {
>>>> >^
>>>> > /nb/rs6000_64/nightly/output-jdk-
>>>> test/support/gensrc/jdk.charsets/sun/nio/cs/ext/EUC_JP_Open.java:65:
>>>> > error: Decoder is not public in EUC_JP; cannot be accessed from
>>>> > outside package
>>>> > private static class Decoder extends EUC_JP.Decoder {
>>>> >^
>>>> > /nb/rs6000_64/nightly/output-jdk-
>>>> test/support/gensrc/jdk.charsets/sun/nio/cs/ext/EUC_JP_Open.java:85:
>>>> > error: Encoder is not public in EUC_JP; cannot be accessed from
>>>> > outside package
>>>> > private static class Encoder extends EUC_JP.Encoder {
>>>> >
>>>> > Best regards, Matthias
>>>> >
>>>> >
>>>> >
>>>> >> -Original Message-
>>>> >> From: ppc-aix-port-dev  On
>>>> >> Behalf Of Ichiroh Takiguchi
>>>> >> Sent: Dienstag, 15. Januar 2019 01:51
>>>> >> To: Alan Bateman 
>>>> >> Cc: build-dev ; ppc-aix-port-dev >>> >> port-...@openjdk.java.net>; core-libs-dev@openjdk.java.net
>>>> >> Subject: Re: RFR: 8214533 IBM-29626C is required for AIX default
>>>> >> charset
>>>> >>
>>>> >> Hello Alan.
>>>> >>
>>>> >> Could you review the fix again ?
>>>> >>
>>>> >> Bug:https://bugs.openjdk.java.net/browse/JDK-8214533
>>>> >> Change: https://cr.openjdk.java.net/~itakiguchi/8214533/webrev.01/
>>>> >>
>>>> >> I

Re: RFR: JDK-8217269: jpackage Makefile improvments

[Magnus Ihse Bursie]

On 2019-01-18 15:18, Andy Herrick wrote:

Please review the jpackage fix for bug [1] at [2].

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).


The webrev includes all the jpackage Makefile Improvements listed in 
[1], other than what is covered by [4], and also includes fix for 
white space errors requested in [3].


Looks good to me! Thanks for fixing this.

In the future, please cc build-dev for build related fixes.

/Magnus



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

[2] http://cr.openjdk.java.net/~herrick/8217269/webrev.01/

[3] https://bugs.openjdk.java.net/browse/JDK-8216521

[4] https://bugs.openjdk.java.net/browse/JDK-8217317

/Andy

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation (update 2)

[Magnus Ihse Bursie]

On 2019-01-17 16:06, Andy Herrick wrote:


If I remove the line -nologo from lib-jdk.jpackage.gmk:


   69 LDFLAGS_windows := -nologo, \

I get the logo during build (same with console andnon-console version):

Microsoft (R) Incremental Linker Version 14.12.25835.0
Copyright (C) Microsoft Corporation.  All rights reserved.
do I need something to include CXXFLAGS_JDKEXE into LDFLAGS ? (there 
is no other LDFLAGS line...)

Ah, good catch. You should add
LDFLAGS := $(LDFLAGS_JDKEXE), \

to your setup.

/Magnus


here's the non-console APPLAUNCHERWEXE case:


# Build non-console version of launcher
ifeq ($(OPENJDK_TARGET_OS), windows)

  $(eval $(call SetupJdkExecutable, BUILD_JPACKAGE_APPLAUNCHERWEXE, \
  NAME := jpackageapplauncherw, \
  OUTPUT_DIR := 
$(JDK_OUTPUTDIR)/modules/$(MODULE)/jdk/jpackage/internal/resources, \
  SYMBOLS_DIR := 
$(SUPPORT_OUTPUTDIR)/native/$(MODULE)/jpackageapplauncherw, \

  SRC := $(JPACKAGE_APPLAUNCHEREXE_SRC), \
  TOOLCHAIN := TOOLCHAIN_LINK_CXX, \
  OPTIMIZATION := LOW, \
  CFLAGS := $(CXXFLAGS_JDKEXE), \
  CFLAGS_windows := -EHsc -DUNICODE -D_UNICODE, \
  LDFLAGS_windows := -nologo, \ 
<-

  LIBS := $(LIBCXX), \
  LIBS_windows :=  user32.lib shell32.lib advapi32.lib, \
  ))

  TARGETS += $(BUILD_JPACKAGE_APPLAUNCHERWEXE)
endif


/Andy


On 1/15/2019 8:09 AM, Magnus Ihse Bursie wrote:

Hi Andy,

This is looking really sweet from a build perspective!

Just a few minor nits:

* In Launcher-jdk.jpackage.gmk, please indent the else clause 
("$(eval $(call SetupBuildLauncher, jpackage ") two spaces.


* In Lib-jdk.jpackage.gmk, I think there's still room to prune some 
more unnecessary compiler flags and parameters to SetupJdkExecutable:

   65 CFLAGS_linux := -fPIC, \
   66 CFLAGS_solaris := -KPIC, \
   67 CFLAGS_macosx := -fPIC, \
 I wonder if these really are needed. Normally, only shared libraries 
neeed PIC code. (And for those we set it automatically.)


   69 LDFLAGS_windows := -nologo, \
This should not be needed, it's incorporated in CXXFLAGS_JDKEXE. 
(Also in the second block, for jpackageapplauncherw).


   72 LIBS_solaris :=  -lc, \
Same here; this should not be needed. It's in GLOBAL_LIBS on Solaris, 
and is always included.


   75 VERSIONINFO_RESOURCE := $(GLOBAL_VERSION_INFO_RESOURCE), \
This is setup by default by SetupJdkExecutable, so you can remove it. 
(Also in the second block, for jpackageapplauncherw).


Finally, I do believe that the setups of jpackageapplauncher and 
jpackageapplauncherw should be done in Launcher-jdk.jpackage.gmk, not 
Lib-jdk.jpackage.gmk. Since they are to be shipped as resources, they 
are not "really" launchers in our normal sense, but they are at least 
more launchers than they are libraries.


As we've talked about privately, in the future I'd like to see 
Windows too use the SetupBuildLauncher method for creating the 
launcher, but this is clearly good enough for inclusion in the mainline.


I also have a question about 
test/jdk/tools/jpackage/resources/license.txt. What is it used for? 
And why the odd (incorrect?) spelling of license?


/Magnus

On 2019-01-11 20:41, Andy Herrick wrote:
This is the second update to the Request For Review of the 
implementation of the Java Packager Tool (jpackager) as described in 
JEP 343: Packaging Tool 
<https://bugs.openjdk.java.net/browse/JDK-8200758>



This webrev corresponds to the second EA build, Build 8 (2019/1/8), 
posted at http://jdk.java.net/jpackage/


This update renames the package used to "jdk.jpackage", removes the 
Single Instance Service (and CLI option --singleton), adds several 
other CLI options, adds more automated tests, and contains fixes to 
the following issues (since update 1 on 11/09/2018):


JDK-8212164 resolve jre.list and jre.module.list
JDK-8212936 Makefile and other improvements for jpackager
JDK-8213385 jpackager Command-Line Argument File.
JDK-8213392 Enhance --help and --version
JDK-8213425 Analyze output from Source code scanner and fix 
where needed.

JDK-8213747 Makefile Improvements to Lib-jdk.packager.gmk
JDK-8213748 jpackager native launcher for macosx, linux.
JDK-8213756 SingleInstance runtime improvements
JDK-8213962 JPackageCreateImageRuntimeModuleTest fails
JDK-8213963 Flatten out jpackager packages and resources.
JDK-8214021 Create additional automated tests for jpackager
JDK-8214051 rename jpackager tool to jpackage
JDK-8214070 Analyze and Fix issues reported by Parfait
JDK-8214143 Reduce Resource files
JDK-8214495 Change behavior of --license-file
JDK-8214575 Exe installers will install application over 
existing installation
JDK-8214899 rename papplauncher and it's library and move src to 
appropriate places
JDK-8214982 jpackage causes failures in existing HelpFlagsTest 
and VersionCheck tests
JDK-8215020 create-jre-i

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation (update 2)

[Magnus Ihse Bursie]

Hi Andy,

This is looking really sweet from a build perspective!

Just a few minor nits:

* In Launcher-jdk.jpackage.gmk, please indent the else clause ("$(eval 
$(call SetupBuildLauncher, jpackage ") two spaces.


* In Lib-jdk.jpackage.gmk, I think there's still room to prune some more 
unnecessary compiler flags and parameters to SetupJdkExecutable:


  65 CFLAGS_linux := -fPIC, \
  66 CFLAGS_solaris := -KPIC, \
  67 CFLAGS_macosx := -fPIC, \

 I wonder if these really are needed. Normally, only shared libraries 
neeed PIC code. (And for those we set it automatically.)


  69 LDFLAGS_windows := -nologo, \

This should not be needed, it's incorporated in CXXFLAGS_JDKEXE. (Also 
in the second block, for jpackageapplauncherw).


  72 LIBS_solaris :=  -lc, \

Same here; this should not be needed. It's in GLOBAL_LIBS on Solaris, 
and is always included.


  75 VERSIONINFO_RESOURCE := $(GLOBAL_VERSION_INFO_RESOURCE), \

This is setup by default by SetupJdkExecutable, so you can remove it. 
(Also in the second block, for jpackageapplauncherw).


Finally, I do believe that the setups of jpackageapplauncher and 
jpackageapplauncherw should be done in Launcher-jdk.jpackage.gmk, not 
Lib-jdk.jpackage.gmk. Since they are to be shipped as resources, they 
are not "really" launchers in our normal sense, but they are at least 
more launchers than they are libraries.


As we've talked about privately, in the future I'd like to see Windows 
too use the SetupBuildLauncher method for creating the launcher, but 
this is clearly good enough for inclusion in the mainline.


I also have a question about 
test/jdk/tools/jpackage/resources/license.txt. What is it used for? And 
why the odd (incorrect?) spelling of license?


/Magnus

On 2019-01-11 20:41, Andy Herrick wrote:
This is the second update to the Request For Review of the 
implementation of the Java Packager Tool (jpackager) as described in 
JEP 343: Packaging Tool 




This webrev corresponds to the second EA build, Build 8 (2019/1/8), 
posted at http://jdk.java.net/jpackage/


This update renames the package used to "jdk.jpackage", removes the 
Single Instance Service (and CLI option --singleton), adds several 
other CLI options, adds more automated tests, and contains fixes to 
the following issues (since update 1 on 11/09/2018):


JDK-8212164 resolve jre.list and jre.module.list
JDK-8212936 Makefile and other improvements for jpackager
JDK-8213385 jpackager Command-Line Argument File.
JDK-8213392 Enhance --help and --version
JDK-8213425 Analyze output from Source code scanner and fix where 
needed.

JDK-8213747 Makefile Improvements to Lib-jdk.packager.gmk
JDK-8213748 jpackager native launcher for macosx, linux.
JDK-8213756 SingleInstance runtime improvements
JDK-8213962 JPackageCreateImageRuntimeModuleTest fails
JDK-8213963 Flatten out jpackager packages and resources.
JDK-8214021 Create additional automated tests for jpackager
JDK-8214051 rename jpackager tool to jpackage
JDK-8214070 Analyze and Fix issues reported by Parfait
JDK-8214143 Reduce Resource files
JDK-8214495 Change behavior of --license-file
JDK-8214575 Exe installers will install application over existing 
installation
JDK-8214899 rename papplauncher and it's library and move src to 
appropriate places
JDK-8214982 jpackage causes failures in existing HelpFlagsTest and 
VersionCheck tests
JDK-8215020 create-jre-installer exe fails when --runtime-image is 
specified.
JDK-8215036 Create initial set of tests for jpackage 
create-installer mode

JDK-8215453 remove unused BundlerParams and fix misleading messages
JDK-8215515 Add a command line option to override internal resources.
JDK-8215900 Without --files args, only jars in the top level of 
--input are added to class-path

JDK-8215903 modify behavior of retaining temporary output dir
JDK-8216190 Remove Single Instance Service support from jpackage
JDK-8216313 Add ToolProvider information to jdk.jpackage API docs
JDK-8216373 temporary build-root left behind when using secondary 
launcher(s)

JDK-8216492 Update copyright of all new jpackage files to 2019

Webrev: http://cr.openjdk.java.net/~herrick/8212780/webrev.03

Note: SingleInstanceService API was removed (see 
https://bugs.openjdk.java.net/browse/JDK-8216190).
An example stand alone implementation can be found at: 
http://cr.openjdk.java.net/~herrick/jpackage/Singleton.java


please send feedback to core-libs-dev@openjdk.java.net

/Andy Herrick

Re: OpenJDK fails to build with GCC when the #include inside zip.cpp comes from a non-sysroot path

[Magnus Ihse Bursie]

On 2019-01-02 00:11, David Holmes wrote:

Hi Patrick,

On 13/12/2018 1:23 pm, Patrick Zhang wrote:

Ping...

Apply for a sponsor for this simple patch.


Seems no one wants to own this one :(
For what it's worth, it looks good to me. (But I'd like to see a core 
library developer chime in as well.)


/Magnus



I doubt if I could have permission to file the issue/bug report 
anywhere, could anyone kindly give a guidance? Thanks.


I have filed:

https://bugs.openjdk.java.net/browse/JDK-8215976

to cover this and linked to the discussion threads.

The appropriate owners still need to review this, but I'm not sure who 
that may be these days.


Cheers,
David
-



Regards
Patrick

-Original Message-
From: core-libs-dev  On 
Behalf Of Patrick Zhang

Sent: Thursday, December 6, 2018 4:28 PM
To: core-libs-dev@openjdk.java.net; David Holmes 


Cc: Florian Weimer 
Subject: RE: OpenJDK fails to build with GCC when the 
#include inside zip.cpp comes from a non-sysroot path


To All,
Who could help sponsor this simple patch (may require a wide range of 
building tests)? Thanks in advance.


Regards
Patrick

-Original Message-
From: David Holmes 
Sent: Monday, December 3, 2018 8:11 AM
To: Patrick Zhang ; Florian Weimer 


Cc: core-libs-dev@openjdk.java.net
Subject: Re: OpenJDK fails to build with GCC when the 
#include inside zip.cpp comes from a non-sysroot path


Hi Patrick,

On 30/11/2018 11:41 pm, Patrick Zhang wrote:
Thanks Florian, the "-isystem /usr/include" is helpful to my case, I 
see gcc.gnu.org says that "it gets the same special treatment that 
is applied to the standard system directories". As such the issue 
gets hidden (error suppressed).


Hi David,
Thanks for your suggestion. My intention was to limit the influence 
range as far as I could since I don't have other systems except 
CentOS/Fedora to verify (even just smoke test) all paths.


You'd need some assistance testing a wider range of platforms but 
that can be arranged.


In addition, if we make below update, does it mean the macro " 
_REENTRANT " can be removed too? This is probably the only place 
where _REENTRANT gets used AFAIK.
_REENTRANT is also examined by system headers. It's probably legacy 
but would require more investigation.


Cheers,
David


#ifdef _MSC_VER // Windows
#define gmtime_r(t, s) gmtime(t)
#endif

Regards
Patrick

-Original Message-
From: Florian Weimer 
Sent: Thursday, November 29, 2018 8:02 PM
To: David Holmes 
Cc: Patrick Zhang ;
jdk-...@openjdk.java.net; core-libs-dev@openjdk.java.net
Subject: Re: OpenJDK fails to build with GCC when the #include
inside zip.cpp comes from a non-sysroot path

* David Holmes:


This should really be being discussed on core-libs-dev.


Okay, moving the conversation.

diff -r 70a423caee44 
src/share/native/com/sun/java/util/jar/pack/zip.cpp
--- a/src/share/native/com/sun/java/util/jar/pack/zip.cpp Tue Oct 
09 08:33:33 2018 +0100
+++ b/src/share/native/com/sun/java/util/jar/pack/zip.cpp Wed Nov 
28 22:13:12 2018 -0500

@@ -415,9 +415,7 @@
    ((uLong)h << 11) | ((uLong)m << 5) | ((uLong)s >> 1);
    }
    -#ifdef _REENTRANT // solaris
-extern "C" struct tm *gmtime_r(const time_t *, struct tm *); -#else
+#if !defined(_REENTRANT) // linux
    #define gmtime_r(t, s) gmtime(t)
    #endif
    /*


Under the theme "two wrongs don't make a right" the use of _REENTRANT
here is totally inappropriate AFAICS. It seems to be a misguided
attempt at determining whether we need the thread-safe gmtime_r or
not
- and the answer to that should always be yes IMHO.

We define _REENTRANT for:
- linux
- gcc
- xlc

So the original code will define:

extern "C" struct tm *gmtime_r(const time_t *, struct tm *);

for linux (and AIX with xlc?) but not Solaris, OS X or Windows.

But Solaris has gmtime_r anyway. So the existing code seems a really
convoluted hack. AFAICS we have gmtime_r everywhere but Windows
(where gmtime is already thread-safe). So it seems to me that all we
should need here is:

-#ifdef _REENTRANT // solaris
-extern "C" struct tm *gmtime_r(const time_t *, struct tm *); -#else
+#ifdef _MSC_VER // Windows
   #define gmtime_r(t, s) gmtime(t)
   #endif


That looks much cleaner.

Thanks,
Florian

Re: Proposal: Use new JDK_EXPORT decorator instead of JNIEXPORT

[Magnus Ihse Bursie]

s not present 
before. We might have had something like (I'm making this specific 
example up) a function "void ZIP_OpenFile(char* name)" with no 
decoration at all, and a "/export:ZIP_OpenFile" on the command line, and 
a ZIP_OpenFile entry in the unix map file. And I converted this to 
"JNIEXPORT void JNICALL ZIP_OpenFile(char* name)", which de facto -- 
although I didn't fully realize this at the time, changed the calling 
convention and name decoration on Windows 32. When something broke, 
perhaps because the user of ZIP_OpenFile did not include the proper 
header file with the JNICALL modifier, the solution was to remove the 
JNICALL part.


And of all the bug reports I've seen on this, the issues has been 
internal linking only, i.e. problems building the JDK, not complaints 
that external tools tried to use internal interfaces and failed. So yes, 
my position is if this should break things, tough shit. That, of 
courses, requires that we do not change public APIs, so we need to be 
careful not to mess with those.


Does that sum it up?

Yep, I think so.


We still need to be sure that we're only changing functions of type 
(b) above.

Yes, definitely.


And I note that this has been driven by the choice to remove JNICALL 
where there was a discrepancy - that leads to the visibility of the 
two kinds of methods. If it had been chosen to add JNICALL where 
missing instead, then we may not have been having this conversation.
Actually, as I said, this has more been the result of a lot of added 
JNICALL where previously there was none.


An alternative course of action is the make sure that *all* calls use 
the JNIEXPORT...JNICALL pattern, even type b ones, and that we fix all 
parts of code to actually accept the decorated names on Windows 32. This 
will lead to a lot more code changes, like the fix for JDK-8214122 (JDWP 
is broken on 32 bit Windows: transport library missing onLoad entry). 
And all this special case handling will be needed only on Windows 32, 
which is a platform we do not want to spend to much time or effort on. 
And finally, I think doing so would make us miss out on an opportunity 
to make the semantics clearer.


This will also need a CSR request due to the change in linking behaviour.
I'm not sure what you mean by this..? My entire intention here is that 
we should make no changes to documented interfaces; this is strictly an 
internal change, so no CSR should be needed. Also, I don't understand 
what you mean by "linking behavior"?


/Magnus


Cheers,
David
-

On 12/12/2018 9:03 pm, Magnus Ihse Bursie wrote:



On 2018-12-11 23:47, David Holmes wrote:

On 12/12/2018 12:34 am, Magnus Ihse Bursie wrote:



On 2018-12-11 00:23, David Holmes wrote:

Hi Magnus,

On 10/12/2018 11:19 pm, Magnus Ihse Bursie wrote:
I propose that we introduce a new define, available to all JDK 
native files (Hotspot included), called JDK_EXPORT. The behavior 
of this symbol will be very similar (as of now, in fact 
identical) to JNIEXPORT; however, the semantics will not.


Currently, we "mis-use" the JNIEXPORT define to mark a function 
for exporting from the library. The problem with this is that 
JNIEXPORT is part of the JNI interface, and is supposed to be 
used when C programs interact with Java. And, when doing this, 
the function should be fully decorated like this: "JNIEXPORT foo 
JNICALL".


I've seen a lot of the emails on this issue and I don't fully 
understand what has been going wrong. But the intent is obviously 
the JNIEXPORT represents what is needed to export this function 
for use by JNI, while JNICALL defines the calling convention. I 
agree there may be some mistmatch when functions are actually not 
intended for general export outside the JDK but are only for 
internal JDK use.


We do have many such JNI exports in our native libraries, but we 
also have a lot of other, non-JNI exports, where one native 
library just provides an interface to other libraries. In these 
cases, we have still used JNIEXPORT for the functionality of 
getting the function exported, but we have not been consistent in 
our use of JNICALL. This has caused us way too much trouble for 
something that should Just Work.


Are you suggesting that the interface between different libraries 
in the JDK should not be a JNI interface? Is this because you 
think the functions in these libraries are only for JDK internal 
use or ... ??


I therefore propose that we define "JDK_EXPORT", with the same 
behavior as JNIEXPORT (that is, flagging the function for 
external visibility in the resulting native library), but which 
is *not* supposed to be exported to Java code using JNI, nor 
supposed to be decorated with 


Just a clarification there. JNI functions are not exported to Java 
code, they are exported to native code. Java code can declare 
native methods and those native methods must be written as JNI 
functions, but that's not what we are dis

Re: [PATCH] Windows 32-bit DLL name decoration

[Magnus Ihse Bursie]

On 2018-12-12 16:34, Alexey Ivanov wrote:



On 12/12/2018 12:58, Magnus Ihse Bursie wrote:



On 2018-12-12 12:35, Alan Bateman wrote:

On 12/12/2018 11:14, Magnus Ihse Bursie wrote:

:

I searched the code for "jdwpTransport_On" to see of there was any 
corresponding OnUnload function (or other), but could not find any.
That's right, an unload wasn't needed when that SPI was originally 
added but could be added in the event that some future debugger 
agent need it. The SPI is a supported/documented JDK-specific 
mechanism, its "spec" is hosted here:


https://docs.oracle.com/en/java/javase/11/docs/specs/jdwp/jdwp-transport.html 

... yet in all that time, we have not fully supported the spec on 
Windows 32. :-( (We never discovered this because of lack of testing, 
I presume, and that our internal usage empoyed a questonable 
workaround.)


The spec does not specify whether the mangled name should be exported 
or not (for Windows 32 bit).


I looked through JNI specification and have found no mention of 
JNIEXPORT or JNICALL.

https://docs.oracle.com/en/java/javase/11/docs/specs/jni/index.html

This example uses extern "C" modifier but neither JNIEXPORT or JNICALL:
https://docs.oracle.com/en/java/javase/11/docs/specs/jni/design.html#native-method-arguments 

I'm not sure if it's worth continuing this discussion, but at the very 
least the specs have been not very clear on this. The links you cite 
point to the JNI specification, not JDWP. I don't bother go looking for 
the exact place in the JNI spec, but I assume they state that you must 
implement the header file as generated by javac (formerly javah). These 
do look like this:


* DO NOT EDIT THIS FILE - it is machine generated */
#include 
/* Header for class sun_nio_ch_FileKey */

#ifndef _Included_sun_nio_ch_FileKey
#define _Included_sun_nio_ch_FileKey
#ifdef __cplusplus
extern "C" {
#endif
/*
 * Class: sun_nio_ch_FileKey
 * Method:init
 * Signature: (Ljava/io/FileDescriptor;)V
 */
JNIEXPORT void JNICALL Java_sun_nio_ch_FileKey_init
  (JNIEnv *, jobject, jobject);

So at least implicitly, they say that you need to use JNIEXPORT and 
JNICALL. And there is no /export thingy here. And no instructions in the 
specs to do a specific export; if it were, we would have needed it for 
all the hundereds of JNI functions in the JDK. So obviously the JNI code 
reads decorated names on Windows 32.


Then again, it is not clear that the spec for JNI should apply to JDWP. 
But, if they state that you must use the JNICALL, and this will, unless 
other actions are taken, such as using /export, by default create a 
decorated name, I think it's very clear that *if* this indeed was 
intended to be the formal interface, it *should* have been explicitly 
written in the spec.


/Magnus








I see this thread is touching on the functions exported by libjli. 
This library is part of the launcher and shouldn't be used directly 
by anything outside of the JDK. However we have to be careful 
because JavaFX `javapackager` tool has been using it. There's a JEP 
to bring a similar tool, jpackage in the current proposal, that will 
supersede it but in the mean time we need to be careful not to break 
it. A second issue is that the libjli is listed in the property list 
(Info.plist) on macOS. This came from Apple in 7u4 and periodically 
things show up that are using it, e.g. that recent breakage with 
Eclipse that was never fully diagnosed but we think it was using 
Info.plist.
The latest patch, as suggested, will not modify JLI, but instead fix 
the broken behaviour of JDWP on Windows 32.


Yes, that's right.
However, I believe the previous versions did not modify libjli.

Regards,
Alexey



/Magnus

Re: [12] RFR for JDK-8214122: JDWP is broken on 32 bit Windows: transport library missing onLoad entry

[Magnus Ihse Bursie]

On 2018-12-12 17:38, Alexey Ivanov wrote:

Hi all,

I have updated the summary of JDK-8214122 and the subject accordingly.

Could you please review the following fix?

https://bugs.openjdk.java.net/browse/JDK-8214122
webrev: http://cr.openjdk.java.net/~aivanov/8214122/webrev.01/

Looks good to me.

/Magnus


*Root cause*
jdwpTransport_OnLoad is exported as _jdwpTransport_OnLoad@16 on 32 bit 
Windows according to the name decorations [1] for __stdcall [2] 
calling conventions.


*Fix*
On 32 bit Windows, look up the decorated name, 
_jdwpTransport_OnLoad@16, first; if not found, look up the undecorated 
name, jdwpTransport_OnLoad.



Regards,
Alexey

[1] 
https://docs.microsoft.com/en-us/cpp/build/reference/decorated-names?view=vs-2017#FormatC

[2] https://docs.microsoft.com/en-ie/cpp/cpp/stdcall?view=vs-2017

On 12/12/2018 11:19, Magnus Ihse Bursie wrote:



On 2018-12-11 18:16, Alexey Ivanov wrote:

Hi Simon,

Thank you for your comments.

The updated webrev:
http://cr.openjdk.java.net/~aivanov/8214122/webrev.01/

Indeed, it looks much cleaner.

Yes! This seems like the correct fix.

Ship it! :)

/Magnus



Regards,
Alexey

On 11/12/2018 16:43, Simon Tooke wrote:

On 2018-12-11 10:05 a.m., Alexey Ivanov wrote:

Hi everyone,

I came up with the following patch:
http://cr.openjdk.java.net/~aivanov/8214122/webrev.00/

It specifically addresses the problem in JDK-8214122 where on 32 bit
Windows jdwpTransport_OnLoad can exported with its plain and
__stdcall-mangled name. I used conditional compilation so that for
other platforms the code remains as it is now.

jshell starts successfully with this fix; an IDE debugger works as 
well.



I am not a reviewer, but this patch only works for the specific case
under discussion; the '@16' refers to the reserved stack size in the
Pascal calling convention.  So, the patch only works for 16 bytes of
parameters.  To be generic, the routine needs to have the stack size
passed in by the caller.  If a generic fix isn't desired (and I 
hope it

is), I'd prefer to see the caller simply pass the decorated or
undecorated name depending on the Win32/64 defines.

 #if defined(_WIN32) && !defined(_WIN64) onLoad =
 (jdwpTransport_OnLoad_t) dbgsysFindLibraryEntry(handle,
 "_jdwpTransport_OnLoad@16"); #else onLoad = 
(jdwpTransport_OnLoad_t)

 dbgsysFindLibraryEntry(handle, "jdwpTransport_OnLoad"); #endif


Thanks,
-Simon


Regards,
Alexey

https://bugs.openjdk.java.net/browse/JDK-8214122

On 10/12/2018 15:11, Magnus Ihse Bursie wrote:
Since removing JNICALL is not an option, there are only two 
options:


1. Add |/export| option to the Makefile or pragma-comment to the
source file;
2. Lookup the decorated name |_jdwpTransport_OnLoad@16| for Win32
with fallback to undecorated one.

Yes.

I think the correct solution here is 2.

Re: [PATCH] Windows 32-bit DLL name decoration

[Magnus Ihse Bursie]

On 2018-12-12 12:35, Alan Bateman wrote:

On 12/12/2018 11:14, Magnus Ihse Bursie wrote:

:

I searched the code for "jdwpTransport_On" to see of there was any 
corresponding OnUnload function (or other), but could not find any.
That's right, an unload wasn't needed when that SPI was originally 
added but could be added in the event that some future debugger agent 
need it. The SPI is a supported/documented JDK-specific mechanism, its 
"spec" is hosted here:


https://docs.oracle.com/en/java/javase/11/docs/specs/jdwp/jdwp-transport.html 

... yet in all that time, we have not fully supported the spec on 
Windows 32. :-( (We never discovered this because of lack of testing, I 
presume, and that our internal usage empoyed a questonable workaround.)




I see this thread is touching on the functions exported by libjli. 
This library is part of the launcher and shouldn't be used directly by 
anything outside of the JDK. However we have to be careful because 
JavaFX `javapackager` tool has been using it. There's a JEP to bring a 
similar tool, jpackage in the current proposal, that will supersede it 
but in the mean time we need to be careful not to break it. A second 
issue is that the libjli is listed in the property list (Info.plist) 
on macOS. This came from Apple in 7u4 and periodically things show up 
that are using it, e.g. that recent breakage with Eclipse that was 
never fully diagnosed but we think it was using Info.plist.
The latest patch, as suggested, will not modify JLI, but instead fix the 
broken behaviour of JDWP on Windows 32.


/Magnus

Re: [PATCH] Windows 32-bit DLL name decoration

[Magnus Ihse Bursie]

On 2018-12-11 18:16, Alexey Ivanov wrote:

Hi Simon,

Thank you for your comments.

The updated webrev:
http://cr.openjdk.java.net/~aivanov/8214122/webrev.01/

Indeed, it looks much cleaner.

Yes! This seems like the correct fix.

Ship it! :)

/Magnus



Regards,
Alexey

On 11/12/2018 16:43, Simon Tooke wrote:

On 2018-12-11 10:05 a.m., Alexey Ivanov wrote:

Hi everyone,

I came up with the following patch:
http://cr.openjdk.java.net/~aivanov/8214122/webrev.00/

It specifically addresses the problem in JDK-8214122 where on 32 bit
Windows jdwpTransport_OnLoad can exported with its plain and
__stdcall-mangled name. I used conditional compilation so that for
other platforms the code remains as it is now.

jshell starts successfully with this fix; an IDE debugger works as 
well.



I am not a reviewer, but this patch only works for the specific case
under discussion; the '@16' refers to the reserved stack size in the
Pascal calling convention.  So, the patch only works for 16 bytes of
parameters.  To be generic, the routine needs to have the stack size
passed in by the caller.  If a generic fix isn't desired (and I hope it
is), I'd prefer to see the caller simply pass the decorated or
undecorated name depending on the Win32/64 defines.

 #if defined(_WIN32) && !defined(_WIN64) onLoad =
 (jdwpTransport_OnLoad_t) dbgsysFindLibraryEntry(handle,
 "_jdwpTransport_OnLoad@16"); #else onLoad = 
(jdwpTransport_OnLoad_t)

 dbgsysFindLibraryEntry(handle, "jdwpTransport_OnLoad"); #endif


Thanks,
-Simon


Regards,
Alexey

https://bugs.openjdk.java.net/browse/JDK-8214122

On 10/12/2018 15:11, Magnus Ihse Bursie wrote:

Since removing JNICALL is not an option, there are only two options:

1. Add |/export| option to the Makefile or pragma-comment to the
source file;
2. Lookup the decorated name |_jdwpTransport_OnLoad@16| for Win32
with fallback to undecorated one.

Yes.

I think the correct solution here is 2.

Re: [PATCH] Windows 32-bit DLL name decoration

[Magnus Ihse Bursie]

On 2018-12-11 18:22, Bob Vandette wrote:

Hotspot has had support for decorated and non-decorated names for the JNI_OnLoad
functions.  Perhaps you should just follow the same procedure for the debug
library.

#define JNI_ONLOAD_SYMBOLS   {"_JNI_OnLoad@8", "JNI_OnLoad"}
#define JNI_ONUNLOAD_SYMBOLS {"_JNI_OnUnload@8", "JNI_OnUnload"}
#define JVM_ONLOAD_SYMBOLS  {"_JVM_OnLoad@12", "JVM_OnLoad"}
#define AGENT_ONLOAD_SYMBOLS{"_Agent_OnLoad@12", "Agent_OnLoad"}
#define AGENT_ONUNLOAD_SYMBOLS  {"_Agent_OnUnload@4", "Agent_OnUnload"}
#define AGENT_ONATTACH_SYMBOLS  {"_Agent_OnAttach@12", “Agent_OnAttach”}
Yes, that sounds mostly reasonable. The latest iteration of the patch is 
doing just this.


I searched the code for "jdwpTransport_On" to see of there was any 
corresponding OnUnload function (or other), but could not find any. But 
if there are other *_OnEvent() functions in the JDK code, they should be 
fixed too to handle decorated names. To the best of my knowledge, 
jdwpTransport_OnLoad was the only such *_OnEvent function that did not 
handle decorated names. I searched the code base for the pattern 
'[a-zA-Z]+_On[A-Z][a-zA-Z]*' and skimmed the result, but could not find 
anything else apart from those listed by you above, and the 
jdwpTransport_OnLoad. Any knowledge of additional such functions but 
with different names must come from the component teams.


/Magnus


Bob.



On Dec 11, 2018, at 11:43 AM, Simon Tooke  wrote:

On 2018-12-11 10:05 a.m., Alexey Ivanov wrote:

Hi everyone,

I came up with the following patch:
http://cr.openjdk.java.net/~aivanov/8214122/webrev.00/

It specifically addresses the problem in JDK-8214122 where on 32 bit
Windows jdwpTransport_OnLoad can exported with its plain and
__stdcall-mangled name. I used conditional compilation so that for
other platforms the code remains as it is now.

jshell starts successfully with this fix; an IDE debugger works as well.


I am not a reviewer, but this patch only works for the specific case
under discussion; the '@16' refers to the reserved stack size in the
Pascal calling convention.  So, the patch only works for 16 bytes of
parameters.  To be generic, the routine needs to have the stack size
passed in by the caller.  If a generic fix isn't desired (and I hope it
is), I'd prefer to see the caller simply pass the decorated or
undecorated name depending on the Win32/64 defines.

#if defined(_WIN32) && !defined(_WIN64) onLoad =
(jdwpTransport_OnLoad_t) dbgsysFindLibraryEntry(handle,
"_jdwpTransport_OnLoad@16"); #else onLoad = (jdwpTransport_OnLoad_t)
dbgsysFindLibraryEntry(handle, "jdwpTransport_OnLoad"); #endif


Thanks,
-Simon


Regards,
Alexey

https://bugs.openjdk.java.net/browse/JDK-8214122

On 10/12/2018 15:11, Magnus Ihse Bursie wrote:

Since removing JNICALL is not an option, there are only two options:

1. Add |/export| option to the Makefile or pragma-comment to the
source file;
2. Lookup the decorated name |_jdwpTransport_OnLoad@16| for Win32
with fallback to undecorated one.

Yes.

I think the correct solution here is 2.

Re: Proposal: Use new JDK_EXPORT decorator instead of JNIEXPORT

[Magnus Ihse Bursie]

On 2018-12-11 23:47, David Holmes wrote:

On 12/12/2018 12:34 am, Magnus Ihse Bursie wrote:



On 2018-12-11 00:23, David Holmes wrote:

Hi Magnus,

On 10/12/2018 11:19 pm, Magnus Ihse Bursie wrote:
I propose that we introduce a new define, available to all JDK 
native files (Hotspot included), called JDK_EXPORT. The behavior of 
this symbol will be very similar (as of now, in fact identical) to 
JNIEXPORT; however, the semantics will not.


Currently, we "mis-use" the JNIEXPORT define to mark a function for 
exporting from the library. The problem with this is that JNIEXPORT 
is part of the JNI interface, and is supposed to be used when C 
programs interact with Java. And, when doing this, the function 
should be fully decorated like this: "JNIEXPORT foo JNICALL".


I've seen a lot of the emails on this issue and I don't fully 
understand what has been going wrong. But the intent is obviously 
the JNIEXPORT represents what is needed to export this function for 
use by JNI, while JNICALL defines the calling convention. I agree 
there may be some mistmatch when functions are actually not intended 
for general export outside the JDK but are only for internal JDK use.


We do have many such JNI exports in our native libraries, but we 
also have a lot of other, non-JNI exports, where one native library 
just provides an interface to other libraries. In these cases, we 
have still used JNIEXPORT for the functionality of getting the 
function exported, but we have not been consistent in our use of 
JNICALL. This has caused us way too much trouble for something that 
should Just Work.


Are you suggesting that the interface between different libraries in 
the JDK should not be a JNI interface? Is this because you think the 
functions in these libraries are only for JDK internal use or ... ??


I therefore propose that we define "JDK_EXPORT", with the same 
behavior as JNIEXPORT (that is, flagging the function for external 
visibility in the resulting native library), but which is *not* 
supposed to be exported to Java code using JNI, nor supposed to be 
decorated with 


Just a clarification there. JNI functions are not exported to Java 
code, they are exported to native code. Java code can declare native 
methods and those native methods must be written as JNI functions, 
but that's not what we are discussing. Libraries expose a JNI 
interface (a set of functions in the library) that can be called by 
application native code, using JNI.
We're apparently looking at "JNI" and "exporting" from two opposite 
sides here. :-) Just to make everything clear: If I have a Java class

class MyClass {
   public static void native myNativeFunc();
}

then I have one half of the JNI function, the Java half. This must be 
matched by a corresponding implementation in native, like this:

JNIEXPORT void JNICALL
Java_MyClass_myNativeFunc(void) {
// ... do stuff
}

And this is the native half of the JNI function. Right? Let's leave 
aside which side is "exporting" to the other for now. :-)


This way of setting up native functions that can be called from Java 
is what I refer to as JNI. And when you declare a native JNI 
function, you *must* use both JNIEXPORT and JNICALL. Alright?


We do have a lot of those functions in our native libraries. And they 
are correct just the way they are.


Yep all well and good. A function declared native in Java must have an 
implementation as you describe. But not all native functions exist to 
provide the native-half of a Java native function!


However, we also have *other* native functions, that are exported, 
not as JNI functions that should be called from Java, but as normal 
native library functions that should be called by other native code. 
Okay so far? And *those* functions have been problematic in how we 
decorate 


But there are again two cases. Those functions exported from a library 
that are expected to be called from external code using the JNI 
interface mechanism - such as all the JNI functions and JVM TI 
functions we export from the JVM - and those "exported" for access 
between libraries within the JDK (such as all the JVM_* functions in 
libjvm).


I think it is only the second group that should be addressed by your 
JDK_EXPORT proposal - though I'm not completely clear exactly how to 
identify them.

Alright, now I think we're on the same page again. :)

Yes, this is what I'm saying. I'm not proposing to modify public APIs.

You identify external APIs by the fact that they are documented. And if 
you are unsure, you ask Jon. ;-)




them. My proposal is that we *refrain* from using JNIEXPORT for those 
functions, and instead use JDK_EXPORT as name for the macro that 
decorates them as exported. That way, we can clearly see that a 
function like this:


JDK_EXPORT void
JLI_ReadEnv(char* env);

is correctly declared, and will be exported to other native 
libraries, but not to Java.


The issue is no

Re: Proposal: Use new JDK_EXPORT decorator instead of JNIEXPORT

[Magnus Ihse Bursie]

On 2018-12-12 09:40, David Holmes wrote:

On 12/12/2018 5:44 pm, Volker Simonis wrote:
On Tue, Dec 11, 2018 at 11:47 PM David Holmes 
 wrote:


On 12/12/2018 12:34 am, Magnus Ihse Bursie wrote:



On 2018-12-11 00:23, David Holmes wrote:

Hi Magnus,

On 10/12/2018 11:19 pm, Magnus Ihse Bursie wrote:
I propose that we introduce a new define, available to all JDK 
native

files (Hotspot included), called JDK_EXPORT. The behavior of this
symbol will be very similar (as of now, in fact identical) to
JNIEXPORT; however, the semantics will not.

Currently, we "mis-use" the JNIEXPORT define to mark a function for
exporting from the library. The problem with this is that JNIEXPORT
is part of the JNI interface, and is supposed to be used when C
programs interact with Java. And, when doing this, the function
should be fully decorated like this: "JNIEXPORT foo JNICALL".


I've seen a lot of the emails on this issue and I don't fully
understand what has been going wrong. But the intent is obviously the
JNIEXPORT represents what is needed to export this function for 
use by

JNI, while JNICALL defines the calling convention. I agree there may
be some mistmatch when functions are actually not intended for 
general

export outside the JDK but are only for internal JDK use.

We do have many such JNI exports in our native libraries, but we 
also

have a lot of other, non-JNI exports, where one native library just
provides an interface to other libraries. In these cases, we have
still used JNIEXPORT for the functionality of getting the function
exported, but we have not been consistent in our use of JNICALL. 
This

has caused us way too much trouble for something that should Just
Work.


Are you suggesting that the interface between different libraries in
the JDK should not be a JNI interface? Is this because you think the
functions in these libraries are only for JDK internal use or ... ??


I therefore propose that we define "JDK_EXPORT", with the same
behavior as JNIEXPORT (that is, flagging the function for external
visibility in the resulting native library), but which is *not*
supposed to be exported to Java code using JNI, nor supposed to be
decorated with


Just a clarification there. JNI functions are not exported to Java
code, they are exported to native code. Java code can declare native
methods and those native methods must be written as JNI functions, 
but
that's not what we are discussing. Libraries expose a JNI 
interface (a

set of functions in the library) that can be called by application
native code, using JNI.

We're apparently looking at "JNI" and "exporting" from two opposite
sides here. :-) Just to make everything clear: If I have a Java class
class MyClass {
public static void native myNativeFunc();
}

then I have one half of the JNI function, the Java half. This must be
matched by a corresponding implementation in native, like this:
JNIEXPORT void JNICALL
Java_MyClass_myNativeFunc(void) {
// ... do stuff
}

And this is the native half of the JNI function. Right? Let's leave
aside which side is "exporting" to the other for now. :-)

This way of setting up native functions that can be called from 
Java is
what I refer to as JNI. And when you declare a native JNI function, 
you

*must* use both JNIEXPORT and JNICALL. Alright?

We do have a lot of those functions in our native libraries. And they
are correct just the way they are.


Yep all well and good. A function declared native in Java must have an
implementation as you describe. But not all native functions exist to
provide the native-half of a Java native function!


However, we also have *other* native functions, that are exported, not
as JNI functions that should be called from Java, but as normal native
library functions that should be called by other native code. Okay so
far? And *those* functions have been problematic in how we decorate


But there are again two cases. Those functions exported from a library
that are expected to be called from external code using the JNI
interface mechanism - such as all the JNI functions and JVM TI 
functions

we export from the JVM - and those "exported" for access between
libraries within the JDK (such as all the JVM_* functions in libjvm).

I think it is only the second group that should be addressed by your
JDK_EXPORT proposal - though I'm not completely clear exactly how to
identify them.


them. My proposal is that we *refrain* from using JNIEXPORT for those
functions, and instead use JDK_EXPORT as name for the macro that
decorates them as exported. That way, we can clearly see that a 
function

like this:

JDK_EXPORT void
JLI_ReadEnv(char* env);

is correctly declared, and will be exported to other native libraries,
but not to Java.


The issue is not whether it is "exported to Java"** but whether it is
exported using the JNI mechanism such that other native code calls it
using the JNI mechanism.

** There is 

Re: Proposal: Use new JDK_EXPORT decorator instead of JNIEXPORT

[Magnus Ihse Bursie]

On 2018-12-11 00:23, David Holmes wrote:

Hi Magnus,

On 10/12/2018 11:19 pm, Magnus Ihse Bursie wrote:
I propose that we introduce a new define, available to all JDK native 
files (Hotspot included), called JDK_EXPORT. The behavior of this 
symbol will be very similar (as of now, in fact identical) to 
JNIEXPORT; however, the semantics will not.


Currently, we "mis-use" the JNIEXPORT define to mark a function for 
exporting from the library. The problem with this is that JNIEXPORT 
is part of the JNI interface, and is supposed to be used when C 
programs interact with Java. And, when doing this, the function 
should be fully decorated like this: "JNIEXPORT foo JNICALL".


I've seen a lot of the emails on this issue and I don't fully 
understand what has been going wrong. But the intent is obviously the 
JNIEXPORT represents what is needed to export this function for use by 
JNI, while JNICALL defines the calling convention. I agree there may 
be some mistmatch when functions are actually not intended for general 
export outside the JDK but are only for internal JDK use.


We do have many such JNI exports in our native libraries, but we also 
have a lot of other, non-JNI exports, where one native library just 
provides an interface to other libraries. In these cases, we have 
still used JNIEXPORT for the functionality of getting the function 
exported, but we have not been consistent in our use of JNICALL. This 
has caused us way too much trouble for something that should Just 
Work.


Are you suggesting that the interface between different libraries in 
the JDK should not be a JNI interface? Is this because you think the 
functions in these libraries are only for JDK internal use or ... ??


I therefore propose that we define "JDK_EXPORT", with the same 
behavior as JNIEXPORT (that is, flagging the function for external 
visibility in the resulting native library), but which is *not* 
supposed to be exported to Java code using JNI, nor supposed to be 
decorated with 


Just a clarification there. JNI functions are not exported to Java 
code, they are exported to native code. Java code can declare native 
methods and those native methods must be written as JNI functions, but 
that's not what we are discussing. Libraries expose a JNI interface (a 
set of functions in the library) that can be called by application 
native code, using JNI.
We're apparently looking at "JNI" and "exporting" from two opposite 
sides here. :-) Just to make everything clear: If I have a Java class

class MyClass {
  public static void native myNativeFunc();
}

then I have one half of the JNI function, the Java half. This must be 
matched by a corresponding implementation in native, like this:

JNIEXPORT void JNICALL
Java_MyClass_myNativeFunc(void) {
// ... do stuff
}

And this is the native half of the JNI function. Right? Let's leave 
aside which side is "exporting" to the other for now. :-)


This way of setting up native functions that can be called from Java is 
what I refer to as JNI. And when you declare a native JNI function, you 
*must* use both JNIEXPORT and JNICALL. Alright?


We do have a lot of those functions in our native libraries. And they 
are correct just the way they are.


However, we also have *other* native functions, that are exported, not 
as JNI functions that should be called from Java, but as normal native 
library functions that should be called by other native code. Okay so 
far? And *those* functions have been problematic in how we decorate 
them. My proposal is that we *refrain* from using JNIEXPORT for those 
functions, and instead use JDK_EXPORT as name for the macro that 
decorates them as exported. That way, we can clearly see that a function 
like this:


JDK_EXPORT void
JLI_ReadEnv(char* env);

is correctly declared, and will be exported to other native libraries, 
but not to Java.


Just to clarify, this has nothing to do with if this is a officially 
supported API or not. In general though, I assume that most (if not 
all?) of our exported functions (apart from the JNI_* stuff) is supposed 
to be consumed by other libraries in the JDK, and is not a public API.


/Magnus





JNICALL. All current instances of JNIEXPORT which is not pure JNI 
native functions should be changed to use JDK_EXPORT instead.


I further propose that this macro should reside in a new file 
"jdk.h", placed in the new directory 
src/java.base/share/native/include/internal. This header file path 
will automatically be provided to all native libraries, but not 
copied to the JDK being built. (The existence of a "include/internal" 
directory with this behavior has been discussed before. There are 
more files that ought to be moved there, if/when it is created.) I 
believe in many cases the #include "jni.h" can be just modified to 
#include "#jdk.h", since most native code will not require "jni.h" 
un

Re: [PATCH] Windows 32-bit DLL name decoration

[Magnus Ihse Bursie]

On 2018-12-10 16:02, Alexey Ivanov wrote:



On 10/12/2018 10:41, Magnus Ihse Bursie wrote:



On 2018-12-07 22:18, Simon Tooke wrote:

Looking at the code, it seems (to me) the "correct" thing to do is to is
to create a Windows-specific version of dbgsysBuildFunName() in
linker_md.c, and have it decorate the function name as appropriate for
32-bit windows.  Note that in transport.c, you'd need to pass in the
parameter stack size (the bit after the @), but it could at least behave
properly on 32 vs 64 bit Windows.

Notice this approach is already used for the library name, via
dbgsysBuildLibName()

If the function is never intended to be called by Java via JNI, then it
should never have been declared JNICALL in the first place - but that
ship has sailed.

I don't think changing the decorated exported name to undecorated is a
good idea, as it obfuscates the calling convention used.

Good analysis, Simon. I agree.

I have now looked more into the situation. In fact, it looks a lot 
like JDWP has been broken on Windows 32-bit for a long time, but we 
have patched around the issue in the JDK by using /export. This is 
the spec we're dealing with: 
https://docs.oracle.com/javase/10/docs/specs/jdwp/jdwp-transport.html. 
I quote:


The transport library must export an/onload/function with the 
following prototype:


|JNIEXPORT jint JNICALL jdwpTransport_OnLoad(JavaVM *jvm, 
jdwpTransportCallback *callback, jint version, jdwpTransportEnv** env);|


This function will be called by the JDWP (or other agent) when the 
library is loaded.





Nothing here says anything that the function should be exported using 
anything other than the normal _stdcall (implied by JNICALL) name 
mangling rules. However, JDWP has not been working according to the 
spec and looking for the correct symbol, and while we could have 
noticed that in the JDK, we didn't, because someone (long ago) added 
/export:jdwpTransport_OnLoad as a workaround to the affected 
libraries, instead of fixing JDWP.


This means that we cannot change the calling convention: it must stay 
as it is now.


I think JDWP has always been working according to the spec. Using 
__stdcall is the recommended calling convention for Win32 and for DLLs 
in particular. Usually function names are exported undecorated in DLL. 
(If my memory serves me well, older Microsoft tools exported |extern 
"C" __stdcall| functions undecorated.)
So then the question becomes: what does the spec mean? That the 
__stdcall convention should be used, or that the function name should be 
explicitly exported under a non __stdcall-convention name? Neither 
behavior seems clearly written out, so this is left to an implicit 
interpretation of what that "usually" means..?


I believe this — exporting the undecorated function names — allows for 
interoperability between 32 bit and 64 bit in cases where you load a 
DLL and dynamically look up a function in it.


There were too many /export options to export Win32 functions 
undecorated for this one to be an accident rather than the intention.

How do you mean?


Now, given that we've shipped code that didn't adhere to the specs 
for so long, we can probably not break that behavior. Instead, I 
propose we amend dbgsysBuildFunName() so that on Windows 32-bit, it 
looks for both the "jdwpTransport_OnLoad", and the symbol as mangled 
by _stdcall rules. I also propose that if both symbols are present, 
the _stdcall named one should take precedence, since that's according 
to the spec (and the other is just a fallback to maintain backwards 
compatibility).


Since removing JNICALL is not an option, there are only two options:

1. Add |/export| option to the Makefile or pragma-comment to the 
source file;
2. Lookup the decorated name |_jdwpTransport_OnLoad@16| for Win32 with 
fallback to undecorated one.

Yes.

I think the correct solution here is 2.



I just wonder how it's possible to implement a generic 
|dbgsysBuildFunName| for an arbitrary function without knowing the 
size of function parameters. Could it be the reason why most DLLs 
export __stdcall functions undecorated?
(I assume you mean dbgsysFindLibraryEntry; there is a dbgsysBuildFunName 
as well but it appears to be dead code.)


The dbgsysFindLibraryEntry does not need to work with an arbitrary 
function. It's implemented in jdk.jdwp.agent, for the sole reason of 
locating the jdwpTransport_OnLoad function.


In general, I assume this to hold true. If you don't know the signature 
of the function you want to call, you're screwed anyway, regardless of 
calling convention.


/Magnus



Regards,
Alexey



/Magnus

-Simon Tooke


On 12/7/2018 2:09 PM, Alexey Ivanov wrote:

Hi Andrew,

Sorry for my belated reply.
Yes, it's also an option… however, this solution seems to be
overcomplicated to export one function or two. The calling convention
of exported functions for JVM cannot be changed, they can be called
from third-party software.

If the funct

Proposal: Use new JDK_EXPORT decorator instead of JNIEXPORT

[Magnus Ihse Bursie]

I propose that we introduce a new define, available to all JDK native 
files (Hotspot included), called JDK_EXPORT. The behavior of this symbol 
will be very similar (as of now, in fact identical) to JNIEXPORT; 
however, the semantics will not.


Currently, we "mis-use" the JNIEXPORT define to mark a function for 
exporting from the library. The problem with this is that JNIEXPORT is 
part of the JNI interface, and is supposed to be used when C programs 
interact with Java. And, when doing this, the function should be fully 
decorated like this: "JNIEXPORT foo JNICALL".


We do have many such JNI exports in our native libraries, but we also 
have a lot of other, non-JNI exports, where one native library just 
provides an interface to other libraries. In these cases, we have still 
used JNIEXPORT for the functionality of getting the function exported, 
but we have not been consistent in our use of JNICALL. This has caused 
us way too much trouble for something that should Just Work.


I therefore propose that we define "JDK_EXPORT", with the same behavior 
as JNIEXPORT (that is, flagging the function for external visibility in 
the resulting native library), but which is *not* supposed to be 
exported to Java code using JNI, nor supposed to be decorated with 
JNICALL. All current instances of JNIEXPORT which is not pure JNI native 
functions should be changed to use JDK_EXPORT instead.


I further propose that this macro should reside in a new file "jdk.h", 
placed in the new directory src/java.base/share/native/include/internal. 
This header file path will automatically be provided to all native 
libraries, but not copied to the JDK being built. (The existence of a 
"include/internal" directory with this behavior has been discussed 
before. There are more files that ought to be moved there, if/when it is 
created.) I believe in many cases the #include "jni.h" can be just 
modified to #include "#jdk.h", since most native code will not require 
"jni.h" unless actually doing JNI calls -- most have included this file 
to get the JNIEXPORT macro, which would explain the pervasive use of 
#include "jni.h" in our code base.


Thoughts?

/Magnus

Re: [12] RFR for JDK-8215123: Crash in runtime image built with jlink --compress=2

[Magnus Ihse Bursie]

Hi Alexey,

On 2018-12-10 13:32, Alexey Ivanov wrote:

Hi,

Could you please review the following fix for jdk12?

bug: https://bugs.openjdk.java.net/browse/JDK-8215123
webrev: http://cr.openjdk.java.net/~aivanov/8215123/webrev.00/

The fix looks good to me.

/Magnus



The problem is that calling convention was changed on ZIP_InflateFully 
function in zip.dll. Yet it hasn't been updated in jimage.dll which 
uses this function.


It could be considered a regression from JDK-8200178 [1] and 
JDK-8201226 [2]. After the first fix, ZIP_InflateFully was exported 
with a mangled name so that function could not be found in zip.dll. 
After the second fix, the function uses __cdecl; mismatched calling 
convention leads to stack corruption.


The fix is to remove JNICALL (__stdcall) from ZIP_InflateFully 
function prototype in imageDecompressor.cpp so that the calling 
convention is the same.



This issue was brought up by Ali İnce from AdoptOpenJDK:
http://mail.openjdk.java.net/pipermail/build-dev/2018-December/024300.html 




Thank you in advance.

Regards,
Alexey

[1] https://bugs.openjdk.java.net/browse/JDK-8200178
[2] https://bugs.openjdk.java.net/browse/JDK-8201226

Re: RFR: 8214077: test java/io/File/SetLastModified.java fails on ARM32

[Magnus Ihse Bursie]

On 2018-12-10 11:56, Nick Gasson (Arm Technology China) wrote:

Hi,

Any update on this one? Or do we want to give up on it until JDK-8165620
is implemented?
I think Alan reviewed it as OK with just a minor fix, and offered to 
sponsor it for you.


Then the discussion started about some major changes, rewriting 
configure to provide individual support for every possible system 
function... I recommend that you go with the fix that solves your problem.


In fact, I disagree with the premise of JDK-8165620. I think it's better 
to do what you're doing here, to update the source code, so that it is 
clear that we know that we're handing 64-bit data.


/Magnus


Thanks,
Nick


On 28/11/2018 11:33, Martin Buchholz wrote:


On Tue, Nov 27, 2018 at 7:25 PM, Nick Gasson mailto:nick.gas...@arm.com>> wrote:

 > I missed one thing then looking at this. In TimeZone_md.c it should be
 > #ifdef MACOSX rather than #ifndef. I can sponsor the change for you but
 > I need to change this one line before pushing.

 Hi Alan,

 Yes feel free to modify it. I think looking at the at other files
 with these #defines more closely, is it the case that the #ifndef
 MACOSX check is only required for statvfs64? As in
 e.g. UnixNativeDispatcher.c. So TimeZone_md.c should look like
 this:

 #if defined(_ALLBSD_SOURCE)
#define stat64 stat
#define lstat64 lstat
#define fstat64 fstat
 #endif

 I don't have access to any OS X machines to test unfortunately.

 But I wonder if a better way to handle this is to check for the
 presence of the stat64 functions in the configure script, and
 then we could just write something like this, which would be a
 lot clearer:

 #if !defined(HAVE_STAT64)
#define stat64 stat
 #endif



The best way is to implement

https://bugs.openjdk.java.net/browse/JDK-8165620

"Entire JDK should be built with -D_FILE_OFFSET_BITS=64"

but yes, another good way is to do as you suggest, have configure define
HAVE_ for all known functions with a 64-bit variant and then put
them into a header file with proper ifdefs for platforms that don't have
them.

You could also "simply" add
#define _FILE_OFFSET_BITS 64
but you have to do it for cliques of files that share ambiguously sized
data simultaneously.

Re: RFR: 8214533 IBM-29626C is required for AIX default charset

[Magnus Ihse Bursie]

On 2018-12-07 21:20, Roger Riggs wrote:

Hi,

It is a nice feature that charsets are selected at build time using 
the stdcs-xxx files.
This change breaks that pattern and embeds os specific information in 
more than one place.

That does not seem like an improvement.  Is there any alternative?

I agree. Why is it not enough just to add it to stdcs-aix?

/Magnus


Thanks, Roger


On 12/06/2018 12:05 PM, Ichiroh Takiguchi wrote:

Hello.
(I'm sorry, I made a mistake, I forgot to change Subject)

Could you review the fix ?

Bug:https://bugs.openjdk.java.net/browse/JDK-8214533
Change: https://cr.openjdk.java.net/~itakiguchi/8214533/webrev.00/

IBM29626C charset is required for AIX default charset.
Java cannot start because of java/lang/ExceptionInInitializerError on 
AIX ja_JP locale.


To build team,
I'd like to change following charsetmapping tool.
* make/jdk/src/classes/build/tools/charsetmapping/Main.java
* make/jdk/src/classes/build/tools/charsetmapping/SPI.java
* make/jdk/src/classes/build/tools/charsetmapping/SRC.java

build.tools.charsetmapping,Main supports "os" tag, but it seems it's 
not used.

Currently, "os" supports "windows" or "unix".
I extended "os" tag's feature.
If "os aix" is there, this charset is only added into AIX platform.
(I assume "type template" should be used)
"aix" comes from "stdcs-aix" file name.
==
charset x-IBM29626C IBM29626C
package sun.nio.cs.ext
typetemplate
os  aix   <=
alias   cp29626C   # JDK historical
alias   ibm29626C
alias   ibm-29626C
alias   29626C
alias   ibm-eucjp
==

If cs.os is null,
this charset is stored into gensrc directory.
Charset name is added into StandardCharsets.java or 
ExtendedCharsets.java

If cs.os is "false",
this charset is NOT stored into gensrc directory.
Charset name is NOT added into StandardCharsets.java or 
ExtendedCharsets.java


"os" tag supports multiple entries by using ",", like "aix,linux"
Then we can store new charset into 
src/jdk.charsets/share/classes/sun/nio/cs/ext/ directory



$ locale charmap
IBM-eucJP
$ jshell
|  JShell  --  12-internal
|: /help intro

jshell> var cs = java.nio.charset.Charset.defaultCharset()
cs ==> x-IBM29626C

jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.IBM29626C"

jshell> System.out.println(String.join("\n", cs.aliases()))
cp29626C
ibm-29626C
ibm-eucjp
ibm29626C
29626C

jshell> /exit
|
$

I tested Linux and Windows build.
==
$ grep 29626 build.log
IBM29626C, x-IBM29626C, null, sun.nio.cs.ext, template  false

$ find support/gensrc/  | grep 29626

$ find support/gensrc/  | grep Charsets
support/gensrc/java.base/sun/nio/cs/StandardCharsets.java
support/gensrc/jdk.charsets/sun/nio/cs/ext/ExtendedCharsets.java

$ find support/gensrc/  | grep Charsets | xargs grep 29626

$
==

I'd like to obtain a sponsor for this issue.

Thanks,
Ichiroh Takiguchi
IBM Japan, Ltd.

On 2018-11-28 19:10, Magnus Ihse Bursie wrote:

On 2018-11-28 10:36, Alan Bateman wrote:

On 28/11/2018 09:28, Magnus Ihse Bursie wrote:
I'm quite unsatisfied with the current handling of character sets 
in the build in general. :-( I'd really like to modernize it. I 
have a, slightly fuzzy, laundry list of things I want to fix from 
a build perspective, but I'm not sure of what "external" 
requirements are coming from AIX and the general core-libs agenda 
regarding character sets in general.


I think there is a good opportunity to solve many problems at the 
same time here, as long as everyone agrees on what is the 
preferred outcome.
The support in the build to configure the charsets to include in 
java.base on each platform has been working well. Charsets that 
aren't in java.base go into the jdk.charsets service provider 
module and that has been working well too. From the result point of 
view, perhaps, but definitely not from the build perspective. ;-) 
But yes, I understand this is functionality that should be kept.
One thing that we lack is some way to add charsets for specific 
platforms and this comes up with the IBM patches where they are 
looking to adding several additional IBM charsets. One starting 
point that we've touched on in several threads here is dropping the 
EBCDIC charsets from the main stream builds. Going there will need 
build support.

So build support for trivially adding specific charsets to specific
platforms? Both to java.base (for AIX) and jdk.charsets, I presume,
then?

Can you expand on the issue of dropping ebcdic? What's the problem
that needs build support?

/Magnus



-Alan

Re: [PATCH] Windows 32-bit DLL name decoration

[Magnus Ihse Bursie]

On 2018-11-27 19:49, Andrew Luo wrote:

By the way, in hotspot we are generating a .def file dynamically while keeping 
the JNICALL calling convention (for symbols such as JNI_CreateJavaVM) I believe 
(just by looking through the code in 
http://hg.openjdk.java.net/jdk/jdk/file/44fe5fab538a/make/hotspot/lib/JvmMapfile.gmk,
 unless this code is inactive - someone who knows this area better than me 
might want to comment...).  Shouldn't the same approach be taken here as well?
This is code that is on it's way out. It should not be copied elsewhere. 
On the contrary, the long-term plan is to use compiler attributes for 
symbol visibility in Hotspot, just as in the rest of the JDK.


/Magnus


Thanks
Andrew

-Original Message-
From: core-libs-dev  On Behalf Of Ali 
Ince
Sent: Monday, November 19, 2018 2:08 PM
To: Alexey Ivanov ; build-dev ; 
core-libs 
Subject: Re: [PATCH] Windows 32-bit DLL name decoration

Hi Alexey,

I don’t have an account on JBS as I’m not an author yet, my OCA has just been 
processed. Would it be possible for someone with an author status to create an 
issue?

Regards,

Ali


On 19 Nov 2018, at 12:12, Alexey Ivanov  wrote:

Hi Ali,

The fix looks good to me provided it resolves your problem.
I am not a reviewer so you'll have to get OK from reviewers, likely from 
build-dev and from core-libs.

Have you submitted the issue in JBS?

You have to sign OCA to be able to contribute to OpenJDK:
https://openjdk.java.net/contribute/
and also
https://openjdk.java.net/sponsor/

Regards,
Alexey

On 18/11/2018 12:07, Ali İnce wrote:

Hi Alexey,

Below is a new patch content that removes JNICALL modifiers from the exported 
functions of interest. This results in exported functions not to be name 
decorated and use __cdecl calling convention.

Do you have any more suggestions, or would you please guide me on next steps?

Thanks,

Ali

# HG changeset patch
# User Ali Ince 
# Date 1542542415 0
#  Sun Nov 18 12:00:15 2018 +
# Node ID a41df44e13e1b761f4b3f05a17ed18953067ae39
# Parent  16d5ec7dbbb49ef3f969e34be870e3f917ea89ff
Remove JNICALL modifiers to prevent name decoration on 32-bit windows
builds

diff -r 16d5ec7dbbb4 -r a41df44e13e1 
src/jdk.jdi/share/native/libdt_shmem/shmemBack.c
--- a/src/jdk.jdi/share/native/libdt_shmem/shmemBack.c  Tue Aug 14 14:28:23 
2018 +0200
+++ b/src/jdk.jdi/share/native/libdt_shmem/shmemBack.c  Sun Nov 18 12:00:15 
2018 +
@@ -339,7 +339,7 @@
  return JDWPTRANSPORT_ERROR_NONE;  }  -JNIEXPORT jint JNICALL
+JNIEXPORT jint
  jdwpTransport_OnLoad(JavaVM *vm, jdwpTransportCallback* cbTablePtr,
   jint version, jdwpTransportEnv** result)  {
diff -r 16d5ec7dbbb4 -r a41df44e13e1 
src/jdk.jdwp.agent/share/native/include/jdwpTransport.h
--- a/src/jdk.jdwp.agent/share/native/include/jdwpTransport.h   Tue Aug 14 
14:28:23 2018 +0200
+++ b/src/jdk.jdwp.agent/share/native/include/jdwpTransport.h   Sun Nov 18 
12:00:15 2018 +
@@ -140,7 +140,7 @@
  void (*free)(void *buffer);  /* Call this for all deallocations */
  } jdwpTransportCallback;
  -typedef jint (JNICALL *jdwpTransport_OnLoad_t)(JavaVM *jvm,
+typedef jint (*jdwpTransport_OnLoad_t)(JavaVM *jvm,
 jdwpTransportCallback 
*callback,
 jint version,
 jdwpTransportEnv**
env); diff -r 16d5ec7dbbb4 -r a41df44e13e1 
src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c
--- a/src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.cTue Aug 
14 14:28:23 2018 +0200
+++ b/src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.cSun Nov 
18 12:00:15 2018 +
@@ -1019,7 +1019,7 @@
  return JDWPTRANSPORT_ERROR_NONE;  }  -JNIEXPORT jint JNICALL
+JNIEXPORT jint
  jdwpTransport_OnLoad(JavaVM *vm, jdwpTransportCallback* cbTablePtr,
   jint version, jdwpTransportEnv** env)  {




On 16 Nov 2018, at 23:03, Alexey Ivanov  wrote:

Hi Ali,

It's exactly what I referred to.

Yes, it does change the calling convention.
Yet it's not a (big) problem because both parties will use the same calling 
convention.

Using a DLL from another build will not be possible. But it's not a good idea 
to do so anyway.


Mapfiles and export options have been removed by 
https://bugs.openjdk.java.net/browse/JDK-8200178 to simplify managing exported 
functions. So that to add or remove an exported function, you don't have modify 
makefiles and/or mapfiles. You just edit the source files.

Regards,
Alexey

On 16/11/2018 22:42, Ali İnce wrote:

Hi Alexey,

Thanks for your reply.

I will definitely give it a try though I’m not sure if that’ll be a breaking 
change. This is because JNICALL modifier is defined to be __stdcall on windows 
which is both specified on jdwpTransport_OnLoad exported functions both for 
dt_socket.dll and dt_shmem.dll.

The __stdcall is ignored on x64 platforms and also name decoration is not 
applied 

Re: [PATCH] JDK-8214122 Prevent name mangling of jdwpTransport_OnLoad in Windows 32-bit DLL name decoration

[Magnus Ihse Bursie]

On 2018-12-07 21:24, Alexey Ivanov wrote:

Hi Ali,

On 06/12/2018 22:49, Ali İnce wrote:

Hi Magnus, Alexey,

I believe we won’t be able to get further opinions from 
serviceability-dev.


Unfortunately, no one has replied as of now.
Have you found any issues with jdwpTransport_OnLoad after removing 
JNICALL?


Andrew Luo suggested using a similar mechanism as is used for jvm.dll 
by using symbol name files mapped by platform (files are under 
make/hotspot/symbols and interestingly windows is almost the only 
platform for which a symbol file doesn’t exist).


Andrew says the .def files are auto-generated for Windows. There's a 
set of shared functions.
JVM cannot change the calling convention, jvm.dll is the public 
interface to JVM.


Another issue, again opened against AdoptOpenJDK 32-bit builds is 
somehow related with the same problem - but this time it is 
jimage.dll depending on zip.dll expecting the ZIP_InflateFully 
function to be decorated with JNICALL - whereas it was removed 
earlier from the implementation and the runtime image just fails with 
access violation just because jimage source code still declared it 
with JNICALL (https://github.com/AdoptOpenJDK/openjdk-build/issues/763).


The usage of ZIP_InflateFully from zip.dll by jimage.dll was 
overlooked during work on 
https://bugs.openjdk.java.net/browse/JDK-8201226.


I can take care of it.
It might be worthwhile to scan the entire code base for JNICALL and 
verify that we have no more mismatches. In general, JNICALL should be 
preserved on all officially supported, exported interfaces. It need not 
be present on interfaces used only internally, and my current thinking 
is that it would be better if it were removed on all internal 
interfaces. But at the very least, it should be consistently specificed 
where exported and imported. (Any misses here is probably due to 
duplicate declarations, instead of properly including header files, so 
that's the correct way to resolve any problems found...)


/Magnus

(I could not build 32 bit Windows. It seem to have overcome the 
problem by adding --disable-warnings-as-errors option to configure.)


However, the report says the resulting image crashes in 64 bit windows 
too which shouldn't be affected by JNICALL mismatch.


I’ve also searched for GetProcAddress calls across the source code - 
and I think it’s important to work on the consistency of JNICALL 
usages across the whole source code.


There are many places where libraries are loaded dynamically or a 
function may be unavailable on older versions of the platform.
Have you identified any other place where functions from JDK DLLs are 
looked up by names?


Another noteworthy thing I’ve noticed is there are still JNICALL 
modifiers for example in 
https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/master/src/jdk.crypto.cryptoki/windows/native/libj2pkcs11/j2secmod_md.c#L49 - 
which I guess will also be reported as broken since these functions 
are again name decorated.


This is a JNI method. It should use JNICALL modifier. If it wasn't 
found, the class sun.security.pkcs11.Secmod would fail to load. I 
guess JVM handles name mangling somehow for native method implementation.


Such functions were never explicitly exported using mapfiles or 
/export options on Windows, at least in the client area. For Linux and 
Solaris, adding or removing a native method required editing mapfiles.


If we’re still determined to remove JNICALL, what about just removing 
__stdcall from JNICALL declaration at 
https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/master/src/java.base/windows/native/include/jni_md.h#L31 ? Wouldn’t 
that be a more consistent move?


We can't do that.
Implementation of Java native methods must use __stdcall calling 
convention.




Regards,

Ali

On 22 Nov 2018, at 17:29, Magnus Ihse Bursie 
<mailto:magnus.ihse.bur...@oracle.com>> wrote:


I think we are in complete agreement. What's missing is some expert 
opinion from serviceability-dev if there is any problem with 
changing the signature. I'd preferably have that.


If no one knows, I'd say, change it, and see if we still pass the 
relevant tests, and if so, ship it.


/Magnus

22 nov. 2018 kl. 18:04 skrev Alexey Ivanov <mailto:alexey.iva...@oracle.com>>:




On 21/11/2018 12:16, Magnus Ihse Bursie wrote:


On 2018-11-20 16:49, Alexey Ivanov wrote:


Hi Ali, Magnus,

I absolutely agree this change has to be reviewed by someone from 
serviceability.


There are several options:

1. Add -export:jdwpTransport_OnLoad to LDFLAGS for Windows
http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023935.html
so it partially reverts the changes from
https://bugs.openjdk.java.net/browse/JDK-8200178

2. Remove JNICALL (__stdcall) modifier, eventually changing the 
calling convention to __cdecl.

http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023969.html

3. Use inline /export option via #pragma:
#pragma comment(linker, "/expo

Re: [PATCH] Windows 32-bit DLL name decoration

[Magnus Ihse Bursie]

On 2018-12-07 22:18, Simon Tooke wrote:

Looking at the code, it seems (to me) the "correct" thing to do is to is
to create a Windows-specific version of dbgsysBuildFunName() in
linker_md.c, and have it decorate the function name as appropriate for
32-bit windows.  Note that in transport.c, you'd need to pass in the
parameter stack size (the bit after the @), but it could at least behave
properly on 32 vs 64 bit Windows.

Notice this approach is already used for the library name, via
dbgsysBuildLibName()

If the function is never intended to be called by Java via JNI, then it
should never have been declared JNICALL in the first place - but that
ship has sailed.

I don't think changing the decorated exported name to undecorated is a
good idea, as it obfuscates the calling convention used.

Good analysis, Simon. I agree.

I have now looked more into the situation. In fact, it looks a lot like 
JDWP has been broken on Windows 32-bit for a long time, but we have 
patched around the issue in the JDK by using /export. This is the spec 
we're dealing with: 
https://docs.oracle.com/javase/10/docs/specs/jdwp/jdwp-transport.html. I 
quote:


The transport library must export an/onload/function with the 
following prototype:


|JNIEXPORT jint JNICALL jdwpTransport_OnLoad(JavaVM *jvm, 
jdwpTransportCallback *callback, jint version, jdwpTransportEnv** env);|


This function will be called by the JDWP (or other agent) when the 
library is loaded.





Nothing here says anything that the function should be exported using 
anything other than the normal _stdcall (implied by JNICALL) name 
mangling rules. However, JDWP has not been working according to the spec 
and looking for the correct symbol, and while we could have noticed that 
in the JDK, we didn't, because someone (long ago) added 
/export:jdwpTransport_OnLoad as a workaround to the affected libraries, 
instead of fixing JDWP.


Now, given that we've shipped code that didn't adhere to the specs for 
so long, we can probably not break that behavior. Instead, I propose we 
amend dbgsysBuildFunName() so that on Windows 32-bit, it looks for both 
the "jdwpTransport_OnLoad", and the symbol as mangled by _stdcall rules. 
I also propose that if both symbols are present, the _stdcall named one 
should take precedence, since that's according to the spec (and the 
other is just a fallback to maintain backwards compatibility).


/Magnus


-Simon Tooke


On 12/7/2018 2:09 PM, Alexey Ivanov wrote:

Hi Andrew,

Sorry for my belated reply.
Yes, it's also an option… however, this solution seems to be
overcomplicated to export one function or two. The calling convention
of exported functions for JVM cannot be changed, they can be called
from third-party software.

If the function is used internally-only, its calling convention can be
changed as long as all components are updated.

Thank you for the pointer to another approach used to handle name
decorations of __stdcall functions. It looks we have to work out a
common approach to this problem.

Regards,
Alexey

On 27/11/2018 18:49, Andrew Luo wrote:

By the way, in hotspot we are generating a .def file dynamically
while keeping the JNICALL calling convention (for symbols such as
JNI_CreateJavaVM) I believe (just by looking through the code in
http://hg.openjdk.java.net/jdk/jdk/file/44fe5fab538a/make/hotspot/lib/JvmMapfile.gmk,
unless this code is inactive - someone who knows this area better
than me might want to comment...).  Shouldn't the same approach be
taken here as well?

Thanks
Andrew

-Original Message-
From: core-libs-dev  On
Behalf Of Ali Ince
Sent: Monday, November 19, 2018 2:08 PM
To: Alexey Ivanov ; build-dev
; core-libs 
Subject: Re: [PATCH] Windows 32-bit DLL name decoration

Hi Alexey,

I don’t have an account on JBS as I’m not an author yet, my OCA has
just been processed. Would it be possible for someone with an author
status to create an issue?

Regards,

Ali


On 19 Nov 2018, at 12:12, Alexey Ivanov 
wrote:

Hi Ali,

The fix looks good to me provided it resolves your problem.
I am not a reviewer so you'll have to get OK from reviewers, likely
from build-dev and from core-libs.

Have you submitted the issue in JBS?

You have to sign OCA to be able to contribute to OpenJDK:
https://openjdk.java.net/contribute/
and also
https://openjdk.java.net/sponsor/

Regards,
Alexey

On 18/11/2018 12:07, Ali İnce wrote:

Hi Alexey,

Below is a new patch content that removes JNICALL modifiers from
the exported functions of interest. This results in exported
functions not to be name decorated and use __cdecl calling convention.

Do you have any more suggestions, or would you please guide me on
next steps?

Thanks,

Ali

# HG changeset patch
# User Ali Ince 
# Date 1542542415 0
#  Sun Nov 18 12:00:15 2018 +
# Node ID a41df44e13e1b761f4b3f05a17ed18953067ae39
# Parent  16d5ec7dbbb49ef3f969e34be870e3f917ea89ff
Remove JNICALL modifiers to prevent name decoration on 32-bit windows
builds

diff -r 16d5ec7dbbb4 -r 

Re: RFR 8214794 : java.specification.version should be only the major version number

[Magnus Ihse Bursie]

Looks good to me, too. 

/Magnus

> 4 dec. 2018 kl. 20:34 skrev Mandy Chung :
> 
> The revised webrev looks okay.
> 
> Mandy
> 
>> On 12/4/18 11:32 AM, Roger Riggs wrote:
>> Hi Mandy, Martin,
>> 
>> The new test is unnecessary, the case is covered by 
>> java/lang/System/Versions test
>> and uses the stronger comparison for the version numbers.
>> 
>> It would not detect the problem unless the version included more than the 
>> major version.
>> 
>> Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-3/
>> 
>> Thanks, Roger
>> 
>>> On 12/04/2018 01:41 PM, Mandy Chung wrote:
>>> 
>>> 
 On 12/4/18 8:16 AM, Roger Riggs wrote:
 Please review correctly setting the java.specification.version property
 with only the major version number.  A test is added to ensure the
 java spec version agrees with the major version.
 
 The symptoms are that jtreg would fail with a full version number.
 
 Webrev:
 http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2/
 
>>> 
>>> Looks okay.   I agree with Martin to go with a stronger assertion without 
>>> converting into a number. test/jdk/java/lang/System/Versions.java looks 
>>> like also covering this test case.  At some point it'd be good to 
>>> consolidate these two tests.
>>> 
>>> Nit:  in GensrcMisc.gmk, I think VERSION_NUMBER and VERSION_PRE etc are a 
>>> relevant group.   VERSION_SPECIFICATION can be moved to group with 
>>> VERSION_CLASSFILE_MAJOR and MINOR.  Magnus may have an opinion.
>>> 
>>> Mandy
>> 
> 

Re: RFR: 8212794 IBM-964 is required for AIX default charset

[Magnus Ihse Bursie]

On 2018-12-04 04:30, Ichiroh Takiguchi wrote:

Hello Roger.

Thank you for your suggestion.


src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM33722.java:

I think this is no longer needed since it only has imports.
By the way, the style is to import each specific class and
avoid wild card imports.
"import sun.nio.cs.*;" is required because of 
SimpleEUCEncoder.java.template.

SimpleEUCEncoder.java.template has conversion loop and IBM964 refers it.
It means that,
* On AIX platform, SimpleEUCEncoder should be in sun.nio.cs package
* On non-AIX platform, SimpleEUCEncoder should be in sun.nio.cs.ext 
package

I could not determine which package has SimpleEUCEncoder.
And same kind code is available on ISO2022_JP.java.
Please let me know if you know another way.
I'm not sure I'm fully following the template intricacies here, but 
would this not be solved if IBM33722.java was made a template as well? 
Then you could do

import $PACKAGE$. SimpleEUCEncoder
Or so I'd think.

/Magnus



TestIBMBugs:
  - Please use a style consistent with other methods in the class.
In this case spaces are needed around "{" and "}, and a space
after "," comma.

I'll changed.


  - The new method bug8212794, includes a test for x-IBM33722.
Is that needed since there does not appear to be a change for 33722?

Yes, it's no need.

Could you review the fix again ?
Bug:https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.03/

Thanks,
Ichiroh Takiguchi

On 2018-12-04 05:50, Roger Riggs wrote:

Hi Ichiroh,

src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM33722.java:

I think this is no longer needed since it only has imports.
By the way, the style is to import each specific class and
avoid wild card imports.

TestIBMBugs:
  - Please use a style consistent with other methods in the class.
In this case spaces are needed around "{" and "}, and a space
after "," comma.

  - The new method bug8212794, includes a test for x-IBM33722.
Is that needed since there does not appear to be a change for 33722?

Regards, Roger


On 11/30/2018 09:58 AM, Magnus Ihse Bursie wrote:



On 2018-11-30 10:49, Ichiroh Takiguchi wrote:

Hello.

Could you review the fix again ?

Bug:https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.02/
I think it looks good but please let someone from core-libs review 
it too.


/Magnus


I just fixed only IBM964 for JDK-8212794.
(IBM29626C fix is not included)

On non AIX platform (Linux),
ibm-euctw alias is added for IBM964.

Without fix
$ jshell
|  Welcome to JShell -- Version 12-ea
|  For an introduction type: /help intro

jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964

jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"

jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm964
964

jshell> /exit
|  Goodbye
$
==

With fix
==
$ jshell
|  Welcome to JShell -- Version 12-internal
|  For an introduction type: /help intro

jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964

jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"

jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964

jshell> /exit
|  Goodbye
$
==

On AIX platform
IBM964 is moved to java.base module from jdk.charset module.

==
$ LANG=zh_TW jshell
|  Welcome to JShell -- Version 12-internal
|  For an introduction type: /help intro

jshell> var cs = java.nio.charset.Charset.defaultCharset()
cs ==> x-IBM964

jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.IBM964"

jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964

jshell> /exit
|  Goodbye
$
==

I'd like to obtain a sponsor for this issue.

Thanks,
Ichiroh Takiguchi
IBM Japan, Ltd.

On 2018-11-29 22:39, Ichiroh Takiguchi wrote:

Hello Alan & Magnus.

Sorry for you confusion.
I did many copy actions and rename actions.
So you may see, I added unexpected code into non AIX platform.

I think I should not put 2 kind of modification.

For this bug id, I'll handle IBM964 and IBM33722.
(also SimpleEUCEncoder.java is required)

I'll submit code review again.

Additionally, I'll touch
make/data/charsetmapping/charsets
make/data/charsetmapping/stdcs-aix

I'll not touch
make/jdk/src/classes/build/tools/charsetmapping/Main.java
make/jdk/src/classes/build/tools/charsetmapping/SRC.java

My build machine is not so fast, after test is done.
I'll post new code.

Thanks,
Ichiroh Takiguchi

On 2018-11-28 19:10, Magnus Ihse Bursie wrote:

On 2018-11-28 10:36, Alan Bateman wrote:

On 28/11/2018 09:28, Magnus Ihse Bursie wrote:
I'm quite unsatisfied with the current handling of charac

Re: RFR: 8212794 IBM-964 is required for AIX default charset

[Magnus Ihse Bursie]

On 2018-11-30 10:49, Ichiroh Takiguchi wrote:

Hello.

Could you review the fix again ?

Bug:https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.02/

I think it looks good but please let someone from core-libs review it too.

/Magnus


I just fixed only IBM964 for JDK-8212794.
(IBM29626C fix is not included)

On non AIX platform (Linux),
ibm-euctw alias is added for IBM964.

Without fix
$ jshell
|  Welcome to JShell -- Version 12-ea
|  For an introduction type: /help intro

jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964

jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"

jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm964
964

jshell> /exit
|  Goodbye
$
==

With fix
==
$ jshell
|  Welcome to JShell -- Version 12-internal
|  For an introduction type: /help intro

jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964

jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"

jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964

jshell> /exit
|  Goodbye
$
==

On AIX platform
IBM964 is moved to java.base module from jdk.charset module.

==
$ LANG=zh_TW jshell
|  Welcome to JShell -- Version 12-internal
|  For an introduction type: /help intro

jshell> var cs = java.nio.charset.Charset.defaultCharset()
cs ==> x-IBM964

jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.IBM964"

jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964

jshell> /exit
|  Goodbye
$
==

I'd like to obtain a sponsor for this issue.

Thanks,
Ichiroh Takiguchi
IBM Japan, Ltd.

On 2018-11-29 22:39, Ichiroh Takiguchi wrote:

Hello Alan & Magnus.

Sorry for you confusion.
I did many copy actions and rename actions.
So you may see, I added unexpected code into non AIX platform.

I think I should not put 2 kind of modification.

For this bug id, I'll handle IBM964 and IBM33722.
(also SimpleEUCEncoder.java is required)

I'll submit code review again.

Additionally, I'll touch
make/data/charsetmapping/charsets
make/data/charsetmapping/stdcs-aix

I'll not touch
make/jdk/src/classes/build/tools/charsetmapping/Main.java
make/jdk/src/classes/build/tools/charsetmapping/SRC.java

My build machine is not so fast, after test is done.
I'll post new code.

Thanks,
Ichiroh Takiguchi

On 2018-11-28 19:10, Magnus Ihse Bursie wrote:

On 2018-11-28 10:36, Alan Bateman wrote:

On 28/11/2018 09:28, Magnus Ihse Bursie wrote:
I'm quite unsatisfied with the current handling of character sets 
in the build in general. :-( I'd really like to modernize it. I 
have a, slightly fuzzy, laundry list of things I want to fix from 
a build perspective, but I'm not sure of what "external" 
requirements are coming from AIX and the general core-libs agenda 
regarding character sets in general.


I think there is a good opportunity to solve many problems at the 
same time here, as long as everyone agrees on what is the 
preferred outcome.
The support in the build to configure the charsets to include in 
java.base on each platform has been working well. Charsets that 
aren't in java.base go into the jdk.charsets service provider 
module and that has been working well too. From the result point of 
view, perhaps, but definitely not from the build perspective. ;-) 
But yes, I understand this is functionality that should be kept.
One thing that we lack is some way to add charsets for specific 
platforms and this comes up with the IBM patches where they are 
looking to adding several additional IBM charsets. One starting 
point that we've touched on in several threads here is dropping the 
EBCDIC charsets from the main stream builds. Going there will need 
build support.

So build support for trivially adding specific charsets to specific
platforms? Both to java.base (for AIX) and jdk.charsets, I presume,
then?

Can you expand on the issue of dropping ebcdic? What's the problem
that needs build support?

/Magnus



-Alan

Re: RFR: JDK-8213362 : Could not find libjava.dylib error when initializing JVM via JNI_CreateJavaVM

[Magnus Ihse Bursie]

On 2018-11-28 23:49, Henry Jen wrote:


Since there is no header file in JDK provides the function prototypes, I don’t 
think this is considered public(supported) APIs.

Anyway, in case a tools is build with launcher code, and shipped separately 
from JDK, that would be impacted by this bug. So I added a test call JLI_Launch 
as well. Please review the updated webrev[1].

Note that, JLI_Launch should not be used directly as it does’t handle the 
argument processing which is done in launcher/main.c.

Cheers,
Henry

[1] http://cr.openjdk.java.net/~henryjen/jdk12/8213362.1/webrev/
The build changes look good, as is the original fix for the problem. As 
far as I can tell, the tests seem reasonable. At the very least, this is 
a much better situation than we had before. So I say, ship it!


/Magnus




On Nov 28, 2018, at 5:28 AM, Kevin Rushforth  wrote:



On 11/28/2018 5:19 AM, Alan Bateman wrote:

On 28/11/2018 13:13, Kevin Rushforth wrote:

The jpackage tool calls JLI_Launch.

I remember that from Andy's webrev but it's not integrated yet. Does the JavaFX 
packager tool do the same?

Yes, the javapackager tool (delivered via JavaFX) in JDK 8/9/10 calls 
JLI_Launch.

-- Kevin

Re: RFR: 8212794 IBM-964 and IBM-29626C are required for AIX default charset

[Magnus Ihse Bursie]

On 2018-11-28 10:36, Alan Bateman wrote:

On 28/11/2018 09:28, Magnus Ihse Bursie wrote:
I'm quite unsatisfied with the current handling of character sets in 
the build in general. :-( I'd really like to modernize it. I have a, 
slightly fuzzy, laundry list of things I want to fix from a build 
perspective, but I'm not sure of what "external" requirements are 
coming from AIX and the general core-libs agenda regarding character 
sets in general.


I think there is a good opportunity to solve many problems at the 
same time here, as long as everyone agrees on what is the preferred 
outcome.
The support in the build to configure the charsets to include in 
java.base on each platform has been working well. Charsets that aren't 
in java.base go into the jdk.charsets service provider module and that 
has been working well too. 
From the result point of view, perhaps, but definitely not from the 
build perspective. ;-) But yes, I understand this is functionality that 
should be kept.
One thing that we lack is some way to add charsets for specific 
platforms and this comes up with the IBM patches where they are 
looking to adding several additional IBM charsets. One starting point 
that we've touched on in several threads here is dropping the EBCDIC 
charsets from the main stream builds. Going there will need build support.
So build support for trivially adding specific charsets to specific 
platforms? Both to java.base (for AIX) and jdk.charsets, I presume, then?


Can you expand on the issue of dropping ebcdic? What's the problem that 
needs build support?


/Magnus



-Alan

Re: RFR: JDK-8214063: OpenJDK will not build on AIX while using the xlc 13.1 compiler

[Magnus Ihse Bursie]

On 2018-11-27 13:26, Ichiroh Takiguchi wrote:

Hello Volker.

Sorry for your confusion.
I want to keep visibility feature on AIX platform for future OpenJDK.

If I can apply workaround for AIX platform...

XLC++ 13.1 is confused destructor order for ~SimpleCriticalSectionLock()
on src/java.base/share/native/libjimage/osSupport.hpp, if visibility 
feature is specified.


Please see following testing.
(I already applied a fix against 
src/java.base/unix/native/include/jni_md.h)


$ sh NativeImageBuffer.o.cmdline
"/home/jdktest/sandbox/jdk/build/aix-ppc64-server-release/support/headers/java.base/jdk_internal_jimage_NativeImageBuffer.h", 
line 15.27: 1540-0040 (S) The text 
"Java_jdk_internal_jimage_NativeImageBuffer_getNativeMap" is 
unexpected.  "visibility" may be undeclared or ambiguous.


If I applied following change
==
$ hg di src/java.base/share/native/libjimage/osSupport.hpp
diff -r 6cf555c2e9ff src/java.base/share/native/libjimage/osSupport.hpp
--- a/src/java.base/share/native/libjimage/osSupport.hpp Sun Nov 25 
21:41:12 2018 +0900
+++ b/src/java.base/share/native/libjimage/osSupport.hpp Tue Nov 27 
21:04:41 2018 +0900

@@ -103,6 +103,7 @@
 SimpleCriticalSection *lock;
 public:

+#ifndef _AIX
 SimpleCriticalSectionLock(SimpleCriticalSection *cslock) {
 this->lock = cslock;
 lock->enter();
@@ -111,6 +112,16 @@
 ~SimpleCriticalSectionLock() {
 lock->exit();
 }
+#else
+~SimpleCriticalSectionLock() {
+lock->exit();
+}
+
+SimpleCriticalSectionLock(SimpleCriticalSection *cslock) {
+this->lock = cslock;
+lock->enter();
+}
+#endif
 };
This is really fascinating! :-) Is there something in this code that 
trips up the AIX compiler? I saw that you could also "fix" it by 
removing the header file completely, or add an additional declaration. 
What if you change the order of include files? And what on earth is 
making the compiler barf on this trivial code? *intrigued*


/Magnus


 #endif  // LIBJIMAGE_OSSUPPORT_HPP
==

No output was displayed by NativeImageBuffer.o.cmdline
$ sh NativeImageBuffer.o.cmdline
$

Adam, if possible, could you double check my code ?

Volker, I appreciate if you reconsider about this issue.

Thanks,
Ichiroh Takiguchi

On 2018-11-27 03:26, Volker Simonis wrote:

On Mon, Nov 26, 2018 at 6:52 PM Ichiroh Takiguchi
 wrote:


Hello Volker.

I posted same kind of fix before:
http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2018-June/003551.html 



I could not find out brace handling issue on XLC++ 13.1.

For workaround,
==
---
old/src/java.base/share/native/libjimage/NativeImageBuffer.cpp 
2018-06-07

21:06:09 +
+++
new/src/java.base/share/native/libjimage/NativeImageBuffer.cpp 
2018-06-07

21:06:09 +
@@ -39,7 +39,9 @@
  #include "imageFile.hpp"
  #include "inttypes.hpp"
  #include "jimage.hpp"
+#if !defined(_AIX)
  #include "osSupport.hpp"
+#endif

  #include "jdk_internal_jimage_NativeImageBuffer.h"
==

I think osSupport.hpp is no need for all platform.
(I tested it on Linux and AIX build)

What do you think ?



Sorry, but I don't understand your mail. Did you saw the same problems
like Adam when compiling "NativeImageBuffer.cpp"?

- If yes, did you fix them by excluding the inclusion of
"osSupport.hpp" ? That would be strange, because it doesn't seem to
related to the problems reported until now at all.

- If no, I'm totally confused...


Thanks,
Ichiroh Takiguchi
IBM Japan, Ltd.

On 2018-11-27 02:06, Volker Simonis wrote:
> On Mon, Nov 26, 2018 at 2:16 PM Adam Farley8 
> wrote:
>>
>> Hi Volker,
>>
>> Apologies for the delay.
>>
>> I ran the contents of the file as requested (neat tip, thanks!) 
and I

>> discovered something:
>>
>> If jdk_internal_jimage_NativeImageBuffer.h contains this:
>>
>> -
>> extern "C" {
>> __attribute__((visibility("default"))) jobject JNICALL
>> Java_jdk_internal_jimage_NativeImageBuffer_getNativeMap
>> (JNIEnv *, jclass, jstring);
>> }
>> -
>>
>> It results in this error:
>>
>> -
>> blah blah "visibility" may be undeclared or ambiguous.
>> -
>>
>> But replacing that bit with this code:
>>
>> -
>> extern "C" __attribute__((visibility("default"))) jobject JNICALL
>> Java_jdk_internal_jimage_NativeImageBuffer_getNativeMap
>> (JNIEnv *, jclass, jstring);
>> -
>>
>> Results in no error.
>>
>> So it seems the difference between an "extern "C"" block and an 
inline

>> "extern"C"" is what's causing the issue.
>>
>
> Thanks for finding this out. Now at least we know the root cause 
of the

> problem.
>
> Without having read through the C++ specification, I'd assume this is
> a problem of XLC 13.
>
> As long as XLC has problems to parse such kinds of constructs, I 
thing
> we should just remove "-qvisibility" from the AIX build as 
proposed by

> Magnus (and called "plan B" in you other mail) because changing the
> javah generator for AIX would be a much larger task.
>
> Thank you and best regards,
> Volker
>
>> I don't understand 

Re: RFR: 8214077: test java/io/File/SetLastModified.java fails on ARM32

[Magnus Ihse Bursie]

> 26 nov. 2018 kl. 13:27 skrev Alan Bateman :
> 
>> On 26/11/2018 09:08, Nick Gasson wrote:
>> Hi Alan,
>> 
>> I've done this here:
>> 
>> http://cr.openjdk.java.net/~njian/8214077/webrev.3/
> This looks good and I think means we no longer have any stat usages in 
> libjava.

Do we have stat usages in other native libraries, or are the entire code base 
in the clear now?

/Magnus

> 
> -Alan
> 

Re: RFR: 8214077: test java/io/File/SetLastModified.java fails on ARM32

[Magnus Ihse Bursie]

On 2018-11-21 11:08, Nick Gasson wrote:


Hi Alan,


Have you looked at replacing the remaining usages of stat changed to
stat64 instead?

I've tried this just now and it also resolves the issue. I can
test on some more platforms and update the webrev if this is the
preferred solution?
I'd say this is preferred to adding compiler flags, yes. The code will 
make it unambiguously clear that it's correct.


/Magnus


Thanks,
Nick


-Original Message-
From: Alan Bateman 
Sent: Wednesday, November 21, 2018 4:55 PM
To: Nick Gasson ; build-dev ; core-libs-dev@openjdk.java.net
Cc: nd 
Subject: Re: RFR: 8214077: test java/io/File/SetLastModified.java fails on
ARM32

On 21/11/2018 02:46, Nick Gasson wrote:

Hi,

Can someone please help me review this small makefile patch to
fix an issue with java.io.File::setLastModified on 32-bit
systems?

https://bugs.openjdk.java.net/browse/JDK-8214077
http://cr.openjdk.java.net/~njian/8214077/webrev.0/

If the file size is > 2GB so that the size won't fit in a signed
32-bit off_t all stat() calls will fail with EOVERFLOW. This causes
the native method UnixFileSystem::setLastModifiedTime to fail as it
uses stat() to preserve the access time. It also causes other methods
like File::length and File::lastModified to return 0.


Have you looked at replacing the remaining usages of stat changed to
stat64 instead?

-Alan

Re: RFR: JDK-8214063: OpenJDK will not build on AIX while using the xlc 13.1 compiler

[Magnus Ihse Bursie]

On 2018-11-20 18:50, Volker Simonis wrote:

On Tue, Nov 20, 2018 at 6:15 PM Thomas Stüfe  wrote:

On Tue, Nov 20, 2018 at 6:12 PM Adam Farley8  wrote:

Heya Tom,

"In JDK11 and JDK12, source files are compiled with -qvisibility=hidden
when using xlc version other than 12.1. That doesn't seem to play well
with link option -bexpall. "

Found that buried in one of the associated Git issues. It appears that
it's OpenJDK's use of that option that's causing the problem, though
I couldn't speculate as to why it was added in the first place.

I see this has also been noted in 
https://bugs.openjdk.java.net/browse/JDK-8204541

Does that answer your question?


Yes, Thank you. Odd. Will have to do archeology on that one.


No I begin to understand the problem as well :)

It was actually change "8202322: AIX: symbol visibility flags not
support on xlc 12.1" [1] which introduced "-qvisibility=hidden" for
XLC version not equal to 12.1. That's kind of a weak check and I
suppose nobody has ever tested this change with an XLC version other
than 12.1 (until you came along :). Maybe that check should be a more
precisly check for >= 13.1 (but I know such version checks are hard to
do in Makefile syntax)?
In configure (where, ideally, all version checks should be made), 
there's the TOOLCHAIN_CHECK_COMPILER_VERSION function, which supports
#   IF_AT_LEAST:   block to run if the compiler is at least this version 
(>=)
#   IF_OLDER_THAN:   block to run if the compiler is older than this 
version (<)

for normal, dot-separated version number schemes.

/Magnus



The thing I don't understand about your patch (the changes in
"jni_md.h" look good although I haven't tested them) is why you need
the extra changes in NativeImageBuffer.cpp?
"jdk_internal_jimage_NativeImageBuffer.h" is a plain, generated JNI
header file. If XLC 13 has problems to parse it, there should be much
more places which need fixing. I think that part of your change needs
a closer evaluation.

Thank you and best regards,
Volker

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


..Thomas


Best Regards

Adam Farley
IBM Runtimes


"Thomas Stüfe"  wrote on 20/11/2018 16:44:07:


From: "Thomas Stüfe" 
To: Adam Farley8 
Cc: Java Core Libs 
Date: 20/11/2018 16:48
Subject: Re: RFR: JDK-8214063: OpenJDK will not build on AIX while
using the xlc 13.1 compiler

Hi Adam,

On Tue, Nov 20, 2018 at 5:12 PM Adam Farley8  wrote:

Hi Tom,

Sounds reasonable. I've added a webex to the bug, and here's a

link to the bug.

https://urldefense.proofpoint.com/v2/url?

u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8214063=DwIFaQ=jf_iaSHvJObTbx-
siA1ZOg=P5m8KWUXJf-
CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=z8YYwBXEfN7UtX1suPjpp9CZSHf8v0GrIMK3XGIC9VY=81TP9mIjhYD2Hmt8g7p2EHWRZXgiep21hxKLYRU7zIQ=

This patch is required because otherwise, when building on AIX

using xlc 3.1,

the build fails with this error:

"Visibility is not allowed on a reference to an imported symbol."

We believe this is caused by JNIEXPORT and JNIIMPORT not being

defined. Without

this, almost no symbols are exported from shared libraries due to use of
-qvisibility=hidden as specified in make/lib/LibCommon.gmk.

Yes but what I try to understand is why does this happen now with
xlc13? Did xlc change the rules for -qvisibility from v12 to v13 ?
That would be quite a break in backward compatibility.


For convenience, here's a summary of the diffs:

--
File 1 of 2) src/java.base/share/native/libjimage/NativeImageBuffer.cpp

  #include "osSupport.hpp"

+#if defined(__xlC__) && (__xlC__ >= 0x0d01)
+/*
+ * Version 13.1.3 of xlc seems to have trouble parsing the `__attribute__`
+ * annotation in the generated header file we're about to

include. Repeating

+ * the forward declaration (without the braces) here avoids the diagnostic:
+ *   1540-0040 (S) The text "void" is unexpected.  "visibility"

may be undeclared or ambiguous.

+ */
+extern "C" JNIEXPORT jobject JNICALL

Java_jdk_internal_jimage_NativeImageBuffer_getNativeMap(JNIEnv *,
jclass, jstring);

+#endif
+
#include "jdk_internal_jimage_NativeImageBuffer.h"
--
File 2 of 2) src/java.base/unix/native/include/jni_md.h

  #define JNIIMPORT __attribute__((visibility("default")))
   #endif
+#elif defined(__xlC__) && (__xlC__ >= 0x0d01) /* xlc version 13.1

or better required */

+  #define JNIEXPORT   __attribute__((visibility("default")))
+  #define JNIIMPORT   __attribute__((visibility("default")))
#else
   #define JNIEXPORT
--


Thank you.

Cheers, Thomas


Best Regards

Adam Farley
IBM Runtimes


"Thomas Stüfe"  wrote on 19/11/2018 18:11:34:


From: "Thomas Stüfe" 
To: Adam Farley8 
Cc: Java Core Libs 
Date: 19/11/2018 18:12
Subject: Re: RFR: JDK-8214063: OpenJDK will not build on AIX while
using the xlc 13.1 compiler

Hi Adam,

could you please include link to the JBS issue and either link to the
patch/webrev or link to the webrev, or at the 

Re: RFR 4947890 : Minimize JNI upcalls in system property initialization

[Magnus Ihse Bursie]

On 2018-11-20 00:37, Roger Riggs wrote:


Hi,

Webrev updated in place:

http://cr.openjdk.java.net/~rriggs/webrev-props-only-raw
Build changes still look fine. But please, in the future, avoid updating 
webrevs in place, but create new ones instead. It's hopeless to figure 
out what has changed otherwise. :-( Updating in place is OK for things 
like typos, but not actual code changes...


/Magnus



On 11/16/2018 06:36 PM, Mandy Chung wrote:

Hi Roger,

Looking good.  I have a few small comments:

I assume VM.saveAndRemoveProperties will be a separate cleanup.
SystemProps::cmdProperties adds the system properties that can
skip adding these internal properties to the Properties object.
In fact, these internal properties are the interface between VM
and libraries that can be replaced with a different mechanism
other than system properties.

fine as a future change


Suggest to move the call to VersionProps.init(props) in
SystemProps.initProperties

That's harder that it might seems, the method can't be made public
and moving it to jdk.internal.util with SystemProps is more difficult
since there is Hotspot code that reads those fields directly. 
(thread.cpp)

That goes back quiet a number of years.

Another future cleanup/decoupling.


The `VENDOR*` build time variables can always have a default
like the other constants in VersionProps.  Then no need to
handle if not set.

Moved defaults to configure,
easy to override with --with-vendor-url, --with-vendor-bug-url, and 
--with-vendor-name.


SystemProps.staticInitOnly_* are a bit odd.  They are temporarily
set with the values and then reset to null.  Perhaps leave
StaticProperty as is for now and re-visit it in the future.
Then I think SystemProps::putDefault can be removed.
ok, reverted the code so the static values are read via 
System.getProperty.


Raw::xxx_NDX are initialized to 1 + previous_NDX.  It's a general
good approach to increment the index but I find it error-prone and
hard to catch mistake since the (adjacent) variable names look
so alike. Perhaps some form of verification or assertion to ensure
the indices are correctly initialized.

Added a test that uses reflection to verify the uniqueness and sequence.

Thanks, Roger


Mandy

On 11/16/18 10:49 AM, Roger Riggs wrote:

Updates to include the suggestions made by Mandy and Brent:

 - Move the build-time properties from native code to the 
VersionProps.java.template
   including java.vendor.* and java.specification.* properties, plus 
the java.class.version (major.minor)
   This included two changes to the build that generates source of 
VersionProps.java.


 - Updated the StaticProperty initialization to be explicitly 
invoked by initProperties.


 - Makes separate calls to native to retrieve the platform 
properties and VM/command line properties


 - (The hotspot function for JVM_GetProperties are unchanged)

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-props-only-raw/

Issue:
https://bugs.openjdk.java.net/browse/JDK-4947890

Thanks, Roger

Re: RFR 4947890 : Minimize JNI upcalls in system property initialization

[Magnus Ihse Bursie]

On 2018-11-16 21:36, Erik Joelsson wrote:

Thanks, looks good to me now.

And to me.

Looks like a nice cleanup in general!

/Magnus



/Erik

On 2018-11-16 12:02, Roger Riggs wrote:

Hi Erik,

Yes, that is removed.
Webrev updated in place.

Thanks, Roger

http://cr.openjdk.java.net/~rriggs/webrev-props-only-raw/

On 11/16/2018 02:37 PM, Erik Joelsson wrote:
Does this mean we can remove $(VERSION_CFLAGS) from System.c in 
make/lib/CoreLibraries.gmk?


Otherwise this looks good from a build point of view.

/Erik

On 2018-11-16 10:49, Roger Riggs wrote:

Updates to include the suggestions made by Mandy and Brent:

 - Move the build-time properties from native code to the 
VersionProps.java.template
   including java.vendor.* and java.specification.* properties, 
plus the java.class.version (major.minor)
   This included two changes to the build that generates source of 
VersionProps.java.


 - Updated the StaticProperty initialization to be explicitly 
invoked by initProperties.


 - Makes separate calls to native to retrieve the platform 
properties and VM/command line properties


 - (The hotspot function for JVM_GetProperties are unchanged)

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-props-only-raw/

Issue:
https://bugs.openjdk.java.net/browse/JDK-4947890

Thanks, Roger

Re: Review Request : JDK-8213362 : Could not find libjava.dylib error when initializing JVM via JNI_CreateJavaVM

[Magnus Ihse Bursie]

I now noticed that this was only sent to build-dev. This is not really a build 
question. Cc:ing core-libs-dev. 

/Magnus

> 5 nov. 2018 kl. 15:21 skrev Magnus Ihse Bursie 
> :
> 
> Hi,
> 
> Fix looks good, but maybe we should have a regression test of GetJREPath()?
> 
> /Magnus
> 
>> 5 nov. 2018 kl. 12:09 skrev Priyanka Mangal :
>> 
>> Hi,
>> 
>> Please review the minor fix for :
>> https://bugs.openjdk.java.net/browse/JDK-8213362
>> http://cr.openjdk.java.net/~pmangal/8213362/webrev.00/
>> 
>> With the JDK-8210931 <https://bugs.openjdk.java.net/browse/JDK-8210931> 
>> libjli has moved from "jli" subdirectory into standard lib directory.
>> However `GetJREPath` method in 
>> `src/java.base/macosx/native/libjli/java_md_macosx.m` still assumes that 
>> `libjli.dylib` should be on `lib/jli/libjli.dylib` path.
>> So corrected the same.
>> 
>> Testing:
>> mach5 : tier1-3
>> http://java.se.oracle.com:10065/mdash/jobs/fmatte-jdk-20181105-0941-8721?search=result.status:*
>> 
>> Thanks
>> Regards
>> Priyanka
> 
> 

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

[Magnus Ihse Bursie]

On 2018-10-24 19:18, Erik Joelsson wrote:

Hello,

Nice to see this finally happening!

Are we actually adding a new demo? I thought we were working towards 
getting rid of the demos completely.


CompileJavaModules.gmk:

The jdk.packager_CLEAN_FILES could be replaced with a simple 
"jdk.packager_CLEAN := .properties" unless you actually need to add 
all the platform specific files to builds for all platforms (which I 
doubt?). To clarify, the current patch will add all linux, windows and 
mac properties to builds of all OSes. Is that intended?


Modules.gmk:

I would rather have the filter be conditional on an inclusive list of 
platforms. Other OpenJDK contributors are building for other OSes like 
AIX and we don't want to have to keep track of all those. The list of 
OSes that jpackager supports is well defined, so better check for 
those explicitly.


src/jdk.jlink/share/classes/module-info.java:

I believe the qualified export needs to be put in a 
module-info.java.extra file since we filter out the target module on 
unsupported platforms.


Launcher-jdk.packager.gmk:

* Please use $$(call FindExecutableDirForModule, $$(MODULE)) for the 
DEST dir in the copy.
* Please adjust the indentation of the big windows/else blocks. We 
indent everything with 2 spaces in conditional blocks.
* A minor style nit on the copy-and-chmod macro, since it's meant to 
be used as a recipe, we usually indent those with tabs to indicate 
that they are recipe lines, in this case, one tab is enough even 
though the surrounding define should be indented with 2 spaces (don't 
combine tab and spaces here).
* The (new) SetupJdkExecutable macro handles VERSIONINFO_RESOURCE 
automatically for you unless you have specific needs. This looks like 
the defaults should be fine.
* Since this is just for windows, the TOOLCHAIN_LINK_CXX should not 
make any difference. (It's only needed for GCC to force linking with 
g++ instead of gcc) So please remove.

* You could consider using FindSrcDirsForComponent for the src dir.

Lib-jdk.packager.gmk:

* The native source files are not organized according to the standards 
introduced with JEP 201. If they were, most of these SetupJdkLibrary 
calls would automatically find the correct sources. I realize there is 
a special case for the windows papplauncherc executable as it's built 
from the same sources as papplauncher, so that will need a special 
case. Building of the executables should be moved to 
Launcher-jdk.packager.gmk however.
* Why are you changing the build output dir and copying debuginfo 
files around? We have a standard way of building and places where 
files are put. If that is not working for you, we need to fix it on a 
global level. Having each native library being built differently is 
not good for maintainability.
* VERSIONINFO_RESOURCE is handled automatically so does not need to be 
specified.
* Please indent the full contents of logic blocks with 2 spaces. Not 
having proper indents makes it much harder to read the code.
* Several lines are too long for future side by side comparisons, 
please break them up. You can use the # lines as a soft guidance.


On top of Erik's comments, I also have the following to add from a build 
perspective:


* In make/CompileJavaModules.gmk:
Do you really need to use GENERATE_JDKBYTECODE_NOWARNINGS? When you add 
new code to OpenJDK, it really *should* be able to build without 
warnings. This has previously only been used for legacy code that has 
not yet been brought up to OpenJDK standards. Introducing new code with 
warnings silenced feels like a step in the wrong direction.


* In make/launcher/Launcher-jdk.packager.gmk:
The setup of BUILD_JPACKAGEREXE is only done for windows, but you have 
"DISABLED_WARNINGS_gcc := unused-result implicit-fallthrough". This does 
not make sense.


The CFLAGS listed look redundant. At the very least I know that -nologo 
is already present in CXXFLAGS_JDKEXE; I believe the same goes for the 
rest (or most of the rest) of the flags. Please only add flags if you 
really need them, since all special configuration/combinations is a 
potential cause for trouble in the future. This same applies to LDFLAGS.


* In src/jdk.packager/unix/scripts/jpackager:

This file  resides in a non-standard directory. We have a small list of 
directories that are supposed to come after the $MODULE/share|$OS/ part 
of the path, and "scripts" is not one of them. While there is no rule 
"forbidding" new kinds of directories, I strongly recommend against this.


Looking more closely at the file, I wonder if you really need it? It's 
sole purpose seems to be to launch java -m 
jdk.packager/jdk.packager.main.Main. For that, we have a much better 
solution. Just change make/launcher/Launcher-jdk.packager.gmk to include 
the following contents:


$(eval $(call SetupBuildLauncher, jpackager, \
    MAIN_CLASS := jdk.packager.main.Main, \
))

It will create a "jpackager" binary. Which works for Windows too, so 
maybe you won't even 

Re: RFC: JEP JDK-8208089: Implement C++14 Language Features

[Magnus Ihse Bursie]

> 4 okt. 2018 kl. 10:57 skrev Martijn Verburg :
> 
> Hi Kim,
> 
> I like this initiative.  I'm wondering if some of these rules can be easily
> codified or written into a jcheck style checker (ccheck?) so that Authors
> can adhere to the conventions and not rely on a Human review to pick out
> where that convention isn't met.

That's an interesting thought!

I googled around a bit, but could find no obvious candidate for doing such a 
check. In fact, parsing and analyzing C++ seems quite a hard problem, compared 
to many other languages. (Sad, but not really surprising.)

I found two possible routes to explore: cpplint [1], the official Google tool 
to verify that the Google C++ Style Guide [2] is followed, and Vera++ [3], a 
framework for creating scripts that can analyze and/or transform C++ code.

Neither seem like an ideal solution. The Google tool is hard coded to support 
the Google rules. It's possible we can fork it and add our own, but it's not 
clear that this is even possible, much less so that it's a good way forward. 
Vera++, on the other hand, seems much more likely to be able to provide the 
tooling needed to write checks that enforce these rules. However, what we have 
is just a framework, and someone's got to write those rules (in TCL of all 
languages...). Then again, Vera++ is an old and quite popular tool, so its 
possible there is already a bunch of predefined rules that we can use as a 
starting point out there. (I didn't go that far in my analysis).

Apart from these two, there appear to be no popular, well-encompassing, open 
source C++ checker out there, that could possibly be verifying rules like 
these. :(

/Magnus

[1] https://github.com/google/styleguide/tree/gh-pages/cpplint
[2] https://google.github.io/styleguide/cppguide.html
[3] https://bitbucket.org/verateam/vera/wiki/Introduction

> 
> Cheers,
> Martijn
> 
> 
>> On Wed, 3 Oct 2018 at 20:13, Kim Barrett  wrote:
>> 
>> I've submitted a JEP for
>> 
>> (1) enabling the use of C++14 Language Features when building the JDK,
>> 
>> (2) define a process for deciding and documenting which new features
>> can be used or are forbidden in HotSpot code,
>> 
>> (3) provide an initial list of permitted and forbidden new features.
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8208089
>> 
>> 

Re: RFR: JDK-8211071: unpack.cpp fails to compile with statement has no effect [-Werror=unused-value]

[Magnus Ihse Bursie]

> 25 sep. 2018 kl. 10:21 skrev Roman Kennke :
> 
> Not sure this is the correct list. Please redirect as appropriate.

I believe core-libs is the appropriate place. Cc:d. 

> 
> Please review the following proposed change:
> 
> 
> There are 3 asserts in unpack.cpp which check only constants, and which
> the compiler rejects as 'has no effect' (in fastdebug builds). This
> seems to be caused by:
> 
> https://bugs.openjdk.java.net/browse/JDK-8211029
> 
> I propose to fix this by defining a STATIC_ASSERT which can evaluate and
> reject this at compile time:
> 
> Webrev:
> http://cr.openjdk.java.net/~rkennke/JDK-8211071/webrev.00/

From a build perspective, this looks good. But please let someone from 
core-libs review also. 

> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8211071
> 
> It might be useful to define the STATIC_ASSERT in a more central place
> to be used elsewhere too. For example, there is a similar #define in
> Hotspot's debug.hpp

Unfortunately, we do not have a good place to share definitions like this 
across JDK libraries. :-( The need for that has struck me more than once. For 
some stuff, we mis-use jni.h. But it would definitely be out of line to add a 
STATIC_ASSERT there. 

/Magnus

> 
> Thanks, Roman
> 

RFR: JDK-8210924 Remove PACKAGE_PATH

[Magnus Ihse Bursie]

The variable PACKAGE_PATH crept into the build when the macosx port was 
integrated. It is not used, however, and should be removed. (This is a 
BSD construct, and the macosx port was based on the BSD port.)


If/when the BSD port ever gets integrated upstream, we'll know better 
then how to restore PACKAGE_PATH on the proper places.


Bug: https://bugs.openjdk.java.net/browse/JDK-8210924
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8210924-remove-PACKAGE_PATH/webrev.01


/Magnus

Re: [12] RFR: 8209880: tzdb.dat is not reproducibly built

[Magnus Ihse Bursie]

On 2018-09-18 19:41, Naoto Sato wrote:

Hello,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8209880

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8209880/webrev.00/

Looks good. Thank you for fixing this!

/Magnus



The fix is to remove the use of HashSet/Map which is not guaranteed to 
preserve the same order on iteration.


Naoto

Re: Why static_jli for java/javaw on Windows?

[Magnus Ihse Bursie]

On 2018-09-14 10:50, Markus Gronlund wrote:

Hi Magnus, Erik and Alan,

Came to think of this issue in relation to this discussion:

https://bugs.openjdk.java.net/browse/JDK-8207139

It might provide some additional information, especially since it describes an 
issue we recently ran into that was related to  static linking.

Thanks Markus!

Just to be clear that I read this correctly: Changing libjli from static 
to dynamic did not introduce a problem for you, instead it actually 
*solved* a problem? (If so, that's even more argument to skip the static 
linking)


/Magnus



Thanks
Markus



-Original Message-
From: Magnus Ihse Bursie
Sent: den 14 september 2018 09:22
To: Erik Joelsson ; Alan Bateman 
; core-libs-dev@openjdk.java.net
Cc: build-dev 
Subject: Re: Why static_jli for java/javaw on Windows?


On 2018-09-14 01:17, Erik Joelsson wrote:

I checked and the copying of java.exe was done in the now legacy jre
installer, so from what I can tell, there is no longer a need for
static linking.

Sounds good. I agree with your reasoning, it seems the main concern was the 
copy to the system directory. A second point was raised in private, that the 
old system of letting the java launcher select another version to launch, could 
be behind this. Neither this system is kept anymore.

Thanks for all input!

/Magnus


/Erik


On 2018-09-13 09:14, Erik Joelsson wrote:

Hello,

Reading that bug, it seems the problem is when the installer copies
java.exe into the Windows system directory. In that case, it may not
have access to the msvcr re-distributables. I will try to find out if
our installers are still doing this.

/Erik


On 2018-09-13 06:32, Alan Bateman wrote:


On 13/09/2018 14:07, Magnus Ihse Bursie wrote:

:

Apparently, someone was trying to get rid of dll dependencies from
java.exe. Why that would be desirable it does not say. Neither why
this should not apply to all other launchers. (Perhaps there were
not that many in these days?)

Do anyone think this still seems relevant? Otherwise I'd like to
get rid of this hack, and link java and javaw just like all the
other launchers.


I don't know if it is still needed but it seems to be related to the
upgrade to VC 7 and an issue related to redistribution issue of the
MS runtime. JDK-6282039 and JDK-6382014 have some info on this.

-Alan.

Re: Why static_jli for java/javaw on Windows?

[Magnus Ihse Bursie]

On 2018-09-14 01:17, Erik Joelsson wrote:
I checked and the copying of java.exe was done in the now legacy jre 
installer, so from what I can tell, there is no longer a need for 
static linking.


Sounds good. I agree with your reasoning, it seems the main concern was 
the copy to the system directory. A second point was raised in private, 
that the old system of letting the java launcher select another version 
to launch, could be behind this. Neither this system is kept anymore.


Thanks for all input!

/Magnus



/Erik


On 2018-09-13 09:14, Erik Joelsson wrote:

Hello,

Reading that bug, it seems the problem is when the installer copies 
java.exe into the Windows system directory. In that case, it may not 
have access to the msvcr re-distributables. I will try to find out if 
our installers are still doing this.


/Erik


On 2018-09-13 06:32, Alan Bateman wrote:



On 13/09/2018 14:07, Magnus Ihse Bursie wrote:

:

Apparently, someone was trying to get rid of dll dependencies from 
java.exe. Why that would be desirable it does not say. Neither why 
this should not apply to all other launchers. (Perhaps there were 
not that many in these days?)


Do anyone think this still seems relevant? Otherwise I'd like to 
get rid of this hack, and link java and javaw just like all the 
other launchers.


I don't know if it is still needed but it seems to be related to the 
upgrade to VC 7 and an issue related to redistribution issue of the 
MS runtime. JDK-6282039 and JDK-6382014 have some info on this.


-Alan.

Re: [12]: RFR: 8209167: Use CLDR's time zone mappings for Windows

[Magnus Ihse Bursie]

On 2018-09-12 19:33, naoto.s...@oracle.com wrote:

Hi Magnus, Erik,

Thank you for your comments. I updated the webrev according to your 
suggestions:


http://cr.openjdk.java.net/~naoto/8209167/webrev.02/

Looks good to me.

/Magnus



As to the duplicated "common" in the dependency, yes that's a separate 
issue. Since it's obvious, I included the fix with this changeset (it 
was actually described as a comment in the jira issue).


Naoto

On 9/12/18 9:08 AM, Erik Joelsson wrote:

On 2018-09-12 03:19, Magnus Ihse Bursie wrote:



On 2018-09-10 23:34, Naoto Sato wrote:

Hello,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8209167

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8209167/webrev.01/


Some comments:
In make/copy/Copy-java.base.gmk:
+ifneq ($(findstring $(OPENJDK_TARGET_OS), aix),)

The findstring construct is hard to read and only needed when we 
test more than one OS. Please rewrite as a single ifeq test for aix.


In GensrcCLDR.gmk:
I don't understand the CLDR_WINTZMAPPINGS file? There's no rule to 
generate it, there's just a dependency..?


It's generated by the same rule as the other file. This is the 
easiest workaround for two files generated from the same rule. It 
sort of works (for clean builds and if the input is chagned), but 
won't handle all cases where one file is removed externally and the 
other is not. I suggested this construct to Naoto. Perhaps a comment 
explaining this would be good.
The removal of the duplicate "common", that seems like a separate 
bug fix?



It does indeed. Before this fix, the wildcards must have returned empty.

/Erik

/Magnus



This fix is to remove the hand crafted map file (lib/tzmappings) on 
Windows, which maps Windows time zones to Java's tzid. It is now 
dynamically generated at the build time, using CLDR's data file 
(windowsZones.xml)


Naoto