[PATCH] D118350: [Clang][Sema][AIX][PowerPC] Emit byval alignment warning only when struct member is passed to a function

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:5212-5215
+  const Expr *AArg = Arg->IgnoreParens();
+  if (const auto *ICE = dyn_cast<ImplicitCastExpr>(AArg)) {
+    AArg = ICE->getSubExpr();
+    if (const auto *ME = dyn_cast<MemberExpr>(AArg)) {
----------------
I don't see a need for `AArg`, so I'd sink it into the uses instead.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:5220-5221
+              Context.toCharUnitsFromBits(AA->getAlignment(Context));
+          if (Alignment.getQuantity() >= 16)
+            Diag(Loc, diag::warn_not_xl_compatible) << FD;
+        }
----------------
I think it'd probably be helpful to tell the user which alignment was calculated (it may not be obvious from the context because the alignment could be hidden behind a macro or something).


================
Comment at: clang/lib/Sema/SemaChecking.cpp:5341-5342
+        if (Context.getTargetInfo().getTriple().isOSAIX() && Arg &&
+            (FDecl->hasLinkage()) &&
+            !(FDecl->getFormalLinkage() == InternalLinkage))
+          checkAIXMemberAlignment((Arg->getExprLoc()), FDecl,
----------------



================
Comment at: clang/test/Sema/aix-attr-align.c:34-35
+  baz(s.a); // no-warning
+  baz(s.b); // expected-warning {{requesting an alignment of 16 bytes or greater for struct members is not binary compatible with IBM XL C/C++ for AIX 16.1.0 and older}}
+  baz(s.c); // expected-warning {{requesting an alignment of 16 bytes or greater for struct members is not binary compatible with IBM XL C/C++ for AIX 16.1.0 and older}}
+
----------------
This diagnostic is a bit odd to me. It says there's a request for alignment, but there's no such request on this line. So it's not clear how the user is supposed to react to the diagnostic. While the current code makes it somewhat obvious because there's only one field in the expression, imagine code like `quux(s.a, s.b);` where it's less clear as to which field causes the diagnostic from looking at the call site.

Personally, I found the old diagnostics to be more clear as to what the issue is. I think we should put the warning on the declaration involving the alignment request, and we should add a note to the call site where the diagnostic is generated from (or vice versa). WDYT?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118350/new/

https://reviews.llvm.org/D118350