Add a warning for missing '#pragma pack (pop)' and suspicious '#pragma pack' uses when including files

[arphaman]

Summary

This patch 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.

The change in the parser is required to avoid a missing diagnostic in the following scenario:

#pragma pack (push, 1)
#include “foo.h” // Without the change, the diagnostic for the 2nd case won’t be emitted since include will get processed by the Sema before the pragma

[aaron.ballman]

include/clang/Basic/DiagnosticSemaKinds.td

716

We don't use "record" in the text for diagnostics. Perhaps replace record with "struct or union members"?

718

This can move up a line.

include/clang/Sema/Sema.h

1162

Please group this with the other private Sema members and give it a \brief comment.

lib/Sema/Sema.cpp

95

This comment doesn't add much value.

lib/Sema/SemaAttr.cpp

235–236

Can you use for (const auto &StackSlot : llvm::reverse(PackStack.Stack)) instead?

286–288

This would be better written using emplace_back() rather than constructing and using push_back().

[arphaman]

Address review comments.

[aaron.ballman]

LGTM!

[arphaman]

This triggered a warning in LLVM itself, in CoverageMapping.h :

error: non-default #pragma pack value might change the alignment of
struct or union members in the included file [-Werror,-Wpragma-pack]
#include "llvm/ProfileData/InstrProfData.inc"
         ^
/mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:513:1:
note: previous '#pragma pack' directive that modifies alignment is
here
LLVM_PACKED_START
^
/mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/llvm/include/llvm/Support/Compiler.h:349:28:
note: expanded from macro 'LLVM_PACKED_START'
# define LLVM_PACKED_START _Pragma("pack(push, 1)")
                           ^
<scratch space>:14:2: note: expanded from here
 pack(push, 1)

I think I might tweak this diagnostic to avoid warning in that case. It might be better to just avoid warning about "non-default #pragma pack value might change the alignment of
struct or union members in the included file" until a first record declaration is encountered in the included file. This will avoid the warning in LLVM as the packed class is declared in the file that includes the header.

include/clang/Basic/DiagnosticGroups.td

 def UninitializedSometimes : DiagGroup<"sometimes-uninitialized">;
 def UninitializedStaticSelfInit : DiagGroup<"static-self-init">;
 def Uninitialized  : DiagGroup<"uninitialized", [UninitializedSometimes,
                                                  UninitializedStaticSelfInit]>;
 def IgnoredPragmaIntrinsic : DiagGroup<"ignored-pragma-intrinsic">;
 def UnknownPragmas : DiagGroup<"unknown-pragmas">;
 def IgnoredPragmas : DiagGroup<"ignored-pragmas", [IgnoredPragmaIntrinsic]>;
 def PragmaClangAttribute : DiagGroup<"pragma-clang-attribute">;
+def PragmaPack : DiagGroup<"pragma-pack">;
 def Pragmas : DiagGroup<"pragmas", [UnknownPragmas, IgnoredPragmas,
-                                    PragmaClangAttribute]>;
+                                    PragmaClangAttribute, PragmaPack]>;
 def UnknownWarningOption : DiagGroup<"unknown-warning-option">;
 def NSobjectAttribute : DiagGroup<"NSObject-attribute">;
 def IndependentClassAttribute : DiagGroup<"IndependentClass-attribute">;
 def UnknownAttributes : DiagGroup<"unknown-attributes">;
 def IgnoredAttributes : DiagGroup<"ignored-attributes">;
 def Attributes : DiagGroup<"attributes", [UnknownAttributes,
                                           IgnoredAttributes]>;
 def UnknownSanitizers : DiagGroup<"unknown-sanitizers">;

include/clang/Basic/DiagnosticSemaKinds.td

 def warn_pragma_options_align_reset_failed : Warning<
   "#pragma options align=reset failed: %0">,
   InGroup<IgnoredPragmas>;
 def err_pragma_options_align_mac68k_target_unsupported : Error<
   "mac68k alignment pragma is not supported on this target">;
 def warn_pragma_pack_invalid_alignment : Warning<
   "expected #pragma pack parameter to be '1', '2', '4', '8', or '16'">,
   InGroup<IgnoredPragmas>;
+def warn_pragma_pack_non_default_at_include : Warning<
+  "non-default #pragma pack value might change the alignment of struct or "
+  "union members in the included file">, InGroup<PragmaPack>;
+def warn_pragma_pack_modified_after_include : Warning<
+  "the current #pragma pack aligment value is modified in the included "
+  "file">, InGroup<PragmaPack>;
+def warn_pragma_pack_no_pop_eof : Warning<"unterminated "
+  "'#pragma pack (push, ...)' at end of file">, InGroup<PragmaPack>;
+def note_pragma_pack_here : Note<
+  "previous '#pragma pack' directive that modifies alignment is here">;
 // Follow the Microsoft implementation.
 def warn_pragma_pack_show : Warning<"value of #pragma pack(show) == %0">;
 def warn_pragma_pack_pop_identifer_and_alignment : Warning<
   "specifying both a name and alignment to 'pop' is undefined">;
 def warn_pragma_pop_failed : Warning<"#pragma %0(pop, ...) failed: %1">,
   InGroup<IgnoredPragmas>;
 def warn_cxx_ms_struct :
   Warning<"ms_struct may not produce Microsoft-compatible layouts for classes "

include/clang/Sema/Sema.h

namespace sema {
   class CapturedRegionScopeInfo;
   class CapturingScopeInfo;
   class CompoundScopeInfo;
   class DelayedDiagnostic;
   class DelayedDiagnosticPool;
   class FunctionScopeInfo;
   class LambdaScopeInfo;
   class PossiblyUnreachableDiag;
+  class SemaPPCallbacks;
   class TemplateDeductionInfo;
 }
 
 namespace threadSafety {
   class BeforeSet;
   void threadSafetyCleanup(BeforeSet* Cache);
 }
 
public:
   };
 
   template<typename ValueType>
   struct PragmaStack {
     struct Slot {
       llvm::StringRef StackSlotLabel;
       ValueType Value;
       SourceLocation PragmaLocation;
-      Slot(llvm::StringRef StackSlotLabel,
-           ValueType Value,
-           SourceLocation PragmaLocation)
+      SourceLocation PragmaPushLocation;
+      Slot(llvm::StringRef StackSlotLabel, ValueType Value,
+           SourceLocation PragmaLocation, SourceLocation PragmaPushLocation)
           : StackSlotLabel(StackSlotLabel), Value(Value),
-          PragmaLocation(PragmaLocation) {}
+            PragmaLocation(PragmaLocation),
+            PragmaPushLocation(PragmaPushLocation) {}
     };
     void Act(SourceLocation PragmaLocation,
              PragmaMsStackAction Action,
              llvm::StringRef StackSlotLabel,
              ValueType Value);
 
     // MSVC seems to add artificial slots to #pragma stacks on entering a C++
     // method body to restore the stacks on exit, so it works like this:
void SentinelAction(PragmaMsStackAction Action, StringRef Label) {
              "Can only push / pop #pragma stack sentinels!");
       Act(CurrentPragmaLocation, Action, Label, CurrentValue);
     }
 
     // Constructors.
     explicit PragmaStack(const ValueType &Default)
         : DefaultValue(Default), CurrentValue(Default) {}
 
+    bool hasValue() const { return CurrentValue != DefaultValue; }
+
     SmallVector<Slot, 2> Stack;
     ValueType DefaultValue; // Value used for PSK_Reset action.
     ValueType CurrentValue;
     SourceLocation CurrentPragmaLocation;
   };
   // FIXME: We should serialize / deserialize these if they occur in a PCH (but
   // we shouldn't do so if they're in a module).
 
   /// \brief Whether to insert vtordisps prior to virtual bases in the Microsoft
   /// C++ ABI.  Possible values are 0, 1, and 2, which mean:
   ///
   /// 0: Suppress all vtordisps
   /// 1: Insert vtordisps in the presence of vbase overrides and non-trivial
   ///    structors
   /// 2: Always insert vtordisps to support RTTI on partially constructed
   ///    objects
   PragmaStack<MSVtorDispAttr::Mode> VtorDispStack;
   // #pragma pack.
   // Sentinel to represent when the stack is set to mac68k alignment.
   static const unsigned kMac68kAlignmentSentinel = ~0U;
   PragmaStack<unsigned> PackStack;
+  // The current #pragma pack values and locations at each #include.
+  SmallVector<std::pair<unsigned, SourceLocation>, 8> PackIncludeStack;
   // Segment #pragmas.
   PragmaStack<StringLiteral *> DataSegStack;
   PragmaStack<StringLiteral *> BSSSegStack;
   PragmaStack<StringLiteral *> ConstSegStack;
   PragmaStack<StringLiteral *> CodeSegStack;
 
   // RAII object to push / pop sentinel slots for all MS #pragma stacks.
   // Actions should be performed only if we enter / exit a C++ method body.
public:
 
   private:
     Sema& S;
     FPOptions OldFPFeaturesState;
   };
 
   void addImplicitTypedef(StringRef Name, QualType T);
 
 public:
   Sema(Preprocessor &pp, ASTContext &ctxt, ASTConsumer &consumer,
        TranslationUnitKind TUKind = TU_Complete,
        CodeCompleteConsumer *CompletionConsumer = nullptr);
   ~Sema();
 
   /// \brief Perform initialization that occurs after the parser has been
   /// initialized but before it parses anything.
   void Initialize();

   /// ActOnPragmaOptionsAlign - Called on well formed \#pragma options align.
   void ActOnPragmaOptionsAlign(PragmaOptionsAlignKind Kind,
                                SourceLocation PragmaLoc);
 
   /// ActOnPragmaPack - Called on well formed \#pragma pack(...).
   void ActOnPragmaPack(SourceLocation PragmaLoc, PragmaMsStackAction Action,
                        StringRef SlotLabel, Expr *Alignment);
 
+  enum class PragmaPackDiagnoseKind {
+    NonDefaultStateAtInclude,
+    ChangedStateAtExit
+  };
+
+  void DiagnoseNonDefaultPragmaPack(PragmaPackDiagnoseKind Kind,
+                                    SourceLocation IncludeLoc);
+  void DiagnoseUnterminatedPragmaPack();
+
   /// ActOnPragmaMSStruct - Called on well formed \#pragma ms_struct [on|off].
   void ActOnPragmaMSStruct(PragmaMSStructKind Kind);
 
   /// ActOnPragmaMSComment - Called on well formed
   /// \#pragma comment(kind, "arg").
   void ActOnPragmaMSComment(SourceLocation CommentLoc, PragmaMSCommentKind Kind,
                             StringRef Arg);
 
private:
 
   /// Nullability type specifiers.
   IdentifierInfo *Ident__Nonnull = nullptr;
   IdentifierInfo *Ident__Nullable = nullptr;
   IdentifierInfo *Ident__Null_unspecified = nullptr;
 
   IdentifierInfo *Ident_NSError = nullptr;
 
+  /// \brief The handler for the FileChanged preprocessor events.
+  ///
+  /// Used for diagnostics that implement custom semantic analysis for #include
+  /// directives, like -Wpragma-pack.
+  sema::SemaPPCallbacks *SemaPPCallbackHandler;
+
 protected:
   friend class Parser;
   friend class InitializationSequence;
   friend class ASTReader;
   friend class ASTDeclReader;
   friend class ASTWriter;
 
 public:

include/clang/Serialization/ASTReader.h

private:
   SourceLocation PointersToMembersPragmaLocation;
 
   /// \brief The pragma pack state.
   Optional<unsigned> PragmaPackCurrentValue;
   SourceLocation PragmaPackCurrentLocation;
   struct PragmaPackStackEntry {
     unsigned Value;
     SourceLocation Location;
+    SourceLocation PushLocation;
     StringRef SlotLabel;
   };
   llvm::SmallVector<PragmaPackStackEntry, 2> PragmaPackStack;
   llvm::SmallVector<std::string, 2> PragmaPackStrings;
 
   /// \brief The OpenCL extension settings.
   OpenCLOptions OpenCLExtensions;
 

lib/Parse/ParsePragma.cpp

struct PragmaPackInfo {
   Token Alignment;
 };
 } // end anonymous namespace
 
 void Parser::HandlePragmaPack() {
   assert(Tok.is(tok::annot_pragma_pack));
   PragmaPackInfo *Info =
     static_cast<PragmaPackInfo *>(Tok.getAnnotationValue());
-  SourceLocation PragmaLoc = ConsumeAnnotationToken();
+  SourceLocation PragmaLoc = Tok.getLocation();
   ExprResult Alignment;
   if (Info->Alignment.is(tok::numeric_constant)) {
     Alignment = Actions.ActOnNumericConstant(Info->Alignment);
-    if (Alignment.isInvalid())
+    if (Alignment.isInvalid()) {
+      ConsumeAnnotationToken();
       return;
     }
+  }
   Actions.ActOnPragmaPack(PragmaLoc, Info->Action, Info->SlotLabel,
                           Alignment.get());
+  // Consume the token after processing the pragma to enable pragma-specific
+  // #include warnings.
+  ConsumeAnnotationToken();
 }
 
 void Parser::HandlePragmaMSStruct() {
   assert(Tok.is(tok::annot_pragma_msstruct));
   PragmaMSStructKind Kind = static_cast<PragmaMSStructKind>(
       reinterpret_cast<uintptr_t>(Tok.getAnnotationValue()));
   Actions.ActOnPragmaMSStruct(Kind);
   ConsumeAnnotationToken();

lib/Sema/Sema.cpp

PrintingPolicy Sema::getPrintingPolicy(const ASTContext &Context,
   return Policy;
 }
 
 void Sema::ActOnTranslationUnitScope(Scope *S) {
   TUScope = S;
   PushDeclContext(S, Context.getTranslationUnitDecl());
 }
 
+namespace clang {
+namespace sema {
+
+class SemaPPCallbacks : public PPCallbacks {
+  Sema *S = nullptr;
+  llvm::SmallVector<SourceLocation, 8> IncludeStack;
+
+public:
+  void set(Sema &S) { this->S = &S; }
+
+  void reset() { S = nullptr; }
+
+  virtual void FileChanged(SourceLocation Loc, FileChangeReason Reason,
+                           SrcMgr::CharacteristicKind FileType,
+                           FileID PrevFID) override {
+    if (!S)
+      return;
+    switch (Reason) {
+    case EnterFile: {
+      SourceManager &SM = S->getSourceManager();
+      SourceLocation IncludeLoc = SM.getIncludeLoc(SM.getFileID(Loc));
+      if (IncludeLoc.isValid()) {
+        IncludeStack.push_back(IncludeLoc);
+        S->DiagnoseNonDefaultPragmaPack(
+            Sema::PragmaPackDiagnoseKind::NonDefaultStateAtInclude, IncludeLoc);
+      }
+      break;
+    }
+    case ExitFile:
+      if (!IncludeStack.empty())
+        S->DiagnoseNonDefaultPragmaPack(
+            Sema::PragmaPackDiagnoseKind::ChangedStateAtExit,
+            IncludeStack.pop_back_val());
+      break;
+    default:
+      break;
+    }
+  }
+};
+
+} // end namespace sema
+} // end namespace clang
+
 Sema::Sema(Preprocessor &pp, ASTContext &ctxt, ASTConsumer &consumer,
            TranslationUnitKind TUKind, CodeCompleteConsumer *CodeCompleter)
     : ExternalSource(nullptr), isMultiplexExternalSource(false),
       FPFeatures(pp.getLangOpts()), LangOpts(pp.getLangOpts()), PP(pp),
       Context(ctxt), Consumer(consumer), Diags(PP.getDiagnostics()),
       SourceMgr(PP.getSourceManager()), CollectStats(false),
       CodeCompleter(CodeCompleter), CurContext(nullptr),
       OriginalLexicalContext(nullptr), MSStructPragmaOn(false),
Sema::Sema(Preprocessor &pp, ASTContext &ctxt, ASTConsumer &consumer,
   ExprEvalContexts.emplace_back(
       ExpressionEvaluationContext::PotentiallyEvaluated, 0, CleanupInfo{},
       nullptr, false);
 
   FunctionScopes.push_back(new FunctionScopeInfo(Diags));
 
   // Initilization of data sharing attributes stack for OpenMP
   InitDataSharingAttributesStack();
+
+  std::unique_ptr<sema::SemaPPCallbacks> Callbacks =
+      llvm::make_unique<sema::SemaPPCallbacks>();
+  SemaPPCallbackHandler = Callbacks.get();
+  PP.addPPCallbacks(std::move(Callbacks));
+  SemaPPCallbackHandler->set(*this);
 }
 
 void Sema::addImplicitTypedef(StringRef Name, QualType T) {
   DeclarationName DN = &Context.Idents.get(Name);
   if (IdResolver.begin(DN) == IdResolver.end())
     PushOnScopeChains(Context.buildImplicitTypedef(T, Name), TUScope);
 }
 
Sema::~Sema() {
   if (isMultiplexExternalSource)
     delete ExternalSource;
 
   threadSafety::threadSafetyCleanup(ThreadSafetyDeclCache);
 
   // Destroys data sharing attributes stack for OpenMP
   DestroyDataSharingAttributesStack();
 
+  // Detach from the PP callback handler which outlives Sema since it's owned
+  // by the preprocessor.
+  SemaPPCallbackHandler->reset();
+
   assert(DelayedTypos.empty() && "Uncorrected typos!");
 }
 
 /// makeUnavailableInSystemHeader - There is an error in the current
 /// context.  If we're still in a system header, and we can plausibly
 /// make the relevant declaration unavailable instead of erroring, do
 /// so and return true.
 bool Sema::makeUnavailableInSystemHeader(SourceLocation loc,
if (TUKind != TU_Prefix) {
     PerformPendingInstantiations();
 
     if (LateTemplateParserCleanup)
       LateTemplateParserCleanup(OpaqueParser);
 
     CheckDelayedMemberExceptionSpecs();
   }
 
+  DiagnoseUnterminatedPragmaPack();
   DiagnoseUnterminatedPragmaAttribute();
 
   // All delayed member exception specs should be checked or we end up accepting
   // incompatible declarations.
   // FIXME: This is wrong for TUKind == TU_Prefix. In that case, we need to
   // write out the lists to the AST file (if any).
   assert(DelayedDefaultedMemberExceptionSpecs.empty());
   assert(DelayedExceptionSpecChecks.empty());

lib/Sema/SemaAttr.cpp

if (Alignment && !SlotLabel.empty())
       Diag(PragmaLoc, diag::warn_pragma_pack_pop_identifer_and_alignment);
     if (PackStack.Stack.empty())
       Diag(PragmaLoc, diag::warn_pragma_pop_failed) << "pack" << "stack empty";
   }
 
   PackStack.Act(PragmaLoc, Action, SlotLabel, AlignmentVal);
 }
 
