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

[Sean Fertile via Phabricator via cfe-commits]

sfertile added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118350

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118350

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118350

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3299-3301
+: Warning<" %0 byte requested alignment for a struct member used as an"
+  " argument is 16 bytes or greater which is not binary"
+  " compatible with IBM XL C/C++ for AIX 16.1.0 or older">,

aaron.ballman wrote:
> 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).
Yes, I cringed a bit seeing it at 3 lines. Thanks. 



Comment at: clang/lib/Sema/SemaChecking.cpp:5221
+<< (unsigned)Alignment.getQuantity() << FD;
+Diag(Loc, diag::note_called_by) << FD->getDeclName();
+  }

aaron.ballman wrote:
> 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.
Ah yes, this seems much clearer to the user now IMO, thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118350

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 updated this revision to Diff 406843.
ZarkoCA added a comment.

- Shorten warning message
- Add new note


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118350

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Analysis/padding_c.c
  clang/test/Analysis/padding_cpp.cpp
  clang/test/CXX/drs/dr6xx.cpp
  clang/test/Sema/aix-attr-align.c
  clang/test/SemaTemplate/instantiate-attr.cpp

Index: clang/test/SemaTemplate/instantiate-attr.cpp
===
--- clang/test/SemaTemplate/instantiate-attr.cpp
+++ clang/test/SemaTemplate/instantiate-attr.cpp
@@ -1,7 +1,4 @@
-// FIXME -Wno-aix-compat added temporarily while the diagnostic is being
-// refined.
-
-// RUN: %clang_cc1 -fsyntax-only -verify -Wno-aix-compat %s
+// RUN: %clang_cc1 -fsyntax-only -verify %s
 // expected-no-diagnostics
 template 
 struct A {
Index: clang/test/Sema/aix-attr-align.c
===
--- clang/test/Sema/aix-attr-align.c
+++ clang/test/Sema/aix-attr-align.c
@@ -6,17 +6,43 @@
 // 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))); // expected-warning {{alignment of 16 bytes or greater for a struct member is not binary compatible with IBM XL C/C++ for AIX 16.1.0 or older}}
+  int c[2] __attribute__((aligned(32))); // expected-warning {{alignment of 16 bytes or greater for a struct member is not binary compatible with IBM XL C/C++ for AIX 16.1.0 or older}}
 };
 
 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 *);
+static void static_baz(int *b) {
+  b = b + 1;
+}
+
+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-note
+  baz(s.b); // expected-note {{'b' used with potentially incompatible alignment here}}
+  baz(s.c); // expected-note {{'c' used with potentially incompatible alignment here}}
+
+  baz(a); // no-note
+  baz(b); // no-note
+  baz(c); // no-note
+
+  static_baz(s.a); // no-note
+  static_baz(s.b); // no-note
+  static_baz(s.c); // no-note
+
+  static_baz(a); // no-note
+  static_baz(b); // no-note
+  static_baz(c); // no-note
+}
Index: clang/test/CXX/drs/dr6xx.cpp
===
--- clang/test/CXX/drs/dr6xx.cpp
+++ clang/test/CXX/drs/dr6xx.cpp
@@ -1,10 +1,8 @@
-// FIXME -Wno-aix-compat added temporarily while the diagnostic is being
-// refined.
-// RUN: %clang_cc1 -std=c++98 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking -Wno-aix-compat
-// RUN: %clang_cc1 -std=c++11 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking -Wno-aix-compat
-// RUN: %clang_cc1 -std=c++14 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking -Wno-aix-compat
-// RUN: %clang_cc1 -std=c++17 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking -Wno-aix-compat
-// RUN: %clang_cc1 -std=c++20 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking -Wno-aix-compat
+// RUN: %clang_cc1 -std=c++98 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking
+// RUN: %clang_cc1 -std=c++11 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking
+// RUN: %clang_cc1 -std=c++14 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking
+// RUN: %clang_cc1 -std=c++17 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking
+// RUN: %clang_cc1 -std=c++20 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking
 
 namespace std {
   struct type_info {};
Index: clang/test/Analysis/padding_cpp.cpp
===
--- clang/test/Analysis/padding_cpp.cpp
+++ clang/test/Analysis/padding_cpp.cpp
@@ -1,6 +1,4 @@
-// FIXME -Wno-aix-compat added temporarily while the diagnostic is being
-// refined.
-// RUN: 

[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 a comment.

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




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3299-3301
+: Warning<" %0 byte requested alignment for a struct member used as an"
+  " argument is 16 bytes or greater which is not binary"
+  " compatible with IBM XL C/C++ for AIX 16.1.0 or older">,

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



Comment at: clang/lib/Sema/SemaChecking.cpp:5221
+<< (unsigned)Alignment.getQuantity() << FD;
+Diag(Loc, diag::note_called_by) << FD->getDeclName();
+  }

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118350

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 added inline comments.



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:
> ZarkoCA wrote:
> > 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. 
> 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).
> 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. 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118350

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 updated this revision to Diff 406822.
ZarkoCA added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118350

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Analysis/padding_c.c
  clang/test/Analysis/padding_cpp.cpp
  clang/test/CXX/drs/dr6xx.cpp
  clang/test/Sema/aix-attr-align.c
  clang/test/SemaTemplate/instantiate-attr.cpp

