[AArch64][ARM][Clang] Unaligned Access Warning Added

[mubashar_]

Summary

Added warning for potential cases of unaligned access when option -mno-unaligned-access has been specified.

[pbarrio]

Suggest clarifying the commit message to something like:

"Added warning for potential cases of unaligned access when option -mno-unaligned-access has been specified".

Nice testing :)

clang/lib/Driver/ToolChains/Arch/AArch64.cpp

463–464

Nit: preserve coding style of the file. Move the bracket to the previous line, preceded by a space. I.e.

if (A->getOption().matches(options::OPT_mno_unaligned_access)) {

[mubashar_]

Cleaned up code to make it more clang format friendly. Added another test suite in C.

[lenary]

Two quick comments about when this is enabled, and then I'm happy!

clang/lib/Driver/ToolChains/Arch/ARM.cpp

772–774

I think we should be enabling this warning everywhere we enable +strict-align between lines 772 and 813, even though in some of those cases an explicit argument has not been provided to the compiler to enable or disable unaligned accesses.

This should probably also be the case for aarch64 too!

784–787

I think you want the curly braces here, as the condition on 774 is looking for both -munaligned-access and -mno-unaligned-access, and then line 776 checks which direction the last argument actually went.

[mubashar_]

Enabled warning whenever +strict-align is present.

[lenary]

LGTM! Thanks for your hard work on this!

I think you've addressed @pbarrio's comments on this, and I'm not sure when he's back, so wait 48h before landing this, in case there are further comments.

[lenary]

This caused some driver test failures. They have been fixed in https://reviews.llvm.org/rG11c67e5a4e99f51ec66c9781710f81955cfd5e24

[thakis]

Thanks for the patch!

1. When committing code, please add "Differential Revision: https://reviews.llvm.org/D116221" as last line to your commit description, to link the commit to the review. See git log for many examples.

2. This breaks tests on Windows, see e.g. http://45.33.8.238/win/52066/step_7.txt Please take a look, and revert for now if it takes a while to fix.

[lenary]

Reverted for the moment in rG3aec4b3d348d69e6c1ead7cba54677b855293983 while we fix the windows tests.

[mubashar_]

Windows tests are now expected to fail.

[lenary]

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.

clang/include/clang/Basic/DiagnosticASTKinds.td

596–597

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.

clang/lib/Driver/ToolChains/Arch/AArch64.cpp

468

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)

clang/test/Sema/test-wunaligned-access.c

167 ↗

