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

[ZarkoCA]

Summary

Previous warning went on whenever a struct with a struct member with alignment => 16
was declared. This led to too many false positives and led to diagnostic lit failures
due to it being emitted too frequently. Only emit the warning when such a struct and
that struct contains a member that has an alignment of 16 bytes is passed to a caller
function since this is where the potential binary compatibility issue with XL 16.1.0
and older exists.

[ZarkoCA]

• Fixed formatting
• Fixed test case
• Check for when Arg is nullptr

[daltenty]

clang/lib/Sema/SemaChecking.cpp

5203–5204

This function definitely seems like the right place for this check. Maybe let's update the comment here to reflect we are checking more than the original alignment property this describes.

clang/test/Sema/aix-attr-align.c

4

Why remove all the -Wno-aix-compat flag cases?

[ZarkoCA]

Addressing comments:

• Change comment before checkArgAlignment()
• Add missing RUN lines

[aaron.ballman]

clang/include/clang/Sema/Sema.h

12693–12696

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)?

clang/lib/Sema/SemaChecking.cpp

5219

There's no case where IgnoreParens() will return null.

5221

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.)

5222–5223

This achieves the same thing, but with less work.

5225–5226

5227–5229

5230–5231

5233–5234

Should we be using char bits rather than a hardcoded value?

[ZarkoCA]

clang/include/clang/Sema/Sema.h

12693–12696

Thanks, that is something I overlooked.

It seems like I can do this everywhere except the call from Sema::CheckConstructorCall. Trying to figure out whether it's something I'm missing.

[ZarkoCA]

Addressed some of the comments:

• Applied code cleanup per suggestions
• Used CharUnits instead of hard coded values

[ZarkoCA]

clang/include/clang/Sema/Sema.h

12693–12696

Thanks for the through review, I think I addressed everything but this comment. I agree with your concern about having Arg and ArgTy getting out of sync. I need to spend more time on that particular call from Sema::CheckConstructorCall and see what can be done.

clang/lib/Sema/SemaChecking.cpp

5221

Yes, thanks. I do have suspicions that IgnoreParens() may be too broad but I am worried that we may miss cases to diagnose otherwise.

[nemanjai]

Would it make sense (and would it be possible) to check the linkage of the callee? Presumably calling something like
static void localfunc(int *)
with an over-aligned member shouldn't be a problem.

[ZarkoCA]

• Warning shouldn't apply to functions that have internal linkage

[ZarkoCA]

In D118350#3282855, @nemanjai wrote:

Would it make sense (and would it be possible) to check the linkage of the callee? Presumably calling something like
static void localfunc(int *)
with an over-aligned member shouldn't be a problem.

That's a really good point. It wouldn't make sense for it to apply there, thanks!

[ZarkoCA]

• Moved AIX check to its own function to hopefully avoid Arg and ArgTy getting out of sync
• Rebased and removed LIT test cases workaround

[ZarkoCA]

clang/include/clang/Sema/Sema.h

12693–12696

@aaron.ballman I moved the check to its own function and only pass Expr *Arg to it. I think this should avoid them getting out of sync.

[aaron.ballman]

clang/lib/Sema/SemaChecking.cpp

5212–5215

I don't see a need for AArg, so I'd sink it into the uses instead.

5220–5221

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).

5348–5349

clang/test/Sema/aix-attr-align.c

34–35

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?

[ZarkoCA]

• Add note for diagnostic pointing to declaration of the struct member
• cleaned up use of unneeded variable

[ZarkoCA]

clang/lib/Sema/SemaChecking.cpp

5220–5221

I tried to address in slightly modifying the warning message to emit the offending alignment and also adding a note for the declaration as you suggested elsewhere.

clang/test/Sema/aix-attr-align.c

34–35

That's a good point actually, there's nothing on the line that would be obvious to a user.

I opted to warn at the use of struct member and to make a note where it was declared. This will hopefully help with determining which struct member is causing this warning instead of searching the code for its cause. I have a slight preference for doing it this way instead of the other way but can change it if preferred.

[aaron.ballman]

clang/test/Sema/aix-attr-align.c

