RFR: 8264413: Data is written to file header even if its CRC32 was calculated

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8264413: Data is written to file header even if its CRC32 was calculated

Yi Yang
Many tests under `runtime/cds/appcds/dynamicArchive` are crashed when turning on VerifySharedSpaces, VM reports inconsistent crc32 between dumptime and runtime(See detailed content on JBS attachments):

$ diff dump.log run.log
17c17
< - base_archive_is_default:        0
---
> - base_archive_is_default:        1
19c19
< - base_archive_name_size:         0
---
> - base_archive_name_size:         113

The root cause is that even if the CRC32 is calculated(Line 1902), ArchiveBuilder is still writing data to FileMapInfo(Line 1903):

https://github.com/openjdk/jdk/blob/4ea6abfbd1818fa7049bc4f6e46d568e842acb34/src/hotspot/share/memory/archiveBuilder.cpp#L1091-L1094

https://github.com/openjdk/jdk/blob/4ea6abfbd1818fa7049bc4f6e46d568e842acb34/src/hotspot/share/memory/filemap.cpp#L1236-L1251

which results in the expected CRC32(serialized in CDS archive) is different from the calculated ones at runtime. This patch addresses this problem, it writes base_archive_name and size before calculating CRC32 if needed.

Thanks~
Yang

-------------

Commit messages:
 - Data is written to file header even if its CRC32 was calculated

Changes: https://git.openjdk.java.net/jdk/pull/3261/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3261&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264413
  Stats: 26 lines in 3 files changed: 16 ins; 8 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3261.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3261/head:pull/3261

PR: https://git.openjdk.java.net/jdk/pull/3261
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264413: Data is written to file header even if its CRC32 was calculated

Yi Yang
On Tue, 30 Mar 2021 09:25:20 GMT, Yi Yang <[hidden email]> wrote:

> Many tests under `runtime/cds/appcds/dynamicArchive` are crashed when turning on VerifySharedSpaces, VM reports inconsistent crc32 between dumptime and runtime(See detailed content on JBS attachments):
>
> $ diff dump.log run.log
> 17c17
> < - base_archive_is_default:        0
> ---
>> - base_archive_is_default:        1
> 19c19
> < - base_archive_name_size:         0
> ---
>> - base_archive_name_size:         113
>
> The root cause is that even if the CRC32 is calculated(Line 1902), ArchiveBuilder is still writing data to FileMapInfo(Line 1903):
>
> https://github.com/openjdk/jdk/blob/4ea6abfbd1818fa7049bc4f6e46d568e842acb34/src/hotspot/share/memory/archiveBuilder.cpp#L1091-L1094
>
> https://github.com/openjdk/jdk/blob/4ea6abfbd1818fa7049bc4f6e46d568e842acb34/src/hotspot/share/memory/filemap.cpp#L1236-L1251
>
> which results in the expected CRC32(serialized in CDS archive) is different from the calculated ones at runtime. This patch addresses this problem, it writes base_archive_name and size before calculating CRC32 if needed.
>
> Thanks~
> Yang

We can freeze(just like Metaspace::freeze) the FileMapInfo after CRC32 was calculated, any subsequent writing actions would be reported as illegal. Do you think this is worth doing? Looking forward to your comments.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3261
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264413: Data is written to file header even if its CRC32 was calculated [v2]

Yi Yang
In reply to this post by Yi Yang
> Many tests under `runtime/cds/appcds/dynamicArchive` are crashed when turning on VerifySharedSpaces, VM reports inconsistent crc32 between dumptime and runtime(See detailed content on JBS attachments):
>
> $ diff dump.log run.log
> 17c17
> < - base_archive_is_default:        0
> ---
>> - base_archive_is_default:        1
> 19c19
> < - base_archive_name_size:         0
> ---
>> - base_archive_name_size:         113
>
> The root cause is that even if the CRC32 is calculated(Line 1902), ArchiveBuilder is still writing data to FileMapInfo(Line 1903):
>
> https://github.com/openjdk/jdk/blob/4ea6abfbd1818fa7049bc4f6e46d568e842acb34/src/hotspot/share/memory/archiveBuilder.cpp#L1091-L1094
>
> https://github.com/openjdk/jdk/blob/4ea6abfbd1818fa7049bc4f6e46d568e842acb34/src/hotspot/share/memory/filemap.cpp#L1236-L1251
>
> Now the expected CRC32(serialized in CDS archive) is different from the calculated ones at runtime. This patch addresses this problem, it writes base_archive_name and size into header **before** calculating CRC32(if needed).
>
> Thanks~
> Yang

