d.zobnin.bugzilla created this revision.
d.zobnin.bugzilla added a reviewer: rnk.
d.zobnin.bugzilla added a subscriber: cfe-commits.
This patch attempts to improve implementation of several MS pragmas that use
stack (vtordisp, { bss | code | const | data }_seg) and fix some compatibility
issues.
Please take a look at the patch and let me know whether it's something worth
working on and whether I'm on the right way of doing this.
This patch:
1. Changes implementation of #pragma vtordisp to use existing PragmaStack<>
template class that *_seg pragmas use;
2. Fixes "#pragma vtordisp()" reset behavior -- it shouldn't affect the stack;
3. Supports "save-and-restore" of pragma state stacks on entering / exiting a
C++ method body, as MSVC does.
If this work is accepted, I propose several to-do's:
1. Change #pragma pack implementation to use the same approach as other "stack"
pragmas use;
2. Introduce diagnostics on popping named stack slots, as MSVC does
Currently I tried to make Clang behave exactly as MSVC behaves, though, I can
think of possible better solutions, please see "Implementation notes" below.
Motivation:
Currently Clang implements "stack" pragmas in several ways: "vtordisp" and
"pack" have their own stacks in Sema, pragma actions are performed differently.
MSVC seems to have a common implementation of all these pragmas. One common
feature is "save-restore" on entering / exiting a C++ method body:
```
#pragma pack(4)
#pragma pack(show)
struct S {
void f() {
#pragma pack(push, 8)
#pragma pack(show)
}
};
#pragma pack(show)
```
MSVC's output: 4, 8, 4
Clang's output: 4, 8, 8
MSVC seems to introduce artificial sentinel slots on entering a C++ method body
and pops them on exit. Consider
```
#pragma pack(push, 4)
struct S {
void f() {
#pragma pack(pop)
}
};
```
MSVC emits:
```
warning C4159: #pragma vtordisp(pop,...): has popped previously pushed
identifier '<InternalPragmaState>'
```
The same approach is used for "vtordisp" pragma, although it doesn't support
#pragma vtordisp (pop, id) syntax.
Please note that in the above example the stack gets corrupted and might lead
to unexpected behavior later.
I implemented "save-and-restore" feature for #pragma vtordisp in
http://reviews.llvm.org/D14467 by copying a skack to a RAII object and back, no
matter how many times "pop" is used. Pragmas other than "vtordisp" in Clang
don't support this feature yet.
Implementation notes:
MSVC seems to use the same name "<InternalPragmaState>" even for methods of
nested classes. It is legal in correct cases, because "#pragma <name>(pop, id)"
will look for the closest slot with mathing name, but if the nested sentinel
slot is mistakenly popped, it will cause popping the stack up to surrounding
sentinel.
Maybe, if we choose this approach, we could use unique sentinel names for
better recovery? Or even mark the sentinels to prevent a programmer from
popping them by accident?
http://reviews.llvm.org/D19361
Files:
include/clang/Sema/Sema.h
lib/Parse/ParsePragma.cpp
lib/Parse/ParseStmt.cpp
lib/Sema/Sema.cpp
lib/Sema/SemaAttr.cpp
test/CodeGenCXX/sections.cpp
test/SemaCXX/pragma-vtordisp.cpp
Index: lib/Parse/ParseStmt.cpp
===================================================================
--- lib/Parse/ParseStmt.cpp
+++ lib/Parse/ParseStmt.cpp
@@ -1928,7 +1928,8 @@
// Save and reset current vtordisp stack if we have entered a C++ method body.
bool IsCXXMethod =
getLangOpts().CPlusPlus && Decl && isa<CXXMethodDecl>(Decl);
- Sema::VtorDispStackRAII SavedVtorDispStack(Actions, IsCXXMethod);
+ Sema::PragmaStackSentinelRAII
+ PragmaStackSentinel(Actions, "InternalPragmaState", IsCXXMethod);
// Do not enter a scope for the brace, as the arguments are in the same scope
// (the function body) as the body itself. Instead, just read the statement
@@ -1972,7 +1973,8 @@
// Save and reset current vtordisp stack if we have entered a C++ method body.
bool IsCXXMethod =
getLangOpts().CPlusPlus && Decl && isa<CXXMethodDecl>(Decl);
- Sema::VtorDispStackRAII SavedVtorDispStack(Actions, IsCXXMethod);
+ Sema::PragmaStackSentinelRAII
+ PragmaStackSentinel(Actions, "InternalPragmaState", IsCXXMethod);
SourceLocation LBraceLoc = Tok.getLocation();
StmtResult FnBody(ParseCXXTryBlockCommon(TryLoc, /*FnTry*/true));
Index: lib/Parse/ParsePragma.cpp
===================================================================
--- lib/Parse/ParsePragma.cpp
+++ lib/Parse/ParsePragma.cpp
@@ -497,11 +497,11 @@
void Parser::HandlePragmaMSVtorDisp() {
assert(Tok.is(tok::annot_pragma_ms_vtordisp));
uintptr_t Value = reinterpret_cast<uintptr_t>(Tok.getAnnotationValue());
- Sema::PragmaVtorDispKind Kind =
- static_cast<Sema::PragmaVtorDispKind>((Value >> 16) & 0xFFFF);
+ Sema::PragmaMsStackAction Action =
+ static_cast<Sema::PragmaMsStackAction>((Value >> 16) & 0xFFFF);
MSVtorDispAttr::Mode Mode = MSVtorDispAttr::Mode(Value & 0xFFFF);
SourceLocation PragmaLoc = ConsumeToken(); // The annotation token.
- Actions.ActOnPragmaMSVtorDisp(Kind, PragmaLoc, Mode);
+ Actions.ActOnPragmaMSVtorDisp(Action, PragmaLoc, Mode);
}
void Parser::HandlePragmaMSPragma() {
@@ -1603,7 +1603,7 @@
}
PP.Lex(Tok);
- Sema::PragmaVtorDispKind Kind = Sema::PVDK_Set;
+ Sema::PragmaMsStackAction Action = Sema::PSK_Set;
const IdentifierInfo *II = Tok.getIdentifierInfo();
if (II) {
if (II->isStr("push")) {
@@ -1614,24 +1614,24 @@
return;
}
PP.Lex(Tok);
- Kind = Sema::PVDK_Push;
+ Action = Sema::PSK_Push_Set;
// not push, could be on/off
} else if (II->isStr("pop")) {
// #pragma vtordisp(pop)
PP.Lex(Tok);
- Kind = Sema::PVDK_Pop;
+ Action = Sema::PSK_Pop;
}
// not push or pop, could be on/off
} else {
if (Tok.is(tok::r_paren)) {
// #pragma vtordisp()
- Kind = Sema::PVDK_Reset;
+ Action = Sema::PSK_Reset;
}
}
uint64_t Value = 0;
- if (Kind == Sema::PVDK_Push || Kind == Sema::PVDK_Set) {
+ if (Action & Sema::PSK_Push || Action & Sema::PSK_Set) {
const IdentifierInfo *II = Tok.getIdentifierInfo();
if (II && II->isStr("off")) {
PP.Lex(Tok);
@@ -1673,7 +1673,7 @@
AnnotTok.setLocation(VtorDispLoc);
AnnotTok.setAnnotationEndLoc(EndLoc);
AnnotTok.setAnnotationValue(reinterpret_cast<void *>(
- static_cast<uintptr_t>((Kind << 16) | (Value & 0xFFFF))));
+ static_cast<uintptr_t>((Action << 16) | (Value & 0xFFFF))));
PP.EnterToken(AnnotTok);
}
Index: lib/Sema/Sema.cpp
===================================================================
--- lib/Sema/Sema.cpp
+++ lib/Sema/Sema.cpp
@@ -82,7 +82,7 @@
PackContext(nullptr), MSStructPragmaOn(false),
MSPointerToMemberRepresentationMethod(
LangOpts.getMSPointerToMemberRepresentationMethod()),
- VtorDispModeStack(1, MSVtorDispAttr::Mode(LangOpts.VtorDispMode)),
+ VtorDispStack(MSVtorDispAttr::Mode(LangOpts.VtorDispMode)),
DataSegStack(nullptr), BSSSegStack(nullptr), ConstSegStack(nullptr),
CodeSegStack(nullptr), CurInitSeg(nullptr), VisContext(nullptr),
IsBuildingRecoveryCallExpr(false),
Index: lib/Sema/SemaAttr.cpp
===================================================================
--- lib/Sema/SemaAttr.cpp
+++ lib/Sema/SemaAttr.cpp
@@ -136,9 +136,9 @@
// FIXME: We should merge AddAlignmentAttributesForRecord with
// AddMsStructLayoutForRecord into AddPragmaAttributesForRecord, which takes
// all active pragmas and applies them as attributes to class definitions.
- if (VtorDispModeStack.back() != getLangOpts().VtorDispMode)
+ if (VtorDispStack.CurrentValue != getLangOpts().VtorDispMode)
RD->addAttr(
- MSVtorDispAttr::CreateImplicit(Context, VtorDispModeStack.back()));
+ MSVtorDispAttr::CreateImplicit(Context, VtorDispStack.CurrentValue));
}
void Sema::ActOnPragmaOptionsAlign(PragmaOptionsAlignKind Kind,
@@ -292,38 +292,22 @@
ImplicitMSInheritanceAttrLoc = PragmaLoc;
}
-void Sema::ActOnPragmaMSVtorDisp(PragmaVtorDispKind Kind,
+void Sema::ActOnPragmaMSVtorDisp(PragmaMsStackAction Action,
SourceLocation PragmaLoc,
MSVtorDispAttr::Mode Mode) {
- switch (Kind) {
- case PVDK_Set:
- VtorDispModeStack.back() = Mode;
- break;
- case PVDK_Push:
- VtorDispModeStack.push_back(Mode);
- break;
- case PVDK_Reset:
- VtorDispModeStack.clear();
- VtorDispModeStack.push_back(MSVtorDispAttr::Mode(LangOpts.VtorDispMode));
- break;
- case PVDK_Pop:
- VtorDispModeStack.pop_back();
- if (VtorDispModeStack.empty()) {
- Diag(PragmaLoc, diag::warn_pragma_pop_failed) << "vtordisp"
- << "stack empty";
- VtorDispModeStack.push_back(MSVtorDispAttr::Mode(LangOpts.VtorDispMode));
- }
- break;
- }
+ if (Action & PSK_Pop && VtorDispStack.Stack.empty())
+ Diag(PragmaLoc, diag::warn_pragma_pop_failed) << "vtordisp"
+ << "stack empty";
+ VtorDispStack.Act(PragmaLoc, Action, StringRef(), Mode);
}
template<typename ValueType>
void Sema::PragmaStack<ValueType>::Act(SourceLocation PragmaLocation,
PragmaMsStackAction Action,
llvm::StringRef StackSlotLabel,
ValueType Value) {
if (Action == PSK_Reset) {
- CurrentValue = nullptr;
+ CurrentValue = DefaultValue;
return;
}
if (Action & PSK_Push)
Index: include/clang/Sema/Sema.h
===================================================================
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -327,40 +327,21 @@
LangOptions::PragmaMSPointersToMembersKind
MSPointerToMemberRepresentationMethod;
- enum PragmaVtorDispKind {
- PVDK_Push, ///< #pragma vtordisp(push, mode)
- PVDK_Set, ///< #pragma vtordisp(mode)
- PVDK_Pop, ///< #pragma vtordisp(pop)
- PVDK_Reset ///< #pragma vtordisp()
- };
-
- enum PragmaMsStackAction {
- PSK_Reset, // #pragma ()
- PSK_Set, // #pragma ("name")
- PSK_Push, // #pragma (push[, id])
- PSK_Push_Set, // #pragma (push[, id], "name")
- PSK_Pop, // #pragma (pop[, id])
- PSK_Pop_Set, // #pragma (pop[, id], "name")
- };
-
- /// \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
- ///
- /// The stack always has at least one element in it.
- SmallVector<MSVtorDispAttr::Mode, 2> VtorDispModeStack;
-
/// Stack of active SEH __finally scopes. Can be empty.
SmallVector<Scope*, 2> CurrentSEHFinally;
/// \brief Source location for newly created implicit MSInheritanceAttrs
SourceLocation ImplicitMSInheritanceAttrLoc;
+ enum PragmaMsStackAction {
+ PSK_Reset = 0x0, // #pragma ()
+ PSK_Set = 0x1, // #pragma ("name")
+ PSK_Push = 0x2, // #pragma (push[, id])
+ PSK_Pop = 0x4, // #pragma (pop[, id])
+ PSK_Push_Set = PSK_Push | PSK_Set, // #pragma (push[, id], "name")
+ PSK_Pop_Set = PSK_Pop | PSK_Set, // #pragma (pop[, id], "name")
+ };
+
template<typename ValueType>
struct PragmaStack {
struct Slot {
@@ -377,18 +358,86 @@
PragmaMsStackAction Action,
llvm::StringRef StackSlotLabel,
ValueType Value);
- explicit PragmaStack(const ValueType &Value)
- : CurrentValue(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:
+ //
+ // struct S {
+ // #pragma <name>(push, InternalPragmaSlot, <current_pragma_value>)
+ // void Method {}
+ // #pragma <name>(pop, InternalPragmaSlot)
+ // };
+ //
+ // It works even with #pragma vtordisp, although MSVC doesn't support
+ // #pragma vtordisp(push [, id], n)
+ // syntax.
+ //
+ // Push / pop a named sentinel slot.
+ void SentinelAction(PragmaMsStackAction Action, StringRef Label) {
+ assert((Action == PSK_Push || Action == PSK_Pop) &&
+ "Can only push / pop #pragma stack sentinels!");
+ Act(CurrentPragmaLocation, Action, Label, CurrentValue);
+ }
+
+ // Constructors.
+ explicit PragmaStack(const ValueType &Default)
+ : DefaultValue(Default), CurrentValue(Default) {}
+ explicit PragmaStack(const ValueType &Default, const ValueType &Value)
+ : DefaultValue(Default), CurrentValue(Value) {}
+
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;
PragmaStack<StringLiteral *> DataSegStack;
PragmaStack<StringLiteral *> BSSSegStack;
PragmaStack<StringLiteral *> ConstSegStack;
PragmaStack<StringLiteral *> CodeSegStack;
+ // TODO: Change implementation of #pragma pack to use PragmaStack<> approach.
+
+ // RAII object to psuh / pop sentinel slots for all MS #pragma stacks.
+ // Actions should be performed only if we enter / exit a C++ method body.
+ class PragmaStackSentinelRAII {
+ public:
+ PragmaStackSentinelRAII(Sema &S, StringRef SlotLabel, bool ShouldAct)
+ : S(S), SlotLabel(SlotLabel), ShouldAct(ShouldAct) {
+ if (ShouldAct) {
+ S.VtorDispStack.SentinelAction(PSK_Push, SlotLabel);
+ S.DataSegStack.SentinelAction(PSK_Push, SlotLabel);
+ S.BSSSegStack.SentinelAction(PSK_Push, SlotLabel);
+ S.ConstSegStack.SentinelAction(PSK_Push, SlotLabel);
+ S.CodeSegStack.SentinelAction(PSK_Push, SlotLabel);
+ }
+ }
+
+ ~PragmaStackSentinelRAII() {
+ if (ShouldAct) {
+ S.VtorDispStack.SentinelAction(PSK_Pop, SlotLabel);
+ S.DataSegStack.SentinelAction(PSK_Pop, SlotLabel);
+ S.BSSSegStack.SentinelAction(PSK_Pop, SlotLabel);
+ S.ConstSegStack.SentinelAction(PSK_Pop, SlotLabel);
+ S.CodeSegStack.SentinelAction(PSK_Pop, SlotLabel);
+ }
+ }
+
+ private:
+ Sema &S;
+ StringRef SlotLabel;
+ bool ShouldAct;
+ };
/// A mapping that describes the nullability we've seen in each header file.
FileNullabilityMap NullabilityMap;
@@ -1011,24 +1060,6 @@
bool OldFPContractState : 1;
};
- /// Records and restores the vtordisp state on entry/exit of C++ method body.
- class VtorDispStackRAII {
- public:
- VtorDispStackRAII(Sema &S, bool ShouldSaveAndRestore)
- : S(S), ShouldSaveAndRestore(ShouldSaveAndRestore), OldVtorDispStack() {
- if (ShouldSaveAndRestore)
- OldVtorDispStack = S.VtorDispModeStack;
- }
- ~VtorDispStackRAII() {
- if (ShouldSaveAndRestore)
- S.VtorDispModeStack = OldVtorDispStack;
- }
- private:
- Sema &S;
- bool ShouldSaveAndRestore;
- SmallVector<MSVtorDispAttr::Mode, 2> OldVtorDispStack;
- };
-
void addImplicitTypedef(StringRef Name, QualType T);
public:
@@ -7666,7 +7697,8 @@
SourceLocation PragmaLoc);
/// \brief Called on well formed \#pragma vtordisp().
- void ActOnPragmaMSVtorDisp(PragmaVtorDispKind Kind, SourceLocation PragmaLoc,
+ void ActOnPragmaMSVtorDisp(PragmaMsStackAction Action,
+ SourceLocation PragmaLoc,
MSVtorDispAttr::Mode Value);
enum PragmaSectionKind {
Index: test/SemaCXX/pragma-vtordisp.cpp
===================================================================
--- test/SemaCXX/pragma-vtordisp.cpp
+++ test/SemaCXX/pragma-vtordisp.cpp
@@ -22,7 +22,8 @@
// Test a reset.
#pragma vtordisp()
-#pragma vtordisp(pop) // expected-warning {{#pragma vtordisp(pop, ...) failed: stack empty}}
+#pragma vtordisp(pop) // stack should NOT be affected by reset.
+ // Now stack contains '1'.
#pragma vtordisp( // expected-warning {{unknown action for '#pragma vtordisp' - ignored}}
#pragma vtordisp(asdf) // expected-warning {{unknown action for '#pragma vtordisp' - ignored}}
@@ -42,6 +43,7 @@
virtual void f();
};
+#pragma vtordisp(pop) // After this stack should be empty.
#pragma vtordisp(pop) // expected-warning {{#pragma vtordisp(pop, ...) failed: stack empty}}
void g() {
Index: test/CodeGenCXX/sections.cpp
===================================================================
--- test/CodeGenCXX/sections.cpp
+++ test/CodeGenCXX/sections.cpp
@@ -31,6 +31,31 @@
#pragma bss_seg(pop)
int TEST2;
+
+// Check "save-restore" of pragma stacks.
+struct Outer {
+ void f() {
+ #pragma bss_seg(push, ".bss3")
+ #pragma code_seg(push, ".my_code1")
+ #pragma const_seg(push, ".my_const1")
+ #pragma data_seg(push, ".data3")
+ struct Inner {
+ void g() {
+ #pragma bss_seg(push, ".bss4")
+ #pragma code_seg(push, ".my_code2")
+ #pragma const_seg(push, ".my_const2")
+ #pragma data_seg(push, ".data4")
+ }
+ };
+ }
+};
+
+void h2(void) {} // should be in ".my_code"
+int TEST3; // should be in ".bss1"
+int d2 = 1; // should be in ".data"
+extern const int b2; // should be in ".my_const"
+const int b2 = 1;
+
#pragma section("read_flag_section", read)
// Even though they are not declared const, these become constant since they are
// in a read-only section.
@@ -63,10 +88,14 @@
//CHECK: @i = global i32 0
//CHECK: @TEST1 = global i32 0
//CHECK: @TEST2 = global i32 0, section ".bss1"
+//CHECK: @TEST3 = global i32 0, section ".bss1"
+//CHECK: @d2 = global i32 1, section ".data"
+//CHECK: @b2 = constant i32 1, section ".my_const"
//CHECK: @unreferenced = constant i32 0, section "read_flag_section"
//CHECK: @referenced = constant i32 42, section "read_flag_section"
//CHECK: @implicitly_read_write = global i32 42, section "no_section_attributes"
//CHECK: @long_var = global i32 42, section "long_section"
//CHECK: @short_var = global i16 42, section "short_section"
//CHECK: define void @g()
//CHECK: define void @h() {{.*}} section ".my_code"
+//CHECK: define void @h2() {{.*}} section ".my_code"
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