34–35

I'd like to understand why you have a preference for this way around.

The use is the point in time at which we know there's a problem, so I definitely agree with waiting until the struct member is used to diagnose anything.

But, to my thinking, the use is not the cause of the issue; the declaration is what's problematic. With that perspective, it seems like we want the warning and the note the other way around: warn about the structure member declaration being the issue, and note where the use that triggered the complaint about the declaration. Then the warning diagnostic is associated most closely with the code that needs to be adjusted by the user in order to silence the warning. This also makes it easier for the user to silence the warning with pragmas (you can put the suppression mechanism in one place, at the declaration site, instead of sprinkling it all over at the use sites).

[ZarkoCA]

• Warn on declaration of struct member and add note for call

[ZarkoCA]

clang/test/Sema/aix-attr-align.c

34–35

Then the warning diagnostic is associated most closely with the code that needs to be adjusted by the user in order to silence the warning.

Thanks, that's a good point and your additional argument about silencing it with pragmas at a single place has convinced me to switch it up.

[aaron.ballman]

Thanks! I think we're pretty close, just some wording nits with the diagnostics.

clang/include/clang/Basic/DiagnosticSemaKinds.td

3299–3301 ↗

(On Diff #406822)

How about this slight rewording from the old form? (Have to re-flow to 80 cols.)

I had previously suggested adding the requested alignment before but now that we're closely tying the diagnostic to the structure member, I think this form is okay (and it's shorter, which is what I was hoping to accomplish).

clang/lib/Sema/SemaChecking.cpp

5221

I don't think this note is the correct one to use (it looks weird in the test cases). I think you'll want to add a new note along the lines of:

def note_misaligned_member_used_here : Note<
  "%0 used with potentially incompatible alignment here">;

