Re: RFR: 8248238: Implementation of JEP: Windows AArch64 Support [v12]

2020-10-01 Thread Daniel D. Daugherty 

So I'm confused... this PR is associated with this bug ID:



  Issue

  * JDK-8248238 :
Implementation of JEP: Windows AArch64 Support




and JDK-8248238 is associated with this JEP:

JDK-8248496  JEP 
388: Windows/AArch64 Port 


Am I missing something here?

Dan


On 10/1/20 5:56 PM, Ludovic Henry wrote:

Hi David,


The JEP is not yet targeted so we have to wait for that formality. But once 
that happens I can sponsor for you.

Perfect, I didn't know about the need for the JEP to be targeted before the 
merge.


Also note that the PR references the wrong JEP so can you please edit the 
description to fix that.

I'll work with @Monica to update the PR's description to point to 
https://openjdk.java.net/jeps/391 instead.

Thank you!
Ludovic

Re: RFR: 8248238: Implementation of JEP: Windows AArch64 Support [v12]

2020-10-01 Thread David Holmes 

Hi,

On 2/10/2020 1:48 am, Ludovic Henry wrote:

Hi,

As we now have a whole bunch of reviews (thank you all!), we would need a 
sponsor to get it merged.


The JEP is not yet targeted so we have to wait for that formality. But 
once that happens I can sponsor for you.


Also note that the PR references the wrong JEP so can you please edit 
the description to fix that.


Meanwhile I'll see if I can take this for a spin through our internal 
testing.


Cheers,
David
-


Thank you :)

-

PR: https://github.com/openjdk/jdk/pull/212

Re: RFR: 8138732: Rename @HotSpotIntrinsicCandidate to @IntrinsicCandidate and move it to the jdk.internal.vm.annotation package [v2]

2020-10-01 Thread Philippe Marschall 




On 23.09.20 07:13, David Holmes wrote:

On Tue, 22 Sep 2020 08:43:04 GMT, Erik Gahlin  wrote:


Marked as reviewed by kvn (Reviewer).


Have you run the JFR tests in test/jdk/jdk/jfr?


@marschall Please do not force-push anything as it breaks the commit history in 
the PR and renders previous
reviews/comments obsolete. There is no way for the reviewers to see what 
changed between the commit they reviewed and
the commit now in the PR.

To update to latest changes you should have just merged your branch with your 
(up-to-date) local master, committed that
merge in your local branch and then pushed that commit to your Personal Fork. 
The skara tooling will flatten the series
of commits in a PR into one single well-formed commit that is pushed to the 
master branch of the repo.


My apologies, I was not aware of this, I won't make this mistake in the
future again. I don't assume there is a way to fix this now.

Philippe

Re: RFR: 8138732: Rename @HotSpotIntrinsicCandidate to @IntrinsicCandidate and move it to the jdk.internal.vm.annotation package [v2]

2020-10-01 Thread Philippe Marschall 




On 22.09.20 10:45, Erik Gahlin wrote:

On Sat, 12 Sep 2020 00:19:00 GMT, Vladimir Kozlov  wrote:


Philippe Marschall has refreshed the contents of this pull request, and 
previous commits have been removed. The
incremental views will show differences compared to the previous content of the 
PR. The pull request contains one new
commit since the last revision:
   8138732: Rename HotSpotIntrinsicCandidate to IntrinsicCandidate


Marked as reviewed by kvn (Reviewer).


Have you run the JFR tests in test/jdk/jdk/jfr?


I initially did not but now I have.
jdk.jfr.event.runtime.TestModuleEvents was failing, I have updated it to
what makes sense to me.

Cheers
Philippe

RE: RFR: 8248238: Implementation of JEP: Windows AArch64 Support [v12]

2020-10-01 Thread Ludovic Henry 
Hi David,

> The JEP is not yet targeted so we have to wait for that formality. But once 
> that happens I can sponsor for you.

Perfect, I didn't know about the need for the JEP to be targeted before the 
merge.

> Also note that the PR references the wrong JEP so can you please edit the 
> description to fix that.

I'll work with @Monica to update the PR's description to point to 
https://openjdk.java.net/jeps/391 instead.

Thank you!
Ludovic

Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v7]

2020-10-01 Thread Ioi Lam 
On Wed, 30 Sep 2020 04:59:20 GMT, Yumin Qi  wrote:

>> This patch is reorganized after 8252725, which is separated from this patch 
>> to refactor jlink glugin code. The previous
>> webrev with hg can be found at: 
>> http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725 
>> integrated, the
>> regeneration of holder classes is simply to call the new added 
>> GenerateJLIClassesHelper.cdsGenerateHolderClasses
>> function.  Tests: tier1-4
>
> Yumin Qi has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Remove trailing word of line which is not used in holder class 
> regeneration. There is a trailing LF (Line Feed) so trim
>   white spaces from both front and end of the line or it will fail method 
> type validation.

Changes requested by iklam (Reviewer).

src/java.base/share/classes/jdk/internal/misc/CDS.java line 30:

> 28: public class CDS {
> 29: // cache the result
> 30: static private boolean isDumpLoadedClassList;

`isDumpLoadedClassList` is not gramatically correct. Also the field should be 
final. How about:

static final private boolean isDumpingClassList = isDumpingClassList0();
public static boolean isDumpingClassList() {
return isDumpingClassList;
}
private static boolean isDumpingClassList0();

src/java.base/share/classes/jdk/internal/misc/CDS.java line 82:

> 80:  * log output to DumpLoadedClassList
> 81:  */
> 82: public static void logTraceResolve(String line) {

`logTraceResolve` is too generic. How about `CDS.logLambdaFormInvoker()` to 
match the `@lambda-form-invoker` in the
classlist file?

-

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

Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v7]

2020-10-01 Thread Ioi Lam 
On Wed, 30 Sep 2020 04:59:20 GMT, Yumin Qi  wrote:

>> This patch is reorganized after 8252725, which is separated from this patch 
>> to refactor jlink glugin code. The previous
>> webrev with hg can be found at: 
>> http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725 
>> integrated, the
>> regeneration of holder classes is simply to call the new added 
>> GenerateJLIClassesHelper.cdsGenerateHolderClasses
>> function.  Tests: tier1-4
>
> Yumin Qi has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Remove trailing word of line which is not used in holder class 
> regeneration. There is a trailing LF (Line Feed) so trim
>   white spaces from both front and end of the line or it will fail method 
> type validation.

Changes requested by iklam (Reviewer).

test/hotspot/jtreg/runtime/cds/appcds/DumpClassListWithLF.java line 70:

> 68: "Hello",
> 69: "@lambda-form-invoker [LF_RESOLVE] 
> java.lang.invoke.DirectMethodHandle$Holder invokeNothing  L7_L
> (anyword)", 70: "@lambda-form-invoker [LF_RESOLVE] 
> java.lang.invoke.DirectMethodHandle$Holder
> invokeNothing  LL_I anyword"),

We shouldn't allow the classlist to contain arbitrary data. These two cases 
should generate an error.

src/hotspot/share/classfile/lambdaFormInvokers.cpp line 52:

> 50:
> 51: // trim white spaces from front and end of string.
> 52: char* trim(char* s) {

I think this creates unnecessary dependency between the C code and the Java 
code. The C code assumes that the Java code
has appended something like "(salvaged)" into the output, and tries to get rid 
of that in a non-obvious way. It's
better to modify the Java code from

static void traceSpeciesType(String cn, Class salvage) {
if (TRACE_RESOLVE || CDS.isDumpLoadedClassList()) {
String traceSP = SPECIES_RESOLVE + " " + cn + (salvage != null ? " 
(salvaged)" : " (generated)");
if (TRACE_RESOLVE) {
System.out.println(traceSP);
}
CDS.logTraceResolve(traceSP);
}
}

to

if (TRACE_RESOLVE || CDS.isDumpLoadedClassList()) {
String traceSP = SPECIES_RESOLVE + " " + cn;
if (TRACE_RESOLVE) {
System.out.println(traceSP + (salvage != null ? " (salvaged)" : " 
(generated)"));
}
CDS.logTraceResolve(traceSP);
}

test/hotspot/jtreg/runtime/cds/appcds/DumpClassListWithLF.java line 85:

> 83: appJar, classlist(
> 84: "Hello",
> 85: "@lambda-form-invoker [LF_XYRESOLVE] 
> java.lang.invoke.DirectMethodHandle$Holder invokeStatic  L7_L
> (any)",

We should not allow incorrect input. This should generate an error.

test/hotspot/jtreg/runtime/cds/appcds/DumpClassListWithLF.java line 78:

> 76: "Hello",
> 77: "@lambda-form-invoker [LF_RESOLVE] 
> my.nonexist.package.MyNonExistClassName$holder invokeStatic
> L7_L", 78: "@lambda-form-invoker [LF_RESOLVE] 
> my.nonexist.package.MyNonExistClassName$holder
> invokeStatic  LL_I"),

I think it's dangerous to allow arbitrary class names here. 
InvokerBytecodeGenerator doesn't check the classname. This
will make it possible to overwrite the contents of arbitrary classes. We should 
have a check here and allow only the
specific holder classes that are supported.

-

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

RE: RFR: 8248238: Implementation of JEP: Windows AArch64 Support [v12]

2020-10-01 Thread Ludovic Henry 
Hi,

As we now have a whole bunch of reviews (thank you all!), we would need a 
sponsor to get it merged.

Thank you :) 

-

PR: https://github.com/openjdk/jdk/pull/212