[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/include/clang/Sema/Sema.h:12693-12695
   void CheckArgAlignment(SourceLocation Loc, NamedDecl *FDecl,
-                         StringRef ParamName, QualType ArgTy, QualType ParamTy);
+                         StringRef ParamName, QualType ArgTy, QualType ParamTy,
+                         const Expr *Arg = nullptr);
----------------
I'm not keen on passing both `Arg` and `ArgTy` such that they can get out of sync. Do all of the places calling `CheckArgAlignment()` have access to the `Expr` so that we can require it be passed (and drop the `ArgTy` parameter)?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:5218
+  if (Context.getTargetInfo().getTriple().isOSAIX() && Arg) {
+    if (Arg->IgnoreParens()) {
+      // Using AArg so as to not modify Arg for the rest of the function.
----------------
There's no case where `IgnoreParens()` will return null.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:5220
+      // Using AArg so as to not modify Arg for the rest of the function.
+      const Expr *AArg = Arg->IgnoreParens();
+      if (AArg->getStmtClass() == Stmt::ImplicitCastExprClass) {
----------------
Are there other things you want to ignore here (such as `IgnoreParenNoopCasts()`)? (I don't have an opinion the current code is wrong, just checking if those sort of cases were considered or not.)


================
Comment at: clang/lib/Sema/SemaChecking.cpp:5221-5222
+      const Expr *AArg = Arg->IgnoreParens();
+      if (AArg->getStmtClass() == Stmt::ImplicitCastExprClass) {
+        const ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(AArg);
+        AArg = ICE->getSubExpr();
----------------
This achieves the same thing, but with less work.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:5224-5225
+        AArg = ICE->getSubExpr();
+        if (AArg->getStmtClass() == Stmt::MemberExprClass) {
+          const auto *ME = dyn_cast<MemberExpr>(AArg);
+          ValueDecl *MD = ME->getMemberDecl();
----------------



================
Comment at: clang/lib/Sema/SemaChecking.cpp:5226-5228
+          ValueDecl *MD = ME->getMemberDecl();
+          auto *FD = dyn_cast<FieldDecl>(MD);
+          if (FD) {
----------------



================
Comment at: clang/lib/Sema/SemaChecking.cpp:5229-5230
+          if (FD) {
+            if (FD->hasAttr<AlignedAttr>()) {
+              auto *AA = FD->getAttr<AlignedAttr>();
+              unsigned Aligned = AA->getAlignment(Context);
----------------



================
Comment at: clang/lib/Sema/SemaChecking.cpp:5232-5233
+              unsigned Aligned = AA->getAlignment(Context);
+              // Divide by 8 to get the bytes instead of using bits.
+              if (Aligned / 8 >= 16)
+                Diag(Loc, diag::warn_not_xl_compatible) << FD;
----------------
Should we be using char bits rather than a hardcoded value?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118350