Re: RFR: 8186780: clang-4.0 fastdebug assertion failure in os_linux_x86:os::verify_stack_alignment()

[Martin Buchholz]

Since the question of how to get the stack pointer in hotspot is
unexpectedly difficult, I propose we split into two separate changes and we
can submit the make/ changes that Erik is happy with.

Re: RFR 8205445: Add RSASSA-PSS Signature support to SunMSCAPI

[Weijun Wang]

> On Jun 23, 2018, at 8:35 AM, Valerie Peng  wrote:
> 
> On 6/22/2018 3:23 PM, Weijun Wang wrote:
>>> On Jun 23, 2018, at 2:30 AM, Valerie Peng  wrote:
>>> 
>>> Max,
>>> 
>>> Good catch on the SunRsaSign provider bug.
>>> 
>>> Looking at the changes, I think we may have to fine-grain the check on the 
>>> ensureInit() call, i.e.
>>> 
>>> use ensureInit(boolean sign) instead of ensureInit(), as the current method 
>>> only ensures that at least one of the privKey, pubKey or fallbackSignature 
>>> is non-null, I think it should check the right one is non-null, i.e. sign 
>>> -> privKey, verify -> pubKey/fallbackSignature.
>> Could anything go wrong? This method just ensures one of initSign() or 
>> initVerify() is called.
> Only when the initSign()/initVerify() does not match the subsequent calls of 
> sign()/verify() I suppose.

I see what you mean.

The Signature class takes care of it:

public final byte[] sign() throws SignatureException {
if (state == SIGN) {
return engineSign();
}
throw new SignatureException("object not initialized for " +
 "signing");
}

Thanks
Max

> Valerie
>>> In the PSS class engineInitVerify(...) method if the specified key is a 
>>> MSCAPI public key, then fallbackSignature is set to null and the native 
>>> verifyPssSignedHash method is used, right?
>> Yes. The native method only fails when trying to import from a blob.
>> 
>> Thanks
>> Max
>> 
>>> Thanks,
>>> 
>>> Valerie
>>> 
>>> On 6/21/2018 10:39 PM, Weijun Wang wrote:
 Webrev updated at
 
   http://cr.openjdk.java.net/~weijun/8205445/webrev.01
 
 
 I think I found a bug in SunRsaSign of the RSASSA-PSS signature. Fixed and 
 added a test.
 
 BTW, I commented out the debug code in security.cpp. Once there is a bug I 
 can use it.
 
 Thanks
 Max
 
 
> On Jun 21, 2018, at 11:23 PM, Weijun Wang 
>  wrote:
> 
> 
> 
> 
>> On Jun 21, 2018, at 11:07 PM, Xuelei Fan 
>>  wrote:
>> 
>> Hi Weijun,
>> 
>> The release note and the following notes look reasonable to me.
>> 
>> For the implementation part, could it be a little bit more 
>> straightforward if wrapping the new attributes 
>> (pss/pssParams/fallbackSignature) and codes (if pss/fallbackSignature, 
>> etc) in the PSS subclass?
>> 
> Sounds good. I'll try it.
> 
> 
>> Did you want to remove the debug code in the security.cpp?  It seems 
>> that they are not used any more.
>> 
> Sure I can.
> 
> Thanks
> Max
> 
> 
>> Xuelei
>> 
>> On 6/21/2018 4:12 AM, Weijun Wang wrote:
>> 
>>> Please take a review on this change
>>>  http://cr.openjdk.java.net/~weijun/8205445/webrev.00/
>>> 
>>>   and the release note at
>>>  https://bugs.openjdk.java.net/browse/JDK-8205471
>>> 
>>> The code change adds RSASSA-PSS signature support to the SunMSCAPI 
>>> provider.
>>> Several notes:
>>> 1. CryptoAPI (which SunMSCAPI is based on and now a deprecated 
>>> technology) does not support RSASSA-PSS. In fact, CNG [1] is used to 
>>> perform the signing and verification. This is certainly not a perfect 
>>> solution and we are thinking of support CNG in a more sophisticated way 
>>> in future releases of JDK.
>>> 2. For unknown reason, the newly added verification code for RSASSA-PSS 
>>> does not work correctly (precisely, ::NCryptTranslateHandle returns 
>>> NTE_INVALID_PARAMETER). A fallback mechanism is added into 
>>> mscapi/RSASignature.java. A SunRsaSign Signature object is actually 
>>> used when a SunMSCAPI Signature is initialized to verify an RSASSA-PSS 
>>> signature.
>>> 3. It looks like CNG only supports PSSParamterSpec with the same 
>>> message hash algorithm and MGF1 hash algorithm, because there is only 
>>> one algorithm field in BCRYPT_PSS_PADDING_INFO [2]. This is checked 
>>> when setting the parameter.
>>> 4. It looks like CNG only supports RSASSA-PSS using these hash 
>>> algorithms: SHA-1, SHA-256, SHA-384, and SHA-512. This is not checked 
>>> at parameter setting but sign() will throw a SignatureException saying 
>>> "Unrecognised hash algorithm". Since the verify() side uses a fallback 
>>> SunRsaSign signature, other hash algorithms are supported.
>>> Thanks
>>> Max
>>> [1]
>>> https://msdn.microsoft.com/en-us/library/windows/desktop/aa376210(v=vs.85).aspx
>>> 
>>> [2]
>>> https://msdn.microsoft.com/en-us/library/windows/desktop/aa375529(v=vs.85).aspx
>>> 
>>> [3]
>>> https://msdn.microsoft.com/en-us/library/windows/desktop/aa375534(v=vs.85).aspx
> 

Re: RFR 8205445: Add RSASSA-PSS Signature support to SunMSCAPI

[Valerie Peng]

On 6/22/2018 3:23 PM, Weijun Wang wrote:

On Jun 23, 2018, at 2:30 AM, Valerie Peng  wrote:

Max,

Good catch on the SunRsaSign provider bug.

Looking at the changes, I think we may have to fine-grain the check on the 
ensureInit() call, i.e.

use ensureInit(boolean sign) instead of ensureInit(), as the current method only 
ensures that at least one of the privKey, pubKey or fallbackSignature is non-null, I 
think it should check the right one is non-null, i.e. sign -> privKey, verify 
-> pubKey/fallbackSignature.

Could anything go wrong? This method just ensures one of initSign() or 
initVerify() is called.
Only when the initSign()/initVerify() does not match the subsequent 
calls of sign()/verify() I suppose.

Valerie

In the PSS class engineInitVerify(...) method if the specified key is a MSCAPI 
public key, then fallbackSignature is set to null and the native 
verifyPssSignedHash method is used, right?

Yes. The native method only fails when trying to import from a blob.

Thanks
Max


Thanks,

Valerie

On 6/21/2018 10:39 PM, Weijun Wang wrote:

Webrev updated at

   
http://cr.openjdk.java.net/~weijun/8205445/webrev.01



I think I found a bug in SunRsaSign of the RSASSA-PSS signature. Fixed and 
added a test.

