8256732: Zero: broken +ZeroTLAB exposes badly initialized memory

[shipilev]

Looks like memory is badly initialized when -XX:+ZeroTLAB is specified.

Manifests like this:

$ CONF=linux-x86_64-zero-fastdebug make exploded-test TEST=compiler/memoryinitialization/ZeroTLABTest.java

command: main -Xcomp -XX:+UseTLAB -XX:+ZeroTLAB compiler.memoryinitialization.ZeroTLABTest
reason: User specified action: run main/othervm -Xcomp -XX:+UseTLAB -XX:+ZeroTLAB compiler.memoryinitialization.ZeroTLABTest
Mode: othervm [/othervm specified]
elapsed time (seconds): 0.098
configuration:
STDOUT:
Error occurred during initialization of VM
java.lang.NullPointerException
at java.lang.System.getProperty(java.base/System.java:836)

The cause is simple: Zero calls ThreadLocalAllocBuffer::allocate:

if (UseTLAB) {
  result = (oop) THREAD->tlab().allocate(obj_size);
}

...which actually does mangle the object space in debug builds:

inline HeapWord* ThreadLocalAllocBuffer::allocate(size_t size) {
  invariants();
  HeapWord* obj = top();
  if (pointer_delta(end(), obj) >= size) {
    // successful thread-local allocation
#ifdef ASSERT
    // Skip mangling the space corresponding to the object header to
    // ensure that the returned space is not considered parsable by
    // any concurrent GC thread.
    size_t hdr_size = oopDesc::header_size();
    Copy::fill_to_words(obj + hdr_size, size - hdr_size, badHeapWordVal);
#endif // ASSERT
    // This addition is safe because we know that top is
    // at least size below end, so the add can't wrap.
    set_top(obj + size);

    invariants();
    return obj;
  }
  return NULL;
}

So if we do +ZeroTLAB in debug builds, Zero skips initializing the object body, and gets scrambled memory for newly allocated object. Then everything breaks.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8256732: Zero: broken +ZeroTLAB exposes badly initialized memory

Reviewers

• David Holmes (@dholmes-ora - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1343/head:pull/1343
$ git checkout pull/1343

[bridgekeeper]

👋 Welcome back shade! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@shipilev The following label will be automatically applied to this pull request:

• hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 03: Full (86d15a5)
• 02: Full (81ace2c)
• 01: Full - Incremental (8200d89)
• 00: Full (bca3106)

[dholmes-ora]

But isn't the memory returned by ThreadLocalAllocBuffer::allocate implicitly already zeroed when ZeroTLAB is specified?

[shipilev]

But isn't the memory returned by ThreadLocalAllocBuffer::allocate implicitly already zeroed when ZeroTLAB is specified?

No! That tripped me too! The comment at ThreadLocalAllocBuffer::allocate actually says: // Allocate size HeapWords. The memory is NOT initialized to zero.

[dholmes-ora]

But isn't the memory returned by ThreadLocalAllocBuffer::allocate implicitly already zeroed when ZeroTLAB is specified?

No! That tripped me too! The comment at ThreadLocalAllocBuffer::allocate actually says: // Allocate size HeapWords. The memory is NOT initialized to zero.

But is that comment true? Does it really mean "The memory is NOT initialized (unless ZeroTLAB has been set)" ?

If ZeroTLAB is not actually zeroing all memory returned via TLAB then something seems far more broken than just Zero!

[bridgekeeper]

@shipilev This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@shipilev This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it!

[shipilev]

If ZeroTLAB is not actually zeroing all memory returned via TLAB then something seems far more broken than just Zero!

I think I figured it out: Zero picks the short stick with space mangling. The rest of Hotspot code does not call that method directly, and instead goes through various MemoryAllocator classes (which I am not sure we can touch without the VM transition). So, I fixed this by allowing debug builds to initialize object field block again.

[shipilev]

Anyone? :)

[openjdk]

@shipilev This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8256732: Zero: broken +ZeroTLAB exposes badly initialized memory

Reviewed-by: dholmes

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 211 new commits pushed to the master branch:

• a31a23d: 8263595: Remove oop type punning in JavaCallArguments
• a1f6591: 8263589: Introduce JavaValue::get_oop/set_oop
• 20297a1: 8263577: C2: reachable nodes shouldn't have dead uses at the end of optimizations
• c484d89: 8263557: Possible NULL dereference in Arena::destruct_contents()
• ba35193: 8263559: Add missing initializers to VM_PopulateDumpSharedSpace
• e03a594: 8262504: Some CLHSDB command cannot know they run on remote debugger
• d896246: 8263420: Incorrect function name in NSAccessibilityStaticText native peer implementation
• 8c1112a: 8261916: gtest/GTestWrapper.java vmErrorTest.unimplemented1_vm_assert failed
• 1e57087: 8263392: Allow current thread to be specified in ExceptionMark
• 4d1c08c: 8263616: 'Deprecatd' typo in src/hotspot/share/classfile/classFileParser.cpp
• ... and 201 more: https://git.openjdk.java.net/jdk/compare/20c93b3b901806cd9d691d75c8f30398c41fec34...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[dholmes-ora]

Sorry clicked too soon and hadn't added this comment. :)

I still think there is some underlying bug here with regards to ZeroTLAB but otherwise the changes you have made seem okay, and the proof as always is in the testing, so I've approved it.

Cheers,
David

[theRealAph]

ZeroTLAB seems to do nothing useful for performance on any platform, even AArch64 which has bulk memory zeroing instructions. Can we not just nuke it?

[shipilev]

ZeroTLAB seems to do nothing useful for performance on any platform, even AArch64 which has bulk memory zeroing instructions. Can we not just nuke it?

Yes, that was my thought too, when I was looking through this code.

[shipilev]

/integrate

[openjdk]

@shipilev Since your change was applied there have been 217 commits pushed to the master branch:

• e33bfb3: 8263450: Simplify LambdaForm.useCount
• 75ef6f5: 8263427: Shenandoah: Trigger weak-LRB even when heap is stable
• 4517d72: 8263612: Unused variables in C1 runtime
• 83a9a02: 8263509: LdapSchemaParser.readNextTag checks array length incorrectly
• 9c50b8e: 8263587: C2: JVMS not cloned when needs_clone_jvms() is true
• 68deb24: 8080272: Refactor I/O stream copying to use InputStream.transferTo/readAllBytes and Files.copy
• a31a23d: 8263595: Remove oop type punning in JavaCallArguments
• a1f6591: 8263589: Introduce JavaValue::get_oop/set_oop
• 20297a1: 8263577: C2: reachable nodes shouldn't have dead uses at the end of optimizations
• c484d89: 8263557: Possible NULL dereference in Arena::destruct_contents()
• ... and 207 more: https://git.openjdk.java.net/jdk/compare/20c93b3b901806cd9d691d75c8f30398c41fec34...master

Your commit was automatically rebased without conflicts.

Pushed as commit dc93138.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.