Index: clang/test/SemaTemplate/instantiate-attr.cpp
===
--- clang/test/SemaTemplate/instantiate-attr.cpp
+++ clang/test/SemaTemplate/instantiate-attr.cpp
@@ -1,7 +1,4 @@
-// FIXME -Wno-aix-compat added temporarily while the diagnostic is being
-// refined.
-
-// RUN: %clang_cc1 -fsyntax-only -verify -Wno-aix-compat %s
+// RUN: %clang_cc1 -fsyntax-only -verify %s
 // expected-no-diagnostics
 template 
 struct A {
Index: clang/test/Sema/aix-attr-align.c
===
--- clang/test/Sema/aix-attr-align.c
+++ clang/test/Sema/aix-attr-align.c
@@ -6,17 +6,43 @@
 // 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))); // expected-warning {{16 byte requested alignment for a struct member used as an argument is 16 bytes or greater which is not binary compatible with IBM XL C/C++ for AIX 16.1.0 or older}}
+  int c[2] __attribute__((aligned(32))); // expected-warning {{32 byte requested alignment for a struct member used as an argument is 16 bytes or greater which is not binary compatible with IBM XL C/C++ for AIX 16.1.0 or older}}
 };
 
 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 *);
+static void static_baz(int *b) {
+  b = b + 1;
+}
+
+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-note
+  baz(s.b); // expected-note {{called by 'b'}}
+  baz(s.c); // expected-note {{called by 'c'}}
+
+  baz(a); // no-note
+  baz(b); // no-note
+  baz(c); // no-note
+
+  static_baz(s.a); // no-note
+  static_baz(s.b); // no-note
+  static_baz(s.c); // no-note
+
+  static_baz(a); // no-note
+  static_baz(b); // no-note
+  static_baz(c); // no-note
+}
Index: clang/test/CXX/drs/dr6xx.cpp
===
--- clang/test/CXX/drs/dr6xx.cpp
+++ clang/test/CXX/drs/dr6xx.cpp
@@ -1,10 +1,8 @@
-// FIXME -Wno-aix-compat added temporarily while the diagnostic is being
-// refined.
-// RUN: %clang_cc1 -std=c++98 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking -Wno-aix-compat
-// RUN: %clang_cc1 -std=c++11 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking -Wno-aix-compat
-// RUN: %clang_cc1 -std=c++14 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking -Wno-aix-compat
-// RUN: %clang_cc1 -std=c++17 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking -Wno-aix-compat
-// RUN: %clang_cc1 -std=c++20 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking -Wno-aix-compat
+// RUN: %clang_cc1 -std=c++98 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking
+// RUN: %clang_cc1 -std=c++11 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking
+// RUN: %clang_cc1 -std=c++14 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking
+// RUN: %clang_cc1 -std=c++17 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking
+// RUN: %clang_cc1 -std=c++20 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking
 
 namespace std {
   struct type_info {};
Index: clang/test/Analysis/padding_cpp.cpp
===
--- clang/test/Analysis/padding_cpp.cpp
+++ clang/test/Analysis/padding_cpp.cpp
@@ -1,6 +1,4 @@
-// FIXME -Wno-aix-compat added temporarily while the diagnostic is being

[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/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}}
+

