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

[Zarko Todorovski via Phabricator via cfe-commits]

ZarkoCA marked 4 inline comments as done.
ZarkoCA added inline comments.


================
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;
+        }
----------------
aaron.ballman wrote:
> 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).
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. 


================
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}}
+
----------------
aaron.ballman wrote:
> 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?
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118350