Differential D133711
[Sema] Reject array element types whose sizes aren't a multiple of their alignments Authored by ahatanak on Sep 12 2022, 10:15 AM.
Details
In the following code, the first element is aligned on a 16-byte boundary, but the remaining elements aren't: typedef char int8_a16 __attribute__((aligned(16))); int8_a16 array[4]; Currently clang doesn't reject the code, but it should since it can cause crashes at runtime. This patch also fixes assertion failures in CodeGen caused by the changes in https://reviews.llvm.org/D123649.
Diff Detail
Event Timeline
Comment Actions Usually the solution is to round up the size to the alignment. Can you provide godbolt examples of what GCC and MSVC do? Comment Actions
gcc prints a similar error, MSVC doesn't support an equivalent alignment attribute. Comment Actions It looks like gcc started rejecting the code relatively recently (11.1). Also, I found out that MSVC doesn't reject the following code: struct T0 { char c; };
struct T2 : virtual T0 { };
T2 array[2];The size of T2 is 5 and its alignment is 4. The size of array is 10 bytes. Comment Actions Unlike gcc or clang. MSVC doesn't round up the size to its alignment. I don't mean it should reject the code. Comment Actions
On what example? The code in the commit message gives an error on gcc 5.
This looks like a backwards-compatibility quirk in the 32-bit x86 MSVC ABI; other MSVC targets round up the size, and even adding __declspec(align(1)) forces the size to something sane. Don't know if we want to try to emulate this. Comment Actions I was looking at another example. The following code is accepted by 10.3, but is rejected by 11.1 because the size isn't a multiple of the alignment. struct __attribute__((packed)) S {
char c;
int i;
};
typedef S AlignedS __attribute__((aligned(4)));
AlignedS array[4];Comment Actions Sorry, my comment was misleading. clang has the same behavior as MSVC when the target is 32-bit (e.g., i686-pc-win32). The size and alignment of T2 and array are the same. If the target isn't 32-bit MSVC, the size is correctly round up to the alignment. Comment Actions Oh, so you mean we need to consider whether we need to avoid emitting an error message when we're dealing with C++ code targeting i686-pc-win32, given we implement the alignment quirk. I think I'd like to try printing an error message, given it's reasonably likely you'll actually end up with a miscompile if you use an array like that. This might cause breakage, but I don't see a good alternative. (We don't try to mitigate the possibility of misaligned operations in clang, so LLVM will see misaligned operations, so LLVM optimizations will likely mess up the code. MSVC gets away with this because it doesn't really do alignment-based optimizations.) For the gcc 10 vs. 11 thing, not sure how many people will notice. The most likely possibility here is that someone tried to align a struct, but put the attribute in the wrong place. Maybe we want a diagnostic note to point out this usage. Comment Actions No, I was thinking perhaps clang has to round up the size to the alignment when the target is 32-bit MSVC too. The code in https://godbolt.org/z/c1EzdYqPc is perfectly valid so I don't think we want to reject it, but we don't want to keep emitting buggy code either. I was just wondering whether we'd run into problems if clang started behaving differently than MSVC (i.e., increase T2's size to 8B from 5B). Comment Actions Messing with the layout is risky. If we give an error, the user will notice something is wrong, and can adapt their code (e.g. explicitly enforcing alignment using alignas). If we silently use a different layout, and the class is used across compilers, the user ends up with a miscompile that's hard to diagnose. Comment Actions I agree, there's no reason to change the i686 MSVC record layout algorithm. It was written, tested, done, it's fragile. We used to have some decent continuous integration testing for ABI issues like this, but as usual these things require care and feeding and it did not survive the passage of time. This patch implements the new GCC behavior, right? Reject arrays when size is not a multiple of alignment? That sounds good to me. Comment Actions This seems like a new error that will be pretty easy to run into, especially in the funky MSVC virtual base case. @aaron.ballman proposed some kind of new breaking change announcement mechanism. Can you add a release note and send an announcement following that process? I think it's just posting to Discourse announcements. Overall this seems like the right thing to do.
Comment Actions I just landed the change to the release notes so there's now a potentially breaking changes section to put the note under. We're not quite ready to post to announcements yet (that channel is locked so we need to remove the gatekeeping mechanism there), but when I'm able to post to announcements, I plan to announce everything already in the release notes anyway. So I'd say adding clang-vendors like @efriedma did and putting in the release notes is great for now. Note, precommit CI found a relevant failure: ******************** TEST 'Clang :: CodeGenCXX/override-layout-packed-base.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/16.0.0/include -nostdsysteminc -triple i686-windows-msvc -w -fdump-record-layouts-simple -foverride-record-layout=/var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGenCXX/Inputs/override-layout-packed-base.layout /var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGenCXX/override-layout-packed-base.cpp | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGenCXX/override-layout-packed-base.cpp
--
Exit Code: 1
Command Output (stderr):
--
/var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGenCXX/override-layout-packed-base.cpp:38:7: error: size of array element of type 'C' (11 bytes) isn't a multiple of its alignment (4 bytes)
C cs[sizeof(C)];
^
/var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGenCXX/override-layout-packed-base.cpp:39:7: error: size of array element of type 'D' (15 bytes) isn't a multiple of its alignment (4 bytes)
D ds[sizeof(D)];
^
2 errors generated.
--
********************Comment Actions
Comment Actions This has caused 2 test suite failures across our bots, for example: https://lab.llvm.org/buildbot/#/builders/174/builds/13518 GCC-C-execute-pr36093.test / GCC-C-execute-pr43783.test Pretty likely the tests are doing something strange, I will look into it. Comment Actions This also fires in non-array cases: $ cat /tmp/a.cc struct T0 { char c; }; struct T2 : virtual T0 { }; T2* f() { return new T2(); } $ /work/llvm-project/build/bin/clang++ -c -target i686-pc-win32 /tmp/a.cc /tmp/a.cc:4:22: error: size of array element of type 'T2' (5 bytes) isn't a multiple of its alignment (4 bytes) T2* f() { return new T2(); } ^ 1 error generated. I don't think we want to error here? Comment Actions Thanks for reporting the bug. Sema::CheckAllocatedType can be called when the allocated type isn't an array, so checkArrayElementAlignment shouldn't be called in that function. Comment Actions Check whether the allocated type is an array type before calling checkArrayElementAlignment. Comment Actions Please try to avoid uploading multiple independent patches into the same revision; it's confusing for anyone who comes looking for the review of the initial patch, and only finds the followup. That said, fix looks fine, sure. | ||||||||||||||||||||||||
I think you need to check that the size is a multiple of the alignment, not just larger than the alignment. (Those are only the same thing if the size is a power of two; it looks like you don't have any test coverage for non-power-of-two sizes.)