ZarkoCA wrote:
> 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. 
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118350

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 updated this revision to Diff 406620.
ZarkoCA added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118350

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Analysis/padding_c.c
  clang/test/Analysis/padding_cpp.cpp
  clang/test/CXX/drs/dr6xx.cpp
  clang/test/Sema/aix-attr-align.c
  clang/test/SemaTemplate/instantiate-attr.cpp

Index: clang/test/SemaTemplate/instantiate-attr.cpp
===
--- clang/test/SemaTemplate/instantiate-attr.cpp
+++ clang/test/SemaTemplate/instantiate-attr.cpp
@@ -1,7 +1,4 @@
-// FIXME -Wno-aix-compat added temporarily while the diagnostic is being
-// refined.
-
-// RUN: %clang_cc1 -fsyntax-only -verify -Wno-aix-compat %s
+// RUN: %clang_cc1 -fsyntax-only -verify %s
 // expected-no-diagnostics
 template 
 struct A {
Index: clang/test/Sema/aix-attr-align.c
===
--- clang/test/Sema/aix-attr-align.c
+++ clang/test/Sema/aix-attr-align.c
@@ -6,17 +6,43 @@
 // 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))); // expected-note {{'b' declared here}}
+  int c[2] __attribute__((aligned(32))); // expected-note {{'c' declared here}}
 };
 
 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 *);
