On Thu, 17 Oct 2024 10:57:24 GMT, Roman Kennke <rken...@openjdk.org> wrote:
>> This is the main body of the JEP 450: Compact Object Headers (Experimental). >> >> It is also a follow-up to #20640, which now also includes (and supersedes) >> #20603 and #20605, plus the Tiny Class-Pointers parts that have been >> previously missing. >> >> Main changes: >> - Introduction of the (experimental) flag UseCompactObjectHeaders. All >> changes in this PR are protected by this flag. The purpose of the flag is to >> provide a fallback, in case that users unexpectedly observe problems with >> the new implementation. The intention is that this flag will remain >> experimental and opt-in for at least one release, then make it on-by-default >> and diagnostic (?), and eventually deprecate and obsolete it. However, there >> are a few unknowns in that plan, specifically, we may want to further >> improve compact headers to 4 bytes, we are planning to enhance the Klass* >> encoding to support virtually unlimited number of Klasses, at which point we >> could also obsolete UseCompressedClassPointers. >> - The compressed Klass* can now be stored in the mark-word of objects. In >> order to be able to do this, we are add some changes to GC forwarding (see >> below) to protect the relevant (upper 22) bits of the mark-word. Significant >> parts of this PR deal with loading the compressed Klass* from the mark-word. >> This PR also changes some code paths (mostly in GCs) to be more careful when >> accessing Klass* (or mark-word or size) to be able to fetch it from the >> forwardee in case the object is forwarded. >> - Self-forwarding in GCs (which is used to deal with promotion failure) now >> uses a bit to indicate 'self-forwarding'. This is needed to preserve the >> crucial Klass* bits in the header. This also allows to get rid of >> preserved-header machinery in SerialGC and G1 (Parallel GC abuses >> preserved-marks to also find all other relevant oops). >> - Full GC forwarding now uses an encoding similar to compressed-oops. We >> have 40 bits for that, and can encode up to 8TB of heap. When exceeding 8TB, >> we turn off UseCompressedClassPointers (except in ZGC, which doesn't use the >> GC forwarding at all). >> - Instances can now have their base-offset (the offset where the field >> layouter starts to place fields) at offset 8 (instead of 12 or 16). >> - Arrays will now store their length at offset 8. >> - CDS can now write and read archives with the compressed header. However, >> it is not possible to read an archive that has been written with an opposite >> setting of UseCompactObjectHeaders. Some build machinery is added so that >> _co... > > Roman Kennke has updated the pull request incrementally with one additional > commit since the last revision: > > Compact header riscv (#3) > > Implement compact headers on RISCV > --------- > > Co-authored-by: hamlin <ham...@rivosinc.com> > I've managed to reproduce the ECoreIndexOf crash locally by running with > -XX:+UseSerialGC -XX:+UnlockExperimentalVMOptions > -XX:+UseCompactObjectHeaders. The crash happens on line 773 when reading past > the needle. > > ``` > │ 762 __ movq(index, needle_len); > │ > │ 763 __ andq(index, 0xf); // nLen % 16 > │ 764 __ movq(offset, 0x10); > │ 765 __ subq(offset, index); // 16 - (nLen % 16) > │ 766 __ movq(index, offset); > │ 767 __ shlq(offset, 1); // * 2 > │ 768 __ negq(index); // -(16 - (nLen % 16)) > │ > │ 769 __ xorq(wr_index, wr_index); > │ 770 > │ 771 __ bind(L_top); > │ 772 // load needle and expand > │ 773 __ vpmovzxbw(xmm0, Address(needle, index, Address::times_1), > Assembler::AVX_256bit); > ``` > > We're reading this address: > > ``` > (SEGV_MAPERR), si_addr: 0x00000007cffffffe > ``` > > which is just before the start of the heap: > > ``` > Heap address: 0x00000007d0000000, size: 768 MB, Compressed Oops mode: Zero > based, Oop shift amount: 3 > ``` > > When this crashed I had: > > ``` > needle: 0x00000007d000000c > needle_len = 0x12 > index = 0xfffffffffffffffe > ``` > > There has been previous fix to not read past the haystack: Fix header < 16 > bytes in indexOf intrinsic, by @sviswa7 > [f65ef5d](https://github.com/openjdk/jdk/commit/f65ef5dc325212155a50a2fc3a7f4aad18b8d9d0) > > maybe we need something similar for the needle. @sviswa7 @vpaprotsk could you have a look? If we can have a reasonable fix for this soon, we could ship it in this PR, otherwise I'd defer it to a follow-up issue and disable indexOf intrinsic when running with +UseCompactObjectHeaders. ------------- PR Comment: https://git.openjdk.org/jdk/pull/20677#issuecomment-2426754934
