[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Xiangling Liao via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Xiangling_L marked 8 inline comments as done.
Closed by commit rGf0abe2aeaca7: [Frontend] Add pragma align natural and sort 
out pragma pack stack effect (authored by Xiangling_L).

Changed prior to commit:
  https://reviews.llvm.org/D87702?vs=315855=316400#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87702

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTReader.h
  clang/include/clang/Serialization/ASTWriter.h
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Driver/aix-pragma-pack.c
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-power-natural-interaction.cpp
  clang/test/PCH/aix-pragma-pack.c
  clang/test/Sema/aix-pragma-pack-and-align.c

Index: clang/test/Sema/aix-pragma-pack-and-align.c
===
--- /dev/null
+++ clang/test/Sema/aix-pragma-pack-and-align.c
@@ -0,0 +1,229 @@
+// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -fxl-pragma-pack -verify -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
+
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -fxl-pragma-pack -verify -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
+
+namespace test1 {
+#pragma align(natural)
+#pragma pack(4)
+#pragma pack(2)
+struct A {
+  int i;
+  double d;
+};
+
+int a = sizeof(A);
+#pragma pack()
+#pragma pack(show) // expected-warning {{value of #pragma pack(show) == 4}}
+#pragma pack(pop)
+#pragma pack(show) // expected-warning {{value of #pragma pack(show) == 8}}
+struct B {
+  int i;
+  double d;
+};
+#pragma align(reset)
+
+int b = sizeof(B);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test1::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=2, preferredalign=2,
+// CHECK-NEXT:|  nvsize=12, nvalign=2, preferrednvalign=2]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test1::B
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  8 |   double d
+// CHECK-NEXT:| [sizeof=16, dsize=16, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=16, nvalign=4, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+#pragma align(natural)
+#pragma pack(2)
+struct A {
+  int i;
+  double d;
+};
+
+int a = sizeof(A);
+#pragma align(reset)
+
+struct B {
+  int i;
+  double d;
+};
+
+int b = sizeof(B);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test2::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=2, preferredalign=2,
+// CHECK-NEXT:|  nvsize=12, nvalign=2, preferrednvalign=2]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test2::B
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+
+} // namespace test2
+
+namespace test3 {
+#pragma pack(2)
+#pragma align(natural)
+struct A {
+  double d;
+};
+#pragma align(reset)
+#pragma pack(pop)
+
+int a = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test3::A
+// CHECK-NEXT:  0 |   double d
+// CHECK-NEXT:| [sizeof=8, dsize=8, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=8, nvalign=4, preferrednvalign=8]
+
+} // namespace test3
+
+namespace test4 {
+#pragma pack(2)
+#pragma align(natural)
+#pragma pack(pop)
+
+struct A {
+  int i;
+  double d;
+} a;
+#pragma align(reset)
+#pragma pack(pop)
+
+int i = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test4::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  8 |   double d
+// CHECK-NEXT:| [sizeof=16, dsize=16, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=16, nvalign=4, preferrednvalign=8]
+
+} // namespace test4
+
+namespace test5 {
+#pragma align(power)
+#pragma align(natural)
+#pragma pack(2)
+#pragma align(reset)
+struct A {
+  int i;
+  double d;
+};
+#pragma align(reset)
+
+int a = sizeof(A);
+
+// CHECK:  *** Dumping AST Record 

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Jason Liu via Phabricator via cfe-commits]

jasonliu accepted this revision.
jasonliu added a comment.

LGTM with minor nits.




Comment at: clang/lib/Sema/SemaAttr.cpp:74
 return;
-  // The #pragma align/pack affected a record in an included file, so Clang
-  // should warn when that the pragma was written in a file that included the
+  // The #pragma align/pack affected a record in an included file,  so Clang
+  // should warn when that pragma was written in a file that included the





Comment at: clang/lib/Sema/SemaAttr.cpp:347-349
+AlignmentVal = CurVal.getPackNumber();
+if (!CurVal.IsPackSet())
   AlignmentVal = 8;

nit



Comment at: clang/lib/Sema/SemaAttr.cpp:372
+  // XL pragma pack does not support identifier syntax.
+  if (IsXLPragma && !SlotLabel.empty()) {
+Diag(PragmaLoc, diag::err_pragma_pack_identifer_not_supported);

Could we move this to the top of the function so that we could have a quick 
exit when we sees this?
If we are going to emit an error, the `AlignPackStack.Act(PragmaLoc, Action, 
StringRef(), Info);` is not necessary any more.



Comment at: clang/lib/Sema/SemaAttr.cpp:555
+  });
+  // If we found the label so pop from there.
+  if (I != Stack.rend()) {





Comment at: clang/lib/Sema/SemaAttr.cpp:581
+} else if (!Stack.empty()) {
+  // xl '#pragma align' sets the base line,
+  // and pragma pack cannot pop over the base line.





Comment at: clang/lib/Sema/SemaAttr.cpp:582
+  // xl '#pragma align' sets the base line,
+  // and pragma pack cannot pop over the base line.
+  if (Value.IsXLStack() && Value.IsPackAttr() && 
CurrentValue.IsAlignAttr())




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

https://reviews.llvm.org/D87702

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

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[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 aside from a few small issues to fix.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:893
+def err_pragma_pack_identifer_not_supported : Error<
+  "specifying an identifier within pragma pack is not supported, identifier is 
ignored">;
 def err_section_conflict : Error<"%0 causes a section type conflict with %1">;

The identifier is no longer ignored now that this is an error rather than a 
warning.



Comment at: clang/test/Sema/aix-pragma-pack-and-align.c:231
+
+// expected-no-warning

Xiangling_L wrote:
> aaron.ballman wrote:
> > Is this comment intentional?
> Yes, my intention is to test no pragma pack or prama align is unterminated.
I don't think we have such a construct that's checked by `-verify` (and if we 
did, I'd say the test file is broken because it contains `expected-warning` 
comments).

Because you're passing `-verify`, any warnings that are triggered on a line 
without a matching `expected-warning` comment will be reported as a test 
failure, so you can safely remove this comment and still get the test coverage 
you want.


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

https://reviews.llvm.org/D87702

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

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 315855.
Xiangling_L marked 4 inline comments as done.
Xiangling_L added a comment.

Rebased on latest master;
Addressed the comments;


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

https://reviews.llvm.org/D87702

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTReader.h
  clang/include/clang/Serialization/ASTWriter.h
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Driver/aix-pragma-pack.c
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-power-natural-interaction.cpp
  clang/test/PCH/aix-pragma-pack.c
  clang/test/Sema/aix-pragma-pack-and-align.c

Index: clang/test/Sema/aix-pragma-pack-and-align.c
===
--- /dev/null
+++ clang/test/Sema/aix-pragma-pack-and-align.c
@@ -0,0 +1,231 @@
+// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -fxl-pragma-pack -verify -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
+
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -fxl-pragma-pack -verify -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
+
+namespace test1 {
+#pragma align(natural)
+#pragma pack(4)
+#pragma pack(2)
+struct A {
+  int i;
+  double d;
+};
+
+int a = sizeof(A);
+#pragma pack()
+#pragma pack(show) // expected-warning {{value of #pragma pack(show) == 4}}
+#pragma pack(pop)
+#pragma pack(show) // expected-warning {{value of #pragma pack(show) == 8}}
+struct B {
+  int i;
+  double d;
+};
+#pragma align(reset)
+
+int b = sizeof(B);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test1::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=2, preferredalign=2,
+// CHECK-NEXT:|  nvsize=12, nvalign=2, preferrednvalign=2]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test1::B
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  8 |   double d
+// CHECK-NEXT:| [sizeof=16, dsize=16, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=16, nvalign=4, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+#pragma align(natural)
+#pragma pack(2)
+struct A {
+  int i;
+  double d;
+};
+
+int a = sizeof(A);
+#pragma align(reset)
+
+struct B {
+  int i;
+  double d;
+};
+
+int b = sizeof(B);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test2::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=2, preferredalign=2,
+// CHECK-NEXT:|  nvsize=12, nvalign=2, preferrednvalign=2]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test2::B
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+
+} // namespace test2
+
+namespace test3 {
+#pragma pack(2)
+#pragma align(natural)
+struct A {
+  double d;
+};
+#pragma align(reset)
+#pragma pack(pop)
+
+int a = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test3::A
+// CHECK-NEXT:  0 |   double d
+// CHECK-NEXT:| [sizeof=8, dsize=8, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=8, nvalign=4, preferrednvalign=8]
+
+} // namespace test3
+
+namespace test4 {
+#pragma pack(2)
+#pragma align(natural)
+#pragma pack(pop)
+
+struct A {
+  int i;
+  double d;
+} a;
+#pragma align(reset)
+#pragma pack(pop)
+
+int i = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test4::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  8 |   double d
+// CHECK-NEXT:| [sizeof=16, dsize=16, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=16, nvalign=4, preferrednvalign=8]
+
+} // namespace test4
+
+namespace test5 {
+#pragma align(power)
+#pragma align(natural)
+#pragma pack(2)
+#pragma align(reset)
+struct A {
+  int i;
+  double d;
+};
+#pragma align(reset)
+
+int a = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test5::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=4, 

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:893
+def warn_pragma_pack_identifer_not_supported : Warning<
+  "specifying an identifier within pragma pack is not supported, identifier is 
ignored">,
+  InGroup;

Xiangling_L wrote:
> aaron.ballman wrote:
> > If the user wrote an identifier, it seems like there's a strong chance that 
> > ignoring the identifier will result in incorrect behavior that would lead 
> > to hard-to-find ODR issues. Should this scenario be an error rather than a 
> > warning where the identifier is ignored?
> Could you show a more concrete example or give more details on how possible 
> incorrect behavior would lead to hard-to-find ODR issues?
I'm not super familiar with this feature, so what I'm thinking of may not be a 
real issue in practice. The case I was thinking of is where the user does 
`#pragma pack(push, r1, 2)` to push a packing by name followed by `#pragma 
pack(push, r2, 4)`, and the later pops come in the reverse order (`#pragma 
pack(pop, r1)` followed by `#pragma pack(pop, r2)`). The first pop will 
actually pop the `r2` packing and not the `r1` packing, which could cause the 
field layout to be different from what the user expected.



Comment at: clang/include/clang/Sema/Sema.h:524-525
+return AlignPackInfo(M, PackNumber, IsXL);
+  else
+return AlignPackInfo(M, IsXL);
+}

No `else` after a `return`.



Comment at: clang/include/clang/Sema/Sema.h:512
+static AlignPackInfo getFromRawEncoding(unsigned Encoding) {
+  static_assert(sizeof(AlignPackInfo) == sizeof(uint32_t),
+"Size is not correct");

Xiangling_L wrote:
> aaron.ballman wrote:
> > I would feel more comfortable with this assertion if the class was using 
> > bit-fields to pack the values together. As it stands, we're kind of hoping 
> > that `bool`, `Mode`, and `unsigned char` will all pack in a particular way 
> > (and `bool`'s representation is implementation-defined).
> Yeah, good point. I will move it back to previous bitfields version.
Oh, that's different from what I was expecting -- I was thinking you'd change 
the data member fields of the class to be bit-fields.

Either way works for me, though, so no change necessary unless you prefer that 
approach.



Comment at: clang/include/clang/Sema/Sema.h:527
+
+unsigned char getPackNumber() const { return PackNumber; }
+

Xiangling_L wrote:
> aaron.ballman wrote:
> > Given that the ctor takes an `int` for this value, should this be returning 
> > an `int`?
> As we know the pack number would not exceed 16 bytes, so the intention of 
> using `unsigned char` here is to save space taken by AlignPackInfo. And 
> that's why I added the diagnostics and assertion in the ctor to make sure the 
> value won't be truncated.
We don't save any space with this function signature though -- the return value 
will most likely wind up being implicitly promoted to `int` anyway. I think the 
fact that we store it as a smaller datatype internally is an implementation 
detail and as far as the consumer of this class is concerned, the pack number 
is an integer type where we don't care about the size. tbh, because the pack 
numbers are always positive, I'd have the ctor and this function both deal with 
an `unsigned` rather than an `int`.

(Note, I'm not talking about the type of the underlying field -- I think it's 
fine to continue to use a smaller datatype there for space savings. I'm only 
talking about the types in the public interface.)


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

https://reviews.llvm.org/D87702

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

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 315126.
Xiangling_L marked 5 inline comments as done.
Xiangling_L added a comment.

Addressed the comments;
Rebased the patch on latest master;


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

https://reviews.llvm.org/D87702

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTReader.h
  clang/include/clang/Serialization/ASTWriter.h
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Driver/aix-pragma-pack.c
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-power-natural-interaction.cpp
  clang/test/PCH/aix-pragma-pack.c
  clang/test/Sema/aix-pragma-pack-and-align.c

Index: clang/test/Sema/aix-pragma-pack-and-align.c
===
--- /dev/null
+++ clang/test/Sema/aix-pragma-pack-and-align.c
@@ -0,0 +1,231 @@
+// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -fxl-pragma-pack -verify -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
+
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -fxl-pragma-pack -verify -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
+
+namespace test1 {
+#pragma align(natural)
+#pragma pack(4)
+#pragma pack(2)
+struct A {
+  int i;
+  double d;
+};
+
+int a = sizeof(A);
+#pragma pack()
+#pragma pack(show) // expected-warning {{value of #pragma pack(show) == 4}}
+#pragma pack(pop)
+#pragma pack(show) // expected-warning {{value of #pragma pack(show) == 8}}
+struct B {
+  int i;
+  double d;
+};
+#pragma align(reset)
+
+int b = sizeof(B);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test1::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=2, preferredalign=2,
+// CHECK-NEXT:|  nvsize=12, nvalign=2, preferrednvalign=2]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test1::B
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  8 |   double d
+// CHECK-NEXT:| [sizeof=16, dsize=16, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=16, nvalign=4, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+#pragma align(natural)
+#pragma pack(2)
+struct A {
+  int i;
+  double d;
+};
+
+int a = sizeof(A);
+#pragma align(reset)
+
+struct B {
+  int i;
+  double d;
+};
+
+int b = sizeof(B);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test2::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=2, preferredalign=2,
+// CHECK-NEXT:|  nvsize=12, nvalign=2, preferrednvalign=2]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test2::B
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+
+} // namespace test2
+
+namespace test3 {
+#pragma pack(2)
+#pragma align(natural)
+struct A {
+  double d;
+};
+#pragma align(reset)
+#pragma pack(pop)
+
+int a = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test3::A
+// CHECK-NEXT:  0 |   double d
+// CHECK-NEXT:| [sizeof=8, dsize=8, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=8, nvalign=4, preferrednvalign=8]
+
+} // namespace test3
+
+namespace test4 {
+#pragma pack(2)
+#pragma align(natural)
+#pragma pack(pop)
+
+struct A {
+  int i;
+  double d;
+} a;
+#pragma align(reset)
+#pragma pack(pop)
+
+int i = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test4::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  8 |   double d
+// CHECK-NEXT:| [sizeof=16, dsize=16, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=16, nvalign=4, preferrednvalign=8]
+
+} // namespace test4
+
+namespace test5 {
+#pragma align(power)
+#pragma align(natural)
+#pragma pack(2)
+#pragma align(reset)
+struct A {
+  int i;
+  double d;
+};
+#pragma align(reset)
+
+int a = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test5::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=4, 

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L marked 9 inline comments as done.
Xiangling_L added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:893
+def warn_pragma_pack_identifer_not_supported : Warning<
+  "specifying an identifier within pragma pack is not supported, identifier is 
ignored">,
+  InGroup;

aaron.ballman wrote:
> If the user wrote an identifier, it seems like there's a strong chance that 
> ignoring the identifier will result in incorrect behavior that would lead to 
> hard-to-find ODR issues. Should this scenario be an error rather than a 
> warning where the identifier is ignored?
Could you show a more concrete example or give more details on how possible 
incorrect behavior would lead to hard-to-find ODR issues?



Comment at: clang/include/clang/Sema/Sema.h:486
+: PackAttr(true), AlignMode(M), PackNumber(Num), XLStack(IsXL) {
+  assert(Num == PackNumber && "Unexpected value.");
+}

aaron.ballman wrote:
> The string literal here doesn't really convey what's being asserted -- it 
> took me a while to figure out that this is trying to catch truncation issues 
> when `Num` cannot be represented by an `unsigned char`.
Sorry for that, I would make it clearer.



Comment at: clang/include/clang/Sema/Sema.h:512
+static AlignPackInfo getFromRawEncoding(unsigned Encoding) {
+  static_assert(sizeof(AlignPackInfo) == sizeof(uint32_t),
+"Size is not correct");

aaron.ballman wrote:
> I would feel more comfortable with this assertion if the class was using 
> bit-fields to pack the values together. As it stands, we're kind of hoping 
> that `bool`, `Mode`, and `unsigned char` will all pack in a particular way 
> (and `bool`'s representation is implementation-defined).
Yeah, good point. I will move it back to previous bitfields version.



Comment at: clang/include/clang/Sema/Sema.h:527
+
+unsigned char getPackNumber() const { return PackNumber; }
+

aaron.ballman wrote:
> Given that the ctor takes an `int` for this value, should this be returning 
> an `int`?
As we know the pack number would not exceed 16 bytes, so the intention of using 
`unsigned char` here is to save space taken by AlignPackInfo. And that's why I 
added the diagnostics and assertion in the ctor to make sure the value won't be 
truncated.



Comment at: clang/test/Sema/aix-pragma-pack-and-align.c:231
+
+// expected-no-warning

aaron.ballman wrote:
> Is this comment intentional?
Yes, my intention is to test no pragma pack or prama align is unterminated.


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

https://reviews.llvm.org/D87702

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

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:858
+def err_pragma_pack_invalid_alignment : Error<
+  "expected #pragma pack parameter to be '1', '2', '4', '8', or '16'">;
 def warn_pragma_pack_non_default_at_include : Warning<





Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:893
+def warn_pragma_pack_identifer_not_supported : Warning<
+  "specifying an identifier within pragma pack is not supported, identifier is 
ignored">,
+  InGroup;

If the user wrote an identifier, it seems like there's a strong chance that 
ignoring the identifier will result in incorrect behavior that would lead to 
hard-to-find ODR issues. Should this scenario be an error rather than a warning 
where the identifier is ignored?



Comment at: clang/include/clang/Sema/Sema.h:486
+: PackAttr(true), AlignMode(M), PackNumber(Num), XLStack(IsXL) {
+  assert(Num == PackNumber && "Unexpected value.");
+}

The string literal here doesn't really convey what's being asserted -- it took 
me a while to figure out that this is trying to catch truncation issues when 
`Num` cannot be represented by an `unsigned char`.



Comment at: clang/include/clang/Sema/Sema.h:494
+
+AlignPackInfo(bool IsXL) : AlignPackInfo(Native, IsXL) {}
+





Comment at: clang/include/clang/Sema/Sema.h:512
+static AlignPackInfo getFromRawEncoding(unsigned Encoding) {
+  static_assert(sizeof(AlignPackInfo) == sizeof(uint32_t),
+"Size is not correct");

I would feel more comfortable with this assertion if the class was using 
bit-fields to pack the values together. As it stands, we're kind of hoping that 
`bool`, `Mode`, and `unsigned char` will all pack in a particular way (and 
`bool`'s representation is implementation-defined).



Comment at: clang/include/clang/Sema/Sema.h:527
+
+unsigned char getPackNumber() const { return PackNumber; }
+

Given that the ctor takes an `int` for this value, should this be returning an 
`int`?



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1314
 void ItaniumRecordLayoutBuilder::InitializeLayout(const Decl *D) {
+
   if (const RecordDecl *RD = dyn_cast(D)) {

Unintended whitespace change?



Comment at: clang/lib/Sema/SemaAttr.cpp:69-71
+  if (IsXLPragma && M == AlignPackInfo::Natural) {
+RD->addAttr(AlignNaturalAttr::CreateImplicit(Context));
   }





Comment at: clang/test/Sema/aix-pragma-pack-and-align.c:231
+
+// expected-no-warning

Is this comment intentional?


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

https://reviews.llvm.org/D87702

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

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 314141.
Xiangling_L marked 6 inline comments as done.
Xiangling_L added a comment.

- used memcpy to burn AlignPackInfo into uint32_t and vice versa;
- named XL on AIX stack effect and related options after  "xl" instead of "aix"
- adjusted natural align related code


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

https://reviews.llvm.org/D87702

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTReader.h
  clang/include/clang/Serialization/ASTWriter.h
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Driver/aix-pragma-pack.c
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-power-natural-interaction.cpp
  clang/test/PCH/aix-pragma-pack.c
  clang/test/Sema/aix-pragma-pack-and-align.c

Index: clang/test/Sema/aix-pragma-pack-and-align.c
===
--- /dev/null
+++ clang/test/Sema/aix-pragma-pack-and-align.c
@@ -0,0 +1,231 @@
+// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -fxl-pragma-pack -verify -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
+
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -fxl-pragma-pack -verify -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
+
+namespace test1 {
+#pragma align(natural)
+#pragma pack(4)
+#pragma pack(2)
+struct A {
+  int i;
+  double d;
+};
+
+int a = sizeof(A);
+#pragma pack()
+#pragma pack(show) // expected-warning {{value of #pragma pack(show) == 4}}
+#pragma pack(pop)
+#pragma pack(show) // expected-warning {{value of #pragma pack(show) == 8}}
+struct B {
+  int i;
+  double d;
+};
+#pragma align(reset)
+
+int b = sizeof(B);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test1::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=2, preferredalign=2,
+// CHECK-NEXT:|  nvsize=12, nvalign=2, preferrednvalign=2]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test1::B
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  8 |   double d
+// CHECK-NEXT:| [sizeof=16, dsize=16, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=16, nvalign=4, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+#pragma align(natural)
+#pragma pack(2)
+struct A {
+  int i;
+  double d;
+};
+
+int a = sizeof(A);
+#pragma align(reset)
+
+struct B {
+  int i;
+  double d;
+};
+
+int b = sizeof(B);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test2::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=2, preferredalign=2,
+// CHECK-NEXT:|  nvsize=12, nvalign=2, preferrednvalign=2]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test2::B
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+
+} // namespace test2
+
+namespace test3 {
+#pragma pack(2)
+#pragma align(natural)
+struct A {
+  double d;
+};
+#pragma align(reset)
+#pragma pack(pop)
+
+int a = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test3::A
+// CHECK-NEXT:  0 |   double d
+// CHECK-NEXT:| [sizeof=8, dsize=8, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=8, nvalign=4, preferrednvalign=8]
+
+} // namespace test3
+
+namespace test4 {
+#pragma pack(2)
+#pragma align(natural)
+#pragma pack(pop)
+
+struct A {
+  int i;
+  double d;
+} a;
+#pragma align(reset)
+#pragma pack(pop)
+
+int i = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test4::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  8 |   double d
+// CHECK-NEXT:| [sizeof=16, dsize=16, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=16, nvalign=4, preferrednvalign=8]
+
+} // namespace test4
+
+namespace test5 {
+#pragma align(power)
+#pragma align(natural)
+#pragma pack(2)
+#pragma align(reset)
+struct A {
+  int i;
+  double d;
+};
+#pragma align(reset)
+
+int a = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test5::A
+// 

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 313986.
Xiangling_L marked an inline comment as done.
Xiangling_L added a comment.

Rebase on renaming NFC patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87702

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTReader.h
  clang/include/clang/Serialization/ASTWriter.h
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Driver/aix-pragma-pack.c
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-power-natural-interaction.cpp
  clang/test/PCH/aix-pragma-pack.c
  clang/test/Sema/aix-pragma-pack-and-align.c

Index: clang/test/Sema/aix-pragma-pack-and-align.c
===
--- /dev/null
+++ clang/test/Sema/aix-pragma-pack-and-align.c
@@ -0,0 +1,231 @@
+// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -faix-pragma-pack -verify -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
+
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -faix-pragma-pack -verify -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
+
+namespace test1 {
+#pragma align(natural)
+#pragma pack(4)
+#pragma pack(2)
+struct A {
+  int i;
+  double d;
+};
+
+int a = sizeof(A);
+#pragma pack()
+#pragma pack(show) // expected-warning {{value of #pragma pack(show) == 4}}
+#pragma pack(pop)
+#pragma pack(show) // expected-warning {{value of #pragma pack(show) == 8}}
+struct B {
+  int i;
+  double d;
+};
+#pragma align(reset)
+
+int b = sizeof(B);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test1::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=2, preferredalign=2,
+// CHECK-NEXT:|  nvsize=12, nvalign=2, preferrednvalign=2]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test1::B
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  8 |   double d
+// CHECK-NEXT:| [sizeof=16, dsize=16, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=16, nvalign=4, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+#pragma align(natural)
+#pragma pack(2)
+struct A {
+  int i;
+  double d;
+};
+
+int a = sizeof(A);
+#pragma align(reset)
+
+struct B {
+  int i;
+  double d;
+};
+
+int b = sizeof(B);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test2::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=2, preferredalign=2,
+// CHECK-NEXT:|  nvsize=12, nvalign=2, preferrednvalign=2]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test2::B
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+
+} // namespace test2
+
+namespace test3 {
+#pragma pack(2)
+#pragma align(natural)
+struct A {
+  double d;
+};
+#pragma align(reset)
+#pragma pack(pop)
+
+int a = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test3::A
+// CHECK-NEXT:  0 |   double d
+// CHECK-NEXT:| [sizeof=8, dsize=8, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=8, nvalign=4, preferrednvalign=8]
+
+} // namespace test3
+
+namespace test4 {
+#pragma pack(2)
+#pragma align(natural)
+#pragma pack(pop)
+
+struct A {
+  int i;
+  double d;
+} a;
+#pragma align(reset)
+#pragma pack(pop)
+
+int i = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test4::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  8 |   double d
+// CHECK-NEXT:| [sizeof=16, dsize=16, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=16, nvalign=4, preferrednvalign=8]
+
+} // namespace test4
+
+namespace test5 {
+#pragma align(power)
+#pragma align(natural)
+#pragma pack(2)
+#pragma align(reset)
+struct A {
+  int i;
+  double d;
+};
+#pragma align(reset)
+
+int a = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test5::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, 

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L marked 2 inline comments as done.
Xiangling_L added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:637
   PragmaStack VtorDispStack;
-  // #pragma pack.
-  // Sentinel to represent when the stack is set to mac68k alignment.
-  static const unsigned kMac68kAlignmentSentinel = ~0U;
-  PragmaStack PackStack;
+  PragmaStack PackStack;
   // The current #pragma pack values and locations at each #include.

jasonliu wrote:
> Xiangling_L wrote:
> > jasonliu wrote:
> > > We should consider renaming PackStack to AlignPackStack across Clang. 
> > > Maybe even as a NFC first. As it is right now, clang already uses one 
> > > stack to record those two informations from `Pragma align` and `Pragma 
> > > pack`. Leave it as PackStack is too confusing, and people could actually 
> > > ignore the pragma Align when developing with PackStack. 
> > That's a good point. I will create a NFC accordingly.
> Please post this NFC when you have time.
Thanks for reminding me of this. A NFC patch is posted here: 
https://reviews.llvm.org/D93901


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

https://reviews.llvm.org/D87702

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

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:505
+// AlignPackInfo::getFromRawEncoding, it should not be inspected directly.
+uint32_t getRawEncoding(const AlignPackInfo ) const {
+  std::uint32_t Encoding{};

this getter do not need to take any parameters. You could just use `*this`.
Or, to make it consistent with the getFromRawEncoding, we could make this a 
static function as well and keep the parameter.



Comment at: clang/include/clang/Sema/Sema.h:506
+uint32_t getRawEncoding(const AlignPackInfo ) const {
+  std::uint32_t Encoding{};
+  if (Info.IsAIXStack())

Since this new structure have the same size as the uint32_t, have you consider 
using memcpy to burn the data directly into the uint32_t, and vice versa?



Comment at: clang/include/clang/Sema/Sema.h:570
+/// \brief True if it is a AIX #pragma align/pack stack.
+bool AIXStack;
+

It might be better to rename it XLStack, as this is the XL compiler behavior on 
AIX. Other compilers on AIX, like gcc, could have different behavior.



Comment at: clang/include/clang/Sema/Sema.h:637
   PragmaStack VtorDispStack;
-  // #pragma pack.
-  // Sentinel to represent when the stack is set to mac68k alignment.
-  static const unsigned kMac68kAlignmentSentinel = ~0U;
-  PragmaStack PackStack;
+  PragmaStack PackStack;
   // The current #pragma pack values and locations at each #include.

Xiangling_L wrote:
> jasonliu wrote:
> > We should consider renaming PackStack to AlignPackStack across Clang. Maybe 
> > even as a NFC first. As it is right now, clang already uses one stack to 
> > record those two informations from `Pragma align` and `Pragma pack`. Leave 
> > it as PackStack is too confusing, and people could actually ignore the 
> > pragma Align when developing with PackStack. 
> That's a good point. I will create a NFC accordingly.
Please post this NFC when you have time.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:699-700
+IsMac68kAlign(false),
+IsNaturalAlign(!Context.getTargetInfo().getTriple().isOSAIX() ? true
+  : false),
+IsMsStruct(false), UnfilledBitsInLastUnit(0),





Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1322
   HandledFirstNonOverlappingEmptyField =
-  !Context.getTargetInfo().defaultsToAIXPowerAlignment();
+  !Context.getTargetInfo().defaultsToAIXPowerAlignment() || IsNaturalAlign;
 

Not sure if this would have a real effect, but reading this code concerns me a 
little bit:
We would set `HandledFirstNonOverlappingEmptyField` based on `IsNaturalAlign`, 
but for AIX, that bit might only get set at line 1343.
It would look like we have a read before write to the variable here. 



Comment at: clang/lib/Sema/SemaAttr.cpp:232
 Action = Sema::PSK_Push_Set;
-Alignment = 0;
+if (IsAIX)
+  ModeVal = AlignPackInfo::Natural;

XLPragmaPack is used to control pack and align stack semantic behavior.
Someone on AIX might want to turn off this compatible pack and align with XL 
behavior (to get the compatible behavior with clang on other platform for 
easier porting purpose), but they would still prefer to have the natural 
alignment to work correctly.
So it might not be desirable to query this option for this natural alignment 
behavior.



Comment at: clang/lib/Sema/SemaAttr.cpp:413
+  // FIXME: PackStack may contain both #pragma align and #pragma pack
+  // information, we should warn about both modified align mode and alignment.
   if (PrevPackState.CurrentValue != PackStack.CurrentValue) {

I don't think there is a preferable solution yet. Pointing out what could go 
wrong is good enough here.
Same for the FIXME below.


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

https://reviews.llvm.org/D87702

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

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 308065.
Xiangling_L added a comment.

Adjusted the logic of operator == for class AlignPackInfo;
Removed mac68k sentinel;


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

https://reviews.llvm.org/D87702

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTReader.h
  clang/include/clang/Serialization/ASTWriter.h
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Driver/aix-pragma-pack.c
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-power-natural-interaction.cpp
  clang/test/PCH/aix-pragma-pack.c
  clang/test/Sema/aix-pragma-pack-and-align.c

Index: clang/test/Sema/aix-pragma-pack-and-align.c
===
--- /dev/null
+++ clang/test/Sema/aix-pragma-pack-and-align.c
@@ -0,0 +1,231 @@
+// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -faix-pragma-pack -verify -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
+
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -faix-pragma-pack -verify -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
+
+namespace test1 {
+#pragma align(natural)
+#pragma pack(4)
+#pragma pack(2)
+struct A {
+  int i;
+  double d;
+};
+
+int a = sizeof(A);
+#pragma pack()
+#pragma pack(show) // expected-warning {{value of #pragma pack(show) == 4}}
+#pragma pack(pop)
+#pragma pack(show) // expected-warning {{value of #pragma pack(show) == 8}}
+struct B {
+  int i;
+  double d;
+};
+#pragma align(reset)
+
+int b = sizeof(B);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test1::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=2, preferredalign=2,
+// CHECK-NEXT:|  nvsize=12, nvalign=2, preferrednvalign=2]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test1::B
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  8 |   double d
+// CHECK-NEXT:| [sizeof=16, dsize=16, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=16, nvalign=4, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+#pragma align(natural)
+#pragma pack(2)
+struct A {
+  int i;
+  double d;
+};
+
+int a = sizeof(A);
+#pragma align(reset)
+
+struct B {
+  int i;
+  double d;
+};
+
+int b = sizeof(B);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test2::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=2, preferredalign=2,
+// CHECK-NEXT:|  nvsize=12, nvalign=2, preferrednvalign=2]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test2::B
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+
+} // namespace test2
+
+namespace test3 {
+#pragma pack(2)
+#pragma align(natural)
+struct A {
+  double d;
+};
+#pragma align(reset)
+#pragma pack(pop)
+
+int a = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test3::A
+// CHECK-NEXT:  0 |   double d
+// CHECK-NEXT:| [sizeof=8, dsize=8, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=8, nvalign=4, preferrednvalign=8]
+
+} // namespace test3
+
+namespace test4 {
+#pragma pack(2)
+#pragma align(natural)
+#pragma pack(pop)
+
+struct A {
+  int i;
+  double d;
+} a;
+#pragma align(reset)
+#pragma pack(pop)
+
+int i = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test4::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  8 |   double d
+// CHECK-NEXT:| [sizeof=16, dsize=16, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=16, nvalign=4, preferrednvalign=8]
+
+} // namespace test4
+
+namespace test5 {
+#pragma align(power)
+#pragma align(natural)
+#pragma pack(2)
+#pragma align(reset)
+struct A {
+  int i;
+  double d;
+};
+#pragma align(reset)
+
+int a = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test5::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 307685.
Xiangling_L added a comment.

Update the logic to tell if two AlignPackInfo are equal;


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

https://reviews.llvm.org/D87702

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTReader.h
  clang/include/clang/Serialization/ASTWriter.h
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Driver/aix-pragma-pack.c
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-power-natural-interaction.cpp
  clang/test/PCH/aix-pragma-pack.c
  clang/test/Sema/aix-pragma-pack-and-align.c

Index: clang/test/Sema/aix-pragma-pack-and-align.c
===
--- /dev/null
+++ clang/test/Sema/aix-pragma-pack-and-align.c
@@ -0,0 +1,231 @@
+// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -faix-pragma-pack -verify -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
+
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -faix-pragma-pack -verify -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
+
+namespace test1 {
+#pragma align(natural)
+#pragma pack(4)
+#pragma pack(2)
+struct A {
+  int i;
+  double d;
+};
+
+int a = sizeof(A);
+#pragma pack()
+#pragma pack(show) // expected-warning {{value of #pragma pack(show) == 4}}
+#pragma pack(pop)
+#pragma pack(show) // expected-warning {{value of #pragma pack(show) == 8}}
+struct B {
+  int i;
+  double d;
+};
+#pragma align(reset)
+
+int b = sizeof(B);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test1::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=2, preferredalign=2,
+// CHECK-NEXT:|  nvsize=12, nvalign=2, preferrednvalign=2]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test1::B
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  8 |   double d
+// CHECK-NEXT:| [sizeof=16, dsize=16, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=16, nvalign=4, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+#pragma align(natural)
+#pragma pack(2)
+struct A {
+  int i;
+  double d;
+};
+
+int a = sizeof(A);
+#pragma align(reset)
+
+struct B {
+  int i;
+  double d;
+};
+
+int b = sizeof(B);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test2::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=2, preferredalign=2,
+// CHECK-NEXT:|  nvsize=12, nvalign=2, preferrednvalign=2]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test2::B
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+
+} // namespace test2
+
+namespace test3 {
+#pragma pack(2)
+#pragma align(natural)
+struct A {
+  double d;
+};
+#pragma align(reset)
+#pragma pack(pop)
+
+int a = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test3::A
+// CHECK-NEXT:  0 |   double d
+// CHECK-NEXT:| [sizeof=8, dsize=8, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=8, nvalign=4, preferrednvalign=8]
+
+} // namespace test3
+
+namespace test4 {
+#pragma pack(2)
+#pragma align(natural)
+#pragma pack(pop)
+
+struct A {
+  int i;
+  double d;
+} a;
+#pragma align(reset)
+#pragma pack(pop)
+
+int i = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test4::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  8 |   double d
+// CHECK-NEXT:| [sizeof=16, dsize=16, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=16, nvalign=4, preferrednvalign=8]
+
+} // namespace test4
+
+namespace test5 {
+#pragma align(power)
+#pragma align(natural)
+#pragma pack(2)
+#pragma align(reset)
+struct A {
+  int i;
+  double d;
+};
+#pragma align(reset)
+
+int a = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test5::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK-NEXT:| 

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:637
   PragmaStack VtorDispStack;
-  // #pragma pack.
-  // Sentinel to represent when the stack is set to mac68k alignment.
-  static const unsigned kMac68kAlignmentSentinel = ~0U;
-  PragmaStack PackStack;
+  PragmaStack PackStack;
   // The current #pragma pack values and locations at each #include.

jasonliu wrote:
> We should consider renaming PackStack to AlignPackStack across Clang. Maybe 
> even as a NFC first. As it is right now, clang already uses one stack to 
> record those two informations from `Pragma align` and `Pragma pack`. Leave it 
> as PackStack is too confusing, and people could actually ignore the pragma 
> Align when developing with PackStack. 
That's a good point. I will create a NFC accordingly.


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

https://reviews.llvm.org/D87702

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

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 307617.
Xiangling_L marked an inline comment as done.
Xiangling_L added a comment.
Herald added a subscriber: dexonsmith.

Addressed the comments;
Handle PCH for AIX;


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

https://reviews.llvm.org/D87702

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTReader.h
  clang/include/clang/Serialization/ASTWriter.h
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Driver/aix-pragma-pack.c
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-power-natural-interaction.cpp
  clang/test/PCH/aix-pragma-pack.c
  clang/test/Sema/aix-pragma-pack-and-align.c

Index: clang/test/Sema/aix-pragma-pack-and-align.c
===
--- /dev/null
+++ clang/test/Sema/aix-pragma-pack-and-align.c
@@ -0,0 +1,231 @@
+// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -faix-pragma-pack -verify -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
+
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -faix-pragma-pack -verify -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
+
+namespace test1 {
+#pragma align(natural)
+#pragma pack(4)
+#pragma pack(2)
+struct A {
+  int i;
+  double d;
+};
+
+int a = sizeof(A);
+#pragma pack()
+#pragma pack(show) // expected-warning {{value of #pragma pack(show) == 4}}
+#pragma pack(pop)
+#pragma pack(show) // expected-warning {{value of #pragma pack(show) == 8}}
+struct B {
+  int i;
+  double d;
+};
+#pragma align(reset)
+
+int b = sizeof(B);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test1::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=2, preferredalign=2,
+// CHECK-NEXT:|  nvsize=12, nvalign=2, preferrednvalign=2]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test1::B
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  8 |   double d
+// CHECK-NEXT:| [sizeof=16, dsize=16, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=16, nvalign=4, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+#pragma align(natural)
+#pragma pack(2)
+struct A {
+  int i;
+  double d;
+};
+
+int a = sizeof(A);
+#pragma align(reset)
+
+struct B {
+  int i;
+  double d;
+};
+
+int b = sizeof(B);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test2::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=2, preferredalign=2,
+// CHECK-NEXT:|  nvsize=12, nvalign=2, preferrednvalign=2]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test2::B
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+
+} // namespace test2
+
+namespace test3 {
+#pragma pack(2)
+#pragma align(natural)
+struct A {
+  double d;
+};
+#pragma align(reset)
+#pragma pack(pop)
+
+int a = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test3::A
+// CHECK-NEXT:  0 |   double d
+// CHECK-NEXT:| [sizeof=8, dsize=8, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=8, nvalign=4, preferrednvalign=8]
+
+} // namespace test3
+
+namespace test4 {
+#pragma pack(2)
+#pragma align(natural)
+#pragma pack(pop)
+
+struct A {
+  int i;
+  double d;
+} a;
+#pragma align(reset)
+#pragma pack(pop)
+
+int i = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test4::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  8 |   double d
+// CHECK-NEXT:| [sizeof=16, dsize=16, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=16, nvalign=4, preferrednvalign=8]
+
+} // namespace test4
+
+namespace test5 {
+#pragma align(power)
+#pragma align(natural)
+#pragma pack(2)
+#pragma align(reset)
+struct A {
+  int i;
+  double d;
+};
+#pragma align(reset)
+
+int a = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test5::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| 

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L marked 8 inline comments as done.
Xiangling_L added inline comments.



Comment at: clang/include/clang/Basic/LangOptions.def:340
 
+LANGOPT(AIXPragmaPack, 1, 0, "AIX #pragma pack handling")
+

jasonliu wrote:
> Not sure if AIXPragmaPack is the best name here. 
> It's more like IBM xl pragma pack handling on AIX.
> Would it be better if we name it `XLPragmaPackOnAIX`?
Just a record of the offline discussion: `XLPragmaPack` is sufficient here, 
following the convention of how we named our C++ ABI on AIX as XLABI.



Comment at: clang/include/clang/Sema/Sema.h:493
+  PackNumber(M == Packed ? 1
+ : (M == Mac68k ? Mac68kAlignmentSentinel
+: UninitPackVal)),

jasonliu wrote:
> I think one of the idea is to use `enum Mode::Mac68k` to replace the need of 
> Mac68kAlignmentSentinel.
> Is there any reason that you would still need PackNumber to contain 
> `Mac68kAlignmentSentinel`?
AIX and other targets have different ways to compare if two `CurrentValue` 
equal. Other targets use only `PackNumber ` while AIX use both align mode and 
PackNumber. So this sentinel `Mac68kAlignmentSentinel` is added to support this.



Comment at: clang/include/clang/Sema/Sema.h:515
+bool operator==(AlignPackInfo Info) const {
+  return AlignMode == Info.AlignMode && PackNumber == Info.PackNumber;
+}

jasonliu wrote:
> Xiangling_L wrote:
> > jasonliu wrote:
> > > This could return true when `PackAttr` in AlignPackInfo are not the same. 
> > > Wouldn't that cause an issue?
> > (1) You mean we have two `AlignPackInfo` with same AlignMode and 
> > PackNumber, but one is PackAttr and the other one is AlignAttr?
> > The example I can think of is:
> > 
> > 
> > ```
> > a)#pragma align(packed)
> >   #pragma pack(1)   //AlignMode = Packed, PackNumber = 1
> > 
> > b) #pragma align(packed)  //AlignMode = Packed, PackNumber = 1
> > ```
> > 
> > But I don't think we have any issue in this case. Before and after my 
> > patch, a == b.
> > Please let me know any other cases concerning you if any.
> > 
> > (2) However, your concerns leads me to think of another case, where 
> > behavior changes with my patch.
> > 
> > ```
> > a) 
> > #pragma align(natural)
> > #pragma pack(1)   /AlignMode = Native,  PackNumber = 1
> > 
> > b)
> > #pragma align(packed) ///AlignMode = Packed, PackNumber = 1
> > 
> > ```
> > Without this patch, a == b for other targets.
> > And I suppose a != b for AIX.
> > 
> In your first example, if I understand correctly,
> a) would return true for IsPackAttr()
> b) would return false for IsPackAttr()
> and yet a == b ?
> I think that's confusing. 
> 
> Any reason why you don't want to just compare all the data members to make 
> sure they are all equal?
Yes, it's confusing but your understanding is correct. For other targets, they 
actually only use `PackNumber` to compare if two CurrentValue equal.



Comment at: clang/lib/Sema/SemaAttr.cpp:367
+  // AIX pragma pack does not support identifier syntax.
+  if (getLangOpts().AIXPragmaPack && !SlotLabel.empty()) {
+Diag(PragmaLoc, diag::warn_pragma_pack_identifer_not_supported);

jasonliu wrote:
> Although IBM xl compiler does not support this form, do we see a harm for us 
> to support this form in clang on AIX?
> Also, if this is indeed not desired to support, we could move this check to 
> the top of this function for an early return. 
We may consider supporting this form in the future, but I don't think we need 
to cover it in this patch. And we don't support it by passing `StringRef()` 
instead, so we still need to wait for a `Info` constructed for us.



Comment at: clang/lib/Sema/SemaAttr.cpp:403
   // Warn about modified alignment after #includes.
   if (PrevPackState.CurrentValue != PackStack.CurrentValue) {
 Diag(IncludeLoc, diag::warn_pragma_pack_modified_after_include);

jasonliu wrote:
> Xiangling_L wrote:
> > jasonliu wrote:
> > > Since we changed the PackStack for it to contain AlignPackInfo instead of 
> > > unsigned. 
> > > This stack no longer only contains Pack information. So we need to 
> > > rethink about how this diagnostic and the one follows should work.
> > > i.e. What's the purpose of these diagnostic? Is it still only for pragma 
> > > pack report? If so, what we are doing here is not correct, since the 
> > > `CurrentValue` could be different, not because of the pragma pack change, 
> > > but because of the pragma align change.
> > > If it's not only for pragma pack any more, but also intend to detect the 
> > > pragma align interaction, then possibly function name and diagnostic 
> > > needs some modification, as they don't match the intent any more.
> > Thanks for pointing this out. I agree that what we are doing here is not 
> > correct. 
> > The original 

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/include/clang/Basic/LangOptions.def:340
 
+LANGOPT(AIXPragmaPack, 1, 0, "AIX #pragma pack handling")
+

Not sure if AIXPragmaPack is the best name here. 
It's more like IBM xl pragma pack handling on AIX.
Would it be better if we name it `XLPragmaPackOnAIX`?



Comment at: clang/include/clang/Sema/Sema.h:493
+  PackNumber(M == Packed ? 1
+ : (M == Mac68k ? Mac68kAlignmentSentinel
+: UninitPackVal)),

I think one of the idea is to use `enum Mode::Mac68k` to replace the need of 
Mac68kAlignmentSentinel.
Is there any reason that you would still need PackNumber to contain 
`Mac68kAlignmentSentinel`?



Comment at: clang/include/clang/Sema/Sema.h:497
+
+AlignPackInfo() : AlignPackInfo(Native, false) {}
+

We could have a empty stack on AIX that returns false when it was asked if it's 
an AIX stack?
It's a bit confusing here. 



Comment at: clang/include/clang/Sema/Sema.h:637
   PragmaStack VtorDispStack;
-  // #pragma pack.
-  // Sentinel to represent when the stack is set to mac68k alignment.
-  static const unsigned kMac68kAlignmentSentinel = ~0U;
-  PragmaStack PackStack;
+  PragmaStack PackStack;
   // The current #pragma pack values and locations at each #include.

We should consider renaming PackStack to AlignPackStack across Clang. Maybe 
even as a NFC first. As it is right now, clang already uses one stack to record 
those two informations from `Pragma align` and `Pragma pack`. Leave it as 
PackStack is too confusing, and people could actually ignore the pragma Align 
when developing with PackStack. 



Comment at: clang/include/clang/Sema/Sema.h:488
+AlignPackInfo(AlignPackInfo::Mode M, int Num, bool IsAIX)
+: PackAttr(true), AlignMode(M), PackNumber(Num), AIXStack(IsAIX) {}
+

Xiangling_L wrote:
> jasonliu wrote:
> > I noticed PackNumber is an unsigned char and we are passing an int type 
> > into it.
> > Could we add an assertion in the constructor to make sure Num would never 
> > be something that's going to get truncated when it converts to an unsigned 
> > char?
> I think the warning/error `expected #pragma pack parameter to be '1', '2', 
> '4', '8', or '16'` have already guaranteed that for us? Or maybe using 
> `unsigned int` makes people more comfortable?
Using a char instead of an int could make this class more compact, especially 
when we actually don't need to represent a big range. 
I understand there is a warning/error somewhere to prevent out of range from 
happening. But an assert here would make sure that no one could pass in an 
unexpected value into here by accident when developing. 



Comment at: clang/include/clang/Sema/Sema.h:515
+bool operator==(AlignPackInfo Info) const {
+  return AlignMode == Info.AlignMode && PackNumber == Info.PackNumber;
+}

Xiangling_L wrote:
> jasonliu wrote:
> > This could return true when `PackAttr` in AlignPackInfo are not the same. 
> > Wouldn't that cause an issue?
> (1) You mean we have two `AlignPackInfo` with same AlignMode and PackNumber, 
> but one is PackAttr and the other one is AlignAttr?
> The example I can think of is:
> 
> 
> ```
> a)#pragma align(packed)
>   #pragma pack(1)   //AlignMode = Packed, PackNumber = 1
> 
> b) #pragma align(packed)  //AlignMode = Packed, PackNumber = 1
> ```
> 
> But I don't think we have any issue in this case. Before and after my patch, 
> a == b.
> Please let me know any other cases concerning you if any.
> 
> (2) However, your concerns leads me to think of another case, where behavior 
> changes with my patch.
> 
> ```
> a) 
> #pragma align(natural)
> #pragma pack(1)   /AlignMode = Native,  PackNumber = 1
> 
> b)
> #pragma align(packed) ///AlignMode = Packed, PackNumber = 1
> 
> ```
> Without this patch, a == b for other targets.
> And I suppose a != b for AIX.
> 
In your first example, if I understand correctly,
a) would return true for IsPackAttr()
b) would return false for IsPackAttr()
and yet a == b ?
I think that's confusing. 

Any reason why you don't want to just compare all the data members to make sure 
they are all equal?



Comment at: clang/lib/Sema/SemaAttr.cpp:367
+  // AIX pragma pack does not support identifier syntax.
+  if (getLangOpts().AIXPragmaPack && !SlotLabel.empty()) {
+Diag(PragmaLoc, diag::warn_pragma_pack_identifer_not_supported);

Although IBM xl compiler does not support this form, do we see a harm for us to 
support this form in clang on AIX?
Also, if this is indeed not desired to support, we could move this check to the 
top of this function for an early return. 



Comment at: clang/lib/Sema/SemaAttr.cpp:403
   // Warn about modified alignment after #includes.
   if 

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 293846.
Xiangling_L marked 3 inline comments as done.
Xiangling_L added a comment.

Addressed the comments;


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

https://reviews.llvm.org/D87702

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Driver/aix-pragma-pack.c
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-power-natural-interaction.cpp
  clang/test/Sema/aix-pragma-pack-and-align.c

Index: clang/test/Sema/aix-pragma-pack-and-align.c
===
--- /dev/null
+++ clang/test/Sema/aix-pragma-pack-and-align.c
@@ -0,0 +1,231 @@
+// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -faix-pragma-pack -verify -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
+
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -faix-pragma-pack -verify -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
+
+namespace test1 {
+#pragma align(natural)
+#pragma pack(4)
+#pragma pack(2)
+struct A {
+  int i;
+  double d;
+};
+
+int a = sizeof(A);
+#pragma pack()
+#pragma pack(show) // expected-warning {{value of #pragma pack(show) == 4}}
+#pragma pack(pop)
+#pragma pack(show) // expected-warning {{value of #pragma pack(show) == 8}}
+struct B {
+  int i;
+  double d;
+};
+#pragma align(reset)
+
+int b = sizeof(B);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test1::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=2, preferredalign=2,
+// CHECK-NEXT:|  nvsize=12, nvalign=2, preferrednvalign=2]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test1::B
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  8 |   double d
+// CHECK-NEXT:| [sizeof=16, dsize=16, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=16, nvalign=4, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+#pragma align(natural)
+#pragma pack(2)
+struct A {
+  int i;
+  double d;
+};
+
+int a = sizeof(A);
+#pragma align(reset)
+
+struct B {
+  int i;
+  double d;
+};
+
+int b = sizeof(B);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test2::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=2, preferredalign=2,
+// CHECK-NEXT:|  nvsize=12, nvalign=2, preferrednvalign=2]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test2::B
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+
+} // namespace test2
+
+namespace test3 {
+#pragma pack(2)
+#pragma align(natural)
+struct A {
+  double d;
+};
+#pragma align(reset)
+#pragma pack(pop)
+
+int a = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test3::A
+// CHECK-NEXT:  0 |   double d
+// CHECK-NEXT:| [sizeof=8, dsize=8, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=8, nvalign=4, preferrednvalign=8]
+
+} // namespace test3
+
+namespace test4 {
+#pragma pack(2)
+#pragma align(natural)
+#pragma pack(pop)
+
+struct A {
+  int i;
+  double d;
+} a;
+#pragma align(reset)
+#pragma pack(pop)
+
+int i = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test4::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  8 |   double d
+// CHECK-NEXT:| [sizeof=16, dsize=16, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=16, nvalign=4, preferrednvalign=8]
+
+} // namespace test4
+
+namespace test5 {
+#pragma align(power)
+#pragma align(natural)
+#pragma pack(2)
+#pragma align(reset)
+struct A {
+  int i;
+  double d;
+};
+#pragma align(reset)
+
+int a = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test5::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+
+} // namespace test5
+
+namespace test6 {
+#pragma align(natural)

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L marked 9 inline comments as done.
Xiangling_L added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:488
+AlignPackInfo(AlignPackInfo::Mode M, int Num, bool IsAIX)
+: PackAttr(true), AlignMode(M), PackNumber(Num), AIXStack(IsAIX) {}
+

jasonliu wrote:
> I noticed PackNumber is an unsigned char and we are passing an int type into 
> it.
> Could we add an assertion in the constructor to make sure Num would never be 
> something that's going to get truncated when it converts to an unsigned char?
I think the warning/error `expected #pragma pack parameter to be '1', '2', '4', 
'8', or '16'` have already guaranteed that for us? Or maybe using `unsigned 
int` makes people more comfortable?



Comment at: clang/include/clang/Sema/Sema.h:515
+bool operator==(AlignPackInfo Info) const {
+  return AlignMode == Info.AlignMode && PackNumber == Info.PackNumber;
+}

jasonliu wrote:
> This could return true when `PackAttr` in AlignPackInfo are not the same. 
> Wouldn't that cause an issue?
(1) You mean we have two `AlignPackInfo` with same AlignMode and PackNumber, 
but one is PackAttr and the other one is AlignAttr?
The example I can think of is:


```
a)#pragma align(packed)
  #pragma pack(1)   //AlignMode = Packed, PackNumber = 1

b) #pragma align(packed)  //AlignMode = Packed, PackNumber = 1
```

But I don't think we have any issue in this case. Before and after my patch, a 
== b.
Please let me know any other cases concerning you if any.

(2) However, your concerns leads me to think of another case, where behavior 
changes with my patch.

```
a) 
#pragma align(natural)
#pragma pack(1)   /AlignMode = Native,  PackNumber = 1

b)
#pragma align(packed) ///AlignMode = Packed, PackNumber = 1

```
Without this patch, a == b for other targets.
And I suppose a != b for AIX.




Comment at: clang/lib/Sema/SemaAttr.cpp:403
   // Warn about modified alignment after #includes.
   if (PrevPackState.CurrentValue != PackStack.CurrentValue) {
 Diag(IncludeLoc, diag::warn_pragma_pack_modified_after_include);

jasonliu wrote:
> Since we changed the PackStack for it to contain AlignPackInfo instead of 
> unsigned. 
> This stack no longer only contains Pack information. So we need to rethink 
> about how this diagnostic and the one follows should work.
> i.e. What's the purpose of these diagnostic? Is it still only for pragma pack 
> report? If so, what we are doing here is not correct, since the 
> `CurrentValue` could be different, not because of the pragma pack change, but 
> because of the pragma align change.
> If it's not only for pragma pack any more, but also intend to detect the 
> pragma align interaction, then possibly function name and diagnostic needs 
> some modification, as they don't match the intent any more.
Thanks for pointing this out. I agree that what we are doing here is not 
correct. 
The original commit[45b40147117668ce65bff4f6a240bdae4ad4bf7d] message shows:

```
This commit adds a new -Wpragma-pack warning. It warns in the following 
cases:

- When a translation unit is missing terminating #pragma pack (pop) 
directives.
- When entering an included file if the current alignment value as 
determined
  by '#pragma pack' directives is different from the default alignment 
value.
- When leaving an included file that changed the state of the current 
alignment
  value.
```

So it looks these warnings are used only for `pragma pack`.


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

https://reviews.llvm.org/D87702

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

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:66
 #include "llvm/Frontend/OpenMP/OMPConstants.h"
+#include 
 #include 

Do we need cmath?



Comment at: clang/include/clang/Sema/Sema.h:488
+AlignPackInfo(AlignPackInfo::Mode M, int Num, bool IsAIX)
+: PackAttr(true), AlignMode(M), PackNumber(Num), AIXStack(IsAIX) {}
+

I noticed PackNumber is an unsigned char and we are passing an int type into it.
Could we add an assertion in the constructor to make sure Num would never be 
something that's going to get truncated when it converts to an unsigned char?



Comment at: clang/include/clang/Sema/Sema.h:504
+unsigned getPackNumber() const { return PackNumber; }
+void setPackNumber(int Num) { PackNumber = Num; }
+

I don't see this function gets referenced in this patch. 



Comment at: clang/include/clang/Sema/Sema.h:514
+
+bool operator==(AlignPackInfo Info) const {
+  return AlignMode == Info.AlignMode && PackNumber == Info.PackNumber;

I think a normal operator== signature would look like this:
bool operator==(const AlignPackInfo ) const;



Comment at: clang/include/clang/Sema/Sema.h:515
+bool operator==(AlignPackInfo Info) const {
+  return AlignMode == Info.AlignMode && PackNumber == Info.PackNumber;
+}

This could return true when `PackAttr` in AlignPackInfo are not the same. 
Wouldn't that cause an issue?



Comment at: clang/include/clang/Sema/Sema.h:519
+bool operator!=(AlignPackInfo Info) const {
+  return AlignMode != Info.AlignMode || PackNumber != Info.PackNumber;
+}

You should be able to implement this using "operator==" right?
e.g. return !(*this == Info);




Comment at: clang/include/clang/Sema/Sema.h:524
+/// \brief True if this is a pragma pack attribute,
+// not a pragma align attribute.
+bool PackAttr;





Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:697
 InferAlignment(false), Packed(false), IsUnion(false),
-IsMac68kAlign(false), IsMsStruct(false), UnfilledBitsInLastUnit(0),
-LastBitfieldTypeSize(0), MaxFieldAlignment(CharUnits::Zero()),
-DataSize(0), NonVirtualSize(CharUnits::Zero()),
+IsMac68kAlign(false), IsNaturalAlign(false), IsMsStruct(false),
+UnfilledBitsInLastUnit(0), LastBitfieldTypeSize(0),

This could probably confuse people, as natural alignment is the default on most 
target except AIX.



Comment at: clang/lib/Sema/SemaAttr.cpp:403
   // Warn about modified alignment after #includes.
   if (PrevPackState.CurrentValue != PackStack.CurrentValue) {
 Diag(IncludeLoc, diag::warn_pragma_pack_modified_after_include);

Since we changed the PackStack for it to contain AlignPackInfo instead of 
unsigned. 
This stack no longer only contains Pack information. So we need to rethink 
about how this diagnostic and the one follows should work.
i.e. What's the purpose of these diagnostic? Is it still only for pragma pack 
report? If so, what we are doing here is not correct, since the `CurrentValue` 
could be different, not because of the pragma pack change, but because of the 
pragma align change.
If it's not only for pragma pack any more, but also intend to detect the pragma 
align interaction, then possibly function name and diagnostic needs some 
modification, as they don't match the intent any more.


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

https://reviews.llvm.org/D87702

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

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 292533.
Xiangling_L added a comment.

Fix the PragmaStack is empty assertion failure when we do: #pragma pack(2) then 
#pragma align(reset)


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

https://reviews.llvm.org/D87702

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Driver/aix-pragma-pack.c
  clang/test/Layout/aix-double-struct-member.cpp
  clang/test/Layout/aix-power-natural-interaction.cpp
  clang/test/Sema/aix-pragma-pack-and-align.c

Index: clang/test/Sema/aix-pragma-pack-and-align.c
===
--- /dev/null
+++ clang/test/Sema/aix-pragma-pack-and-align.c
@@ -0,0 +1,231 @@
+// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -faix-pragma-pack -verify -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
+
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -faix-pragma-pack -verify -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
+
+namespace test1 {
+#pragma align(natural)
+#pragma pack(4)
+#pragma pack(2)
+struct A {
+  int i;
+  double d;
+};
+
+int a = sizeof(A);
+#pragma pack()
+#pragma pack(show) // expected-warning {{value of #pragma pack(show) == 4}}
+#pragma pack(pop)
+#pragma pack(show) // expected-warning {{value of #pragma pack(show) == 8}}
+struct B {
+  int i;
+  double d;
+};
+#pragma align(reset)
+
+int b = sizeof(B);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test1::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=2, preferredalign=2,
+// CHECK-NEXT:|  nvsize=12, nvalign=2, preferrednvalign=2]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test1::B
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  8 |   double d
+// CHECK-NEXT:| [sizeof=16, dsize=16, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=16, nvalign=4, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+#pragma align(natural)
+#pragma pack(2)
+struct A {
+  int i;
+  double d;
+};
+
+int a = sizeof(A);
+#pragma align(reset)
+
+struct B {
+  int i;
+  double d;
+};
+
+int b = sizeof(B);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test2::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=2, preferredalign=2,
+// CHECK-NEXT:|  nvsize=12, nvalign=2, preferrednvalign=2]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test2::B
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+
+} // namespace test2
+
+namespace test3 {
+#pragma pack(2)
+#pragma align(natural)
+struct A {
+  double d;
+};
+#pragma align(reset)
+#pragma pack(pop)
+
+int a = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test3::A
+// CHECK-NEXT:  0 |   double d
+// CHECK-NEXT:| [sizeof=8, dsize=8, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=8, nvalign=4, preferrednvalign=8]
+
+} // namespace test3
+
+namespace test4 {
+#pragma pack(2)
+#pragma align(natural)
+#pragma pack(pop)
+
+struct A {
+  int i;
+  double d;
+} a;
+#pragma align(reset)
+#pragma pack(pop)
+
+int i = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test4::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  8 |   double d
+// CHECK-NEXT:| [sizeof=16, dsize=16, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=16, nvalign=4, preferrednvalign=8]
+
+} // namespace test4
+
+namespace test5 {
+#pragma align(power)
+#pragma align(natural)
+#pragma pack(2)
+#pragma align(reset)
+struct A {
+  int i;
+  double d;
+};
+#pragma align(reset)
+
+int a = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test5::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+
+} // namespace test5
+
+namespace test6 

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 291950.
Xiangling_L added a comment.

Removed redundant header file include;


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

https://reviews.llvm.org/D87702

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Driver/aix-pragma-pack.c
  clang/test/Layout/aix-power-natural-interaction.cpp
  clang/test/Sema/aix-pragma-pack-and-align.c

Index: clang/test/Sema/aix-pragma-pack-and-align.c
===
--- /dev/null
+++ clang/test/Sema/aix-pragma-pack-and-align.c
@@ -0,0 +1,212 @@
+// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -faix-pragma-pack -verify -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
+
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -faix-pragma-pack -verify -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
+
+namespace test1 {
+#pragma align(natural)
+#pragma pack(4)
+#pragma pack(2)
+struct A {
+  int i;
+  double d;
+};
+
+int a = sizeof(A);
+#pragma pack()
+#pragma pack(show) // expected-warning {{value of #pragma pack(show) == 4}}
+#pragma pack(pop)
+#pragma pack(show) // expected-warning {{value of #pragma pack(show) == 8}}
+struct B {
+  int i;
+  double d;
+};
+#pragma align(reset)
+
+int b = sizeof(B);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test1::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=2, preferredalign=2,
+// CHECK-NEXT:|  nvsize=12, nvalign=2, preferrednvalign=2]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test1::B
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  8 |   double d
+// CHECK-NEXT:| [sizeof=16, dsize=16, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=16, nvalign=4, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+#pragma align(natural)
+#pragma pack(2)
+struct A {
+  int i;
+  double d;
+};
+
+int a = sizeof(A);
+#pragma align(reset)
+
+struct B {
+  int i;
+  double d;
+};
+
+int b = sizeof(B);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test2::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=2, preferredalign=2,
+// CHECK-NEXT:|  nvsize=12, nvalign=2, preferrednvalign=2]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test2::B
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+
+} // namespace test2
+
+namespace test3 {
+#pragma pack(2)
+#pragma align(natural)
+struct A {
+  double d;
+};
+#pragma align(reset)
+#pragma pack(pop)
+
+int a = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test3::A
+// CHECK-NEXT:  0 |   double d
+// CHECK-NEXT:| [sizeof=8, dsize=8, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=8, nvalign=4, preferrednvalign=8]
+
+} // namespace test3
+
+namespace test4 {
+#pragma pack(2)
+#pragma align(natural)
+#pragma pack(pop)
+
+struct A {
+  int i;
+  double d;
+} a;
+#pragma align(reset)
+#pragma pack(pop)
+
+int i = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test4::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  8 |   double d
+// CHECK-NEXT:| [sizeof=16, dsize=16, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=16, nvalign=4, preferrednvalign=8]
+
+} // namespace test4
+
+namespace test5 {
+#pragma align(power)
+#pragma align(natural)
+#pragma pack(2)
+#pragma align(reset)
+struct A {
+  int i;
+  double d;
+};
+#pragma align(reset)
+
+int a = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test5::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+
+} // namespace test5
+
+namespace test6 {
+#pragma align(natural)
+#pragma pack(0)// expected-error {{expected #pragma pack parameter to be '1', 

[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L created this revision.
Xiangling_L added reviewers: jasonliu, hubert.reinterpretcast, efriedma, 
jyknight, rnk, rsmith, aaron.ballman.
Herald added subscribers: cfe-commits, dang.
Herald added a project: clang.
Xiangling_L requested review of this revision.

1. Implementing the natural align for AIX
2. Sort out pragma pack stack effect
3. Add -faix-pragma-stack option to enable AIX pragma stack effect


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D87702

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Driver/aix-pragma-pack.c
  clang/test/Layout/aix-power-natural-interaction.cpp
  clang/test/Sema/aix-pragma-pack-and-align.c

Index: clang/test/Sema/aix-pragma-pack-and-align.c
===
--- /dev/null
+++ clang/test/Sema/aix-pragma-pack-and-align.c
@@ -0,0 +1,212 @@
+// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -faix-pragma-pack -verify -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
+
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -fdump-record-layouts \
+// RUN: -faix-pragma-pack -verify -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
+
+namespace test1 {
+#pragma align(natural)
+#pragma pack(4)
+#pragma pack(2)
+struct A {
+  int i;
+  double d;
+};
+
+int a = sizeof(A);
+#pragma pack()
+#pragma pack(show) // expected-warning {{value of #pragma pack(show) == 4}}
+#pragma pack(pop)
+#pragma pack(show) // expected-warning {{value of #pragma pack(show) == 8}}
+struct B {
+  int i;
+  double d;
+};
+#pragma align(reset)
+
+int b = sizeof(B);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test1::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=2, preferredalign=2,
+// CHECK-NEXT:|  nvsize=12, nvalign=2, preferrednvalign=2]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test1::B
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  8 |   double d
+// CHECK-NEXT:| [sizeof=16, dsize=16, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=16, nvalign=4, preferrednvalign=8]
+
+} // namespace test1
+
+namespace test2 {
+#pragma align(natural)
+#pragma pack(2)
+struct A {
+  int i;
+  double d;
+};
+
+int a = sizeof(A);
+#pragma align(reset)
+
+struct B {
+  int i;
+  double d;
+};
+
+int b = sizeof(B);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test2::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=2, preferredalign=2,
+// CHECK-NEXT:|  nvsize=12, nvalign=2, preferrednvalign=2]
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test2::B
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  4 |   double d
+// CHECK-NEXT:| [sizeof=12, dsize=12, align=4, preferredalign=4,
+// CHECK-NEXT:|  nvsize=12, nvalign=4, preferrednvalign=4]
+
+} // namespace test2
+
+namespace test3 {
+#pragma pack(2)
+#pragma align(natural)
+struct A {
+  double d;
+};
+#pragma align(reset)
+#pragma pack(pop)
+
+int a = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test3::A
+// CHECK-NEXT:  0 |   double d
+// CHECK-NEXT:| [sizeof=8, dsize=8, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=8, nvalign=4, preferrednvalign=8]
+
+} // namespace test3
+
+namespace test4 {
+#pragma pack(2)
+#pragma align(natural)
+#pragma pack(pop)
+
+struct A {
+  int i;
+  double d;
+} a;
+#pragma align(reset)
+#pragma pack(pop)
+
+int i = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test4::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT:  8 |   double d
+// CHECK-NEXT:| [sizeof=16, dsize=16, align=4, preferredalign=8,
+// CHECK-NEXT:|  nvsize=16, nvalign=4, preferrednvalign=8]
+
+} // namespace test4
+
+namespace test5 {
+#pragma align(power)
+#pragma align(natural)
+#pragma pack(2)
+#pragma align(reset)
+struct A {
+  int i;
+  double d;
+};
+#pragma align(reset)
+
+int a = sizeof(A);
+
+// CHECK:  *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test5::A
+// CHECK-NEXT:  0 |   int i
+// CHECK-NEXT: