Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file

[Weijun Wang]

> On Jun 14, 2019, at 5:00 AM, Sean Mullan  wrote:
> 
> On 6/12/19 9:52 PM, Weijun Wang wrote:
>> Hi Sean,
>> The previous fix for JDK-8193255 already made the creation date useless. 
>> Before that, each time cacerts was regenerated the date changed. I compared 
>> cacerts of different releases and the same cert have difference creation 
>> dates.
>> The only other solution I can think of is to look at 
>> make/autoconf/version-numbers and use the DEFAULT_VERSION_DATE=2019-09-17 
>> there.
> 
> Yes, a possibility, use the GA release date, which maybe "makes more sense" 
> as a creation date, although a bit odd to have creation dates in the future 
> before GA.
> 
> I'll leave it up to you. I think nobody really looks at the creationDate.

I'll use notBefore, it does not change forever.

> 
>> Have you reviewed the code changes? You can see I stored the hash of the 
>> whole file into the test. This means anyone updating the CA certs will have 
>> to create cacerts and calculate the correct hash and update this test. I 
>> suppose this won't be a hassle.
> 
> Not really, since you had to update VerifyCACerts anyway whenever a change to 
> cacerts was made, but what's the benefit of the hash? Are you worried the 
> cacerts binary will be corrupted somehow, or you just want extra assurance 
> something didn't go wrong?

I just want extra assurance that the output is indeed consistent.

> 
> It might be useful to run older versions of keytool against the cacerts 
> binary you created - this would give you more assurance that your hand-coded 
> format is correct. I would also try various commands, etc.

I can hack only 3 lines of JavaKeyStore.java itself and generate the identical 
cacerts. Therefore it should be a genuine JKS file.

BTW, something not related but similar: Do you like me to also sort aliases 
alphabetically in the output of "keytool -list"?

Thanks,
Max

> 
> --Sean
> 
>> Thanks,
>> Max
>>> On Jun 13, 2019, at 4:15 AM, Sean Mullan  wrote:
>>> 
>>> On 6/12/19 4:01 PM, Erik Joelsson wrote:
 Hello,
 We cannot rely on querying mercurial at build time. The source must be 
 buildable from a source distribution.
>>> 
>>> I had a feeling it wouldn't work but thought I would ask anyway.
>>> 
>>> Well, offhand I can't think of any better solution than notBefore then, 
>>> unless we included it as a comment in the PEM file, and then extracted it. 
>>> With notBefore, someone might be curious about why some keystore entries 
>>> were created so long ago (ex: the earliest notBefore date is 1996). But the 
>>> creationDate doesn't really have any usefulness for cacerts, so it's 
>>> probably ok.
>>> 
>>> --Sean
>>> 
 /Erik
 On 2019-06-12 11:39, Sean Mullan wrote:
> Using the certificate's notBefore date as the KeyStore entry creation 
> date is misleading since many of these root certs were not integrated 
> into the JDK until after they were created by the CA. Can we somehow 
> extract the last revision time of each PEM file instead? That is more 
> aligned with the previous creation date that we used.
> 
> --Sean
> 
> On 6/12/19 12:38 PM, Erik Joelsson wrote:
>> Hello Max,
>> 
>> Much appreciated! I will need to have this fixed one way or other in JDK 
>> 13, so depending on if you get your fix there in time, I will retract my 
>> proposal. If your fix only hits 14, I will push mine to 13.
>> 
>> /Erik
>> 
>> On 2019-06-12 08:41, Weijun Wang wrote:
>>> This is my version of the fix:
>>> 
>>> http://cr.openjdk.java.net/~weijun/8225392/webrev.00/
>>> 
>>> Now you can still compare cacerts bit by bit.
>>> 
>>> Thanks,
>>> Max
>>> 
 On Jun 12, 2019, at 10:50 PM, Weijun Wang  
 wrote:
 
 Hi Erik,
 
 Are you going to fix this bug soon?
 
 I am inspired by Martin's words and would like to update 
 GenerateCacerts.java so that as long as the certs and their aliases 
 are unchanged, the output cacerts will always be the same. I can send 
 out a code review today.
 
 Thanks,
 Max
 
> On Jun 12, 2019, at 10:59 AM, Weijun Wang  
> wrote:
> 
> Good idea about the creation time.
> 
> --Max
> 
>> On Jun 12, 2019, at 10:53 AM, Martin Buchholz  
>> wrote:
>> 
>> Google culture really likes build output determinism, and we 
>> recently built our own cacerts generator.
>> 
>> To get determinism, we are using cert digest as alias (must have a 
>> unique alias, but value doesn't seem to matter much), and using cert 
>> notBefore instead of current (build) timestamp.
>> 
>> On Mon, Jun 10, 2019 at 12:40 PM Erik Joelsson 
>>  wrote:
>> Since JDK-8193255, when we started generating the cacerts file in the
>> 

Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file

[Sean Mullan]

On 6/12/19 9:52 PM, Weijun Wang wrote:

Hi Sean,

The previous fix for JDK-8193255 already made the creation date useless. Before 
that, each time cacerts was regenerated the date changed. I compared cacerts of 
different releases and the same cert have difference creation dates.

The only other solution I can think of is to look at 
make/autoconf/version-numbers and use the DEFAULT_VERSION_DATE=2019-09-17 there.


Yes, a possibility, use the GA release date, which maybe "makes more 
sense" as a creation date, although a bit odd to have creation dates in 
the future before GA.


I'll leave it up to you. I think nobody really looks at the creationDate.


Have you reviewed the code changes? You can see I stored the hash of the whole 
file into the test. This means anyone updating the CA certs will have to create 
cacerts and calculate the correct hash and update this test. I suppose this 
won't be a hassle.


Not really, since you had to update VerifyCACerts anyway whenever a 
change to cacerts was made, but what's the benefit of the hash? Are you 
worried the cacerts binary will be corrupted somehow, or you just want 
extra assurance something didn't go wrong?


It might be useful to run older versions of keytool against the cacerts 
binary you created - this would give you more assurance that your 
hand-coded format is correct. I would also try various commands, etc.


--Sean



Thanks,
Max


On Jun 13, 2019, at 4:15 AM, Sean Mullan  wrote:

On 6/12/19 4:01 PM, Erik Joelsson wrote:

Hello,
We cannot rely on querying mercurial at build time. The source must be 
buildable from a source distribution.


I had a feeling it wouldn't work but thought I would ask anyway.

Well, offhand I can't think of any better solution than notBefore then, unless 
we included it as a comment in the PEM file, and then extracted it. With 
notBefore, someone might be curious about why some keystore entries were 
created so long ago (ex: the earliest notBefore date is 1996). But the 
creationDate doesn't really have any usefulness for cacerts, so it's probably 
ok.

--Sean


/Erik
On 2019-06-12 11:39, Sean Mullan wrote:

Using the certificate's notBefore date as the KeyStore entry creation date is 
misleading since many of these root certs were not integrated into the JDK 
until after they were created by the CA. Can we somehow extract the last 
revision time of each PEM file instead? That is more aligned with the previous 
creation date that we used.

--Sean

On 6/12/19 12:38 PM, Erik Joelsson wrote:

Hello Max,

Much appreciated! I will need to have this fixed one way or other in JDK 13, so 
depending on if you get your fix there in time, I will retract my proposal. If 
your fix only hits 14, I will push mine to 13.

/Erik

On 2019-06-12 08:41, Weijun Wang wrote:

This is my version of the fix:

 http://cr.openjdk.java.net/~weijun/8225392/webrev.00/

Now you can still compare cacerts bit by bit.

Thanks,
Max


On Jun 12, 2019, at 10:50 PM, Weijun Wang  wrote:

Hi Erik,

Are you going to fix this bug soon?

I am inspired by Martin's words and would like to update GenerateCacerts.java 
so that as long as the certs and their aliases are unchanged, the output 
cacerts will always be the same. I can send out a code review today.

Thanks,
Max


On Jun 12, 2019, at 10:59 AM, Weijun Wang  wrote:

Good idea about the creation time.

--Max


On Jun 12, 2019, at 10:53 AM, Martin Buchholz  wrote:

Google culture really likes build output determinism, and we recently built our 
own cacerts generator.

To get determinism, we are using cert digest as alias (must have a unique 
alias, but value doesn't seem to matter much), and using cert notBefore instead 
of current (build) timestamp.

On Mon, Jun 10, 2019 at 12:40 PM Erik Joelsson  wrote:
Since JDK-8193255, when we started generating the cacerts file in the
build, the build compare baseline builds have started failing. It seems
the cacerts binary file has some non determinism built in so it doesn't
get generated exactly the same given the same input. This patch adds
special handling when comparing that file by comparing the output of
"keytool -list" on the files instead.

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

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

/Erik

Re: RFR: [8u] JDK-8223219: Backport of JDK-8199552 to OpenJDK 8 leads to duplicate -fstack-protector flags, overriding --with-extra-cflags

[Severin Gehwolf]

Hi Andrew,

On Wed, 2019-06-12 at 20:34 +0100, Andrew John Hughes wrote:
> Webrev: https://cr.openjdk.java.net/~andrew/openjdk8/8223219/webrev.01/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8223219
> 
> There's quite a long story to this one, more detail of which is in the
> bug report. In short, JDK-8199552 was backported as part of a CPU with
> no review and so little explanation for the changes contained therein,
> which differ from the same change in OpenJDK 11.
> 
> The end result is that -fstack-protector is added for only two
> architectures - x86 and x86_64 - and it ends up appearing twice for the
> JDK part of the build, the second appearance overriding any options
> specified using --with-extra-cflags. This is a problem for distros which
> may want to use -fstack-protector-strong instead. This patch simplifies
> it down to one addition for all architectures.
> 
> We've been using this patch (or a variant of it) since the January CPU
> on all architectures we build on (AArch64, s390, s390x, ppc, ppc64,
> ppc64le, x86 & x86_64) without known issues.
> 
> Before patch:
> 
>  [7] CFLAGS := -Wall -Wno-parentheses -Wextra -Wno-unused
> -Wno-unused-parameter -Wformat=2 -pipe -D_GNU_SOURCE -D_REENTRANT
> -D_LARGEFILE64_SOURCE -fno-omit-frame-pointer -fstack-protector
> -D_LP64=1 -D_LITTLE_ENDIAN -DLINUX -DARCH='"amd64"' -Damd64 -DNDEBUG
> -DRELEASE='"1.8.0-internal"' -I/home/andrew/builder/8u-dev/jdk/include
> -I/home/andrew/builder/8u-dev/jdk/include/linux
> -I/home/andrew/projects/openjdk/upstream/jdk8u-dev/jdk/src/share/javavm/export
> -I/home/andrew/projects/openjdk/upstream/jdk8u-dev/jdk/src/solaris/javavm/export
> -I/home/andrew/projects/openjdk/upstream/jdk8u-dev/jdk/src/share/native/common
> -I/home/andrew/projects/openjdk/upstream/jdk8u-dev/jdk/src/solaris/native/common
> -O2 -pipe -march=core2 -mno-tls-direct-seg-refs -Wno-error=return-type
> -Wno-error=deprecated-declarations -fno-strict-aliasing
> -fstack-protector -fno-delete-null-pointer-checks -fno-lifetime-dse
> -fPIC -I/home/andrew/builder/8u-dev/jdk/gensrc_headers
> -I/home/andrew/projects/openjdk/upstream/jdk8u-dev/jdk/src/share/nativ\
> e/java/lang/fdlibm/include
> 
> After patch (the -fstack-protector duplicate after -fno-strict-aliasing
> is gone):
> 
>  [7] CFLAGS := -Wall -Wno-parentheses -Wextra -Wno-unused
> -Wno-unused-parameter -Wformat=2 -pipe -fstack-protector -D_GNU_SOURCE
> -D_REENTRANT -D_LARGEFILE64_SOURCE -fno-omit-frame-pointer -D_LP64=1
> -D_LITTLE_ENDIAN -DLINUX -DARCH='"amd64"' -Damd64 -DNDEBUG
> -DRELEASE='"1.8.0-internal"' -I/home/andrew/builder/8u-dev/jdk/include
> -I/home/andrew/builder/8u-dev/jdk/include/linux
> -I/home/andrew/projects/openjdk/upstream/jdk8u-dev/jdk/src/share/javavm/export
> -I/home/andrew/projects/openjdk/upstream/jdk8u-dev/jdk/src/solaris/javavm/export
> -I/home/andrew/projects/openjdk/upstream/jdk8u-dev/jdk/src/share/native/common
> -I/home/andrew/proje\
> cts/openjdk/upstream/jdk8u-dev/jdk/src/solaris/native/common -O2 -pipe
> -march=core2 -ggdb -mno-tls-direct-seg-refs -Wno-error=return-type
> -Wno-error=deprecated-declarations -fno-strict-aliasing
> -fno-delete-null-pointer-checks -fno-lifetime-dse -fPIC
> -I/home/andrew/builder/8u-dev/jdk/gensrc_headers
> -I/home/andrew/projects/openjdk/upstream/jdk8u-dev/jdk/src/share/native/java/lang/fdlibm/include
> 
> Ok to push?

This looks reasonable to me.

Thanks,
Severin

Re: RFR: 8224851: AArch64: fix warnings and errors with Clang and GCC 8.3

[Andrew Haley]

On 6/12/19 10:35 AM, Nick Gasson wrote:
> I also replaced the call to _get_previous_fp() in os::current_frame() with
> 
>*(intptr_t **)__builtin_frame_address(0);
> 
> As it generates the same code and avoids the `register intptr_t **fp 
> __asm__ (SPELL_REG_FP);' declaration which clang doesn't support. Also 
> the following comment in _get_previous_fp seems to be wrong:
> 
>// 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

All of this seems horribly fragile. I'll have a look at what this is supposed
to be used for and think about a stable and robust way to do it.

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. 
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: RFR: 8224851: AArch64: fix warnings and errors with Clang and GCC 8.3

[Nick Gasson]

Hi Kim,

Thanks for the feedback. I'll make these changes.



Is there a reason to conditionalize use of __atomic_add_fetch for
clang and continue to use __sync_add_and_fetch for gcc, rather than
using __atomic_add_fetch unconditionally?


I think I did it that way because I didn't want to affect the build with 
gcc, but actually gcc compiles this to the same instructions with either 
variant so we can get rid of the #ifdef.




Is __ATOMIC_RELEASE and a following FULL_MEM_BARRIER the right usage
to get the equivalent of __sync_add_and_fetch?  Or should
__ATOMIC_SEQ_CST be used?  Or should the latter be actively avoided?
I remember some discussion and questions in that area, about how to
implement memory_order_seq_cst on ARM processors, but wasn't paying
close attention since I don't deal with ARM much.



It generates the same instruction sequence as the existing 
__sync_fetch_and_add.


The __sync_XXX builtins have a full barrier at the end [1], so GCC 
compiles this to:


.L2:
ldxrw2, [x1]
add w2, w2, w0
stlxr   w3, w2, [x1]; Store release
cbnzw3, .L2
dmb ish ; Full barrier
mov w0, w2
ret

Whereas using the __atomic builtin with __ATOMIC_SEQ_CST we get two 
half-barriers like this:


.L2:
ldaxr   w2, [x1]; Load acquire
add w2, w2, w0
stlxr   w3, w2, [x1]; Store release
cbnzw3, .L2
mov w0, w2
ret


Thanks,
Nick


[1] https://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html