DHowett-MSFT created this revision.
Herald added a project: All.
DHowett-MSFT updated this revision to Diff 495195.
DHowett-MSFT added a comment.
DHowett-MSFT published this revision for review.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

- Add a test


DHowett-MSFT added a comment.

This is a fix for https://github.com/llvm/llvm-project/issues/60543.

I originally approached this fix by moving `PragmaPackOptions` up to 
`Lex/Token.h` to live near `PragmaLoopHintInfo`; however, that required moving 
much more of the `PragmaMsStack` machinery than I was comfortable with.

There is precedent for some pragma infos living in `Sema` as well as 
`ASTReader`/`ASTWriter` poking into `Sema` to do their jobs.

I've included a test that both validates that the frontend no longer crashes 
_and_ that the resultant structures are packed as expected.


Without this, GCH serialization of late template expansions will
encounter an assertion failure.

Changes were required to ASTReader::ReadString to add support for
reading a string from a RecordDataImpl (which is the type consumed by
ReadToken) rather than a RecordData.

`#pragma pack` slot names are read into the preexisting string storage
PragmaAlignPackStrings.

This requires incidental changes [Parse,Sema] to move PragmaPackInfo to Sema

There is precedent for ASTReader/ASTWriter to poke into Sema.

Fixes https://github.com/llvm/llvm-project/issues/60543


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D143410

Files:
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/PCH/delayed-template-with-pragma-pack.cpp

Index: clang/test/PCH/delayed-template-with-pragma-pack.cpp
===================================================================
--- /dev/null
+++ clang/test/PCH/delayed-template-with-pragma-pack.cpp
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 -emit-pch -o %t.pch %s
+// RUN: %clang_cc1 -fdelayed-template-parsing -emit-pch -o %t.delayed.pch %s
+// RUN: %clang_cc1 -DMAIN_FILE \
+// RUN:   -include-pch %t.pch \
+// RUN:   -emit-llvm -verify -o - %s | FileCheck %s
+// RUN: %clang_cc1 -DMAIN_FILE -fdelayed-template-parsing \
+// RUN:   -include-pch %t.delayed.pch \
+// RUN:   -emit-llvm -verify -o - %s | FileCheck %s
+
+#ifndef MAIN_FILE
+
+template <typename T>
+int func() {
+#pragma pack(push, 1)
+  struct s { short a; T b; };
+#pragma pack(pop)
+  return sizeof(s);
+}
+
+#else
+
+// CHECK-LABEL: define linkonce_odr dso_local noundef i32 @"??$func@D@@YAHXZ"(
+// CHECK: ret i32 3
+int foo() {
+  return func<char>();
+}
+
+// CHECK-LABEL: define linkonce_odr dso_local noundef i32 @"??$func@F@@YAHXZ"(
+// CHECK: ret i32 4
+int bar() {
+  return func<short>();
+}
+
+// expected-no-diagnostics
+
+#endif
Index: clang/lib/Serialization/ASTWriter.cpp
===================================================================
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -4412,6 +4412,14 @@
         AddToken(T, Record);
       break;
     }
+    case tok::annot_pragma_pack: {
+      auto *Info =
+          static_cast<Sema::PragmaPackInfo *>(Tok.getAnnotationValue());
+      Record.push_back(static_cast<unsigned>(Info->Action));
+      AddString(Info->SlotLabel, Record);
+      AddToken(Info->Alignment, Record);
+      break;
+    }
     // Some annotation tokens do not use the PtrData field.
     case tok::annot_pragma_openmp:
     case tok::annot_pragma_openmp_end:
Index: clang/lib/Serialization/ASTReader.cpp
===================================================================
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -1687,6 +1687,15 @@
       Tok.setAnnotationValue(static_cast<void *>(Info));
       break;
     }
+    case tok::annot_pragma_pack: {
+      auto *Info = new (PP.getPreprocessorAllocator()) Sema::PragmaPackInfo;
+      Info->Action = static_cast<Sema::PragmaMsStackAction>(Record[Idx++]);
+      PragmaAlignPackStrings.push_back(ReadString(Record, Idx));
+      Info->SlotLabel = PragmaAlignPackStrings.back();
+      Info->Alignment = ReadToken(F, Record, Idx);
+      Tok.setAnnotationValue(static_cast<void *>(Info));
+      break;
+    }
     // Some annotation tokens do not use the PtrData field.
     case tok::annot_pragma_openmp:
     case tok::annot_pragma_openmp_end:
@@ -9089,7 +9098,7 @@
 }
 
 // Read a string
-std::string ASTReader::ReadString(const RecordData &Record, unsigned &Idx) {
+std::string ASTReader::ReadString(const RecordDataImpl &Record, unsigned &Idx) {
   unsigned Len = Record[Idx++];
   std::string Result(Record.data() + Idx, Record.data() + Idx + Len);
   Idx += Len;
Index: clang/lib/Parse/ParsePragma.cpp
===================================================================
--- clang/lib/Parse/ParsePragma.cpp
+++ clang/lib/Parse/ParsePragma.cpp
@@ -665,18 +665,10 @@
   Actions.ActOnPragmaVisibility(VisType, VisLoc);
 }
 
-namespace {
-struct PragmaPackInfo {
-  Sema::PragmaMsStackAction Action;
-  StringRef SlotLabel;
-  Token Alignment;
-};
-} // end anonymous namespace
-
 void Parser::HandlePragmaPack() {
   assert(Tok.is(tok::annot_pragma_pack));
-  PragmaPackInfo *Info =
-    static_cast<PragmaPackInfo *>(Tok.getAnnotationValue());
+  Sema::PragmaPackInfo *Info =
+    static_cast<Sema::PragmaPackInfo *>(Tok.getAnnotationValue());
   SourceLocation PragmaLoc = Tok.getLocation();
   ExprResult Alignment;
   if (Info->Alignment.is(tok::numeric_constant)) {
@@ -2110,8 +2102,8 @@
     return;
   }
 
-  PragmaPackInfo *Info =
-      PP.getPreprocessorAllocator().Allocate<PragmaPackInfo>(1);
+  Sema::PragmaPackInfo *Info =
+      PP.getPreprocessorAllocator().Allocate<Sema::PragmaPackInfo>(1);
   Info->Action = Action;
   Info->SlotLabel = SlotLabel;
   Info->Alignment = Alignment;
Index: clang/include/clang/Serialization/ASTReader.h
===================================================================
--- clang/include/clang/Serialization/ASTReader.h
+++ clang/include/clang/Serialization/ASTReader.h
@@ -2236,7 +2236,7 @@
                               unsigned &Idx, LocSeq *Seq = nullptr);
 
   // Read a string
-  static std::string ReadString(const RecordData &Record, unsigned &Idx);
+  static std::string ReadString(const RecordDataImpl &Record, unsigned &Idx);
 
   // Skip a string
   static void SkipString(const RecordData &Record, unsigned &Idx) {
Index: clang/include/clang/Sema/Sema.h
===================================================================
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -483,6 +483,12 @@
     PSK_Pop_Set   = PSK_Pop | PSK_Set,  // #pragma (pop[, id], value)
   };
 
+  struct PragmaPackInfo {
+    PragmaMsStackAction Action;
+    StringRef SlotLabel;
+    Token Alignment;
+  };
+
   // #pragma pack and align.
   class AlignPackInfo {
   public:
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to