Move construction clobbers data through [[no_unique_address]]

[danakj]

Godbolt: https://godbolt.org/z/e96axcv1b

#include <stdint.h>

#include <iostream>
#include <string>
#include <vector>

struct S2 {
    uint64_t a;
};

struct S : public S2 {
    S(uint8_t b, uint64_t a) : S2(a), b(b) {}
    uint8_t b;
    // 3 bytes padding.
};
static_assert(sizeof(S) == 16);

union U {
    U() {}
    [[no_unique_address]] S s;
};
static_assert(sizeof(U) == 16);

struct D {
    D(uint8_t i) : i(i) {}

    [[no_unique_address]] U u;
    uint8_t i;
};
static_assert(sizeof(D) == 16);

int main() {
    D d(1u);
    std::cout << (int)d.i << "\n";  // Prints 1

    // Regular construction into `D::u` doesn't touch `D::i`. Good.
    new (&d.u.s) S(2, 3);
    std::cout << (int)d.i << "\n";  // Prints 1

    S s(4, 5);
    // Move construction into `D::u` overwrites `D::i`. Bad.
    new (&d.u.s) S(std::move(s));
    std::cout << (int)d.i << "\n";  // Prints 0
    return d.i;
}

[danakj]

GCC does also similar things: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108775

(MSVC does not use no_unique_address, so has no similar issue)

[shafik]

@zygoloid this looks similar to #36340 in which the memcpy we are emitting is not taking into account the overlap. If that is indeed the can do yo know how much work it would be to apply the same machinery to this case?

[zygoloid]

@shafik It looks like we're properly passing through the overlap information when emitting a constructor call here, but it's set incorrectly for new expressions here. I think that changing the latter to MayOverlap would fix this, but we can probably be smarter than that: if the new expression uses a replaceable global allocation function, we can conclude that we have a NoOverlap situation.

Array new initialization is also assuming NoOverlap, but I think that's OK, because I think an array element won't ever overlap with something outside the array (at least, not in the Itanium ABI rules) -- but it'd be a good idea for someone to go through the Itanium ABI spec and double-check that.

[frederick-vs-ja]

Array new initialization is also assuming NoOverlap, but I think that's OK, because I think an array element won't ever overlap with something outside the array (at least, not in the Itanium ABI rules) -- but it'd be a good idea for someone to go through the Itanium ABI spec and double-check that.

Array element subobjects are never potentially-overlapping so IIUC they are not allowed to overlap with those things. However, given CWG2527 is closed as NAD, it seems that a member subobject of an array type can be itself potentially-overlapping.

I think this example is sufficiently different from #36340. This example reused the storage reuse of a potentially-overlapping subobject happens in this example, which ends the lifetime of every enclosing object and "actually" overlapping subobject of them ([basic.life]/1.5, [basic.life]/8.4).

So the lifetime of d.i is ended by the placement-new, and then it's undefined behavior to read its value, and the storage it occupied is allowed to be written.

[llvmbot]

@llvm/issue-subscribers-c-20

Godbolt: https://godbolt.org/z/e96axcv1b

#include <stdint.h>

#include <iostream>
#include <string>
#include <vector>

struct S2 {
    uint64_t a;
};

struct S : public S2 {
    S(uint8_t b, uint64_t a) : S2(a), b(b) {}
    uint8_t b;
    // 3 bytes padding.
};
static_assert(sizeof(S) == 16);

union U {
    U() {}
    [[no_unique_address]] S s;
};
static_assert(sizeof(U) == 16);

struct D {
    D(uint8_t i) : i(i) {}

    [[no_unique_address]] U u;
    uint8_t i;
};
static_assert(sizeof(D) == 16);

int main() {
    D d(1u);
    std::cout << (int)d.i << "\n";  // Prints 1

    // Regular construction into `D::u` doesn't touch `D::i`. Good.
    new (&d.u.s) S(2, 3);
    std::cout << (int)d.i << "\n";  // Prints 1

    S s(4, 5);
    // Move construction into `D::u` overwrites `D::i`. Bad.
    new (&d.u.s) S(std::move(s));
    std::cout << (int)d.i << "\n";  // Prints 0
    return d.i;
}

[danakj]

It seems like this issue is due to the mistake of calling in-place new a no_unique_address field, which was also the cause of #70494.

Then this is not a bug in clang, and is a bug in the code. And the documentation of no_unique_address needs some very important explanations of how to not hold it.

@zygoloid before I close this, does this sound right to you, or is there another bug here too?

[zygoloid]

Yes, that sounds right to me.

I am idly wondering whether we should guarantee to not clobber tail padding in cases like this, as a vendor extension. I think we can't guarantee that new (&x) T(FunctionReturningT()) doesn't clobber tail padding, because functions in general get to assume that they own their entire return object including its padding, but I think for any other kind of initialization we can give that guarantee. (The other case that seems tricky is that a complete-object constructor variant might assume it can overwrite the whole object, but I don't think we currently take advantage of that.)

[danakj]

Hm, I'm not sure the vendor extension is worth it. You have to build the extra structure out for replacing your values through other methods anyway, at which point there's nothing really to benefit from the extension, I think? It would let initialize the lower fields with ctor initializers I guess, instead of initializing them after the copy/move construction in the union is done in the body of the ctor. Maybe that matters when the fields below the union are not trivial and can't be left uninitialized, but that's not the case with expected/Result-like or optional/Option-like things anyway.