[Serialization] Add support for (de)serializing #pragma pack

[DHowett-MSFT]

Summary

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 preprocessor storage.

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

[DHowett-MSFT]

• Add a test

[DHowett-MSFT]

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.

[mikerice]

clang/test/PCH/delayed-template-with-pragma-pack.cpp

15

Your test should include testing of the slot string too, so we are sure all fields are serialized and restored correctly. Otherwise this looks reasonable to me.

[DHowett-MSFT]

While this fixes the assertion failure and the immediate issue of whether packing _works_ inside delay-parsed templates in a PCH, it does reveal a follow-on issue that I can't quite trace out.

c++
template <typename T>
void foo() {
#pragma pack(push, 1)
#pragma pack(show)
#pragma pack(pop)
}

results in...

unterminated '#pragma pack (push, ...)' at end of file

I've stepped through ASTReader and verified that we do get all three expected pack pragma nodes

{PSK_Push, ..., 1}
{PSK_Show}
{PSK_Pop, ...}

suggesting that there's something later down the line that messes up the pack stack.

clang/test/PCH/delayed-template-with-pragma-pack.cpp

15

So, I've identified an additional wrinkle...

Unlike #pragma omp, #pragma pack and ... align don't show up in the AST dump/reprint and I cannot verify their continued existence; this is true _even without delayed template expansion or PCH generation._

alignment/packing seem to not be stored as Attrs, so they don't get printed via attr print on the AST nodes or (prettily) via the tablegen-generated pretty printers.

Fixing this is somewhat beyond me at the moment. :)

[mikerice]

In D143410#4107862, @DHowett-MSFT wrote:

While this fixes the assertion failure and the immediate issue of whether packing _works_ inside delay-parsed templates in a PCH, it does reveal a follow-on issue that I can't quite trace out.

c++
template <typename T>
void foo() {
#pragma pack(push, 1)
#pragma pack(show)
#pragma pack(pop)
}

results in...

unterminated '#pragma pack (push, ...)' at end of file

Tried your patch, but am not seeing this. Can you share the exact test case where you see this?

Also please add a -triple to your test so it is stable across platforms (maybe -triple x86_64-windows-msvc)

clang/test/PCH/delayed-template-with-pragma-pack.cpp

15

So, I've identified an additional wrinkle...

Unlike #pragma omp, #pragma pack and ... align don't show up in the AST dump/reprint and I cannot verify their continued existence; this is true _even without delayed template expansion or PCH generation._

alignment/packing seem to not be stored as Attrs, so they don't get printed via attr print on the AST nodes or (prettily) via the tablegen-generated pretty printers.

True, so you have to verify it with the generated code instead.

[DHowett-MSFT]

@mikerice Alright, it turns out that PragmaPackAlignStorage isn't suitable for this (actually, I'm not sure what it _is_
suitable for) because it's deleted by the time the template gets parsed again.

The push, slot case failed nondeterministically because the slot name is being UAF'd.

For now (and I am not sure this is correct), I've chosen to store the slot name in the preprocessor storage.

I suspect that other serialized pack info (not from the pragma, but the attr version that is stored in PragmaPackAlignStorage) is subject to the same limitation...

[DHowett-MSFT]

The new test checks how tightly things were packed across a stack of packs, pushed and popped. :)

[DHowett-MSFT]

• Use .copy() like the other nearby code

[DHowett-MSFT]

Test failures seem related to recent driver changes. There's a commit 6a8a423c1864ced060a7041bf6ada7574f35ad4d that purports to fix them.

[mikerice]

LGTM. Thanks for taking care of this.

[DHowett-MSFT]

Thanks for the review!

I’ll need somebody to help land this, as I don’t have write access to the project. :)

clang/include/clang/Sema/Sema.h

enum PragmaMsStackAction {
     PSK_Set       = 0x1,                // #pragma (value)
     PSK_Push      = 0x2,                // #pragma (push[, id])
     PSK_Pop       = 0x4,                // #pragma (pop[, id])
     PSK_Show      = 0x8,                // #pragma (show) -- only for "pack"!
     PSK_Push_Set  = PSK_Push | PSK_Set, // #pragma (push[, id], value)
     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:
     // `Native` represents default align mode, which may vary based on the
     // platform.
     enum Mode : unsigned char { Native, Natural, Packed, Mac68k };
 
     // #pragma pack info constructor

clang/include/clang/Serialization/ASTReader.h

FileID TranslateFileID(ModuleFile &F, FileID FID) const {
     return FileID::get(F.SLocEntryBaseID + FID.ID - 1);
   }
 
   /// Read a source range.
   SourceRange ReadSourceRange(ModuleFile &F, const RecordData &Record,
                               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) {
     Idx += Record[Idx] + 1;
   }
 
   // Read a path
   std::string ReadPath(ModuleFile &F, const RecordData &Record, unsigned &Idx);

clang/lib/Parse/ParsePragma.cpp