Yi Yang 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:

  Data is written to file header even if its CRC32 was calculated

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3261/files
  - new: https://git.openjdk.java.net/jdk/pull/3261/files/c5a051aa..40b70a2a

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3261&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3261&range=00-01

  Stats: 4 lines in 2 files changed: 1 ins; 2 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3261.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3261/head:pull/3261

PR: https://git.openjdk.java.net/jdk/pull/3261
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264413: Data is written to file header even if its CRC32 was calculated [v3]

Yi Yang
In reply to this post by Yi Yang
> Many tests under `runtime/cds/appcds/dynamicArchive` are crashed when turning on VerifySharedSpaces, VM reports inconsistent crc32 between dumptime and runtime(See detailed content on JBS attachments):
>
> $ diff dump.log run.log
> 17c17
> < - base_archive_is_default:        0
> ---
>> - base_archive_is_default:        1
> 19c19
> < - base_archive_name_size:         0
> ---
>> - base_archive_name_size:         113
>
> The root cause is that even if the CRC32 is calculated(Line 1902), ArchiveBuilder is still writing data to FileMapInfo(Line 1903):
>
> https://github.com/openjdk/jdk/blob/4ea6abfbd1818fa7049bc4f6e46d568e842acb34/src/hotspot/share/memory/archiveBuilder.cpp#L1091-L1094
>
> https://github.com/openjdk/jdk/blob/4ea6abfbd1818fa7049bc4f6e46d568e842acb34/src/hotspot/share/memory/filemap.cpp#L1236-L1251
>
> Now the expected CRC32(serialized in CDS archive) is different from the calculated ones at runtime. This patch addresses this problem, it writes base_archive_name and size into header **before** calculating CRC32(if needed).
>
> Thanks~
> Yang

Yi Yang 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:

  Data is written to file header even if its CRC32 was calculated

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3261/files
  - new: https://git.openjdk.java.net/jdk/pull/3261/files/40b70a2a..4502220d

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3261&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3261&range=01-02

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

PR: https://git.openjdk.java.net/jdk/pull/3261
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264413: Data is written to file header even if its CRC32 was calculated [v3]

Calvin Cheung
On Tue, 30 Mar 2021 13:53:34 GMT, Yi Yang <[hidden email]> wrote:

>> Many tests under `runtime/cds/appcds/dynamicArchive` are crashed when turning on VerifySharedSpaces, VM reports inconsistent crc32 between dumptime and runtime(See detailed content on JBS attachments):
>>
>> $ diff dump.log run.log
>> 17c17
>> < - base_archive_is_default:        0
>> ---
>>> - base_archive_is_default:        1
>> 19c19
>> < - base_archive_name_size:         0
>> ---
>>> - base_archive_name_size:         113
>>
>> The root cause is that even if the CRC32 is calculated(Line 1902), ArchiveBuilder is still writing data to FileMapInfo(Line 1903):
>>
>> https://github.com/openjdk/jdk/blob/4ea6abfbd1818fa7049bc4f6e46d568e842acb34/src/hotspot/share/memory/archiveBuilder.cpp#L1091-L1094
>>
>> https://github.com/openjdk/jdk/blob/4ea6abfbd1818fa7049bc4f6e46d568e842acb34/src/hotspot/share/memory/filemap.cpp#L1236-L1251
>>
>> Now the expected CRC32(serialized in CDS archive) is different from the calculated ones at runtime. This patch addresses this problem, it writes base_archive_name and size into header **before** calculating CRC32(if needed).
>>
>> Thanks~
>> Yang
>
> Yi Yang 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:
>
>   Data is written to file header even if its CRC32 was calculated

Looks good.
I've tried your patch on linux-x64. It passed all cds tests and also in dynamic archive mode by specifying `-vmoptions:'-Dtest.dynamic.cds.archive=true -XX:+VerifySharedSpaces'` and test group `<open repo>/test/hotspot/jtreg:hotspot_appcds_dynamic` to the jtreg command line. It also passed the first 4 tiers of testing on Oracle supported platforms.
I've filed the following RFE for adding a test group for running CDS tests with -XX:+VerifySharedSpaces:
https://bugs.openjdk.java.net/browse/JDK-8264472