+static void static_baz(int *b) {
+  b = b + 1;
+}
+
+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 {{16 byte requested alignment for a struct member used as an argument is 16 bytes or greater which is not binary compatible with IBM XL C/C++ for AIX 16.1.0 or older}}
+  baz(s.c); // expected-warning {{32 byte requested alignment for a struct member used as an argument is 16 bytes or greater which is not binary compatible with IBM XL C/C++ for AIX 16.1.0 or older}}
+
+  baz(a); // no-warning
+  baz(b); // no-warning
+  baz(c); // no-warning
+
+  static_baz(s.a); // no-warning
+  static_baz(s.b); // no-warning
+  static_baz(s.c); // no-warning
+
+  static_baz(a); // no-warning
+  static_baz(b); // no-warning
+  static_baz(c); // no-warning
+}
Index: clang/test/CXX/drs/dr6xx.cpp
===
--- clang/test/CXX/drs/dr6xx.cpp
+++ clang/test/CXX/drs/dr6xx.cpp
@@ -1,10 +1,8 @@
-// FIXME -Wno-aix-compat added temporarily while the diagnostic is being
-// refined.
-// RUN: %clang_cc1 -std=c++98 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking -Wno-aix-compat
-// RUN: %clang_cc1 -std=c++11 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking -Wno-aix-compat
-// RUN: %clang_cc1 -std=c++14 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking -Wno-aix-compat
-// RUN: %clang_cc1 -std=c++17 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking -Wno-aix-compat
-// RUN: %clang_cc1 -std=c++20 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking -Wno-aix-compat
+// RUN: %clang_cc1 -std=c++98 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking
+// RUN: %clang_cc1 -std=c++11 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking
+// RUN: %clang_cc1 -std=c++14 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking
+// RUN: %clang_cc1 -std=c++17 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking
+// RUN: %clang_cc1 -std=c++20 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking
 
 namespace std {
   struct type_info {};
Index: clang/test/Analysis/padding_cpp.cpp
===
--- clang/test/Analysis/padding_cpp.cpp
+++ clang/test/Analysis/padding_cpp.cpp
@@ 

[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(AArg)) {
+AArg = ICE->getSubExpr();
+if (const auto *ME = dyn_cast(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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 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);

ZarkoCA wrote:
> ZarkoCA wrote:
> > aaron.ballman wrote:
> > > 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)?
> > 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. 
> 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. 
@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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118350

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 updated this revision to Diff 406086.
ZarkoCA added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118350

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Analysis/padding_c.c
  clang/test/Analysis/padding_cpp.cpp
  clang/test/CXX/drs/dr6xx.cpp
  clang/test/Sema/aix-attr-align.c
  clang/test/SemaTemplate/instantiate-attr.cpp

Index: clang/test/SemaTemplate/instantiate-attr.cpp
===
--- clang/test/SemaTemplate/instantiate-attr.cpp
+++ clang/test/SemaTemplate/instantiate-attr.cpp
@@ -1,7 +1,4 @@
-// FIXME -Wno-aix-compat added temporarily while the diagnostic is being
-// refined.
-
-// RUN: %clang_cc1 -fsyntax-only -verify -Wno-aix-compat %s
+// RUN: %clang_cc1 -fsyntax-only -verify %s
 // expected-no-diagnostics
 template 
 struct A {
Index: clang/test/Sema/aix-attr-align.c
===
--- clang/test/Sema/aix-attr-align.c
+++ clang/test/Sema/aix-attr-align.c
@@ -6,17 +6,43 @@
 // 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 *);
+static void static_baz(int *b) {
+  b = b + 1;
+}
+
+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
+
+  static_baz(s.a); // no-warning
+  static_baz(s.b); // no-warning
+  static_baz(s.c); // no-warning
+
+  static_baz(a); // no-warning
+  static_baz(b); // no-warning
+  static_baz(c); // no-warning
+}
Index: clang/test/CXX/drs/dr6xx.cpp
===
--- clang/test/CXX/drs/dr6xx.cpp
+++ clang/test/CXX/drs/dr6xx.cpp
@@ -1,10 +1,8 @@
-// FIXME -Wno-aix-compat added temporarily while the diagnostic is being
-// refined.
-// RUN: %clang_cc1 -std=c++98 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking -Wno-aix-compat
-// RUN: %clang_cc1 -std=c++11 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking -Wno-aix-compat
-// RUN: %clang_cc1 -std=c++14 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking -Wno-aix-compat
-// RUN: %clang_cc1 -std=c++17 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking -Wno-aix-compat
-// RUN: %clang_cc1 -std=c++20 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking -Wno-aix-compat
+// RUN: %clang_cc1 -std=c++98 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking
+// RUN: %clang_cc1 -std=c++11 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking
+// RUN: %clang_cc1 -std=c++14 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking
+// RUN: %clang_cc1 -std=c++17 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking
+// RUN: %clang_cc1 -std=c++20 %s -verify -fexceptions -fcxx-exceptions -pedantic-errors -fno-spell-checking
 
 namespace std {
   struct type_info {};
Index: clang/test/Analysis/padding_cpp.cpp
===
--- clang/test/Analysis/padding_cpp.cpp
+++ clang/test/Analysis/padding_cpp.cpp
@@ -1,6 +1,4 @@
-// FIXME -Wno-aix-compat added temporarily while the diagnostic is being
-// refined.
-// RUN: %clang_analyze_cc1 

[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 added a comment.

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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118350

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 updated this revision to Diff 404600.
ZarkoCA added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118350

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Sema/aix-attr-align.c

Index: clang/test/Sema/aix-attr-align.c
===
--- clang/test/Sema/aix-attr-align.c
+++ clang/test/Sema/aix-attr-align.c
@@ -6,17 +6,43 @@
 // 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 *);
+static void static_baz(int *b) {
+  b = b + 1;
+}
+
+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
+
+  static_baz(s.a); // no-warning
+  static_baz(s.b); // no-warning
+  static_baz(s.c); // no-warning
+
+  static_baz(a); // no-warning
+  static_baz(b); // no-warning
+  static_baz(c); // no-warning
+}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -4321,13 +4321,6 @@
 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(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
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -5201,18 +5201,44 @@
   }
 }
 
-/// 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) {
 
   // If a function accepts a pointer or reference type
   if (!ParamTy->isPointerType() && !ParamTy->isReferenceType())
 return;
 
+  // 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 &&
+  (FDecl->hasLinkage()) &&
+  !(FDecl->getFormalLinkage() == InternalLinkage)) {
+// Using AArg so as to not modify Arg for the rest of the function.
+const Expr *AArg = Arg->IgnoreParens();
+if (const auto *ICE = dyn_cast(AArg)) {
+  

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

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118350

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 8 inline comments as done.
ZarkoCA 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);

ZarkoCA wrote:
> aaron.ballman wrote:
> > 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)?
> 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. 
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. 



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

aaron.ballman wrote:
> 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.)
Yes, thanks. I do have suspicions that `IgnoreParens()` may be too broad but I 
am worried that we may miss cases to diagnose otherwise. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118350

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 updated this revision to Diff 404190.
ZarkoCA added a comment.

