[PATCH] D35484: Add a warning for missing '#pragma pack (pop)' and suspicious '#pragma pack' uses when including files
arphaman added a comment.
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)")
^
: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.
Repository:
rL LLVM
https://reviews.llvm.org/D35484
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35484: Add a warning for missing '#pragma pack (pop)' and suspicious '#pragma pack' uses when including files
This revision was automatically updated to reflect the committed changes.
Closed by commit rL308327: Add a warning for missing '#pragma pack (pop)' and
suspicious uses (authored by arphaman).
Changed prior to commit:
https://reviews.llvm.org/D35484?vs=107090=107130#toc
Repository:
rL LLVM
https://reviews.llvm.org/D35484
Files:
cfe/trunk/include/clang/Basic/DiagnosticGroups.td
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/include/clang/Serialization/ASTReader.h
cfe/trunk/lib/Parse/ParsePragma.cpp
cfe/trunk/lib/Sema/Sema.cpp
cfe/trunk/lib/Sema/SemaAttr.cpp
cfe/trunk/lib/Serialization/ASTReader.cpp
cfe/trunk/lib/Serialization/ASTWriter.cpp
cfe/trunk/test/OpenMP/declare_simd_messages.cpp
cfe/trunk/test/PCH/pragma-pack.c
cfe/trunk/test/PCH/suspicious-pragma-pack.c
cfe/trunk/test/Parser/pragma-options.c
cfe/trunk/test/Parser/pragma-options.cpp
cfe/trunk/test/Parser/pragma-pack.c
cfe/trunk/test/Sema/Inputs/pragma-pack1.h
cfe/trunk/test/Sema/Inputs/pragma-pack2.h
cfe/trunk/test/Sema/pragma-pack.c
cfe/trunk/test/Sema/suspicious-pragma-pack.c
cfe/trunk/test/SemaObjC/Inputs/empty.h
cfe/trunk/test/SemaObjC/suspicious-pragma-pack.m
Index: cfe/trunk/include/clang/Sema/Sema.h
===
--- cfe/trunk/include/clang/Sema/Sema.h
+++ cfe/trunk/include/clang/Sema/Sema.h
@@ -208,6 +208,7 @@
class FunctionScopeInfo;
class LambdaScopeInfo;
class PossiblyUnreachableDiag;
+ class SemaPPCallbacks;
class TemplateDeductionInfo;
}
@@ -381,11 +382,12 @@
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 @@
explicit PragmaStack(const ValueType )
: DefaultValue(Default), CurrentValue(Default) {}
+bool hasValue() const { return CurrentValue != DefaultValue; }
+
SmallVector Stack;
ValueType DefaultValue; // Value used for PSK_Reset action.
ValueType CurrentValue;
@@ -437,6 +441,8 @@
// Sentinel to represent when the stack is set to mac68k alignment.
static const unsigned kMac68kAlignmentSentinel = ~0U;
PragmaStack PackStack;
+ // The current #pragma pack values and locations at each #include.
+ SmallVector, 8> PackIncludeStack;
// Segment #pragmas.
PragmaStack DataSegStack;
PragmaStack BSSSegStack;
@@ -8185,6 +8191,15 @@
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);
@@ -10378,6 +10393,12 @@
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;
Index: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticGroups.td
+++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td
@@ -467,8 +467,9 @@
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">;
Index:
[PATCH] D35484: Add a warning for missing '#pragma pack (pop)' and suspicious '#pragma pack' uses when including files
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Repository: rL LLVM https://reviews.llvm.org/D35484 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35484: Add a warning for missing '#pragma pack (pop)' and suspicious '#pragma pack' uses when including files
arphaman updated this revision to Diff 107090.
arphaman marked 5 inline comments as done.
arphaman added a comment.
Address review comments.
Repository:
rL LLVM
https://reviews.llvm.org/D35484
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Sema/Sema.h
include/clang/Serialization/ASTReader.h
lib/Parse/ParsePragma.cpp
lib/Sema/Sema.cpp
lib/Sema/SemaAttr.cpp
lib/Serialization/ASTReader.cpp
lib/Serialization/ASTWriter.cpp
test/PCH/pragma-pack.c
test/PCH/suspicious-pragma-pack.c
test/Parser/pragma-options.c
test/Parser/pragma-options.cpp
test/Parser/pragma-pack.c
test/Sema/Inputs/pragma-pack1.h
test/Sema/Inputs/pragma-pack2.h
test/Sema/pragma-pack.c
test/Sema/suspicious-pragma-pack.c
test/SemaObjC/Inputs/empty.h
test/SemaObjC/suspicious-pragma-pack.m
Index: test/SemaObjC/suspicious-pragma-pack.m
===
--- /dev/null
+++ test/SemaObjC/suspicious-pragma-pack.m
@@ -0,0 +1,6 @@
+// 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)
Index: test/SemaObjC/Inputs/empty.h
===
--- /dev/null
+++ test/SemaObjC/Inputs/empty.h
@@ -0,0 +1 @@
+// empty
Index: test/Sema/suspicious-pragma-pack.c
===
--- /dev/null
+++ test/Sema/suspicious-pragma-pack.c
@@ -0,0 +1,44 @@
+// 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
Index: test/Sema/pragma-pack.c
===
--- test/Sema/pragma-pack.c
+++ test/Sema/pragma-pack.c
@@ -25,3 +25,8 @@
#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 ()
Index: test/Sema/Inputs/pragma-pack2.h
===
--- /dev/null
+++ test/Sema/Inputs/pragma-pack2.h
@@ -0,0 +1,6 @@
+
+#ifdef SET_SECOND_HEADER
+#pragma pack (8) // expected-note 2 {{previous '#pragma pack' directive that modifies alignment is here}}
+#endif
+
+struct S { int x; };
Index: test/Sema/Inputs/pragma-pack1.h
===
--- /dev/null
+++ test/Sema/Inputs/pragma-pack1.h
@@ -0,0 +1,23 @@
+
+#ifdef SET_FIRST_HEADER
+#pragma pack (16)
+#ifndef SET_SECOND_HEADER
+// expected-note@-2
[PATCH] D35484: Add a warning for missing '#pragma pack (pop)' and suspicious '#pragma pack' uses when including files
aaron.ballman added inline comments.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:716
+def warn_pragma_pack_non_default_at_include : Warning<
+ "non-default #pragma pack value might change the alignment of records in the
"
+ "included file">,
We don't use "record" in the text for diagnostics. Perhaps replace record with
"struct or union members"?
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:718
+ "included file">,
+ InGroup;
+def warn_pragma_pack_modified_after_include : Warning<
This can move up a line.
Comment at: include/clang/Sema/Sema.h:1162
+ sema::SemaPPCallbacks *SemaPPCallbackHandler;
+
Please group this with the other private Sema members and give it a \brief
comment.
Comment at: lib/Sema/Sema.cpp:95
+ if (IncludeLoc.isValid()) {
+// Entering an include.
+IncludeStack.push_back(IncludeLoc);
This comment doesn't add much value.
Comment at: lib/Sema/SemaAttr.cpp:235-236
+return;
+ for (auto I = PackStack.Stack.rbegin(), E = PackStack.Stack.rend(); I != E;
+ ++I)
+Diag(I->PragmaPushLocation, diag::warn_pragma_pack_no_pop_eof);
Can you use `for (const auto : llvm::reverse(PackStack.Stack))`
instead?
Comment at: lib/Sema/SemaAttr.cpp:287-288
if (Action & PSK_Push)
-Stack.push_back(Slot(StackSlotLabel, CurrentValue, CurrentPragmaLocation));
+Stack.push_back(Slot(StackSlotLabel, CurrentValue, CurrentPragmaLocation,
+ PragmaLocation));
else if (Action & PSK_Pop) {
This would be better written using `emplace_back()` rather than constructing
and using `push_back()`.
Repository:
rL LLVM
https://reviews.llvm.org/D35484
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35484: Add a warning for missing '#pragma pack (pop)' and suspicious '#pragma pack' uses when including files
arphaman created this revision.
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
Repository:
rL LLVM
https://reviews.llvm.org/D35484
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Sema/Sema.h
include/clang/Serialization/ASTReader.h
lib/Parse/ParsePragma.cpp
lib/Sema/Sema.cpp
lib/Sema/SemaAttr.cpp
lib/Serialization/ASTReader.cpp
lib/Serialization/ASTWriter.cpp
test/PCH/pragma-pack.c
test/PCH/suspicious-pragma-pack.c
test/Parser/pragma-options.c
test/Parser/pragma-options.cpp
test/Parser/pragma-pack.c
test/Sema/Inputs/pragma-pack1.h
test/Sema/Inputs/pragma-pack2.h
test/Sema/pragma-pack.c
test/Sema/suspicious-pragma-pack.c
test/SemaObjC/Inputs/empty.h
test/SemaObjC/suspicious-pragma-pack.m
Index: test/SemaObjC/suspicious-pragma-pack.m
===
--- /dev/null
+++ test/SemaObjC/suspicious-pragma-pack.m
@@ -0,0 +1,6 @@
+// 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 records in the included file}}
+
+#pragma pack (pop)
Index: test/SemaObjC/Inputs/empty.h
===
--- /dev/null
+++ test/SemaObjC/Inputs/empty.h
@@ -0,0 +1 @@
+// empty
Index: test/Sema/suspicious-pragma-pack.c
===
--- /dev/null
+++ test/Sema/suspicious-pragma-pack.c
@@ -0,0 +1,44 @@
+// 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 records 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
Index: test/Sema/pragma-pack.c
===
--- test/Sema/pragma-pack.c
+++ test/Sema/pragma-pack.c
@@ -25,3 +25,8 @@
#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 ()
Index: test/Sema/Inputs/pragma-pack2.h