-------------

Marked as reviewed by ccheung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3261
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264413: Data is written to file header even if its CRC32 was calculated [v4]

Yi Yang
In reply to this post by Yi Yang
> Many tests under `runtime/cds/appcds/dynamicArchive` are crashed when turning on VerifySharedSpaces, VM reports inconsistent crc32 between dumptime and runtime(See detailed content on JBS attachments):
>
> $ diff dump.log run.log
> 17c17
> < - base_archive_is_default:        0
> ---
>> - base_archive_is_default:        1
> 19c19
> < - base_archive_name_size:         0
> ---
>> - base_archive_name_size:         113
>
> The root cause is that even if the CRC32 is calculated(Line 1902), ArchiveBuilder is still writing data to FileMapInfo(Line 1903):
>
> https://github.com/openjdk/jdk/blob/4ea6abfbd1818fa7049bc4f6e46d568e842acb34/src/hotspot/share/memory/archiveBuilder.cpp#L1091-L1094
>
> https://github.com/openjdk/jdk/blob/4ea6abfbd1818fa7049bc4f6e46d568e842acb34/src/hotspot/share/memory/filemap.cpp#L1236-L1251
>
> Now the expected CRC32(serialized in CDS archive) is different from the calculated ones at runtime. This patch addresses this problem, it writes base_archive_name and size into header **before** calculating CRC32(if needed).
>
> Thanks~
> Yang

Yi Yang has updated the pull request incrementally with one additional commit since the last revision:

  add new jtreg test with VerifySharedSpaces

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3261/files
  - new: https://git.openjdk.java.net/jdk/pull/3261/files/4502220d..c4b27fdc

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3261&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3261&range=02-03

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

PR: https://git.openjdk.java.net/jdk/pull/3261
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264413: Data is written to file header even if its CRC32 was calculated [v3]

Yi Yang
In reply to this post by Calvin Cheung
On Wed, 31 Mar 2021 01:59:35 GMT, Calvin Cheung <[hidden email]> wrote:

> I've tried your patch on linux-x64. It passed all cds tests and also in dynamic archive mode by specifying -vmoptions:'-Dtest.dynamic.cds.archive=true -XX:+VerifySharedSpaces' and test group <open repo>/test/hotspot/jtreg:hotspot_appcds_dynamic to the jtreg command line. It also passed the first 4 tiers of testing on Oracle supported platforms.

Besides, I've added a new dynamic cds test with -XX:+VerifySharedSpaces.

> I've filed the following RFE for adding a test group for running CDS tests with -XX:+VerifySharedSpaces:
https://bugs.openjdk.java.net/browse/JDK-8264472

Looks good!

-------------

PR: https://git.openjdk.java.net/jdk/pull/3261
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264413: Data is written to file header even if its CRC32 was calculated [v4]

Calvin Cheung
In reply to this post by Yi Yang
On Wed, 31 Mar 2021 03:20:53 GMT, Yi Yang <[hidden email]> wrote:

>> Many tests under `runtime/cds/appcds/dynamicArchive` are crashed when turning on VerifySharedSpaces, VM reports inconsistent crc32 between dumptime and runtime(See detailed content on JBS attachments):
>>
>> $ diff dump.log run.log
>> 17c17
>> < - base_archive_is_default:        0
>> ---
>>> - base_archive_is_default:        1
>> 19c19
>> < - base_archive_name_size:         0
>> ---
>>> - base_archive_name_size:         113
>>
>> The root cause is that even if the CRC32 is calculated(Line 1902), ArchiveBuilder is still writing data to FileMapInfo(Line 1903):
>>
>> https://github.com/openjdk/jdk/blob/4ea6abfbd1818fa7049bc4f6e46d568e842acb34/src/hotspot/share/memory/archiveBuilder.cpp#L1091-L1094
>>
>> https://github.com/openjdk/jdk/blob/4ea6abfbd1818fa7049bc4f6e46d568e842acb34/src/hotspot/share/memory/filemap.cpp#L1236-L1251
>>
>> Now the expected CRC32(serialized in CDS archive) is different from the calculated ones at runtime. This patch addresses this problem, it writes base_archive_name and size into header **before** calculating CRC32(if needed).
>>
>> Thanks~
>> Yang
>
> Yi Yang has updated the pull request incrementally with one additional commit since the last revision:
>
>   add new jtreg test with VerifySharedSpaces

