Recommit r308327 3rd time: Add a warning for missing

'#pragma pack (pop)' and suspicious uses of '#pragma pack' in included files

The second recommit (r309106) was reverted because the "non-default #pragma
pack value chages the alignment of struct or union members in the included file"
warning proved to be too aggressive for external projects like Chromium
(https://bugs.chromium.org/p/chromium/issues/detail?id=749197). This recommit
makes the problematic warning a non-default one, and gives it the
-Wpragma-pack-suspicious-include warning option.

The first recommit (r308441) caused a "non-default #pragma pack value might
change the alignment of struct or union members in the included file" warning
in LLVM itself. This recommit tweaks the added warning to avoid warnings for
#includes that don't have any records that are affected by the non-default
alignment. This tweak avoids the previously emitted warning in LLVM.

Original message:

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.

rdar://10184173

Differential Revision: https://reviews.llvm.org/D35484

llvm-svn: 309386

clang/include/clang/Basic/DiagnosticGroups.td

@@ -471,8 +471,10 @@ def IgnoredPragmaIntrinsic : DiagGroup<"ignored-pragma-intrinsic">;
 def UnknownPragmas : DiagGroup<"unknown-pragmas">;
 def IgnoredPragmas : DiagGroup<"ignored-pragmas", [IgnoredPragmaIntrinsic]>;
 def PragmaClangAttribute : DiagGroup<"pragma-clang-attribute">;
+def PragmaPackSuspiciousInclude : DiagGroup<"pragma-pack-suspicious-include">;
+def PragmaPack : DiagGroup<"pragma-pack", [PragmaPackSuspiciousInclude]>;
 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">;

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -712,6 +712,17 @@ def err_pragma_options_align_mac68k_target_unsupported : Error<
 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 changes the alignment of struct or union "
+  "members in the included file">, InGroup<PragmaPackSuspiciousInclude>,
+  DefaultIgnore;
+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<

clang/include/clang/Lex/PPCallbacks.h

@@ -381,6 +381,12 @@ class PPChainedCallbacks : public PPCallbacks {
     Second->Ident(Loc, str);
   }
 
+  void PragmaDirective(SourceLocation Loc,
+                       PragmaIntroducerKind Introducer) override {
+    First->PragmaDirective(Loc, Introducer);
+    Second->PragmaDirective(Loc, Introducer);
+  }
+
   void PragmaComment(SourceLocation Loc, const IdentifierInfo *Kind,
                      StringRef Str) override {
     First->PragmaComment(Loc, Kind, Str);

clang/include/clang/Sema/Sema.h

@@ -208,6 +208,7 @@ namespace sema {
   class FunctionScopeInfo;
   class LambdaScopeInfo;
   class PossiblyUnreachableDiag;
+  class SemaPPCallbacks;
   class TemplateDeductionInfo;
 }
 
@@ -381,11 +382,12 @@ class Sema {
       llvm::StringRef StackSlotLabel;
       ValueType Value;
       SourceLocation PragmaLocation;
-      Slot(llvm::StringRef StackSlotLabel,
-           ValueType Value,
-           SourceLocation PragmaLocation)
-        : StackSlotLabel(StackSlotLabel), Value(Value),
-          PragmaLocation(PragmaLocation) {}
+      SourceLocation PragmaPushLocation;
+      Slot(llvm::StringRef StackSlotLabel, ValueType Value,
+           SourceLocation PragmaLocation, SourceLocation PragmaPushLocation)
+          : StackSlotLabel(StackSlotLabel), Value(Value),
+            PragmaLocation(PragmaLocation),
+            PragmaPushLocation(PragmaPushLocation) {}
     };
     void Act(SourceLocation PragmaLocation,
              PragmaMsStackAction Action,
@@ -416,6 +418,8 @@ class Sema {
     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;
@@ -437,6 +441,13 @@ class Sema {
   // 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.
+  struct PackIncludeState {
+    unsigned CurrentValue;
+    SourceLocation CurrentPragmaLocation;
+    bool HasNonDefaultValue, ShouldWarnOnInclude;
+  };
+  SmallVector<PackIncludeState, 8> PackIncludeStack;
   // Segment #pragmas.
   PragmaStack<StringLiteral *> DataSegStack;
   PragmaStack<StringLiteral *> BSSSegStack;
@@ -8182,6 +8193,15 @@ class Sema {
   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);
 
@@ -10390,6 +10410,12 @@ class Sema {
 
   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;

clang/include/clang/Serialization/ASTReader.h

@@ -825,6 +825,7 @@ class ASTReader
   struct PragmaPackStackEntry {
     unsigned Value;
     SourceLocation Location;
+    SourceLocation PushLocation;
     StringRef SlotLabel;
   };
   llvm::SmallVector<PragmaPackStackEntry, 2> PragmaPackStack;

clang/lib/Parse/ParsePragma.cpp

@@ -422,15 +422,20 @@ 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() {

clang/lib/Sema/Sema.cpp

@@ -70,6 +70,49 @@ void Sema::ActOnTranslationUnitScope(Scope *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),
@@ -122,6 +165,12 @@ Sema::Sema(Preprocessor &pp, ASTContext &ctxt, ASTConsumer &consumer,
 
   // 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) {
@@ -306,6 +355,10 @@ Sema::~Sema() {
   // 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!");
 }
 
@@ -766,6 +819,7 @@ void Sema::ActOnEndOfTranslationUnit() {
     CheckDelayedMemberExceptionSpecs();
   }
 
+  DiagnoseUnterminatedPragmaPack();
   DiagnoseUnterminatedPragmaAttribute();
 
   // All delayed member exception specs should be checked or we end up accepting

clang/lib/Sema/SemaAttr.cpp

@@ -61,6 +61,17 @@ void Sema::AddAlignmentAttributesForRecord(RecordDecl *RD) {
       RD->addAttr(MaxFieldAlignmentAttr::CreateImplicit(Context,
                                                         Alignment * 8));
   }
+  if (PackIncludeStack.empty())
+    return;
+  // The #pragma pack affected a record in an included file,  so Clang should
+  // warn when that pragma was written in a file that included the included
+  // file.
+  for (auto &PackedInclude : llvm::reverse(PackIncludeStack)) {
+    if (PackedInclude.CurrentPragmaLocation != PackStack.CurrentPragmaLocation)
+      break;
+    if (PackedInclude.HasNonDefaultValue)
+      PackedInclude.ShouldWarnOnInclude = true;
+  }
 }
 
 void Sema::AddMsStructLayoutForRecord(RecordDecl *RD) {
@@ -202,6 +213,47 @@ void Sema::ActOnPragmaPack(SourceLocation PragmaLoc, PragmaMsStackAction Action,
   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).
+    // The warning is delayed until the end of the file to avoid warnings
+    // for files that don't have any records that are affected by the modified
+    // alignment.
+    bool HasNonDefaultValue =
+        PackStack.hasValue() &&
+        (PackIncludeStack.empty() ||
+         PackIncludeStack.back().CurrentPragmaLocation != PrevLocation);
+    PackIncludeStack.push_back(
+        {PackStack.CurrentValue,
+         PackStack.hasValue() ? PrevLocation : SourceLocation(),
+         HasNonDefaultValue, /*ShouldWarnOnInclude*/ false});
+    return;
+  }
+
+  assert(Kind == PragmaPackDiagnoseKind::ChangedStateAtExit && "invalid kind");
+  PackIncludeState PrevPackState = PackIncludeStack.pop_back_val();
+  if (PrevPackState.ShouldWarnOnInclude) {
+    // Emit the delayed non-default alignment at #include warning.
+    Diag(IncludeLoc, diag::warn_pragma_pack_non_default_at_include);
+    Diag(PrevPackState.CurrentPragmaLocation, diag::note_pragma_pack_here);
+  }
+  // Warn about modified alignment after #includes.
+  if (PrevPackState.CurrentValue != 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);
 }
@@ -249,7 +301,8 @@ void Sema::PragmaStack<ValueType>::Act(SourceLocation 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.

clang/lib/Serialization/ASTReader.cpp

@@ -3382,6 +3382,7 @@ ASTReader::ReadASTBlock(ModuleFile &F, unsigned ClientLoadCapabilities) {
         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);
@@ -7570,13 +7571,14 @@ void ASTReader::UpdateSema() {
              "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");

clang/lib/Serialization/ASTWriter.cpp

@@ -4295,6 +4295,7 @@ void ASTWriter::WritePackPragmaOptions(Sema &SemaRef) {
   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);

clang/test/OpenMP/declare_simd_messages.cpp

@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple=x86_64-pc-win32 -verify -fopenmp -x c++ -std=c++11 -fms-extensions %s
+// RUN: %clang_cc1 -triple=x86_64-pc-win32 -verify -fopenmp -x c++ -std=c++11 -fms-extensions -Wno-pragma-pack %s
 
 // expected-error@+1 {{expected an OpenMP directive}}
 #pragma omp declare

clang/test/PCH/pragma-pack.c

@@ -1,21 +1,21 @@
 // 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

clang/test/PCH/suspicious-pragma-pack.c

@@ -0,0 +1,8 @@
+// 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}}