[Sema] Fix false positive warnings for misaligned member access

[vvuksanovic]

These warnings are reported on a per expression basis, however some potential misaligned accesses are discarded before that happens. The problem is when a new expression starts while processing another expression. The new expression will end first and emit all potential misaligned accesses collected up to that point. That includes candidates that were found in the parent expression, even though they might have gotten discarded later.

Fixed by checking if the candidate is located withing the currently processed expression.

Fixes #144729

[llvmbot]

@llvm/pr-subscribers-clang

Author: Vladimir Vuksanovic (vvuksanovic)

Changes

These warnings are reported on a per expression basis, however some potential misaligned accesses are discarded before that happens. The problem is when a new expression starts while processing another expression. The new expression will end first and emit all potential misaligned accesses collected up to that point. That includes candidates that were found in the parent expression, even though they might have gotten discarded later.

Fixed by checking if the candidate is located withing the currently processed expression.

Fixes #144729

---

Full diff: https://github.com/llvm/llvm-project/pull/150025.diff

3 Files Affected:

• (modified) clang/include/clang/Sema/Sema.h (+1-1)
• (modified) clang/lib/Sema/SemaChecking.cpp (+19-8)
• (modified) clang/test/Sema/address-packed.c (+8)

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index b331acbe606b7..a2ad749d67259 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -2655,7 +2655,7 @@ class Sema final : public SemaBase {
   /// Diagnoses the current set of gathered accesses. This typically
   /// happens at full expression level. The set is cleared after emitting the
   /// diagnostics.
-  void DiagnoseMisalignedMembers();
+  void DiagnoseMisalignedMembers(const Expr *E);
 
   /// This function checks if the expression is in the sef of potentially
   /// misaligned members and it is converted to some pointer type T with lower
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 5e523fe887318..032cda4bd5d58 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -14086,7 +14086,7 @@ void Sema::CheckCompletedExpr(Expr *E, SourceLocation CheckLoc,
     CheckUnsequencedOperations(E);
   if (!IsConstexpr && !E->isValueDependent())
     CheckForIntOverflow(E);
-  DiagnoseMisalignedMembers();
+  DiagnoseMisalignedMembers(E);
 }
 
 void Sema::CheckBitFieldInitialization(SourceLocation InitLoc,
@@ -15534,17 +15534,28 @@ void Sema::AddPotentialMisalignedMembers(Expr *E, RecordDecl *RD, ValueDecl *MD,
   MisalignedMembers.emplace_back(E, RD, MD, Alignment);
 }
 
-void Sema::DiagnoseMisalignedMembers() {
-  for (MisalignedMember &m : MisalignedMembers) {
-    const NamedDecl *ND = m.RD;
+void Sema::DiagnoseMisalignedMembers(const Expr *E) {
+  // Only emit diagnostics related to the current expression.
+  const auto *EmitFrom = MisalignedMembers.begin();
+  if (E->getSourceRange().isValid()) {
+    EmitFrom = MisalignedMembers.end();
+    for (const auto &m : llvm::reverse(MisalignedMembers)) {
+      if (m.E->getSourceRange().isValid() &&
+          !E->getSourceRange().fullyContains(m.E->getSourceRange()))
+        break;
+      EmitFrom = &m;
+    }
+  }
+  for (const auto *m = EmitFrom; m != MisalignedMembers.end(); ++m) {
+    const NamedDecl *ND = m->RD;
     if (ND->getName().empty()) {
-      if (const TypedefNameDecl *TD = m.RD->getTypedefNameForAnonDecl())
+      if (const TypedefNameDecl *TD = m->RD->getTypedefNameForAnonDecl())
         ND = TD;
     }
-    Diag(m.E->getBeginLoc(), diag::warn_taking_address_of_packed_member)
-        << m.MD << ND << m.E->getSourceRange();
+    Diag(m->E->getBeginLoc(), diag::warn_taking_address_of_packed_member)
+        << m->MD << ND << m->E->getSourceRange();
   }
-  MisalignedMembers.clear();
+  MisalignedMembers.erase(EmitFrom, MisalignedMembers.end());
 }
 
 void Sema::DiscardMisalignedMemberAddress(const Type *T, Expr *E) {
diff --git a/clang/test/Sema/address-packed.c b/clang/test/Sema/address-packed.c
index 29f12490e9fab..f826b7d57d91c 100644
--- a/clang/test/Sema/address-packed.c
+++ b/clang/test/Sema/address-packed.c
@@ -338,3 +338,11 @@ struct Invalid0 {
 void *g14(struct Invalid0 *ivl) {
   return &(ivl->x);
 }
+
+void to_void_with_expr(void *ptr, int expr);
+
+void g15(void) {
+  struct Arguable arguable;
+  to_void_with_expr(&arguable.x, 3); // no-warning
+  to_void_with_expr(&arguable.x, ({3;})); // no-warning
+}

[shafik]

Thank you for the fix, please add a release note.

[erichkeane]

Mostly good to me, I have a couple questions, but thats about it. I don't want to approve, but no longer need a 'request changes'(and github doesn't have a way of clearing it).

[erichkeane]

Minor preference for a better 'alias' name in the 1 section (or none at all), but otherwise this is fine to me. I'm Ok either way.

[erichkeane]

I still approve, LMK if you need someone to click the 'merge' button for you, or if you can merge it yourself.

[vvuksanovic]

Yes, can you merge this? I don't have commit access.

[erichkeane]

Yes, can you merge this? I don't have commit access.

I've set it up to auto-merge once CI passes. Ping me here if CI fails and you think it isn't your fault.

[vvuksanovic]

Had to update to fix conflicts. Can you merge?