 void Parser::HandlePragmaVisibility() {
   assert(Tok.is(tok::annot_pragma_vis));
   const IdentifierInfo *VisType =
     static_cast<IdentifierInfo *>(Tok.getAnnotationValue());
   SourceLocation VisLoc = ConsumeAnnotationToken();
   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)) {
     Alignment = Actions.ActOnNumericConstant(Info->Alignment);
     if (Alignment.isInvalid()) {
       ConsumeAnnotationToken();
       return;
     }
void PragmaPackHandler::HandlePragma(Preprocessor &PP,
 
   SourceLocation RParenLoc = Tok.getLocation();
   PP.Lex(Tok);
   if (Tok.isNot(tok::eod)) {
     PP.Diag(Tok.getLocation(), diag::warn_pragma_extra_tokens_at_eol) << "pack";
     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;
 
   MutableArrayRef<Token> Toks(PP.getPreprocessorAllocator().Allocate<Token>(1),
                               1);
   Toks[0].startToken();
   Toks[0].setKind(tok::annot_pragma_pack);

clang/lib/Serialization/ASTReader.cpp

case tok::annot_pragma_loop_hint: {
       SmallVector<Token, 4> Toks;
       Toks.reserve(NumTokens);
       for (unsigned I = 0; I < NumTokens; ++I)
         Toks.push_back(ReadToken(F, Record, Idx));
       Info->Toks = llvm::ArrayRef(Toks).copy(PP.getPreprocessorAllocator());
       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++]);
+      auto SlotLabel = ReadString(Record, Idx);
+      Info->SlotLabel =
+          llvm::StringRef(SlotLabel).copy(PP.getPreprocessorAllocator());
+      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:
     case tok::annot_pragma_unused:
       break;
     default:
       llvm_unreachable("missing deserialization code for annotation token");
     }

 }
 
 /// Read a floating-point value
 llvm::APFloat ASTRecordReader::readAPFloat(const llvm::fltSemantics &Sem) {
   return llvm::APFloat(Sem, readAPInt());
 }
 
 // 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;
   return Result;
 }
 
 std::string ASTReader::ReadPath(ModuleFile &F, const RecordData &Record,
                                 unsigned &Idx) {

clang/lib/Serialization/ASTWriter.cpp

case tok::annot_pragma_loop_hint: {
       auto *Info = static_cast<PragmaLoopHintInfo *>(Tok.getAnnotationValue());
       AddToken(Info->PragmaName, Record);
       AddToken(Info->Option, Record);
       Record.push_back(Info->Toks.size());
       for (const auto &T : Info->Toks)
         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:
     case tok::annot_pragma_unused:
       break;
     default:
       llvm_unreachable("missing serialization code for annotation token");
     }

clang/test/PCH/delayed-template-with-pragma-pack.cpp

+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -emit-pch -o %t.pch %s
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -fdelayed-template-parsing -emit-pch -o %t.delayed.pch %s
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -DMAIN_FILE \
+// RUN:   -include-pch %t.pch \
+// RUN:   -emit-llvm -verify -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -DMAIN_FILE -fdelayed-template-parsing \
+// RUN:   -include-pch %t.delayed.pch \
+// RUN:   -emit-llvm -verify -o - %s | FileCheck %s
+
+#ifndef MAIN_FILE
+
+extern "C" void consume(int b);
+
+template <int I>
+void function() {
+#pragma pack(push, 1)
+  struct packedAt1 {
+    char a;
+    unsigned long long b;
+    char c;
+    unsigned long long d;
+    // 18 bytes total
+  };
+#pragma pack(push, slot1, 2)
+  struct packedAt2 {
+    char a; // +1 byte of padding
+    unsigned long long b;
+    char c; // +1 byte of padding
+    unsigned long long d;
+    // 20 bytes total
+  };
+#pragma pack(push, 4)
+  struct packedAt4 {
+    char a; // +3 bytes of padding
+    unsigned long long b;
+    char c; // +3 bytes of padding
+    unsigned long long d;
+    // 24 bytes total
+  };
+#pragma pack(push, 16)
+  struct packedAt16 {
+    char a; // +7 bytes of padding
+    unsigned long long b;
+    char c; // +7 bytes of padding
+    unsigned long long d;
+    // 32 bytes total
+  };
+#pragma pack(pop, slot1) // This should return packing to 1 (established before push(slot1))
+  struct packedAfterPopBackTo1 {
+    char a;
+    unsigned long long b;
+    char c;
+    unsigned long long d;
+  };
+#pragma pack(pop)
+
+  consume(sizeof(packedAt1)); // 18
+  consume(sizeof(packedAt2)); // 20
+  consume(sizeof(packedAt4)); // 24
+  consume(sizeof(packedAt16)); // 32
+  consume(sizeof(packedAfterPopBackTo1)); // 18 again
+}
+
+#else
+
+// CHECK-LABEL: define linkonce_odr dso_local void @"??$function@$0A@@@YAXXZ"(
+// CHECK: call void @consume(i32 noundef 18)
+// CHECK-NEXT: call void @consume(i32 noundef 20)
+// CHECK-NEXT: call void @consume(i32 noundef 24)
+// CHECK-NEXT: call void @consume(i32 noundef 32)
+// CHECK-NEXT: call void @consume(i32 noundef 18)
+void foo() {
+  function<0>();
+}
+
+// expected-no-diagnostics
+
+#endif