lenary added a comment.
This is coming together well.
I think structs `U23` to `U26` in the C test are duplicates - how about
surrounding them all in `#pragma pack(push, 2)` so they're different from the
earlier cases?
Quite a few more comments inline on the tests.
================
Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:596-597
+def warn_unaligned_access : Warning<
+ "field %1 within its parent %0 has an alignment greater than its parent "
+ "this may be caused by %0 being packed and can lead to unaligned accesses">,
InGroup<UnalignedAccess>, DefaultIgnore;
}
----------------
I think the warning text here is misleading, as we are not inspecting the
alignment of the parent, we're inspecting the original alignment of the field's
struct type vs the resulting alignment of the field.
How about this wording, with `%2` standing for the field type's name, which you
will have to give to the Diag() formatter.
Unfortunately, this will mean the wording of the expectations in the tests need
to be updated.
================
Comment at: clang/lib/Driver/ToolChains/Arch/AArch64.cpp:470
Features.push_back("+strict-align");
- } else if (Triple.isOSOpenBSD())
+ CmdArgs.push_back("-Wunaligned-access");
+ }
----------------
Please guard all the `CmdArgs.push_back(…)` with `if (!ForAS)` please -
otherwise you get the assembler warnings that I had to fix last time (the same
goes for these in ARM.cpp)
================
Comment at: clang/test/Sema/test-wunaligned-access.c:167
+
+struct __attribute__((packed)) U23 {
+ char a;
----------------
isn't this the same case as `struct U1` in this file?
================
Comment at: clang/test/Sema/test-wunaligned-access.c:173
+
+struct U24 {
+ char a;
----------------
this looks like the same case as `struct U5`.
================
Comment at: clang/test/Sema/test-wunaligned-access.c:179
+
+struct U25 {
+ char a;
----------------
This looks the same as `struct U8`
================
Comment at: clang/test/Sema/test-wunaligned-access.c:188
+
+struct U26 {
+ char a;
----------------
this looks the same as `struct U16`.
================
Comment at: clang/test/Sema/test-wunaligned-access.c:299
+ char a;
+#pragma pack(push, 4)
+ struct T5 b;
----------------
This had me scratching my head a bit. Did you expect this to apply to the field
itself? Looking at the AST, it doesn't: https://godbolt.org/z/c47KorT3G (note
no MaxAlign attribute on the struct)
================
Comment at: clang/test/Sema/test-wunaligned-access.c:322-341
+#pragma pack(push, 1)
+struct A4 {
+ char a;
+ struct T1 b;
+};
+
+struct A5 {
----------------
These struct definitions are unused.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116221/new/
https://reviews.llvm.org/D116221
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