Addressed some of the comments:

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118350

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Sema/aix-attr-align.c

Index: clang/test/Sema/aix-attr-align.c
===
--- clang/test/Sema/aix-attr-align.c
+++ clang/test/Sema/aix-attr-align.c
@@ -6,17 +6,31 @@
 // 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
+}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -4321,13 +4321,6 @@
 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(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
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -5201,18 +5201,42 @@
   }
 }
 
-/// 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) {
 
   // If a function accepts a pointer or reference type
   if (!ParamTy->isPointerType() && !ParamTy->isReferenceType())
 return;
 
+  // 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) {
+// Using AArg so as to not modify Arg for the rest of the function.
+const Expr *AArg = Arg->IgnoreParens();
+if (const auto *ICE = dyn_cast(AArg)) {
+  AArg = ICE->getSubExpr();
+  if (const auto *ME = dyn_cast(AArg)) {
+if (auto *FD = dyn_cast(ME->getMemberDecl())) {
+  if (const auto *AA = FD->getAttr()) {
+CharUnits Alignment =
+Context.toCharUnitsFromBits(AA->getAlignment(Context));
+  

[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 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);

aaron.ballman wrote:
> 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)?
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118350

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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(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(AArg);
+  ValueDecl *MD = ME->getMemberDecl();





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





Comment at: clang/lib/Sema/SemaChecking.cpp:5229-5230
+  if (FD) {
+if (FD->hasAttr()) {
+  auto *AA = FD->getAttr();
+  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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 updated this revision to Diff 404090.
ZarkoCA added a comment.

Addressing comments:

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118350

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Sema/aix-attr-align.c

Index: clang/test/Sema/aix-attr-align.c
===
--- clang/test/Sema/aix-attr-align.c
+++ clang/test/Sema/aix-attr-align.c
@@ -6,17 +6,31 @@
 // 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
+}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -4321,13 +4321,6 @@
 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(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
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -5201,13 +5201,44 @@
   }
 }
 
-/// 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(AArg);
+AArg = ICE->getSubExpr();
+if (AArg->getStmtClass() == Stmt::MemberExprClass) {
+  const auto *ME = dyn_cast(AArg);
+  ValueDecl *MD = ME->getMemberDecl();
+  auto *FD = dyn_cast(MD);
+  if (FD) {
+if (FD->hasAttr()) {
+  auto *AA = FD->getAttr();
+  unsigned Aligned 

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

[David Tenty via Phabricator via cfe-commits]

daltenty added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:5204
 
 /// 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

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.




Comment at: clang/test/Sema/aix-attr-align.c:4
 // 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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118350

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 updated this revision to Diff 403857.
ZarkoCA edited the summary of this revision.
ZarkoCA added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118350

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Sema/aix-attr-align.c

Index: clang/test/Sema/aix-attr-align.c
===
--- clang/test/Sema/aix-attr-align.c
+++ clang/test/Sema/aix-attr-align.c
@@ -1,22 +1,33 @@
 // 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
+}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -4320,13 +4320,6 @@
 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(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
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -5207,7 +5207,37 @@
 /// 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 passed to 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(AArg);
+AArg = ICE->getSubExpr();
+if (AArg->getStmtClass() == Stmt::MemberExprClass) {
+  const auto *ME = dyn_cast(AArg);
+  ValueDecl *MD = ME->getMemberDecl();
+  auto *FD = dyn_cast(MD);
+  if (FD) {
+if (FD->hasAttr()) {
+  auto *AA = FD->getAttr();
+  unsigned Aligned = AA->getAlignment(Context);
+  // Divide by 8 to get the bytes instead of using bits.