RFR: 8160404: RelocationHolder constructors have bugs [v2]

[Jorn Vernee]

On Mon, 12 Dec 2022 17:56:11 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> 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
>> relocation 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.
>
> Kim Barrett has updated the pull request incrementally with one additional commit since the last revision:
> 
>   blank lines in include blocks

src/hotspot/share/code/relocInfo.hpp line 528:

> 526:     Relocation* reloc = ctor(_relocbuf);
> 527:     check_reloc_placement(reloc);
> 528:   }

It seems that the current code is like this, rather than using `typename T, typename...Args` (where `T` is the relocation type), because it is not possible to specify explicit type arguments when calling a constructor template? (So you can't specify `T`).

But, you could truck in the type argument on `Construct`, and have it be inferred. So, this could become:

Suggestion:

  template<typename T>
  struct Construct {};          // Tag for selecting this constructor.
  template<typename T, typename... Args> RelocationHolder(Construct<T>, const Args&...args) {
    check_reloc_type<T>();
    Relocation* reloc = ::new (_relocbuf) T(args...);
    check_reloc_placement(reloc);
  }

Callers provide e.g. `Construct<T>()` in `construct` or `Construct<Relocation>()` in the default constructor. `construct` can just forward the rest of the args.

This seems a little simpler (by avoiding the use of lambdas, and making it more locally obvious that the constructor is doing a placement new into `_relocbuf`), so maybe this is preferable?

src/hotspot/share/code/relocInfo.hpp line 544:

> 542:                               return ::new (p) T(args...);
> 543:                             });
> 544:   }

This would become
Suggestion:

  static RelocationHolder construct(const Args&... args) {
    return RelocationHolder(Construct<T>(), args...);
  }

src/hotspot/share/code/relocInfo.hpp line 859:

> 857:   // We never heap allocate a Relocation, so never delete through a base pointer.
> 858:   // RelocationHolder depends on (and verifies) the destructor for all relocation
> 859:   // types is trivial, so can't be virtual.

Should this be:
Suggestion:

  // types is trivial, so can be non-virtual.

?

src/hotspot/share/code/relocInfo.hpp line 892:

> 890: inline RelocationHolder::RelocationHolder() :
> 891:   RelocationHolder(Construct(), [&] (void* p) { return ::new (p) Relocation(); })
> 892: {}

And this would become
Suggestion:

inline RelocationHolder::RelocationHolder() : RelocationHolder(Construct<Relocation>())
{}

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

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