RFR: 8160404: RelocationHolder constructors have bugs [v2]
Kim Barrett
kbarrett at openjdk.org
Mon Dec 12 17:56:11 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.
Kim Barrett has updated the pull request incrementally with one additional commit since the last revision:
blank lines in include blocks
-------------
Changes:
- all: https://git.openjdk.org/jdk/pull/11618/files
- new: https://git.openjdk.org/jdk/pull/11618/files/254b094a..2b764714
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=11618&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=11618&range=00-01
Stats: 2 lines in 2 files changed: 2 ins; 0 del; 0 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