BTW, I commented out the debug code in security.cpp. Once there is a bug I can 
use it.

Thanks
Max



On Jun 21, 2018, at 11:23 PM, Weijun Wang 
  wrote:





On Jun 21, 2018, at 11:07 PM, Xuelei Fan 
  wrote:

Hi Weijun,

The release note and the following notes look reasonable to me.

For the implementation part, could it be a little bit more straightforward if 
wrapping the new attributes (pss/pssParams/fallbackSignature) and codes (if 
pss/fallbackSignature, etc) in the PSS subclass?


Sounds good. I'll try it.



Did you want to remove the debug code in the security.cpp?  It seems that they 
are not used any more.


Sure I can.

Thanks
Max



Xuelei

On 6/21/2018 4:12 AM, Weijun Wang wrote:


Please take a review on this change
  
http://cr.openjdk.java.net/~weijun/8205445/webrev.00/


   and the release note at
  
https://bugs.openjdk.java.net/browse/JDK-8205471


The code change adds RSASSA-PSS signature support to the SunMSCAPI provider.
Several notes:
1. CryptoAPI (which SunMSCAPI is based on and now a deprecated technology) does 
not support RSASSA-PSS. In fact, CNG [1] is used to perform the signing and 
verification. This is certainly not a perfect solution and we are thinking of 
support CNG in a more sophisticated way in future releases of JDK.
2. For unknown reason, the newly added verification code for RSASSA-PSS does 
not work correctly (precisely, ::NCryptTranslateHandle returns 
NTE_INVALID_PARAMETER). A fallback mechanism is added into 
mscapi/RSASignature.java. A SunRsaSign Signature object is actually used when a 
SunMSCAPI Signature is initialized to verify an RSASSA-PSS signature.
3. It looks like CNG only supports PSSParamterSpec with the same message hash 
algorithm and MGF1 hash algorithm, because there is only one algorithm field in 
BCRYPT_PSS_PADDING_INFO [2]. This is checked when setting the parameter.
4. It looks like CNG only supports RSASSA-PSS using these hash algorithms: SHA-1, 
SHA-256, SHA-384, and SHA-512. This is not checked at parameter setting but sign() will 
throw a SignatureException saying "Unrecognised hash algorithm". Since the 
verify() side uses a fallback SunRsaSign signature, other hash algorithms are supported.
Thanks
Max
[1]
https://msdn.microsoft.com/en-us/library/windows/desktop/aa376210(v=vs.85).aspx

[2]
https://msdn.microsoft.com/en-us/library/windows/desktop/aa375529(v=vs.85).aspx

[3]
https://msdn.microsoft.com/en-us/library/windows/desktop/aa375534(v=vs.85).aspx

Re: RFR 8205445: Add RSASSA-PSS Signature support to SunMSCAPI

[Weijun Wang]

> On Jun 23, 2018, at 2:30 AM, Valerie Peng  wrote:
> 
> Max,
> 
> Good catch on the SunRsaSign provider bug.
> 
> Looking at the changes, I think we may have to fine-grain the check on the 
> ensureInit() call, i.e.
> 
> use ensureInit(boolean sign) instead of ensureInit(), as the current method 
> only ensures that at least one of the privKey, pubKey or fallbackSignature is 
> non-null, I think it should check the right one is non-null, i.e. sign -> 
> privKey, verify -> pubKey/fallbackSignature.

Could anything go wrong? This method just ensures one of initSign() or 
initVerify() is called.

> 
> In the PSS class engineInitVerify(...) method if the specified key is a 
> MSCAPI public key, then fallbackSignature is set to null and the native 
> verifyPssSignedHash method is used, right?

Yes. The native method only fails when trying to import from a blob.

Thanks
Max

> 
> Thanks,
> 
> Valerie
> 
> On 6/21/2018 10:39 PM, Weijun Wang wrote:
>> Webrev updated at
>> 
>>   
>> http://cr.openjdk.java.net/~weijun/8205445/webrev.01
>> 
>> 
>> I think I found a bug in SunRsaSign of the RSASSA-PSS signature. Fixed and 
>> added a test.
>> 
>> BTW, I commented out the debug code in security.cpp. Once there is a bug I 
>> can use it.
>> 
>> Thanks
>> Max
>> 
>> 
>>> On Jun 21, 2018, at 11:23 PM, Weijun Wang 
>>>  wrote:
>>> 
>>> 
>>> 
>>> 
 On Jun 21, 2018, at 11:07 PM, Xuelei Fan 
  wrote:
 
 Hi Weijun,
 
 The release note and the following notes look reasonable to me.
 
 For the implementation part, could it be a little bit more straightforward 
 if wrapping the new attributes (pss/pssParams/fallbackSignature) and codes 
 (if pss/fallbackSignature, etc) in the PSS subclass?
 
>>> Sounds good. I'll try it.
>>> 
>>> 
 Did you want to remove the debug code in the security.cpp?  It seems that 
 they are not used any more.
 
>>> Sure I can.
>>> 
>>> Thanks
>>> Max
>>> 
>>> 
 Xuelei
 
 On 6/21/2018 4:12 AM, Weijun Wang wrote:
 
> Please take a review on this change
>  
> http://cr.openjdk.java.net/~weijun/8205445/webrev.00/
> 
>   and the release note at
>  
> https://bugs.openjdk.java.net/browse/JDK-8205471
> 
> The code change adds RSASSA-PSS signature support to the SunMSCAPI 
> provider.
> Several notes:
> 1. CryptoAPI (which SunMSCAPI is based on and now a deprecated 
> technology) does not support RSASSA-PSS. In fact, CNG [1] is used to 
> perform the signing and verification. This is certainly not a perfect 
> solution and we are thinking of support CNG in a more sophisticated way 
> in future releases of JDK.
> 2. For unknown reason, the newly added verification code for RSASSA-PSS 
> does not work correctly (precisely, ::NCryptTranslateHandle returns 
> NTE_INVALID_PARAMETER). A fallback mechanism is added into 
> mscapi/RSASignature.java. A SunRsaSign Signature object is actually used 
> when a SunMSCAPI Signature is initialized to verify an RSASSA-PSS 
> signature.
> 3. It looks like CNG only supports PSSParamterSpec with the same message 
> hash algorithm and MGF1 hash algorithm, because there is only one 
> algorithm field in BCRYPT_PSS_PADDING_INFO [2]. This is checked when 
> setting the parameter.
> 4. It looks like CNG only supports RSASSA-PSS using these hash 
> algorithms: SHA-1, SHA-256, SHA-384, and SHA-512. This is not checked at 
> parameter setting but sign() will throw a SignatureException saying 
> "Unrecognised hash algorithm". Since the verify() side uses a fallback 
> SunRsaSign signature, other hash algorithms are supported.
> Thanks
> Max
> [1] 
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa376210(v=vs.85).aspx
> 
> [2] 
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa375529(v=vs.85).aspx
> 
> [3] 
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa375534(v=vs.85).aspx
> 