Marked as reviewed by ccheung (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/3261
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264413: Data is written to file header even if its CRC32 was calculated [v4]

Yumin Qi-3
In reply to this post by Yi Yang
On Wed, 31 Mar 2021 03:20:53 GMT, Yi Yang <[hidden email]> wrote:

>> Many tests under `runtime/cds/appcds/dynamicArchive` are crashed when turning on VerifySharedSpaces, VM reports inconsistent crc32 between dumptime and runtime(See detailed content on JBS attachments):
>>
>> $ diff dump.log run.log
>> 17c17
>> < - base_archive_is_default:        0
>> ---
>>> - base_archive_is_default:        1
>> 19c19
>> < - base_archive_name_size:         0
>> ---
>>> - base_archive_name_size:         113
>>
>> The root cause is that even if the CRC32 is calculated(Line 1902), ArchiveBuilder is still writing data to FileMapInfo(Line 1903):
>>
>> https://github.com/openjdk/jdk/blob/4ea6abfbd1818fa7049bc4f6e46d568e842acb34/src/hotspot/share/memory/archiveBuilder.cpp#L1091-L1094
>>
>> https://github.com/openjdk/jdk/blob/4ea6abfbd1818fa7049bc4f6e46d568e842acb34/src/hotspot/share/memory/filemap.cpp#L1236-L1251
>>
>> Now the expected CRC32(serialized in CDS archive) is different from the calculated ones at runtime. This patch addresses this problem, it writes base_archive_name and size into header **before** calculating CRC32(if needed).
>>
>> Thanks~
>> Yang
>
> Yi Yang has updated the pull request incrementally with one additional commit since the last revision:
>
>   add new jtreg test with VerifySharedSpaces

LGTM.

-------------

Marked as reviewed by minqi (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3261
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264413: Data is written to file header even if its CRC32 was calculated [v4]

Yi Yang
In reply to this post by Calvin Cheung
On Wed, 31 Mar 2021 16:00:24 GMT, Calvin Cheung <[hidden email]> wrote:

>> Yi Yang has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   add new jtreg test with VerifySharedSpaces
>
> Marked as reviewed by ccheung (Reviewer).

Thanks @calvinccheung @yminqi for reviews.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3261
Reply | Threaded
Open this post in threaded view
|

Integrated: 8264413: Data is written to file header even if its CRC32 was calculated

Yi Yang
In reply to this post by Yi Yang
On Tue, 30 Mar 2021 09:25:20 GMT, Yi Yang <[hidden email]> wrote:

> Many tests under `runtime/cds/appcds/dynamicArchive` are crashed when turning on VerifySharedSpaces, VM reports inconsistent crc32 between dumptime and runtime(See detailed content on JBS attachments):
>
> $ diff dump.log run.log
> 17c17
> < - base_archive_is_default:        0
> ---
>> - base_archive_is_default:        1
> 19c19
> < - base_archive_name_size:         0
> ---
>> - base_archive_name_size:         113
>
> The root cause is that even if the CRC32 is calculated(Line 1902), ArchiveBuilder is still writing data to FileMapInfo(Line 1903):
>
> https://github.com/openjdk/jdk/blob/4ea6abfbd1818fa7049bc4f6e46d568e842acb34/src/hotspot/share/memory/archiveBuilder.cpp#L1091-L1094
>
> https://github.com/openjdk/jdk/blob/4ea6abfbd1818fa7049bc4f6e46d568e842acb34/src/hotspot/share/memory/filemap.cpp#L1236-L1251
>
> Now the expected CRC32(serialized in CDS archive) is different from the calculated ones at runtime. This patch addresses this problem, it writes base_archive_name and size into header **before** calculating CRC32(if needed).
>
> Thanks~
> Yang

This pull request has now been integrated.

Changeset: de495df7
Author:    Yi Yang <[hidden email]>
Committer: Calvin Cheung <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/de495df7
Stats:     98 lines in 4 files changed: 87 ins; 8 del; 3 mod

8264413: Data is written to file header even if its CRC32 was calculated

Reviewed-by: ccheung, minqi

-------------

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