RFR: 8160404: RelocationHolder constructors have bugs

Kim Barrett kbarrett at openjdk.org
Fri Dec 9 22:11:41 UTC 2022


Please review this change to construction and copying of the Relocation and
RelocationHolder classes, to eliminate some questionable C++ usage.

The array type for RelocationHandle::_relocbuf is changed from void* to char,
because using a char array for raw memory is countenanced by the standard,
while not so much for an array of void*.  The desired alignment is maintained
via a union, since using alignas is not (yet) permitted in HotSpot code.

There is also now a comment discussing the use of _relocbuf in more detail,
including some areas of continued sketchiness wrto standard conformance and
reliance on implementation dependent behavior. 

No longer use trivial copy and assignment for RelocationHolder, since that
isn't technically correct.  The Relocation in the holder is not trivially
copyable, since it is polymorphic.  It seemed to work in practice with the
supported compilers, but we shouldn't (and don't need to) rely on it.  Instead
we have a new virtual function Relocation::copy_into that copies the most
derived object into the holder's _relocbuf using placement new.

Eliminated the implict conversion constructor from Relocation to holder that
wordwise copied (to possibly beyond the end of) the Relocation into the
holder's _relocbuf.  We could have implemented this more carefully with the
new approach (using copy_into), but we don't actually need this conversion.
The only use of it was actually a wasted copy (in assembler_x86.cpp).

Eliminated the use of placement new syntax via operator new with a holder
argument to copy a Resource object into a holder.  This included runtime
verification that the size of the object is not greater than the size of
_relocbuf; we now do corresponding verification at compile-time.  This also
included an incorrect attempt at a runtime check that the Relocation base
class would be at the same address as the derived class being constructed; we
now perform that check correctly.  We also discuss in a comment the layout
assumption being made (that isn't required by the standard but is provided by
all supported compilers), and what to do if we encounter a compiler that
behaves differently.

Eliminated the idiom of making a default-constructed holder and then
overwriting its held relocation with a newly constructed one, using the afore
mentioned (and eliminated) operator new.  Instead, RelocationHolder now has a
factory function template (construct<T>) for creating holders with a given
resource type, constructed using provided arguments.  (The arguments are taken
as const-ref rather than using perfect forwarding, as the tools for the latter
are not (yet) approved for use in HotSpot. Using const-ref is good enough in
this case.)

Describe and verify other assumptions being made, such as all Relocation
classes being trivially destructible.

Testing:
mach5 tier1-5

Future work:

* RelocationHolder::reloc isn't const-correct.  Making it so will require
adjustment of some callers.  I'll follow up with an RFE to address this.

* Relocation classes have many virtual function overrides that are unmarked.
I'll follow up with an RFE to add "override" specifiers.

Potential issue: The removal of RelocationHolder(Relocation*) might not work
for some platforms.  I've tested on platforms supported by Oracle (where there
was only one (mistaken) use).  There might be uses by other platforms.

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

Commit messages:
 - fix constructors and assigns

Changes: https://git.openjdk.org/jdk/pull/11618/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11618&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8160404
  Stats: 260 lines in 3 files changed: 147 ins; 51 del; 62 mod
  Patch: https://git.openjdk.org/jdk/pull/11618.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11618/head:pull/11618

PR: https://git.openjdk.org/jdk/pull/11618


More information about the hotspot-compiler-dev mailing list