Re: RFR(M): 8205207: Port Graal unit tests under jtreg

[Ekaterina Pavlova]

Fixed and regenerated webrev at the same location:
 http://cr.openjdk.java.net/~epavlova//8205207/webrev.01/index.html

Erik,
thanks again for your detailed reviews!

-katya

On 6/22/18 2:38 PM, Erik Joelsson wrote:

Hello Katya,

This looks much better, thanks!

A few suggestions still:

Main.gmk: instead of repeating the assignment in both if and else block:

ifeq ($(INCLUDE_GRAAL), true)
   JVM_TEST_IMAGE_TARGETS += test-image-hotspot-jtreg-graal
endif

I think it's fine to do that without the ?= because an alternative JVM 
implementation is unlikely to be using graal anyway.

In JtregGraalUnit.gmk: Some minor style nits:
54: align )) with first eval line as you have done with the other eval calls
118: Since 117 is a one line parameter, please move comma to 117
133: Same as for 118
130-132: Please indent 4 spaces for continuation

/Erik

On 2018-06-22 14:16, Ekaterina Pavlova wrote:

Erik, Doug,

thank you a lot for your reviews and advises.
I fixed everything what Erik has pointed out, please see my answers inlined.
As about moving more staff in 'updategraalinopenjdk' can we consider this as 
next step?
I am not quite familiar with 'updategraalinopenjdk' and didn't contribute into 
Graal ws yet,
so I will prefer to do this improvement/refactoring as part of separate JDK 
issue.

New webrev is here:
  webrev: http://cr.openjdk.java.net/~epavlova//8205207/webrev.01/index.html
 JBS: https://bugs.openjdk.java.net/browse/JDK-8205207
 testing: tested by building and running graal unit tests


On 6/19/18 2:00 PM, Erik Joelsson wrote:

Hello,

Please always include build-dev when reviewing build changes.

In general this looks pretty good but there are some details that need fixing.

(Aside from this particular review, I must state my reservation against all the 
special treatment graal is enjoying from a source and test layout and build 
perspective. My understanding here is that someone will have to regularly 
update the wrapper jtreg tests manually using the script. This in addition to 
having to implement this very convoluted redirection logic because the tests 
are in the wrong place. Wouldn't it make a lot more sense to put the 
complicated logic in the import procedure for graal source so that it would fit 
nicely into the OpenJDK source/build/test model, instead of adding more and 
more complicated workarounds in the OpenJDK build and test procedures?)

On 2018-06-18 22:26, Ekaterina Pavlova wrote:

Hi All,

