[PATCH] D116221: [AArch64][ARM][Clang] Unaligned Access Warning Added

[Sam Elliott via Phabricator via cfe-commits]

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