RFR: 8274322: Problems with oopDesc construction

[David Holmes]

On Tue, 28 Sep 2021 03:12:52 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

> Please review this change to the default constructor for markWord and
> associated "change" to construction of oopDesc.
> 
> The current code never invokes the constructor for oopDesc or any of its
> derived classes.  For that to be permissible according to the Standard,
> those classes must be trivially default constructible.  And for that to be
> the case, the markWord default constructor must be trivial.
> 
> This change consists of three parts.
> 
> (1) The markWord default constructor is changed to be trivial, so the
> default constructors for oopDesc and classes derived from it will also be
> trivial. It wasn't previously trivial because the mechanism for making it so
> (a default definition) is a C++11 feature that wasn't yet supported when the
> previous constructor was defined.
> 
> (2) This change also adds static asserts to verify the relevant classes have
> trivial default constructors, to prevent later changes from unintentionally
> breaking this.
> 
> (3) This change also makes oopDesc noncopyable, to prevent inadvertent usage
> of these operations that don't make any sense.
> 
> A different approach would be to always use placement new with an
> appropriate constructor to perform the initialization, perhaps encapsulated
> in factory functions.  I did some exploration in that direction. It's a much
> larger and more complex change, though the final behavior (use constructors
> for initialization) is simpler.
> 
> Testing:
> tier1

Hi Kim,

Based on your detailed description the changes look good. A couple of minor comments.

Thanks,
David

src/hotspot/share/oops/oop.hpp line 35:

> 33: #include "runtime/atomic.hpp"
> 34: #include "utilities/macros.hpp"
> 35: #include "utilities/globalDefinitions.hpp"

Nit: not included in alphabetic order (and it will include macros.hpp itself anyway).

src/hotspot/share/oops/oop.hpp line 326:

> 324: // the Java heap, and static functions provided here on HeapWord* are used
> 325: // to fill in certain parts of that memory.  For that to be valid, the
> 326: // object must not have non-trivial initialization (C++14 3.8).  For that to

Can we avoid the double-negative and state it "must have trivial initialization"?

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

Marked as reviewed by dholmes (Reviewer).

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