And you should pass in FD rather than FD->getDeclName() (the diagnostics engine knows how to print the names of NamedDecl subclasses and has special logic for that.

[ZarkoCA]

• Shorten warning message
• Add new note

[ZarkoCA]

clang/include/clang/Basic/DiagnosticSemaKinds.td

3299–3301 ↗

(On Diff #406822)

Yes, I cringed a bit seeing it at 3 lines. Thanks.

clang/lib/Sema/SemaChecking.cpp

5221

Ah yes, this seems much clearer to the user now IMO, thanks

[aaron.ballman]

LGTM, thank you!

[ZarkoCA]

After offline discussion with @sfertile , the warning should occur only when the struct member is passed on the stack. I will be updating the patch shortly.

@aaron.ballman Thank you for the thorough reviews comments and sorry for the churn.

[sfertile]

Hey Zarko. I really like how we only issue the warning when the struct is used, however I think this is still overly broad because of where the incompatibility lies between xlc and clang. I believe the layout of the structures will be the same for both compilers, and globals of this type will have the same alignment restrictions. A function like void baz(int *); will be compatible between the 2 compilers since the argument is just a pointer. The difference occurs when passing the structure by value on the stack, where xlc doesn't align the struct to the expected alignment, and clang/llvm does. Since the incompatibility is in the calling convention only when the struct is passed byval, that should be the only time we emit the diagnostic.

There are 2 other things we should verify since I'm not sure if they are compatible or not:

1. When passing these structures byval but in argument passing registers, I'm guessing that xlc doesn't skip registers whose image in the PSA is under-aligned while llvm will.
2. Whether xlc passes these misaligned when its done through va_args.

The answers to those 2 questions will determine if we need to warn for all byval arguments that have alignments from attribute that is greater than 8, or if we can limit it a bit further.

[ZarkoCA]

Reworked implementation so the diagnosis is more correct:

• Warn when the struct is passed that contains the member
• Warn when alignment is 16 and not 16 and greater

[sfertile]

clang/lib/Sema/SemaChecking.cpp

5212

Nit: To avoid the deep nesting, can we instead have a series of casts, and if the resulting pointer is null we return early?

clang/test/Sema/aix-attr-align.c

10

As far as I am aware, the layout of a 16-byte aligned member is exactly the same between the all the compilers on AIX, and the only incompatibility is when passing them byval. If that's true then warning on the declaration here is much too verbose, as most uses will be fine and warning on the callsite (and later on the function definition) calls attention to it only when there is indeed the possibility of an incompatability.

35

I get the following diagnostic from the compiler now:

baz(p1, p2, s.b, p3, b, p5, s);        // expected-note {{'b' used with potentially incompatible alignment here}}
                            ^

I can't seem to get the formatting exact, but the caret is pointing to 's' which is the correct argument to warn on, but the note uses the name 'b'. Before adjusting the warning using the member name made sense, but I think now that we are warning on the byval argument, we should be using s, and also reword the note to something like
passing byval argument 's' which is 16 byte aligned may be incompatible with IBM XL C/C++ for AIX 16.1.0 or older

[ZarkoCA]

• Try to remove deeply nested ifs by using early returns
• Fix note so it refers to struct
• Add a struct that isn't called byval in test case to make it clearer that

warning isn't emitted on every declaration

[ZarkoCA]

• Remove formatting changes to unrelated code

[ZarkoCA]

clang/lib/Sema/SemaChecking.cpp

5212

Reworked it to use a few early returns. Does that work better?

clang/test/Sema/aix-attr-align.c

10

The way this test case was written it made it seem as if it would warn on any such declaration. I added struct R here where it has a member aligned 16 but it's not passed byval anywhere and no warning is expected. I also added a comment to make it clearer.

[sfertile]

Thanks for the updates Zarko. I think we are almost there.

clang/lib/Sema/SemaChecking.cpp

5208

Real minor nit: Missing space after period.

5221

I find these longer lines hard to read. I way you had the original checks structured was good, my suggestion was really meant to change only the nesting. I think keeping the same format you would have ended up with something that looks like

const auto *ICE = dyn_cast<ImplicitCastExpr>(Arg->IgnoreParens());
if (!ICE)
  return;

const auto *DR = dyn_cast<DeclRefExpr>(ICE->getSubExpr());
if (!DR)
  return;

auto *PD = dyn_cast<ParmVarDecl>(DR->getDecl();
if (!PD || !PD->getType()->isRecordType())
  return;

[ZarkoCA]

• Restructured checks
• fixed comment
• removed unnecessary parameter passed to function
• slightly reworded summary of patch

[ZarkoCA]

clang/lib/Sema/SemaChecking.cpp

5221

I agree I find it easier to read this way as well. Thanks for clearing that up.

[sfertile]

LGTM Zarko, sorry about taking so long to review this.

[ZarkoCA]

• Rebase and add FDecl check before calling checkAIXMemberAlignment()

clang/include/clang/Sema/Sema.h

bool CheckObjCMethodCall(ObjCMethodDecl *Method, SourceLocation loc,
                            ArrayRef<const Expr *> Args);
   bool CheckPointerCall(NamedDecl *NDecl, CallExpr *TheCall,
                         const FunctionProtoType *Proto);
   bool CheckOtherCall(CallExpr *TheCall, const FunctionProtoType *Proto);
   void CheckConstructorCall(FunctionDecl *FDecl, QualType ThisType,
                             ArrayRef<const Expr *> Args,
                             const FunctionProtoType *Proto, SourceLocation Loc);
 
   void CheckArgAlignment(SourceLocation Loc, NamedDecl *FDecl,
-                         StringRef ParamName, QualType ArgTy, QualType ParamTy);
+                         StringRef ParamName, QualType ArgTy, QualType ParamTy,
+                         const Expr *Arg = nullptr);
 
   void checkCall(NamedDecl *FDecl, const FunctionProtoType *Proto,
                  const Expr *ThisArg, ArrayRef<const Expr *> Args,
                  bool IsMemberFunction, SourceLocation Loc, SourceRange Range,
                  VariadicCallType CallType);
 
   bool CheckObjCString(Expr *Arg);
   ExprResult CheckOSLogFormatStringArg(Expr *Arg);
 

clang/lib/Sema/SemaChecking.cpp

static void CheckNonNullArguments(Sema &S,
 
   // Check for non-null arguments.
   for (unsigned ArgIndex = 0, ArgIndexEnd = NonNullArgs.size();
        ArgIndex != ArgIndexEnd; ++ArgIndex) {
     if (NonNullArgs[ArgIndex])
       CheckNonNullArgument(S, Args[ArgIndex], CallSiteLoc);
   }
 }
 
-/// Warn if a pointer or reference argument passed to a function points to an
-/// object that is less aligned than the parameter. This can happen when
+/// Check for problematic alignment properties of an argument. For example,
+/// warn if a pointer or reference argument passed to a function points to
+/// an object that is less aligned than the parameter. This can happen when
 /// creating a typedef with a lower alignment than the original type and then
 /// calling functions defined in terms of the original type.
 void Sema::CheckArgAlignment(SourceLocation Loc, NamedDecl *FDecl,
                              StringRef ParamName, QualType ArgTy,
-                             QualType ParamTy) {
+                             QualType ParamTy, const Expr *Arg) {
+
+  // 16 byte ByVal alignment not due to a vector member is not honoured by XL
+  // on AIX. Emit a warning here that users are generating binary incompatible
+  // code to be safe.
+  // Here we try to get information about the alignment of the struct member
+  // argument being passed to the caller function.
+  if (Context.getTargetInfo().getTriple().isOSAIX() && Arg) {
+    if (Arg->IgnoreParens()) {
+      // Using AArg so as to not modify Arg for the rest of the function.
+      const Expr *AArg = Arg->IgnoreParens();
+      if (AArg->getStmtClass() == Stmt::ImplicitCastExprClass) {
+        const ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(AArg);
+        AArg = ICE->getSubExpr();
+        if (AArg->getStmtClass() == Stmt::MemberExprClass) {
+          const auto *ME = dyn_cast<MemberExpr>(AArg);
+          ValueDecl *MD = ME->getMemberDecl();
+          auto *FD = dyn_cast<FieldDecl>(MD);
+          if (FD) {
+            if (FD->hasAttr<AlignedAttr>()) {
+              auto *AA = FD->getAttr<AlignedAttr>();
+              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;
+            }
+          }
+        }
+      }
+    }
+  }
 
   // If a function accepts a pointer or reference type
   if (!ParamTy->isPointerType() && !ParamTy->isReferenceType())
     return;
 
   // If the parameter is a pointer type, get the pointee type for the
   // argument too. If the parameter is a reference type, don't try to get
   // the pointee type for the argument.
for (unsigned ArgIdx = 0; ArgIdx < N; ++ArgIdx) {
       // Args[ArgIdx] can be null in malformed code.
       if (const Expr *Arg = Args[ArgIdx]) {
         if (Arg->containsErrors())
           continue;
 
         QualType ParamTy = Proto->getParamType(ArgIdx);
         QualType ArgTy = Arg->getType();
         CheckArgAlignment(Arg->getExprLoc(), FDecl, std::to_string(ArgIdx + 1),
-                          ArgTy, ParamTy);
+                          ArgTy, ParamTy, Arg);
       }
     }
   }
 
   if (FDecl && FDecl->hasAttr<AllocAlignAttr>()) {
     auto *AA = FDecl->getAttr<AllocAlignAttr>();
     const Expr *Arg = Args[AA->getParamIndex().getASTIndex()];
     if (!Arg->isValueDependent()) {
       Expr::EvalResult Align;
void Sema::CheckConstructorCall(FunctionDecl *FDecl, QualType ThisType,
                                 ArrayRef<const Expr *> Args,
                                 const FunctionProtoType *Proto,
                                 SourceLocation Loc) {
   VariadicCallType CallType =
       Proto->isVariadic() ? VariadicConstructor : VariadicDoesNotApply;
 
   auto *Ctor = cast<CXXConstructorDecl>(FDecl);
   CheckArgAlignment(Loc, FDecl, "'this'", Context.getPointerType(ThisType),
-                    Context.getPointerType(Ctor->getThisObjectType()));
+                    Context.getPointerType(Ctor->getThisObjectType()), nullptr);
 
   checkCall(FDecl, Proto, /*ThisArg=*/nullptr, Args, /*IsMemberFunction=*/true,
             Loc, SourceRange(), CallType);
 }
 
 /// CheckFunctionCall - Check a direct function call for various correctness
 /// and safety properties not strictly enforced by the C type system.
 bool Sema::CheckFunctionCall(FunctionDecl *FDecl, CallExpr *TheCall,
if (!ThisType->isPointerType()) {
       assert(!ThisType->isReferenceType());
       ThisType = Context.getPointerType(ThisType);
     }
 
     QualType ThisTypeFromDecl =
         Context.getPointerType(cast<CXXMethodDecl>(FDecl)->getThisObjectType());
 
     CheckArgAlignment(TheCall->getRParenLoc(), FDecl, "'this'", ThisType,
-                      ThisTypeFromDecl);
+                      ThisTypeFromDecl, nullptr);
   }
 
   checkCall(FDecl, Proto, ImplicitThis, llvm::makeArrayRef(Args, NumArgs),
             IsMemberFunction, TheCall->getRParenLoc(),
             TheCall->getCallee()->getSourceRange(), CallType);
 
   IdentifierInfo *FnInfo = FDecl->getIdentifier();
   // None of the checks below are needed for functions that don't have

clang/lib/Sema/SemaDeclAttr.cpp

void Sema::AddAlignedAttr(Decl *D, const AttributeCommonInfo &CI, Expr *E,
   // FIXME: Cache the number on the AL object?
   llvm::APSInt Alignment;
   ExprResult ICE = VerifyIntegerConstantExpression(
       E, &Alignment, diag::err_aligned_attribute_argument_not_int);
   if (ICE.isInvalid())
     return;
 
   uint64_t AlignVal = Alignment.getZExtValue();
-  // 16 byte ByVal alignment not due to a vector member is not honoured by XL
-  // on AIX. Emit a warning here that users are generating binary incompatible
-  // code to be safe.
-  if (AlignVal >= 16 && isa<FieldDecl>(D) &&
-      Context.getTargetInfo().getTriple().isOSAIX())
-    Diag(AttrLoc, diag::warn_not_xl_compatible) << E->getSourceRange();
-
   // C++11 [dcl.align]p2:
   //   -- if the constant expression evaluates to zero, the alignment
   //      specifier shall have no effect
   // C11 6.7.5p6:
   //   An alignment specification of zero has no effect.
   if (!(TmpAttr.isAlignas() && !Alignment)) {
     if (!llvm::isPowerOf2_64(AlignVal)) {
       Diag(AttrLoc, diag::err_alignment_not_power_of_two)

clang/test/Sema/aix-attr-align.c

 // off-no-diagnostics
 // RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -verify -fsyntax-only %s
 // RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -verify -fsyntax-only %s
 // RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -verify=off -Wno-aix-compat -fsyntax-only %s
 // RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -verify=off -Wno-aix-compat -fsyntax-only %s
 // RUN: %clang_cc1 -triple powerpc64le-unknown-linux -verify=off -fsyntax-only %s
 
 struct S {
-  int a[8] __attribute__((aligned(8))); // no-warning
+  int a[8] __attribute__((aligned(8)));  // no-warning
+  int b[8] __attribute__((aligned(16))); // no-warning
+  int c[2] __attribute__((aligned(32))); // no-warning
 };
 
 struct T {
-  int a[4] __attribute__((aligned(16))); // 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}}
+  int a[4] __attribute__((aligned(16))); // no-warning
 };
 
 struct U {
-  int a[2] __attribute__((aligned(32))); // 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}}
+  int a[2] __attribute__((aligned(32))); // no-warning
 };
 
 int a[8] __attribute__((aligned(8)));  // no-warning
 int b[4] __attribute__((aligned(16))); // no-warning
 int c[2] __attribute__((aligned(32))); // no-warning
+
+void baz(int *);
+void foo(int p1, int p2, int p3, int p4, int p5, int p6, int p7, int p8,
+         struct S s) {
+  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}}
+
+  baz(a); // no-warning
+  baz(b); // no-warning
+  baz(c); // no-warning
+}