This is an archive of the discontinued LLVM Phabricator instance.

PR36992 don't overwrite virtual bases in tail padding
ClosedPublic

Authored by rsmith on Apr 4 2018, 6:04 PM.

Details

Summary

When initializing a base class, we might accidentally overwrite a virtual base in its tail padding in some situations (in particular, when emitting a trivial copy constructor as a memcpy, we copy the full object size rather than only the dsize):

struct A { char c; A(const A&) : c(1) {} };
struct B { int n; char c[3]; ~B(); };
struct C : B, virtual A {};
C f(C c) { return c; }

(Here, Clang emits an 8-byte memcpy for the B base in f, overwriting the already-initialized A vbase.)

It's easy enough to prevent that from happening entirely, but we would still like to copy the padded-to-alignment size whenever we can (for example, see the tests in test/CodeGenCXX/assign-construct-memcpy.cpp). This patch adds a flag to AggValueSlot to indicate whether the tail padding of the aggregate might overlap some other object, and propagate that flag far enough that we can see it when emitting a trivial copy ctor call. When initializing a base class, we treat it as potentially-overlapping only if the base class lies partly outside the nvsize of the derived class -- otherwise, because we know non-virtual subobjects are initialized in address order, an overlarge store is valid.

This patch also lays some groundwork for P0840, which permits packing into the tail padding for fields marked with a new attribute, wherein the same situation can arise (a vbase can be constructed in the tail padding of the last field if it has the attribute):

struct A { char c; A(const A&) : c(1) {} };
struct B { int n; char c[3]; ~B(); };
struct C : virtual A { [[no_unique_address]] B b; };
C f(C c) { return c; }

(Copying a C object must not perform an 8-byte copy for the B subobject, even though it's a most-derived object, because its tail padding contains an A.)

It also prepares us for the possibility that WG21 might decide that guaranteed copy elision is supposed to apply to base class initialization (for classes without virtual bases), which would mean that we cannot assume that the tail padding of the return slot of a function has no overlap with other objects:

struct A { char c; };
struct B { int n; char c[3]; ~B(); };
B f() { return /*...*/ }
struct C : B, virtual A {
  C() : A{1}, B(f()) {}
};

If B(f()) is not permitted to make a copy, then f() cannot trample on its return slot's tail padding (which in this example might contain an already-initialized A object).

Diff Detail

Repository
rC Clang

Event Timeline

rsmith created this revision.Apr 4 2018, 6:04 PM

Changing the requirements on the return-value slot would be an ABI break, no? And it would force to us to treat all complete-object constructions as nvsize-limited unless they're constructions of known sizeof-sized allocations.

CodeGen/CGDecl.cpp
1462 ↗(On Diff #141097)

How is 'does not overlap' justified here? Should this be propagated into the LValue?

CodeGen/CGExprAgg.cpp
1721 ↗(On Diff #141097)

emitArrayLength drills down to the base non-array type; it handles nested VLAs.

rsmith updated this revision to Diff 141205.Apr 5 2018, 1:06 PM
rsmith marked 2 inline comments as done.

Changing the requirements on the return-value slot would be an ABI break, no?

It would be, yes. But WG21 seems to have a taste for breaking changes at the moment, so who knows... I'm happy to remove overlapForReturnValue for now; it's easy enough to find these places again should the change ever happen.

And it would force to us to treat all complete-object constructions as nvsize-limited unless they're constructions of known sizeof-sized allocations.

P0840 pushes us there already. Fortunately, we don't treat complete-object constructors specially in this regard currently, and no other compiler seems to do so either. However, when performing the trivial copy/move constructor -> memcpy optimization (which is I think the most interesting case for extending a store into the tail padding), we do know which object is being initialized, so we can take into account whether it could have something else allocated into its tail padding.

CodeGen/CGDecl.cpp
1462 ↗(On Diff #141097)

That's what I get for trusting comments :)

No functional change from DoesNotOverlap here yet, though, because in practice this is always called with some kind of VarDecl (which is DoesNotOverlap because it's a complete object) or some kind of FieldDecl (which is DoesNotOverlap until P0840 lands).

rjmccall accepted this revision.Apr 5 2018, 1:20 PM

Okay, LGTM.

This revision is now accepted and ready to land.Apr 5 2018, 1:20 PM
This revision was automatically updated to reflect the committed changes.