(On Diff #399271)

isn't this the same case as struct U1 in this file?

173 ↗

(On Diff #399271)

this looks like the same case as struct U5.

179 ↗

(On Diff #399271)

This looks the same as struct U8

188 ↗

(On Diff #399271)

this looks the same as struct U16.

299 ↗

(On Diff #399271)

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)

322–341 ↗

(On Diff #399271)

These struct definitions are unused.

[mubashar_]

Addressed comments.

[mubashar_]

Addressed comments from @lenary.

[lenary]

LGTM. I've looked a lot closer at the tests this time around, and am happy with them, and I think the message is phrased better, and there shouldn't be warnings with -cc1as.

[pbarrio]

LGTM too.

[ychen]

clang/lib/Driver/ToolChains/Arch/AArch64.cpp

221–223

Sorry if it has been discussed, why the compiler option is not added during constructing the clang job but here? All kinds of clients could call this method, only clang care about this option.

clang/include/clang/Basic/DiagnosticASTKinds.td

def warn_padded_struct_anon_field : Warning<
   "padding %select{struct|interface|class}0 %1 with %2 "
   "%select{byte|bit}3%s2 to align anonymous bit-field">,
   InGroup<Padded>, DefaultIgnore;
 def warn_padded_struct_size : Warning<
   "padding size of %0 with %1 %select{byte|bit}2%s1 to alignment boundary">,
   InGroup<Padded>, DefaultIgnore;
 def warn_unnecessary_packed : Warning<
   "packed attribute is unnecessary for %0">, InGroup<Packed>, DefaultIgnore;
+
+// -Wunaligned-access
+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;
 }

clang/include/clang/Basic/DiagnosticGroups.td

 def CalledOnceParameter : DiagGroup<"called-once-parameter", [CompletionHandler]>;
 def OpenCLUnsupportedRGBA: DiagGroup<"opencl-unsupported-rgba">;
 def UnderalignedExceptionObject : DiagGroup<"underaligned-exception-object">;
 def DeprecatedObjCIsaUsage : DiagGroup<"deprecated-objc-isa-usage">;
 def ExplicitInitializeCall : DiagGroup<"explicit-initialize-call">;
 def OrderedCompareFunctionPointers : DiagGroup<"ordered-compare-function-pointers">;
 def Packed : DiagGroup<"packed">;
 def Padded : DiagGroup<"padded">;
+def UnalignedAccess : DiagGroup<"unaligned-access">;
 
 def PessimizingMove : DiagGroup<"pessimizing-move">;
 def ReturnStdMove : DiagGroup<"return-std-move">;
 
 def PointerArith : DiagGroup<"pointer-arith">;
 def PoundWarning : DiagGroup<"#warnings">;
 def PoundPragmaMessage : DiagGroup<"#pragma-messages">,
                          DiagCategory<"#pragma message Directive">;

clang/lib/AST/RecordLayoutBuilder.cpp

if (DefaultsToAIXPowerAlignment && !alignedAttrCanDecreaseAIXAlignment() &&
     }
   }
 
   // The align if the field is not packed. This is to check if the attribute
   // was unnecessary (-Wpacked).
   CharUnits UnpackedFieldAlign =
       !DefaultsToAIXPowerAlignment ? FieldAlign : PreferredAlign;
   CharUnits UnpackedFieldOffset = FieldOffset;
+  CharUnits OriginalFieldAlign = UnpackedFieldAlign;
 
   if (FieldPacked) {
     FieldAlign = CharUnits::One();
     PreferredAlign = CharUnits::One();
   }
   CharUnits MaxAlignmentInChars =
       Context.toCharUnitsFromBits(D->getMaxAlignment());
   FieldAlign = std::max(FieldAlign, MaxAlignmentInChars);
void ItaniumRecordLayoutBuilder::LayoutField(const FieldDecl *D,
   } else {
     setSize(std::max(getSizeInBits(),
                      (uint64_t)Context.toBits(FieldOffset + FieldSize)));
   }
 
   // Remember max struct/class ABI-specified alignment.
   UnadjustedAlignment = std::max(UnadjustedAlignment, FieldAlign);
   UpdateAlignment(FieldAlign, UnpackedFieldAlign, PreferredAlign);
+
+  // For checking the alignment of inner fields against
+  // the alignment of its parent record.
+  if (const RecordDecl* RD = D->getParent())
+  {
+    // Check if packed attribute or pragma pack is present.
+    if (RD->hasAttr<PackedAttr>() || !MaxFieldAlignment.isZero())
+      if (FieldAlign < OriginalFieldAlign)
+        if (D->getType()->isRecordType())
+        {
+          // If the offset is a multiple of the alignment of
+          // the type, raise the warning.
+          // TODO: Takes no account the alignment of the outer struct
+          if (FieldOffset % OriginalFieldAlign != 0)
+            Diag(D->getLocation(), diag::warn_unaligned_access)
+              << Context.getTypeDeclType(RD)
+              << D->getName();
+        }
+  }
 }
 
 void ItaniumRecordLayoutBuilder::FinishLayout(const NamedDecl *D) {
   // In C++, records cannot be of size 0.
   if (Context.getLangOpts().CPlusPlus && getSizeInBits() == 0) {
     if (const CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(D)) {
       // Compatibility with gcc requires a class (pod or non-pod)
       // which is not empty but of size 0; such as having fields of

clang/lib/Driver/ToolChains/Arch/AArch64.h

 
 namespace clang {
 namespace driver {
 namespace tools {
 namespace aarch64 {
 
 void getAArch64TargetFeatures(const Driver &D, const llvm::Triple &Triple,
                               const llvm::opt::ArgList &Args,
+                              llvm::opt::ArgStringList &CmdArgs,
                               std::vector<llvm::StringRef> &Features,
                               bool ForAS);
 
 std::string getAArch64TargetCPU(const llvm::opt::ArgList &Args,
                                 const llvm::Triple &Triple, llvm::opt::Arg *&A);
 
 } // end namespace aarch64
 } // end namespace target
 } // end namespace driver
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_ARCH_AARCH64_H

clang/lib/Driver/ToolChains/Arch/AArch64.cpp

getAArch64MicroArchFeaturesFromMcpu(const Driver &D, StringRef Mcpu,
   if (!DecodeAArch64Mcpu(D, McpuLowerCase, CPU, DecodedFeature))
     return false;
 
   return getAArch64MicroArchFeaturesFromMtune(D, CPU, Args, Features);
 }
 
 void aarch64::getAArch64TargetFeatures(const Driver &D,
                                        const llvm::Triple &Triple,
                                        const ArgList &Args,
+                                       llvm::opt::ArgStringList &CmdArgs,
                                        std::vector<StringRef> &Features,
                                        bool ForAS) {
   Arg *A;
   bool success = true;
   // Enable NEON by default.
   Features.push_back("+neon");
   llvm::StringRef WaMArch;
   if (ForAS)
     for (const auto *A :
fp16_fml_fallthrough:
   const char *Archs[] = {"+v8.6a", "+v8.7a", "+v9.1a", "+v9.2a"};
   auto Pos = std::find_first_of(Features.begin(), Features.end(),
                                 std::begin(Archs), std::end(Archs));
   if (Pos != std::end(Features))
     Pos = Features.insert(std::next(Pos), {"+i8mm", "+bf16"});
 
   if (Arg *A = Args.getLastArg(options::OPT_mno_unaligned_access,
                                options::OPT_munaligned_access)) {
     if (A->getOption().matches(options::OPT_mno_unaligned_access))
+    {
       Features.push_back("+strict-align");
+      CmdArgs.push_back("-Wunaligned-access");
+    }
   } else if (Triple.isOSOpenBSD())
     Features.push_back("+strict-align");
 
   if (Args.hasArg(options::OPT_ffixed_x1))
     Features.push_back("+reserve-x1");
 
   if (Args.hasArg(options::OPT_ffixed_x2))
     Features.push_back("+reserve-x2");
 

clang/lib/Driver/ToolChains/Arch/ARM.cpp

if (Arg *A = Args.getLastArg(options::OPT_mexecute_only, options::OPT_mno_execute_only)) {
             D.Diag(diag::err_opt_not_valid_with_opt) << A->getAsString(Args) << B->getAsString(Args);
         }
         Features.push_back("+execute-only");
       }
     }
   }
 
   // Kernel code has more strict alignment requirements.
   if (KernelOrKext)
     Features.push_back("+strict-align");
   else if (Arg *A = Args.getLastArg(options::OPT_mno_unaligned_access,
                                     options::OPT_munaligned_access)) {
     if (A->getOption().matches(options::OPT_munaligned_access)) {
       // No v6M core supports unaligned memory access (v6M ARM ARM A3.2).
       if (Triple.getSubArch() == llvm::Triple::SubArchType::ARMSubArch_v6m)
         D.Diag(diag::err_target_unsupported_unaligned) << "v6m";
       // v8M Baseline follows on from v6M, so doesn't support unaligned memory
       // access either.
       else if (Triple.getSubArch() == llvm::Triple::SubArchType::ARMSubArch_v8m_baseline)
         D.Diag(diag::err_target_unsupported_unaligned) << "v8m.base";
     } else
       Features.push_back("+strict-align");
+      CmdArgs.push_back("-Wunaligned-access");
   } else {
     // Assume pre-ARMv6 doesn't support unaligned accesses.
     //
     // ARMv6 may or may not support unaligned accesses depending on the
     // SCTLR.U bit, which is architecture-specific. We assume ARMv6
     // Darwin and NetBSD targets support unaligned accesses, and others don't.
     //
     // ARMv7 always has SCTLR.U set to 1, but it has a new SCTLR.A bit
     // which raises an alignment fault on unaligned accesses. Linux

clang/lib/Driver/ToolChains/Clang.cpp

case llvm::Triple::riscv64:
     riscv::getRISCVTargetFeatures(D, Triple, Args, Features);
     break;
   case llvm::Triple::systemz:
     systemz::getSystemZTargetFeatures(D, Args, Features);
     break;
   case llvm::Triple::aarch64:
   case llvm::Triple::aarch64_32:
   case llvm::Triple::aarch64_be:
-    aarch64::getAArch64TargetFeatures(D, Triple, Args, Features, ForAS);
+    aarch64::getAArch64TargetFeatures(D, Triple, Args, CmdArgs, Features, ForAS);
     break;
   case llvm::Triple::x86:
   case llvm::Triple::x86_64:
     x86::getX86TargetFeatures(D, Triple, Args, Features);
     break;
   case llvm::Triple::hexagon:
     hexagon::getHexagonTargetFeatures(D, Args, Features);
     break;

clang/test/Sema/test-wunaligned-access.cpp

+// RUN: %clang_cc1 %s -verify -fsyntax-only -Wunaligned-access
+
+// Packed-Unpacked Tests (No Pragma)
+
+struct T1
+{
+    char a;
+    int b;
+};
+
+struct __attribute__((packed)) U1 // Causes warning
+{
+    char a;
+    T1 b; // expected-warning {{field b within its parent 'U1' has an alignment greater than its parent this may be caused by 'U1' being packed and can lead to unaligned accesses}}
+    int c;
+};
+
+struct __attribute__((packed)) U2 // No warning
+{
+    char a;
+    T1 b __attribute__((aligned(4)));
+    int c;
+};
+
+struct __attribute__((packed)) U3 // No warning
+{
+    char a;
+    char b;
+    short c;
+    T1 d;
+};
+
+struct __attribute__((packed)) U4 // No warning
+{
+    T1 a;
+    int b;
+};
+
+struct __attribute__((aligned(4), packed)) U5 // Causes warning
+{
+    char a;
+    T1 b; // expected-warning {{field b within its parent 'U5' has an alignment greater than its parent this may be caused by 'U5' being packed and can lead to unaligned accesses}}
+    int c;
+};
+
+struct __attribute__((aligned(4), packed)) U6 // No warning
+{
+    char a;
+    char b;
+    short c;
+    T1 d;
+};
+
+// Packed-Unpacked Tests with Pragma
+
+#pragma pack(push, 1)
+
+struct __attribute__((packed)) U7 // Causes warning
+{
+    char a;
+    T1 b; // expected-warning {{field b within its parent 'U7' has an alignment greater than its parent this may be caused by 'U7' being packed and can lead to unaligned accesses}}
+    int c;
+};
+
+struct __attribute__((packed)) U8
+{
+    char a;
+    T1 b __attribute__((aligned(4))); // expected-warning {{field b within its parent 'U8' has an alignment greater than its parent this may be caused by 'U8' being packed and can lead to unaligned accesses}}
+    int c;
+};
+
+struct __attribute__((aligned(4))) U9
+{
+    char a;
+    T1 b; // expected-warning {{field b within its parent 'U9' has an alignment greater than its parent this may be caused by 'U9' being packed and can lead to unaligned accesses}}
+    int c;
+};
+
+struct U10
+{
+    char a;
+    T1 b; // expected-warning {{field b within its parent 'U10' has an alignment greater than its parent this may be caused by 'U10' being packed and can lead to unaligned accesses}}
+    int c;
+};
+
+#pragma pack(pop)
+
+// Packed-Packed Tests
+
+struct __attribute__((packed)) T2
+{
+    char a;
+    int b;
+};
+
+struct __attribute__((packed)) U11
+{
+    char a;
+    T2 b;
+    int c;
+};
+
+#pragma pack(push, 1)
+struct U12 // No warning
+{
+    char a;
+    T2 b;
+    int c;
+};
+#pragma pack(pop)
+
+// Unpacked-Packed Tests
+
+struct U13 // No warning
+{
+    char a;
+    T2 b;
+    int c;
+};
+
+struct U14 // No warning
+{
+    char a;
+    T2 b __attribute__((aligned(4)));
+    int c;
+};
+
+// Unpacked-Unpacked Test
+
+struct T3
+{
+    char a;
+    int b;
+};
+
+struct U15 // No warning
+{
+    char a;
+    T3 b;
+    int c;
+};
+
+// Packed-Packed-Unpacked Test (No pragma)
+
+struct __attribute__((packed)) A1
+{
+    char a;
+    T1 b; // expected-warning {{field b within its parent 'A1' has an alignment greater than its parent this may be caused by 'A1' being packed and can lead to unaligned accesses}}
+};
+
+struct __attribute__((packed)) U16 // No warning
+{
+    char a;
+    A1 b;
+    int c;
+};
+
+struct __attribute__((packed)) A2 // No warning
+{
+    char a;
+    T1 b __attribute__((aligned(4)));
+};
+
+struct __attribute__((packed)) U17 // Caused warning
+{
+    char a;
+    A2 b; // expected-warning {{field b within its parent 'U17' has an alignment greater than its parent this may be caused by 'U17' being packed and can lead to unaligned accesses}}
+    int c;
+};
+
+// Packed-Unpacked-Packed tests
+
+struct A3
+{
+    char a;
+    T2 b;
+};
+
+struct __attribute__((packed)) U18
+{
+    char a;
+    A3 b;
+    int c;
+};
+
+struct A4
+{
+    char a;
+    T2 b;
+    int c;
+};
+
+#pragma pack(push, 1)
+struct U19 // Caused warning
+{
+    char a;
+    A4 b; // expected-warning {{field b within its parent 'U19' has an alignment greater than its parent this may be caused by 'U19' being packed and can lead to unaligned accesses}}
+    int c;
+};
+#pragma pack(pop)
+
+// Packed-Unpacked-Unpacked tests
+
+struct A5
+{
+    char a;
+    T1 b;
+};
+
+struct __attribute__((packed)) U20 // Caused warning
+{
+    char a;
+    A5 b; // expected-warning {{field b within its parent 'U20' has an alignment greater than its parent this may be caused by 'U20' being packed and can lead to unaligned accesses}}
+    int c;
+};
+
+struct A6
+{
+    char a;
+    T1 b;
+};
+
+#pragma pack(push, 1)
+struct U21 // Caused warning
+{
+    char a;
+    A6 b; // expected-warning {{field b within its parent 'U21' has an alignment greater than its parent this may be caused by 'U21' being packed and can lead to unaligned accesses}}
+    int c;
+};
+#pragma pack(pop)
+
+// Unpacked-Packed-Packed test
+
+struct __attribute__((packed)) A7 // No warning
+{
+    char a;
+    T2 b;
+};
+
+struct U22 // No warning
+{
+    char a;
+    A7 b;
+    int c;
+};
+
+// Unpacked-Packed-Unpacked tests
+
+struct __attribute__((packed)) A8 // Should cause warning
+{
+    char a;
+    T1 b; // expected-warning {{field b within its parent 'A8' has an alignment greater than its parent this may be caused by 'A8' being packed and can lead to unaligned accesses}}
+};
+
+struct U23 // No warning
+{
+    char a;
+    A8 b;
+    int c;
+};
+
+struct __attribute__((packed)) A9 // No warning
+{
+    char a;
+    T1 b __attribute__((aligned(4)));
+};
+
+struct U24 // No warning
+{
+    char a;
+    A9 b;
+    int c;
+};
+
+struct U1 s1;
+struct U2 s2;
+struct U3 s3;
+struct U4 s4;
+struct U5 s5;
+struct U6 s6;
+struct U7 s7;
+struct U8 s8;
+struct U9 s9;
+struct U10 s10;
+struct U11 s11;
+struct U12 s12;
+struct U13 s13;
+struct U14 s14;
+struct U15 s15;
+struct U16 s16;
+struct U17 s17;
+struct U18 s18;
+struct U19 s19;
+struct U20 s20;
+struct U21 s21;
+struct U22 s22;
+struct U23 s23;
+struct U24 s24;