+void Sema::DiagnoseNonDefaultPragmaPack(PragmaPackDiagnoseKind Kind,
+                                        SourceLocation IncludeLoc) {
+  if (Kind == PragmaPackDiagnoseKind::NonDefaultStateAtInclude) {
+    SourceLocation PrevLocation = PackStack.CurrentPragmaLocation;
+    // Warn about non-default alignment at #includes (without redundant
+    // warnings for the same directive in nested includes).
+    if (PackStack.hasValue() &&
+        (PackIncludeStack.empty() ||
+         PackIncludeStack.back().second != PrevLocation)) {
+      Diag(IncludeLoc, diag::warn_pragma_pack_non_default_at_include);
+      Diag(PrevLocation, diag::note_pragma_pack_here);
+    }
+    PackIncludeStack.push_back(
+        {PackStack.CurrentValue,
+         PackStack.hasValue() ? PrevLocation : SourceLocation()});
+    return;
+  }
+
+  assert(Kind == PragmaPackDiagnoseKind::ChangedStateAtExit && "invalid kind");
+  unsigned PreviousValue = PackIncludeStack.pop_back_val().first;
+  // Warn about modified alignment after #includes.
+  if (PreviousValue != PackStack.CurrentValue) {
+    Diag(IncludeLoc, diag::warn_pragma_pack_modified_after_include);
+    Diag(PackStack.CurrentPragmaLocation, diag::note_pragma_pack_here);
+  }
+}
+
+void Sema::DiagnoseUnterminatedPragmaPack() {
+  if (PackStack.Stack.empty())
+    return;
+  for (const auto &StackSlot : llvm::reverse(PackStack.Stack))
+    Diag(StackSlot.PragmaPushLocation, diag::warn_pragma_pack_no_pop_eof);
+}
+
 void Sema::ActOnPragmaMSStruct(PragmaMSStructKind Kind) { 
   MSStructPragmaOn = (Kind == PMSST_ON);
 }
 
 void Sema::ActOnPragmaMSComment(SourceLocation CommentLoc,
                                 PragmaMSCommentKind Kind, StringRef Arg) {
   auto *PCD = PragmaCommentDecl::Create(
       Context, Context.getTranslationUnitDecl(), CommentLoc, Kind, Arg);
void Sema::PragmaStack<ValueType>::Act(SourceLocation PragmaLocation,
                                        llvm::StringRef StackSlotLabel,
                                        ValueType Value) {
   if (Action == PSK_Reset) {
     CurrentValue = DefaultValue;
     CurrentPragmaLocation = PragmaLocation;
     return;
   }
   if (Action & PSK_Push)
-    Stack.push_back(Slot(StackSlotLabel, CurrentValue, CurrentPragmaLocation));
+    Stack.emplace_back(StackSlotLabel, CurrentValue, CurrentPragmaLocation,
+                       PragmaLocation);
   else if (Action & PSK_Pop) {
     if (!StackSlotLabel.empty()) {
       // If we've got a label, try to find it and jump there.
       auto I = llvm::find_if(llvm::reverse(Stack), [&](const Slot &x) {
         return x.StackSlotLabel == StackSlotLabel;
       });
       // If we found the label so pop from there.
       if (I != Stack.rend()) {
         CurrentValue = I->Value;

lib/Serialization/ASTReader.cpp

case PACK_PRAGMA_OPTIONS: {
       unsigned NumStackEntries = Record[2];
       unsigned Idx = 3;
       // Reset the stack when importing a new module.
       PragmaPackStack.clear();
       for (unsigned I = 0; I < NumStackEntries; ++I) {
         PragmaPackStackEntry Entry;
         Entry.Value = Record[Idx++];
         Entry.Location = ReadSourceLocation(F, Record[Idx++]);
+        Entry.PushLocation = ReadSourceLocation(F, Record[Idx++]);
         PragmaPackStrings.push_back(ReadString(Record, Idx));
         Entry.SlotLabel = PragmaPackStrings.back();
         PragmaPackStack.push_back(Entry);
       }
       break;
     }
     }
   }
if (PragmaPackCurrentValue) {
     // popping entries that were included/imported from a PCH/module.
     bool DropFirst = false;
     if (!PragmaPackStack.empty() &&
         PragmaPackStack.front().Location.isInvalid()) {
       assert(PragmaPackStack.front().Value == SemaObj->PackStack.DefaultValue &&
              "Expected a default alignment value");
       SemaObj->PackStack.Stack.emplace_back(
           PragmaPackStack.front().SlotLabel, SemaObj->PackStack.CurrentValue,
-          SemaObj->PackStack.CurrentPragmaLocation);
+          SemaObj->PackStack.CurrentPragmaLocation,
+          PragmaPackStack.front().PushLocation);
       DropFirst = true;
     }
     for (const auto &Entry :
          llvm::makeArrayRef(PragmaPackStack).drop_front(DropFirst ? 1 : 0))
       SemaObj->PackStack.Stack.emplace_back(Entry.SlotLabel, Entry.Value,
-                                            Entry.Location);
+                                            Entry.Location, Entry.PushLocation);
     if (PragmaPackCurrentLocation.isInvalid()) {
       assert(*PragmaPackCurrentValue == SemaObj->PackStack.DefaultValue &&
              "Expected a default alignment value");
       // Keep the current values.
     } else {
       SemaObj->PackStack.CurrentValue = *PragmaPackCurrentValue;
       SemaObj->PackStack.CurrentPragmaLocation = PragmaPackCurrentLocation;
     }

lib/Serialization/ASTWriter.cpp

void ASTWriter::WritePackPragmaOptions(Sema &SemaRef) {
 
   RecordData Record;
   Record.push_back(SemaRef.PackStack.CurrentValue);
   AddSourceLocation(SemaRef.PackStack.CurrentPragmaLocation, Record);
   Record.push_back(SemaRef.PackStack.Stack.size());
   for (const auto &StackEntry : SemaRef.PackStack.Stack) {
     Record.push_back(StackEntry.Value);
     AddSourceLocation(StackEntry.PragmaLocation, Record);
+    AddSourceLocation(StackEntry.PragmaPushLocation, Record);
     AddString(StackEntry.StackSlotLabel, Record);
   }
   Stream.EmitRecord(PACK_PRAGMA_OPTIONS, Record);
 }
 
 void ASTWriter::WriteModuleFileExtension(Sema &SemaRef,
                                          ModuleFileExtensionWriter &Writer) {
   // Enter the extension block.

test/PCH/pragma-pack.c

 // Test this without pch.
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -include %s -verify -fsyntax-only -DSET
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -include %s -verify -fsyntax-only -DRESET
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -include %s -verify -fsyntax-only -DPUSH
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -include %s -verify -fsyntax-only -DPUSH_POP
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -include %s -verify -fsyntax-only -DPUSH_POP_LABEL
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -include %s -verify -fsyntax-only -Wno-pragma-pack -DSET
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -include %s -verify -fsyntax-only -Wno-pragma-pack -DRESET
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -include %s -verify -fsyntax-only -Wno-pragma-pack -DPUSH
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -include %s -verify -fsyntax-only -Wno-pragma-pack -DPUSH_POP
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -include %s -verify -fsyntax-only -Wno-pragma-pack -DPUSH_POP_LABEL
 
 // Test with pch.
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -DSET -emit-pch -o %t
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -DSET -verify -include-pch %t
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -DRESET -emit-pch -o %t
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -DRESET -verify -include-pch %t
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -DPUSH -emit-pch -o %t
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -DPUSH -verify -include-pch %t
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -DPUSH_POP -emit-pch -o %t
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -DPUSH_POP -verify -include-pch %t
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -DPUSH_POP_LABEL -emit-pch -o %t
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -DPUSH_POP_LABEL -verify -include-pch %t
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -Wno-pragma-pack -DSET -emit-pch -o %t
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -Wno-pragma-pack -DSET -verify -include-pch %t
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -Wno-pragma-pack -DRESET -emit-pch -o %t
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -Wno-pragma-pack -DRESET -verify -include-pch %t
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -Wno-pragma-pack -DPUSH -emit-pch -o %t
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -Wno-pragma-pack -DPUSH -verify -include-pch %t
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -Wno-pragma-pack -DPUSH_POP -emit-pch -o %t
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -Wno-pragma-pack -DPUSH_POP -verify -include-pch %t
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -Wno-pragma-pack -DPUSH_POP_LABEL -emit-pch -o %t
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -Wno-pragma-pack -DPUSH_POP_LABEL -verify -include-pch %t
 
 #ifndef HEADER
 #define HEADER
 
 #ifdef SET
 #pragma pack(1)
 #endif
 

test/PCH/suspicious-pragma-pack.c

+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -verify -emit-pch -o %t
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 %s -verify -include-pch %t
+
+#ifndef HEADER
+#define HEADER
+#pragma pack (push, 1)
+#endif
+// expected-warning@-2 {{unterminated '#pragma pack (push, ...)' at end of file}}

test/Parser/pragma-options.c

-// RUN: %clang_cc1 -triple i386-apple-darwin9 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple i386-apple-darwin9 -Wno-pragma-pack -fsyntax-only -verify %s
 
 /* expected-warning {{expected 'align' following '#pragma options'}} */ #pragma options
 /* expected-warning {{expected '=' following '#pragma options align'}} */ #pragma options align
 /* expected-warning {{expected identifier in '#pragma options'}} */ #pragma options align =
 /* expected-warning {{invalid alignment option in '#pragma options align'}} */ #pragma options align = foo
 /* expected-warning {{extra tokens at end of '#pragma options'}} */ #pragma options align = reset foo
 
 #pragma options align=natural

test/Parser/pragma-options.cpp

-// RUN: %clang_cc1 -triple i386-apple-darwin9 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple i386-apple-darwin9 -Wno-pragma-pack -fsyntax-only -verify %s
 // expected-no-diagnostics
 
 class C {
 #pragma options align=natural
 };

test/Parser/pragma-pack.c

-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-pragma-pack -verify %s
 
 // Note that this puts the expected lines before the directives to work around
 // limitations in the -verify mode.
 
 /* expected-warning {{missing '(' after '#pragma pack'}}*/ #pragma pack 10
 #pragma pack()
 #pragma pack(8)
 

test/Sema/Inputs/pragma-pack1.h

+
+#ifdef SET_FIRST_HEADER
+#pragma pack (16)
+#ifndef SET_SECOND_HEADER
+// expected-note@-2 2 {{previous '#pragma pack' directive that modifies alignment is here}}
+#else
+// expected-note@-4 1 {{previous '#pragma pack' directive that modifies alignment is here}}
+#endif
+// expected-warning@+3 {{non-default #pragma pack value might change the alignment of struct or union members in the included file}}
+#endif
+
+#include "pragma-pack2.h"
+
+#ifdef SET_SECOND_HEADER
+// expected-warning@-3 {{the current #pragma pack aligment value is modified in the included file}}
+#endif
+
+#ifdef PUSH_POP_FIRST_HEADER
+// This is fine, we don't change the current value.
+#pragma pack (push, 4)
+
+#pragma pack (pop)
+#endif

test/Sema/Inputs/pragma-pack2.h

+
+#ifdef SET_SECOND_HEADER
+#pragma pack (8) // expected-note 2 {{previous '#pragma pack' directive that modifies alignment is here}}
+#endif
+
+struct S { int x; };

test/Sema/pragma-pack.c

 #pragma pack(pop, eek)
 /* expected-warning {{value of #pragma pack(show) == 2}} */ #pragma pack(show)
 /* expected-warning {{pack(pop, ...) failed: stack empty}} */ #pragma pack(pop)
 
 #pragma pack(push)
 #pragma pack(pop, 16)
 /* expected-warning {{value of #pragma pack(show) == 16}} */ #pragma pack(show)
 
+
+// Warn about unbalanced pushes.
+#pragma pack (push,4) // expected-warning {{unterminated '#pragma pack (push, ...)' at end of file}}
+#pragma pack (push)   // expected-warning {{unterminated '#pragma pack (push, ...)' at end of file}}
+#pragma pack ()

test/Sema/suspicious-pragma-pack.c

+// RUN: %clang_cc1 -triple i686-apple-darwin9 -fsyntax-only -I %S/Inputs -DSAFE -verify %s
+// RUN: %clang_cc1 -triple i686-apple-darwin9 -fsyntax-only -I %S/Inputs -DPUSH_HERE -DSAFE -verify %s
+// RUN: %clang_cc1 -triple i686-apple-darwin9 -fsyntax-only -I %S/Inputs -DPUSH_HERE -DPUSH_SET_HERE -verify %s
+// RUN: %clang_cc1 -triple i686-apple-darwin9 -fsyntax-only -I %S/Inputs -DPUSH_HERE -DRESET_HERE -DSAFE -verify %s
+// RUN: %clang_cc1 -triple i686-apple-darwin9 -fsyntax-only -I %S/Inputs -DPUSH_HERE -DSET_FIRST_HEADER -DWARN_MODIFIED_HEADER -verify %s
+// RUN: %clang_cc1 -triple i686-apple-darwin9 -fsyntax-only -I %S/Inputs -DPUSH_HERE -DRESET_HERE -DSET_FIRST_HEADER -DWARN_MODIFIED_HEADER -verify %s
+// RUN: %clang_cc1 -triple i686-apple-darwin9 -fsyntax-only -I %S/Inputs -DPUSH_HERE -DPUSH_SET_HERE -DSET_FIRST_HEADER -DWARN_MODIFIED_HEADER -verify %s
+// RUN: %clang_cc1 -triple i686-apple-darwin9 -fsyntax-only -I %S/Inputs -DPUSH_HERE -DPUSH_SET_HERE -DSET_SECOND_HEADER -DWARN_MODIFIED_HEADER -verify %s
+// RUN: %clang_cc1 -triple i686-apple-darwin9 -fsyntax-only -I %S/Inputs -DPUSH_HERE -DPUSH_SET_HERE -DSET_FIRST_HEADER -DSET_SECOND_HEADER -DWARN_MODIFIED_HEADER -verify %s
+
+// RUN: %clang_cc1 -triple i686-apple-darwin9 -fsyntax-only -I %S/Inputs -DPUSH_POP_FIRST_HEADER -DSAFE -verify %s
+
+
+#ifdef SAFE
+// expected-no-diagnostics
+#endif
+
+#ifdef PUSH_HERE
+#pragma pack (push)
+#endif
+
+#ifdef PUSH_SET_HERE
+#pragma pack (push, 4) // expected-note {{previous '#pragma pack' directive that modifies alignment is here}}
+// expected-warning@+8 {{non-default #pragma pack value might change the alignment of struct or union members in the included file}}
+#endif
+
+#ifdef RESET_HERE
+#pragma pack (4)
+#pragma pack () // no warning after reset as the value is default.
+#endif
+
+#include "pragma-pack1.h"
+
+#ifdef WARN_MODIFIED_HEADER
+// expected-warning@-3 {{the current #pragma pack aligment value is modified in the included file}}
+#endif
+
+#ifdef PUSH_SET_HERE
+#pragma pack (pop)
+#endif
+
+#ifdef PUSH_HERE
+#pragma pack (pop)
+#endif

test/SemaObjC/Inputs/empty.h

+// empty

test/SemaObjC/suspicious-pragma-pack.m

+// RUN: %clang_cc1 -triple i686-apple-darwin9 -fsyntax-only -I%S/Inputs -verify %s
+
+#pragma pack (push, 1) // expected-note {{previous '#pragma pack' directive that modifies alignment is here}}
+#import "empty.h" // expected-warning {{non-default #pragma pack value might change the alignment of struct or union members in the included file}}
+
+#pragma pack (pop)