please review porting of Graal unit tests under jtreg. The main idea of this 
porting is to keep Graal unit tests as is
(located in src/jdk.internal.vm.compiler/share/classes/*.test) and run them 
similar way they are run in Graal project.
To achieve this 
test/hotspot/jtreg/compiler/graalunit/common/GraalUnitTestLauncher.java helper 
class has been written
which simply launches com.oracle.mxtool.junit.MxJUnitWrapper to run specified 
set of Graal unit tests. The groups of
Graal unit tests to run are defined in auto-generated 
test/hotspot/jtreg/compiler/graalunit/*.java jtreg tests.

New make/test/JtregGraalUnit.gmk is used to build Graal unit tests into 
jdk.vm.compiler.tests.jar and then install
it in $(TEST_IMAGE_DIR)/hotspot/jtreg/graal/.


GRAALUNIT_LIB is never defined (in the open). Since this is used in open 
makefiles, we need an open configure option to set it.


ok, I created open/make/autoconf/lib-tests.m4 and did put Graal related staff 
there.


To make things clearer, I would rename LIB_DIR to something like LIB_OUTPUTDIR 
to signal that this is a location to put files in, rather than read them from.


ok, renamed



FLATTEN has no effect in the SetupCopyFiles call since all the jar files found 
by wildcard can only be in one directory anyway.


ok, removed


BUILDTOOLS_OUTPUTDIR is meant for tools used during the build. Tests classes and 
jars should be built into $(SUPPORT_OUTPUTDIR)/test/..> Please create a 
suitable sub directory there for the output from this makefile.


ok, introduced COMPILE_OUTPUTDIR := $(SUPPORT_OUTPUTDIR)/test/graalunit to be 
used to build graal unit test classes.


The all and test-image targets are never called so no need to declare them.

There are some style issues too. (Please see 
http://openjdk.java.net/groups/build/doc/code-conventions.html for details.)
* 43 logic indent 2 spaces
* 52 we like to put the ending )) on a new line with ', \' at the end of the 
line before
* 58 continuation indent 4 spaces
* 88, 89 please break long lines
* 90 continuation indent 4 spaces
* 99 continuation indent 4 spaces
* 120 break before ))


I think I fixed everything


make/Main.gmk adds proper dependencies for build-test-hotspot-jtreg-graal and 
test-image-hotspot-jtreg-graal.


The target build-test-hotspot-jtreg-graal only needs to depend on 
exploded-image-optimize.


fixed


The new graal specific targets should be guarded by INCLUDE_GRAAL in Main.gmk. 
Otherwise those targets will silently do nothing if invoked without graal. That 
means adding them 

Re: RFR(M): 8205207: Port Graal unit tests under jtreg

[Ekaterina Pavlova]

On 6/22/18 2:29 PM, Doug Simon wrote:




On 22 Jun 2018, at 23:16, Ekaterina Pavlova  
wrote:

Erik, Doug,

thank you a lot for your reviews and advises.
I fixed everything what Erik has pointed out, please see my answers inlined.
As about moving more staff in 'updategraalinopenjdk' can we consider this as 
next step?
I am not quite familiar with 'updategraalinopenjdk' and didn't contribute into 
Graal ws yet,
so I will prefer to do this improvement/refactoring as part of separate JDK 
issue.


Sorry for not being clearer. I don't expect you to fix/enhance 
updategraalinopenjdk. I was simply commenting on a possible solution to Joel's 
reasonabl> request for having Graal sources and test layout more normalized in 
the JDK code base.


ok, good :)

-katya

Re: RFR(M): 8205207: Port Graal unit tests under jtreg

[Doug Simon]

> On 22 Jun 2018, at 23:29, Doug Simon  wrote:
> 
> 
> 
>> On 22 Jun 2018, at 23:16, Ekaterina Pavlova  
>> wrote:
>> 
>> Erik, Doug,
>> 
>> thank you a lot for your reviews and advises.
>> I fixed everything what Erik has pointed out, please see my answers inlined.
>> As about moving more staff in 'updategraalinopenjdk' can we consider this as 
>> next step?
>> I am not quite familiar with 'updategraalinopenjdk' and didn't contribute 
>> into Graal ws yet,
>> so I will prefer to do this improvement/refactoring as part of separate JDK 
>> issue.
> 
> Sorry for not being clearer. I don't expect you to fix/enhance 
> updategraalinopenjdk. I was simply commenting on a possible solution to 
> Joel's reasonable request for having Graal sources and test layout more 
> normalized in the JDK code base.

Of course I meant Erik's request ;-)

-Doug

Re: RFR: JDK-8189429: SA: MacOSX: Replace the deprecated PT_ATTACH with PT_ATTACHEXC

[Erik Joelsson]

Hello Jini,

In general this looks pretty good, but it's also breaking some new 
ground as it's adding generation of native source in the java gensrc 
step. Mixing native code with the java code that the genrcs targets and 
gensrc output directories are meant for seems ok for now, but may cause 
trouble in the future. I'm going to accept it for now though.


In Gensrc-jdk.hotspot.agent.gmk: Please put the new macosx stuff in its 
own section (as delimited by the 80x # lines) and put that whole thing 
inside a conditional for macosx. Also please break line 47 (for recipe 
lines, indent with tab and 4 additional spaces for continuation [1]).


In Lib-jdk.hotspot.agent.gmk: I believe adding extra src should be 
enough as that will implicitly add the same directories as header dirs 
by default. At least that's the intention.


/Erik

[1] http://openjdk.java.net/groups/build/doc/code-conventions.html

On 2018-06-22 11:11, Jini George wrote:

Hi all,

[Including build-dev also since this includes build related changes].

Requesting reviews for:

https://bugs.openjdk.java.net/browse/JDK-8189429 (SA: MacOSX: Replace 
the deprecated PT_ATTACH with PT_ATTACHEXC)


Webrev: http://cr.openjdk.java.net/~jgeorge/8189429/webrev.04/

This is the follow-up solution for the temporary restoration of 
PT_ATTACH to fix https://bugs.openjdk.java.net/browse/JDK-8184042 
(several serviceability/sa tests timed out on MacOS X), which was done 
in Oct 2017. The mails related to this are at:


http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-August/021702.html 



Changes as compared to the patch sent last year 
(http://cr.openjdk.java.net/~jgeorge/8184042/webrev.00/):


* Addressed the review comments which were provided by Poonam, Dan, 
Dmitry.
* A major change as compared to what was done last year is that the 
MIG generated files have been included as a part of the JDK build 
process.
* The test case which was provided in the patch last year is no longer 
required since we have ClhsdbAttach.java testing the same 
functionality as a part of the SA testsuite now.
* Other than that, some minor improvements have been done wrt error 
handling.


The steps for the proposed solution have been provided in JBS.

Testing: ALL the SA tests pass on MacOSX and the other Mach5 platforms.

Thanks to Sharath, Robin, Gerard and Dan for looking into the changes 
and providing valuable comments.


Thanks,
Jini.

Re: RFR(M): 8205207: Port Graal unit tests under jtreg

[Erik Joelsson]

Hello Katya,

This looks much better, thanks!

A few suggestions still:

Main.gmk: instead of repeating the assignment in both if and else block:

ifeq ($(INCLUDE_GRAAL), true)
  JVM_TEST_IMAGE_TARGETS += test-image-hotspot-jtreg-graal
endif

I think it's fine to do that without the ?= because an alternative JVM 
implementation is unlikely to be using graal anyway.


In JtregGraalUnit.gmk: Some minor style nits:
54: align )) with first eval line as you have done with the other eval calls
118: Since 117 is a one line parameter, please move comma to 117
133: Same as for 118
130-132: Please indent 4 spaces for continuation

/Erik

On 2018-06-22 14:16, Ekaterina Pavlova wrote:

Erik, Doug,

thank you a lot for your reviews and advises.
I fixed everything what Erik has pointed out, please see my answers 
inlined.
As about moving more staff in 'updategraalinopenjdk' can we consider 
this as next step?
I am not quite familiar with 'updategraalinopenjdk' and didn't 
contribute into Graal ws yet,
so I will prefer to do this improvement/refactoring as part of 
separate JDK issue.


New webrev is here:
  webrev: 
http://cr.openjdk.java.net/~epavlova//8205207/webrev.01/index.html

 JBS: https://bugs.openjdk.java.net/browse/JDK-8205207
 testing: tested by building and running graal unit tests


On 6/19/18 2:00 PM, Erik Joelsson wrote:

Hello,

Please always include build-dev when reviewing build changes.

In general this looks pretty good but there are some details that 
need fixing.


(Aside from this particular review, I must state my reservation 
against all the special treatment graal is enjoying from a source and 
test layout and build perspective. My understanding here is that 
someone will have to regularly update the wrapper jtreg tests 
manually using the script. This in addition to having to implement 
this very convoluted redirection logic because the tests are in the 
wrong place. Wouldn't it make a lot more sense to put the complicated 
logic in the import procedure for graal source so that it would fit 
nicely into the OpenJDK source/build/test model, instead of adding 
more and more complicated workarounds in the OpenJDK build and test 
procedures?)


On 2018-06-18 22:26, Ekaterina Pavlova wrote:

Hi All,

please review porting of Graal unit tests under jtreg. The main idea 
of this porting is to keep Graal unit tests as is
(located in src/jdk.internal.vm.compiler/share/classes/*.test) and 
run them similar way they are run in Graal project.
To achieve this 
test/hotspot/jtreg/compiler/graalunit/common/GraalUnitTestLauncher.java 
helper class has been written
which simply launches com.oracle.mxtool.junit.MxJUnitWrapper to run 
specified set of Graal unit tests. The groups of
Graal unit tests to run are defined in auto-generated 
test/hotspot/jtreg/compiler/graalunit/*.java jtreg tests.


New make/test/JtregGraalUnit.gmk is used to build Graal unit tests 
into jdk.vm.compiler.tests.jar and then install

it in $(TEST_IMAGE_DIR)/hotspot/jtreg/graal/.

GRAALUNIT_LIB is never defined (in the open). Since this is used in 
open makefiles, we need an open configure option to set it.


ok, I created open/make/autoconf/lib-tests.m4 and did put Graal 
related staff there.


To make things clearer, I would rename LIB_DIR to something like 
LIB_OUTPUTDIR to signal that this is a location to put files in, 
rather than read them from.


ok, renamed



FLATTEN has no effect in the SetupCopyFiles call since all the jar 
files found by wildcard can only be in one directory anyway.


ok, removed

BUILDTOOLS_OUTPUTDIR is meant for tools used during the build. Tests 
classes and jars should be built into $(SUPPORT_OUTPUTDIR)/test/..> 
Please create a suitable sub directory there for the output from this 
makefile.


ok, introduced COMPILE_OUTPUTDIR := 
$(SUPPORT_OUTPUTDIR)/test/graalunit to be used to build graal unit 
test classes.


The all and test-image targets are never called so no need to declare 
them.


There are some style issues too. (Please see 
http://openjdk.java.net/groups/build/doc/code-conventions.html for 
details.)

* 43 logic indent 2 spaces
* 52 we like to put the ending )) on a new line with ', \' at the end 
of the line before

* 58 continuation indent 4 spaces
* 88, 89 please break long lines
* 90 continuation indent 4 spaces
* 99 continuation indent 4 spaces
* 120 break before ))


I think I fixed everything

make/Main.gmk adds proper dependencies for 
build-test-hotspot-jtreg-graal and test-image-hotspot-jtreg-graal.


The target build-test-hotspot-jtreg-graal only needs to depend on 
exploded-image-optimize.


fixed

The new graal specific targets should be guarded by INCLUDE_GRAAL in 
Main.gmk. Otherwise those targets will silently do nothing if invoked 
without graal. That means adding them to JVM_TEST_IMAGE_TARGETS needs 
to be conditional too.


fixed

test/TestCommon.gmk passes TEST_IMAGE_GRAAL_DIR to jtreg so jtreg 
knows where to find Graal tests and libs.


This needs to be duplicated 

Re: RFR(M): 8205207: Port Graal unit tests under jtreg

[Doug Simon]

> On 22 Jun 2018, at 23:16, Ekaterina Pavlova  
> wrote:
> 
> Erik, Doug,
> 
> thank you a lot for your reviews and advises.
> I fixed everything what Erik has pointed out, please see my answers inlined.
> As about moving more staff in 'updategraalinopenjdk' can we consider this as 
> next step?
> I am not quite familiar with 'updategraalinopenjdk' and didn't contribute 
> into Graal ws yet,
> so I will prefer to do this improvement/refactoring as part of separate JDK 
> issue.

Sorry for not being clearer. I don't expect you to fix/enhance 
updategraalinopenjdk. I was simply commenting on a possible solution to Joel's 
reasonable request for having Graal sources and test layout more normalized in 
the JDK code base.

-Doug

Re: RFR(M): 8205207: Port Graal unit tests under jtreg

[Ekaterina Pavlova]

Erik, Doug,

thank you a lot for your reviews and advises.
I fixed everything what Erik has pointed out, please see my answers inlined.
As about moving more staff in 'updategraalinopenjdk' can we consider this as 
next step?
I am not quite familiar with 'updategraalinopenjdk' and didn't contribute into 
Graal ws yet,
so I will prefer to do this improvement/refactoring as part of separate JDK 
issue.

New webrev is here:
  webrev: http://cr.openjdk.java.net/~epavlova//8205207/webrev.01/index.html
 JBS: https://bugs.openjdk.java.net/browse/JDK-8205207
 testing: tested by building and running graal unit tests


On 6/19/18 2:00 PM, Erik Joelsson wrote:

Hello,

Please always include build-dev when reviewing build changes.

In general this looks pretty good but there are some details that need fixing.

(Aside from this particular review, I must state my reservation against all the 
special treatment graal is enjoying from a source and test layout and build 
perspective. My understanding here is that someone will have to regularly 
update the wrapper jtreg tests manually using the script. This in addition to 
having to implement this very convoluted redirection logic because the tests 
are in the wrong place. Wouldn't it make a lot more sense to put the 
complicated logic in the import procedure for graal source so that it would fit 
nicely into the OpenJDK source/build/test model, instead of adding more and 
more complicated workarounds in the OpenJDK build and test procedures?)

On 2018-06-18 22:26, Ekaterina Pavlova wrote:

Hi All,

please review porting of Graal unit tests under jtreg. The main idea of this 
porting is to keep Graal unit tests as is
(located in src/jdk.internal.vm.compiler/share/classes/*.test) and run them 
similar way they are run in Graal project.
To achieve this 
test/hotspot/jtreg/compiler/graalunit/common/GraalUnitTestLauncher.java helper 
class has been written
which simply launches com.oracle.mxtool.junit.MxJUnitWrapper to run specified 
set of Graal unit tests. The groups of
Graal unit tests to run are defined in auto-generated 
test/hotspot/jtreg/compiler/graalunit/*.java jtreg tests.

New make/test/JtregGraalUnit.gmk is used to build Graal unit tests into 
jdk.vm.compiler.tests.jar and then install
it in $(TEST_IMAGE_DIR)/hotspot/jtreg/graal/.


GRAALUNIT_LIB is never defined (in the open). Since this is used in open 
makefiles, we need an open configure option to set it.


ok, I created open/make/autoconf/lib-tests.m4 and did put Graal related staff 
there.


To make things clearer, I would rename LIB_DIR to something like LIB_OUTPUTDIR 
to signal that this is a location to put files in, rather than read them from.


ok, renamed



FLATTEN has no effect in the SetupCopyFiles call since all the jar files found 
by wildcard can only be in one directory anyway.


ok, removed


BUILDTOOLS_OUTPUTDIR is meant for tools used during the build. Tests classes and 
jars should be built into $(SUPPORT_OUTPUTDIR)/test/..> Please create a 
suitable sub directory there for the output from this makefile.


ok, introduced COMPILE_OUTPUTDIR := $(SUPPORT_OUTPUTDIR)/test/graalunit to be 
used to build graal unit test classes.


The all and test-image targets are never called so no need to declare them.

There are some style issues too. (Please see 
http://openjdk.java.net/groups/build/doc/code-conventions.html for details.)
* 43 logic indent 2 spaces
* 52 we like to put the ending )) on a new line with ', \' at the end of the 
line before
* 58 continuation indent 4 spaces
* 88, 89 please break long lines
* 90 continuation indent 4 spaces
* 99 continuation indent 4 spaces
* 120 break before ))


I think I fixed everything


make/Main.gmk adds proper dependencies for build-test-hotspot-jtreg-graal and 
test-image-hotspot-jtreg-graal.


The target build-test-hotspot-jtreg-graal only needs to depend on 
exploded-image-optimize.


fixed


The new graal specific targets should be guarded by INCLUDE_GRAAL in Main.gmk. 
Otherwise those targets will silently do nothing if invoked without graal. That 
means adding them to JVM_TEST_IMAGE_TARGETS needs to be conditional too.


fixed


test/TestCommon.gmk passes TEST_IMAGE_GRAAL_DIR to jtreg so jtreg knows where 
to find Graal tests and libs.


This needs to be duplicated for make/RunTest.gmk so that these tests work with "make 
run-test" as well.


added

thanks,
-katya


/Erik

test/hotspot/jtreg/compiler/graalunit/TestPackages.txt file defines "testName -> 
testPrefix [requiresStatement]" mapping
which is used by test/hotspot/jtreg/compiler/graalunit/generateTests.sh script 
to generate jtreg tests.

test/hotspot/jtreg/compiler/graalunit/com.oracle.mxtool.junit was ported from 
mx project without any changes.


    JBS: https://bugs.openjdk.java.net/browse/JDK-8205207
 webrev: http://cr.openjdk.java.net/~epavlova//8205207/webrev.00/index.html
testing: new tests were executed as part of automatic HS testing for several 
months.


thanks,
-katya

p.s.
 

Re: RFR 8205445: Add RSASSA-PSS Signature support to SunMSCAPI

[Valerie Peng]

Max,

Good catch on the SunRsaSign provider bug.

Looking at the changes, I think we may have to fine-grain the check on 
the ensureInit() call, i.e.


use ensureInit(boolean sign) instead of ensureInit(), as the current 
method only ensures that at least one of the privKey, pubKey or 
fallbackSignature is non-null, I think it should check the right one is 
non-null, i.e. sign -> privKey, verify -> pubKey/fallbackSignature.


In the PSS class engineInitVerify(...) method if the specified key is a 
MSCAPI public key, then fallbackSignature is set to null and the native 
verifyPssSignedHash method is used, right?


Thanks,

Valerie

On 6/21/2018 10:39 PM, Weijun Wang wrote:

Webrev updated at

   http://cr.openjdk.java.net/~weijun/8205445/webrev.01

I think I found a bug in SunRsaSign of the RSASSA-PSS signature. Fixed and 
added a test.

BTW, I commented out the debug code in security.cpp. Once there is a bug I can 
use it.

Thanks
Max


On Jun 21, 2018, at 11:23 PM, Weijun Wang  wrote:




On Jun 21, 2018, at 11:07 PM, Xuelei Fan  wrote:

Hi Weijun,

The release note and the following notes look reasonable to me.

For the implementation part, could it be a little bit more straightforward if 
wrapping the new attributes (pss/pssParams/fallbackSignature) and codes (if 
pss/fallbackSignature, etc) in the PSS subclass?

Sounds good. I'll try it.


Did you want to remove the debug code in the security.cpp?  It seems that they 
are not used any more.

Sure I can.

Thanks
Max


Xuelei

On 6/21/2018 4:12 AM, Weijun Wang wrote:

Please take a review on this change
  http://cr.openjdk.java.net/~weijun/8205445/webrev.00/
   and the release note at
  https://bugs.openjdk.java.net/browse/JDK-8205471
The code change adds RSASSA-PSS signature support to the SunMSCAPI provider.
Several notes:
1. CryptoAPI (which SunMSCAPI is based on and now a deprecated technology) does 
not support RSASSA-PSS. In fact, CNG [1] is used to perform the signing and 
verification. This is certainly not a perfect solution and we are thinking of 
support CNG in a more sophisticated way in future releases of JDK.
2. For unknown reason, the newly added verification code for RSASSA-PSS does 
not work correctly (precisely, ::NCryptTranslateHandle returns 
NTE_INVALID_PARAMETER). A fallback mechanism is added into 
mscapi/RSASignature.java. A SunRsaSign Signature object is actually used when a 
SunMSCAPI Signature is initialized to verify an RSASSA-PSS signature.
3. It looks like CNG only supports PSSParamterSpec with the same message hash 
algorithm and MGF1 hash algorithm, because there is only one algorithm field in 
BCRYPT_PSS_PADDING_INFO [2]. This is checked when setting the parameter.
4. It looks like CNG only supports RSASSA-PSS using these hash algorithms: SHA-1, 
SHA-256, SHA-384, and SHA-512. This is not checked at parameter setting but sign() will 
throw a SignatureException saying "Unrecognised hash algorithm". Since the 
verify() side uses a fallback SunRsaSign signature, other hash algorithms are supported.
Thanks
Max
[1] 
https://msdn.microsoft.com/en-us/library/windows/desktop/aa376210(v=vs.85).aspx
[2] 
https://msdn.microsoft.com/en-us/library/windows/desktop/aa375529(v=vs.85).aspx
[3] 
https://msdn.microsoft.com/en-us/library/windows/desktop/aa375534(v=vs.85).aspx

RFR: JDK-8189429: SA: MacOSX: Replace the deprecated PT_ATTACH with PT_ATTACHEXC

[Jini George]

Hi all,

[Including build-dev also since this includes build related changes].

Requesting reviews for:

https://bugs.openjdk.java.net/browse/JDK-8189429 (SA: MacOSX: Replace 
the deprecated PT_ATTACH with PT_ATTACHEXC)


Webrev: http://cr.openjdk.java.net/~jgeorge/8189429/webrev.04/

This is the follow-up solution for the temporary restoration of 
PT_ATTACH to fix https://bugs.openjdk.java.net/browse/JDK-8184042 
(several serviceability/sa tests timed out on MacOS X), which was done 
in Oct 2017. The mails related to this are at:


http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-August/021702.html

Changes as compared to the patch sent last year 
(http://cr.openjdk.java.net/~jgeorge/8184042/webrev.00/):


* Addressed the review comments which were provided by Poonam, Dan, Dmitry.
* A major change as compared to what was done last year is that the MIG 
generated files have been included as a part of the JDK build process.
* The test case which was provided in the patch last year is no longer 
required since we have ClhsdbAttach.java testing the same functionality 
as a part of the SA testsuite now.
* Other than that, some minor improvements have been done wrt error 
handling.


The steps for the proposed solution have been provided in JBS.

Testing: ALL the SA tests pass on MacOSX and the other Mach5 platforms.

Thanks to Sharath, Robin, Gerard and Dan for looking into the changes 
and providing valuable comments.


Thanks,
Jini.

Re: RFR: 8186780: clang-4.0 fastdebug assertion failure in os_linux_x86:os::verify_stack_alignment()

[Martin Buchholz]

I see stack pointers (and frame pointers) declared as intptr_t* which makes
no sense to me.  These are all "just" pointers, so declare them as either
void* or intptr_t or address

In the construct below, we could elide the cast if we had declared esp
right in the first place?!

  intptr_t* esp;
  __asm__ __volatile__ ("mov %%"SPELL_REG_SP", %0":"=r"(esp):);
  return (address) esp;

Re: RFR: 8186780: clang-4.0 fastdebug assertion failure in os_linux_x86:os::verify_stack_alignment()

[Martin Buchholz]

There sure seems to be a lot of confusion about how many stack frames we're
getting at the hot end of the stack, e.g.

  register intptr_t **fp __asm__ (SPELL_REG_FP);

  // fp is for this frame (_get_previous_fp). We want the fp for the
  // caller of os::current_frame*(), so go up two frames. However, for
  // optimized builds, _get_previous_fp() will be inlined, so only go
  // up 1 frame in that case.
  #ifdef _NMT_NOINLINE_
return **(intptr_t***)fp;
  #else
return *fp;
  #endif

Re: RFR: 8186780: clang-4.0 fastdebug assertion failure in os_linux_x86:os::verify_stack_alignment()

[Martin Buchholz]

(I keep trying not to become a hotspot engineer...)

I would define current_stack_pointer as a macro using expression
statements: prototype is:

#if defined(__clang__)
#define sp_3() ({ intptr_t rsp; __asm__ __volatile__ ("mov %% rsp,
%0":"=r"(rsp):); rsp; })
#else
#define sp_3() ({ register intptr_t rsp asm ("rsp"); rsp; })
#endif

Then we can call that everywhere and actually get the right answer.
(Other compilers and cpu architectures left as an exercise for the reader)
(Probably we won't be able to do this for every compiler we'd like to use)

Then we can make all the
os::verify_stack_alignment functions in non-product mode NOINLINE so that
they have a real stack frame with a real stack pointer.

But that's starting to be a big change and a hotspot culture change, so can
hotspot engineers please take over ?!

On Fri, Jun 22, 2018 at 8:27 AM, Thomas Stüfe 
wrote:

> On Fri, Jun 22, 2018 at 1:57 PM, David Holmes 
> wrote:
> > On 21/06/2018 10:37 PM, Thomas Stüfe wrote:
> >>
> >> On Thu, Jun 21, 2018 at 1:27 PM, David Holmes 
> >> wrote:
> >>>
> >>> On 21/06/2018 10:05 AM, Martin Buchholz wrote:
> 
> 
>  On Wed, Jun 20, 2018 at 4:03 PM, Martin Buchholz   > wrote:
> 
>   Hi David and build-dev folk,
> 
>   After way too much build/hotspot hacking, I have a better fix:
> 
>   clang inlined os::current_stack_pointer into its caller __in the
>   same translation unit___ (that could be fixed in a separate
> change)
>   so of course in this case it didn't have to follow the ABI.  Fix
> is
>   obvious in hindsight:
> 
>   -address os::current_stack_pointer() {
>   +NOINLINE address os::current_stack_pointer() {
> 
> 
>  If y'all like the addition of NOINLINE, it should probably be added to
>  all
>  of the 14 variants of os::current_stack_pointer.
>  Gives me a chance to try out the submit repo.
> >>>
> >>>
> >>>
> >>> I can't help but think other platforms actually rely on it being
> inlined
> >>> so
> >>> that it really does return the stack pointer of the method calling
> >>> os::current_stack_pointer!
> >>>
> >>
> >> But we only inline today if caller is in the same translation unit and
> >> builds with optimization, no?
> >
> >
> > Don't know, but Martin's encountering a case where it is being inlined -
> is
> > that likely to be unique for some reason?
> >
>
> Okay I may have confused myself.
>
> My original reasoning was: A lot of calls to
> os::current_stack_pointer() today happen not-inlined. That includes
> calls from outside os__.cpp, and calls from inside
> os__.cpp if slowdebug. Hence, since the VM - in particular
> the slowdebug one - seems to work fine, it should be okay to mark
> os::current_stack_pointer() unconditionally as NOINLINE.
>
> However, then I saw that the only "real" function (real meaning not
> just asserting something) using os::current_stack_pointer() and living
> in the same translation unit is os::current_frame(), e.g. on linux:
>
> frame os::current_frame() {
>   intptr_t* fp = _get_previous_fp();
>   frame myframe((intptr_t*)os::current_stack_pointer(),
> (intptr_t*)fp,
> CAST_FROM_FN_PTR(address, os::current_frame));
>   if (os::is_first_C_frame()) {
> // stack is not walkable
> return frame();
>   } else {
> return os::get_sender_for_C_frame();
>   }
> }
>
> how does this even work if os::current_stack_pointer() is not inlined?
> In slowdebug? Would the frame object in this case not contain the SP
> from the frame built up for os::current_stack_pointer()?
>
> So, now I wonder if making os::current_stack_pointer() NOINLINE would
> break os::current_frame() - make if produce frames with a slightly-off
> SP. os::current_frame() is used e.g. in NMT. So, I wonder if NMT still
> works if os::current_stack_pointer is made NOINLINE, or in slowdebug.
>
> > I never assume the compiler does the obvious these days :) or that there
> > can't be clever link-time tricks as well.
>
> Oh. I did not think of that at all, you are right.
>
> >
> > Maybe the safest thing to do is to only make a change for the clang case
> and
> > leave everything else alone.
> >
>
> It would be better to know for sure, though.
>
> ..Thomas
>
> > David
> > -
> >
> >>
> >> ..Thomas
> >>
> >>> David
>

Re: RFR: 8186780: clang-4.0 fastdebug assertion failure in os_linux_x86:os::verify_stack_alignment()

[Thomas Stüfe]

On Fri, Jun 22, 2018 at 1:57 PM, David Holmes  wrote:
> On 21/06/2018 10:37 PM, Thomas Stüfe wrote:
>>
>> On Thu, Jun 21, 2018 at 1:27 PM, David Holmes 
>> wrote:
>>>
>>> On 21/06/2018 10:05 AM, Martin Buchholz wrote:


 On Wed, Jun 20, 2018 at 4:03 PM, Martin Buchholz >>> > wrote:

  Hi David and build-dev folk,

  After way too much build/hotspot hacking, I have a better fix:

  clang inlined os::current_stack_pointer into its caller __in the
  same translation unit___ (that could be fixed in a separate change)
  so of course in this case it didn't have to follow the ABI.  Fix is
  obvious in hindsight:

  -address os::current_stack_pointer() {
  +NOINLINE address os::current_stack_pointer() {


 If y'all like the addition of NOINLINE, it should probably be added to
 all
 of the 14 variants of os::current_stack_pointer.
 Gives me a chance to try out the submit repo.
>>>
>>>
>>>
>>> I can't help but think other platforms actually rely on it being inlined
>>> so
>>> that it really does return the stack pointer of the method calling
>>> os::current_stack_pointer!
>>>
>>
>> But we only inline today if caller is in the same translation unit and
>> builds with optimization, no?
>
>
> Don't know, but Martin's encountering a case where it is being inlined - is
> that likely to be unique for some reason?
>

Okay I may have confused myself.

My original reasoning was: A lot of calls to
os::current_stack_pointer() today happen not-inlined. That includes
calls from outside os__.cpp, and calls from inside
os__.cpp if slowdebug. Hence, since the VM - in particular
the slowdebug one - seems to work fine, it should be okay to mark
os::current_stack_pointer() unconditionally as NOINLINE.

However, then I saw that the only "real" function (real meaning not
just asserting something) using os::current_stack_pointer() and living
in the same translation unit is os::current_frame(), e.g. on linux:

frame os::current_frame() {
  intptr_t* fp = _get_previous_fp();
  frame myframe((intptr_t*)os::current_stack_pointer(),
(intptr_t*)fp,
CAST_FROM_FN_PTR(address, os::current_frame));
  if (os::is_first_C_frame()) {
// stack is not walkable
return frame();
  } else {
return os::get_sender_for_C_frame();
  }
}

how does this even work if os::current_stack_pointer() is not inlined?
In slowdebug? Would the frame object in this case not contain the SP
from the frame built up for os::current_stack_pointer()?

So, now I wonder if making os::current_stack_pointer() NOINLINE would
break os::current_frame() - make if produce frames with a slightly-off
SP. os::current_frame() is used e.g. in NMT. So, I wonder if NMT still
works if os::current_stack_pointer is made NOINLINE, or in slowdebug.

> I never assume the compiler does the obvious these days :) or that there
> can't be clever link-time tricks as well.

Oh. I did not think of that at all, you are right.

>
> Maybe the safest thing to do is to only make a change for the clang case and
> leave everything else alone.
>

It would be better to know for sure, though.

..Thomas

> David
> -
>
>>
>> ..Thomas
>>
>>> David

Re: RFR 8205445: Add RSASSA-PSS Signature support to SunMSCAPI

[Xuelei Fan]

Looks fine to me.

Thanks,
Xuelei

On 6/21/2018 10:39 PM, Weijun Wang wrote:

Webrev updated at

   http://cr.openjdk.java.net/~weijun/8205445/webrev.01

I think I found a bug in SunRsaSign of the RSASSA-PSS signature. Fixed and 
added a test.

BTW, I commented out the debug code in security.cpp. Once there is a bug I can 
use it.

Thanks
Max


On Jun 21, 2018, at 11:23 PM, Weijun Wang  wrote:




On Jun 21, 2018, at 11:07 PM, Xuelei Fan  wrote:

Hi Weijun,

The release note and the following notes look reasonable to me.

For the implementation part, could it be a little bit more straightforward if 
wrapping the new attributes (pss/pssParams/fallbackSignature) and codes (if 
pss/fallbackSignature, etc) in the PSS subclass?


Sounds good. I'll try it.



Did you want to remove the debug code in the security.cpp?  It seems that they 
are not used any more.


Sure I can.

Thanks
Max



Xuelei

On 6/21/2018 4:12 AM, Weijun Wang wrote:

Please take a review on this change
  http://cr.openjdk.java.net/~weijun/8205445/webrev.00/
   and the release note at
  https://bugs.openjdk.java.net/browse/JDK-8205471
The code change adds RSASSA-PSS signature support to the SunMSCAPI provider.
Several notes:
1. CryptoAPI (which SunMSCAPI is based on and now a deprecated technology) does 
not support RSASSA-PSS. In fact, CNG [1] is used to perform the signing and 
verification. This is certainly not a perfect solution and we are thinking of 
support CNG in a more sophisticated way in future releases of JDK.
2. For unknown reason, the newly added verification code for RSASSA-PSS does 
not work correctly (precisely, ::NCryptTranslateHandle returns 
NTE_INVALID_PARAMETER). A fallback mechanism is added into 
mscapi/RSASignature.java. A SunRsaSign Signature object is actually used when a 
SunMSCAPI Signature is initialized to verify an RSASSA-PSS signature.
3. It looks like CNG only supports PSSParamterSpec with the same message hash 
algorithm and MGF1 hash algorithm, because there is only one algorithm field in 
BCRYPT_PSS_PADDING_INFO [2]. This is checked when setting the parameter.
4. It looks like CNG only supports RSASSA-PSS using these hash algorithms: SHA-1, 
SHA-256, SHA-384, and SHA-512. This is not checked at parameter setting but sign() will 
throw a SignatureException saying "Unrecognised hash algorithm". Since the 
verify() side uses a fallback SunRsaSign signature, other hash algorithms are supported.
Thanks
Max
[1] 
https://msdn.microsoft.com/en-us/library/windows/desktop/aa376210(v=vs.85).aspx
[2] 
https://msdn.microsoft.com/en-us/library/windows/desktop/aa375529(v=vs.85).aspx
[3] 
https://msdn.microsoft.com/en-us/library/windows/desktop/aa375534(v=vs.85).aspx

Re: RFR: 8186780: clang-4.0 fastdebug assertion failure in os_linux_x86:os::verify_stack_alignment()

[David Holmes]

On 21/06/2018 10:37 PM, Thomas Stüfe wrote:

On Thu, Jun 21, 2018 at 1:27 PM, David Holmes  wrote:

On 21/06/2018 10:05 AM, Martin Buchholz wrote:


On Wed, Jun 20, 2018 at 4:03 PM, Martin Buchholz mailto:marti...@google.com>> wrote:

 Hi David and build-dev folk,

 After way too much build/hotspot hacking, I have a better fix:

 clang inlined os::current_stack_pointer into its caller __in the
 same translation unit___ (that could be fixed in a separate change)
 so of course in this case it didn't have to follow the ABI.  Fix is
 obvious in hindsight:

 -address os::current_stack_pointer() {
 +NOINLINE address os::current_stack_pointer() {


If y'all like the addition of NOINLINE, it should probably be added to all
of the 14 variants of os::current_stack_pointer.
Gives me a chance to try out the submit repo.



I can't help but think other platforms actually rely on it being inlined so
that it really does return the stack pointer of the method calling
os::current_stack_pointer!



But we only inline today if caller is in the same translation unit and
builds with optimization, no?


Don't know, but Martin's encountering a case where it is being inlined - 
is that likely to be unique for some reason?


I never assume the compiler does the obvious these days :) or that there 
can't be clever link-time tricks as well.


Maybe the safest thing to do is to only make a change for the clang case 
and leave everything else alone.


David
-



..Thomas


David

Re: [Fwd: Re: Build breakage with system jpeg and lcms and jdk-11+18]

[John Paul Adrian Glaubitz]

Hi Severin!

On 06/22/2018 01:19 PM, Severin Gehwolf wrote:
>> Now, before I create the bug report, do you happen to know which changeset
>> introduced the regression? We usually include that in the bug report.
> 
> http://hg.openjdk.java.net/jdk/jdk/rev/f0aeede1b855
> 
>> And I also need to know whom to credit for the patch because of the several
>> backs and forths, I lost the overview.
> 
> I'd propose:
> 
> Contributed-by: Magnuse Ihse Bursie , Fridrich 
> Strba 

Perfect, thank you!

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [Fwd: Re: Build breakage with system jpeg and lcms and jdk-11+18]

[Severin Gehwolf]

On Fri, 2018-06-22 at 12:53 +0200, John Paul Adrian Glaubitz wrote:
> Hi Fridrich!
> 
> On 06/21/2018 02:34 PM, Fridrich Strba wrote:
> > Yes, I have tested this consolidated version and it builds just fine for me.
> 
> I have just verified that the problem exists on Debian unstable as well when
> using the system headers for libjpeg and the other libraries. I can also
> confirm that the patch you provided fixes the problem for me.
> 
> Now, before I create the bug report, do you happen to know which changeset
> introduced the regression? We usually include that in the bug report.

http://hg.openjdk.java.net/jdk/jdk/rev/f0aeede1b855

> And I also need to know whom to credit for the patch because of the several
> backs and forths, I lost the overview.

I'd propose:

Contributed-by: Magnuse Ihse Bursie , Fridrich 
Strba 

Thanks,
Severin

Re: [Fwd: Re: Build breakage with system jpeg and lcms and jdk-11+18]

[John Paul Adrian Glaubitz]

Hi Fridrich!

On 06/21/2018 02:34 PM, Fridrich Strba wrote:
> Yes, I have tested this consolidated version and it builds just fine for me.
I have just verified that the problem exists on Debian unstable as well when
using the system headers for libjpeg and the other libraries. I can also
confirm that the patch you provided fixes the problem for me.

Now, before I create the bug report, do you happen to know which changeset
introduced the regression? We usually include that in the bug report.

And I also need to know whom to credit for the patch because of the several
backs and forths, I lost the overview.

Thanks,
Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913