MinGW-clang: Converting constructor of class with no-unique-address members overwrites last byte with zeroes #64077
Labels
Comments
crtrott
added
clang
crtrott
changed the title
|
FIY, this works [[no_unique_address]] M map;
[[no_unique_address]] A acc;
T* ptr;``
And is probably more what you wanted to express, although that does not mean there isn't a bug |
|
I'm a bit confused...
|
|
Couple things: a) in mdspan ptr is not necessarily a T* it can very well be a class, and in fact it COULD be an empty class if you are naughty enough to exploit every possible way you can exploit customization points in mdspan. For example: accessor could be a a file system access thing, and ptr could be a handle containing a compile time string defining the file - not that I believe anyone should do that, but its possible. And yeah the failing case on the left hand side T* and M should NOT be overlapping since M is not empty. The one I am constructing from however had an empty M. |
|
Oh and reordering the arguments here, may very well fix my test case, but it may break another one where I test screwy things the other way around: i.e. I make M have a single size_t member every time (i.e. dextents<size_t, 1>), have an accessor with a member on the left, and an empty accessor in the original thing. It will just shift the problem to another corner case. |
|
Is this still something we need to fix for 17.x? I think I saw some other fixes for mdspan. |
I believe so. The thing that was merged was the general implementation (which was merged slightly after the 17.x branch was made, and then backported) - it has some |
|
Yeah this issue is still relevant. Note that the godbolt reproducer above does not require mdspan. It's just that it surfaces in mdspan testing, when I test legal misuse of our customization points (i.e. I test user provided implementations of customization points, which are idiosyncratic naughty, but legal). |
|
I commented on the other issue, but basically the godbolt example fails also on PowerPC with Clang 14, and presumably with clang 17 (which I am still in the process of compiling on my power platform). (Minor note: had to remove the concept and span include which were unnecessary anyway). |
|
This sounds a lot like #64253 - see the very minimal testcase in the comment at #64253 (comment). |
|
See https://reviews.llvm.org/D157332 for a potential fix for this case. |
mstorsjo
added a commit
that referenced
this issue
Aug 15, 2023
An empty struct is handled as a struct with a dummy i8, on all targets. Most targets treat an empty struct return value as essentially void - but some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't treat it as void.) When intializing a struct with such a no_unique_address member, make sure we don't write the dummy i8 into the struct where there's no space allocated for it. Previously it would clobber the actual valid data of the struct. Fixes #64253, and possibly #64077 and #64427 as well. We should omit the store for any empty record (not only ones declared with no_unique_address); we can have a situation where a class doesn't have the no_unique_address attribute, but is embedded in an outer struct with the no_unique_address attribute - like this: struct S {}; S f(); struct S2 : public S { S2();}; S2::S2() : S(f()) {} struct S3 { int x; [[no_unique_address]] S2 y; S3(); }; S3::S3() : x(1), y() {} Here, the problematic store (which this patch omits) is in the constructor of S2. In the case of S3, S2 has no valid storage and aliases x - thus the constructor of S2 should omit the dummy store. Differential Revision: https://reviews.llvm.org/D157332
llvmbot
pushed a commit
to llvm/llvm-project-release-prs
that referenced
this issue
Aug 15, 2023
An empty struct is handled as a struct with a dummy i8, on all targets. Most targets treat an empty struct return value as essentially void - but some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't treat it as void.) When intializing a struct with such a no_unique_address member, make sure we don't write the dummy i8 into the struct where there's no space allocated for it. Previously it would clobber the actual valid data of the struct. Fixes llvm/llvm-project#64253, and possibly llvm/llvm-project#64077 and llvm/llvm-project#64427 as well. We should omit the store for any empty record (not only ones declared with no_unique_address); we can have a situation where a class doesn't have the no_unique_address attribute, but is embedded in an outer struct with the no_unique_address attribute - like this: struct S {}; S f(); struct S2 : public S { S2();}; S2::S2() : S(f()) {} struct S3 { int x; [[no_unique_address]] S2 y; S3(); }; S3::S3() : x(1), y() {} Here, the problematic store (which this patch omits) is in the constructor of S2. In the case of S3, S2 has no valid storage and aliases x - thus the constructor of S2 should omit the dummy store. Differential Revision: https://reviews.llvm.org/D157332 (cherry picked from commit d60c3d0)
tru
pushed a commit
to llvm/llvm-project-release-prs
that referenced
this issue
Aug 16, 2023
An empty struct is handled as a struct with a dummy i8, on all targets. Most targets treat an empty struct return value as essentially void - but some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't treat it as void.) When intializing a struct with such a no_unique_address member, make sure we don't write the dummy i8 into the struct where there's no space allocated for it. Previously it would clobber the actual valid data of the struct. Fixes llvm/llvm-project#64253, and possibly llvm/llvm-project#64077 and llvm/llvm-project#64427 as well. We should omit the store for any empty record (not only ones declared with no_unique_address); we can have a situation where a class doesn't have the no_unique_address attribute, but is embedded in an outer struct with the no_unique_address attribute - like this: struct S {}; S f(); struct S2 : public S { S2();}; S2::S2() : S(f()) {} struct S3 { int x; [[no_unique_address]] S2 y; S3(); }; S3::S3() : x(1), y() {} Here, the problematic store (which this patch omits) is in the constructor of S2. In the case of S3, S2 has no valid storage and aliases x - thus the constructor of S2 should omit the dummy store. Differential Revision: https://reviews.llvm.org/D157332 (cherry picked from commit d60c3d0)
|
can this one be closed now @mstorsjo ? |
razmser
pushed a commit
to SuduIDE/llvm-project
that referenced
this issue
Oct 2, 2023
An empty struct is handled as a struct with a dummy i8, on all targets. Most targets treat an empty struct return value as essentially void - but some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't treat it as void.) When intializing a struct with such a no_unique_address member, make sure we don't write the dummy i8 into the struct where there's no space allocated for it. Previously it would clobber the actual valid data of the struct. Fixes llvm#64253, and possibly llvm#64077 and llvm#64427 as well. We should omit the store for any empty record (not only ones declared with no_unique_address); we can have a situation where a class doesn't have the no_unique_address attribute, but is embedded in an outer struct with the no_unique_address attribute - like this: struct S {}; S f(); struct S2 : public S { S2();}; S2::S2() : S(f()) {} struct S3 { int x; [[no_unique_address]] S2 y; S3(); }; S3::S3() : x(1), y() {} Here, the problematic store (which this patch omits) is in the constructor of S2. In the case of S3, S2 has no valid storage and aliases x - thus the constructor of S2 should omit the dummy store. Differential Revision: https://reviews.llvm.org/D157332
razmser
pushed a commit
to SuduIDE/llvm-project
that referenced
this issue
Oct 2, 2023
An empty struct is handled as a struct with a dummy i8, on all targets. Most targets treat an empty struct return value as essentially void - but some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't treat it as void.) When intializing a struct with such a no_unique_address member, make sure we don't write the dummy i8 into the struct where there's no space allocated for it. Previously it would clobber the actual valid data of the struct. Fixes llvm#64253, and possibly llvm#64077 and llvm#64427 as well. We should omit the store for any empty record (not only ones declared with no_unique_address); we can have a situation where a class doesn't have the no_unique_address attribute, but is embedded in an outer struct with the no_unique_address attribute - like this: struct S {}; S f(); struct S2 : public S { S2();}; S2::S2() : S(f()) {} struct S3 { int x; [[no_unique_address]] S2 y; S3(); }; S3::S3() : x(1), y() {} Here, the problematic store (which this patch omits) is in the constructor of S2. In the case of S3, S2 has no valid storage and aliases x - thus the constructor of S2 should omit the dummy store. Differential Revision: https://reviews.llvm.org/D157332
razmser
pushed a commit
to SuduIDE/llvm-project
that referenced
this issue
Oct 2, 2023
An empty struct is handled as a struct with a dummy i8, on all targets. Most targets treat an empty struct return value as essentially void - but some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't treat it as void.) When intializing a struct with such a no_unique_address member, make sure we don't write the dummy i8 into the struct where there's no space allocated for it. Previously it would clobber the actual valid data of the struct. Fixes llvm#64253, and possibly llvm#64077 and llvm#64427 as well. We should omit the store for any empty record (not only ones declared with no_unique_address); we can have a situation where a class doesn't have the no_unique_address attribute, but is embedded in an outer struct with the no_unique_address attribute - like this: struct S {}; S f(); struct S2 : public S { S2();}; S2::S2() : S(f()) {} struct S3 { int x; [[no_unique_address]] S2 y; S3(); }; S3::S3() : x(1), y() {} Here, the problematic store (which this patch omits) is in the constructor of S2. In the case of S3, S2 has no valid storage and aliases x - thus the constructor of S2 should omit the dummy store. Differential Revision: https://reviews.llvm.org/D157332
razmser
pushed a commit
to SuduIDE/llvm-project
that referenced
this issue
Oct 3, 2023
An empty struct is handled as a struct with a dummy i8, on all targets. Most targets treat an empty struct return value as essentially void - but some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't treat it as void.) When intializing a struct with such a no_unique_address member, make sure we don't write the dummy i8 into the struct where there's no space allocated for it. Previously it would clobber the actual valid data of the struct. Fixes llvm#64253, and possibly llvm#64077 and llvm#64427 as well. We should omit the store for any empty record (not only ones declared with no_unique_address); we can have a situation where a class doesn't have the no_unique_address attribute, but is embedded in an outer struct with the no_unique_address attribute - like this: struct S {}; S f(); struct S2 : public S { S2();}; S2::S2() : S(f()) {} struct S3 { int x; [[no_unique_address]] S2 y; S3(); }; S3::S3() : x(1), y() {} Here, the problematic store (which this patch omits) is in the constructor of S2. In the case of S3, S2 has no valid storage and aliases x - thus the constructor of S2 should omit the dummy store. Differential Revision: https://reviews.llvm.org/D157332
razmser
pushed a commit
to SuduIDE/llvm-project
that referenced
this issue
Oct 3, 2023
An empty struct is handled as a struct with a dummy i8, on all targets. Most targets treat an empty struct return value as essentially void - but some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't treat it as void.) When intializing a struct with such a no_unique_address member, make sure we don't write the dummy i8 into the struct where there's no space allocated for it. Previously it would clobber the actual valid data of the struct. Fixes llvm#64253, and possibly llvm#64077 and llvm#64427 as well. We should omit the store for any empty record (not only ones declared with no_unique_address); we can have a situation where a class doesn't have the no_unique_address attribute, but is embedded in an outer struct with the no_unique_address attribute - like this: struct S {}; S f(); struct S2 : public S { S2();}; S2::S2() : S(f()) {} struct S3 { int x; [[no_unique_address]] S2 y; S3(); }; S3::S3() : x(1), y() {} Here, the problematic store (which this patch omits) is in the constructor of S2. In the case of S3, S2 has no valid storage and aliases x - thus the constructor of S2 should omit the dummy store. Differential Revision: https://reviews.llvm.org/D157332
razmser
pushed a commit
to SuduIDE/llvm-project
that referenced
this issue
Oct 6, 2023
An empty struct is handled as a struct with a dummy i8, on all targets. Most targets treat an empty struct return value as essentially void - but some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't treat it as void.) When intializing a struct with such a no_unique_address member, make sure we don't write the dummy i8 into the struct where there's no space allocated for it. Previously it would clobber the actual valid data of the struct. Fixes llvm#64253, and possibly llvm#64077 and llvm#64427 as well. We should omit the store for any empty record (not only ones declared with no_unique_address); we can have a situation where a class doesn't have the no_unique_address attribute, but is embedded in an outer struct with the no_unique_address attribute - like this: struct S {}; S f(); struct S2 : public S { S2();}; S2::S2() : S(f()) {} struct S3 { int x; [[no_unique_address]] S2 y; S3(); }; S3::S3() : x(1), y() {} Here, the problematic store (which this patch omits) is in the constructor of S2. In the case of S3, S2 has no valid storage and aliases x - thus the constructor of S2 should omit the dummy store. Differential Revision: https://reviews.llvm.org/D157332
razmser
pushed a commit
to SuduIDE/llvm-project
that referenced
this issue
Oct 11, 2023
An empty struct is handled as a struct with a dummy i8, on all targets. Most targets treat an empty struct return value as essentially void - but some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't treat it as void.) When intializing a struct with such a no_unique_address member, make sure we don't write the dummy i8 into the struct where there's no space allocated for it. Previously it would clobber the actual valid data of the struct. Fixes llvm#64253, and possibly llvm#64077 and llvm#64427 as well. We should omit the store for any empty record (not only ones declared with no_unique_address); we can have a situation where a class doesn't have the no_unique_address attribute, but is embedded in an outer struct with the no_unique_address attribute - like this: struct S {}; S f(); struct S2 : public S { S2();}; S2::S2() : S(f()) {} struct S3 { int x; [[no_unique_address]] S2 y; S3(); }; S3::S3() : x(1), y() {} Here, the problematic store (which this patch omits) is in the constructor of S2. In the case of S3, S2 has no valid storage and aliases x - thus the constructor of S2 should omit the dummy store. Differential Revision: https://reviews.llvm.org/D157332
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
While implementing
mdspanI ran into an issue during MinGW CI. I reproduced this standalone on GodBolt: https://godbolt.org/z/KK8aj5bs7The related change revision is found here: https://reviews.llvm.org/D154367
Shortish version, you have something like this:
If you now do
mdspan<const T, M2, A2>(mdspan<T, M1, A1>);where M1, A1 and A2 are empty, but M2 is not, and A1->A2 happens via a conversion operator in A1, not a converting constructor in A2 the last byte ofptrgets zeroed out.The text was updated successfully, but these errors were encountered: