[win] Replace the .seh_startchained and .sehendchained instructions with .seh_splitchained #172895
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
@llvm/pr-subscribers-platform-windows @llvm/pr-subscribers-llvm-mc Author: Daniel Paoliello (dpaoliello) ChangesLLVM's existing support for chaining x64 Windows SEH frame infos is broken. Windows requires that each frame info (including the parent) is adjacent, but not overlapping, and the whole function is covered (i.e., each instruction maps to exactly one frame info). Therefore, having start/end chaining pseudo instructions doesn't make any sense, as every "end chain" pseudo would need a start pseudo immediately after it or be at the end of function. This change switches having a "split chain" pseudo instruction that ends the current frame and starts a new chained frame. Added a release note about the replacement - to my knowledge there is no one actually using frame info chaining, so it is highly unlikely that it will break any code. Split off from #159206 Full diff: https://github.com/llvm/llvm-project/pull/172895.diff 10 Files Affected:
diff --git a/llvm/docs/ReleaseNotes.md b/llvm/docs/ReleaseNotes.md
index ee285fe56b51a..ebc872985d2d9 100644
--- a/llvm/docs/ReleaseNotes.md
+++ b/llvm/docs/ReleaseNotes.md
@@ -164,6 +164,8 @@ Changes to the Windows Target
-----------------------------
* `-fpseudo-probe-for-profiling` is now supported for COFF.
+* The `.seh_startchained` and `.seh_endchained` assembly instructions have been removed and replaced
+ with a new `.seh_splitchained` instruction.
Changes to the X86 Backend
--------------------------
diff --git a/llvm/include/llvm/MC/MCStreamer.h b/llvm/include/llvm/MC/MCStreamer.h
index 79c715e3820a6..2a0095c9b7613 100644
--- a/llvm/include/llvm/MC/MCStreamer.h
+++ b/llvm/include/llvm/MC/MCStreamer.h
@@ -1024,8 +1024,7 @@ class LLVM_ABI MCStreamer {
/// for the frame. We cannot use the End marker, as it is not set at the
/// point of emitting .xdata, in order to indicate that the frame is active.
virtual void emitWinCFIFuncletOrFuncEnd(SMLoc Loc = SMLoc());
- virtual void emitWinCFIStartChained(SMLoc Loc = SMLoc());
- virtual void emitWinCFIEndChained(SMLoc Loc = SMLoc());
+ virtual void emitWinCFISplitChained(SMLoc Loc = SMLoc());
virtual void emitWinCFIPushReg(MCRegister Register, SMLoc Loc = SMLoc());
virtual void emitWinCFISetFrame(MCRegister Register, unsigned Offset,
SMLoc Loc = SMLoc());
diff --git a/llvm/include/llvm/MC/MCWinEH.h b/llvm/include/llvm/MC/MCWinEH.h
index b3d5c9cc8620a..9fbfd34da8e64 100644
--- a/llvm/include/llvm/MC/MCWinEH.h
+++ b/llvm/include/llvm/MC/MCWinEH.h
@@ -59,7 +59,7 @@ struct FrameInfo {
uint8_t Version = DefaultVersion;
int LastFrameInst = -1;
- const FrameInfo *ChainedParent = nullptr;
+ FrameInfo *ChainedParent = nullptr;
std::vector<Instruction> Instructions;
struct Epilog {
std::vector<Instruction> Instructions;
@@ -90,9 +90,9 @@ struct FrameInfo {
FrameInfo(const MCSymbol *Function, const MCSymbol *BeginFuncEHLabel)
: Begin(BeginFuncEHLabel), Function(Function) {}
FrameInfo(const MCSymbol *Function, const MCSymbol *BeginFuncEHLabel,
- const FrameInfo *ChainedParent)
+ FrameInfo *ChainedParent)
: Begin(BeginFuncEHLabel), Function(Function),
- ChainedParent(ChainedParent) {}
+ Version(ChainedParent->Version), ChainedParent(ChainedParent) {}
bool empty() const {
if (!Instructions.empty())
diff --git a/llvm/lib/MC/MCAsmStreamer.cpp b/llvm/lib/MC/MCAsmStreamer.cpp
index e2543058394a2..269edb383087c 100644
--- a/llvm/lib/MC/MCAsmStreamer.cpp
+++ b/llvm/lib/MC/MCAsmStreamer.cpp
@@ -375,8 +375,7 @@ class MCAsmStreamer final : public MCStreamer {
void emitWinCFIStartProc(const MCSymbol *Symbol, SMLoc Loc) override;
void emitWinCFIEndProc(SMLoc Loc) override;
void emitWinCFIFuncletOrFuncEnd(SMLoc Loc) override;
- void emitWinCFIStartChained(SMLoc Loc) override;
- void emitWinCFIEndChained(SMLoc Loc) override;
+ void emitWinCFISplitChained(SMLoc Loc) override;
void emitWinCFIPushReg(MCRegister Register, SMLoc Loc) override;
void emitWinCFISetFrame(MCRegister Register, unsigned Offset,
SMLoc Loc) override;
@@ -2175,17 +2174,10 @@ void MCAsmStreamer::emitWinCFIFuncletOrFuncEnd(SMLoc Loc) {
EmitEOL();
}
-void MCAsmStreamer::emitWinCFIStartChained(SMLoc Loc) {
- MCStreamer::emitWinCFIStartChained(Loc);
+void MCAsmStreamer::emitWinCFISplitChained(SMLoc Loc) {
+ MCStreamer::emitWinCFISplitChained(Loc);
- OS << "\t.seh_startchained";
- EmitEOL();
-}
-
-void MCAsmStreamer::emitWinCFIEndChained(SMLoc Loc) {
- MCStreamer::emitWinCFIEndChained(Loc);
-
- OS << "\t.seh_endchained";
+ OS << "\t.seh_splitchained";
EmitEOL();
}
diff --git a/llvm/lib/MC/MCParser/COFFAsmParser.cpp b/llvm/lib/MC/MCParser/COFFAsmParser.cpp
index d9ec04c6b6100..fe8a15400035a 100644
--- a/llvm/lib/MC/MCParser/COFFAsmParser.cpp
+++ b/llvm/lib/MC/MCParser/COFFAsmParser.cpp
@@ -79,10 +79,8 @@ class COFFAsmParser : public MCAsmParserExtension {
".seh_endproc");
addDirectiveHandler<&COFFAsmParser::parseSEHDirectiveEndFuncletOrFunc>(
".seh_endfunclet");
- addDirectiveHandler<&COFFAsmParser::parseSEHDirectiveStartChained>(
- ".seh_startchained");
- addDirectiveHandler<&COFFAsmParser::parseSEHDirectiveEndChained>(
- ".seh_endchained");
+ addDirectiveHandler<&COFFAsmParser::parseSEHDirectiveSplitChained>(
+ ".seh_splitchained");
addDirectiveHandler<&COFFAsmParser::parseSEHDirectiveHandler>(
".seh_handler");
addDirectiveHandler<&COFFAsmParser::parseSEHDirectiveHandlerData>(
@@ -142,8 +140,7 @@ class COFFAsmParser : public MCAsmParserExtension {
bool parseSEHDirectiveStartProc(StringRef, SMLoc);
bool parseSEHDirectiveEndProc(StringRef, SMLoc);
bool parseSEHDirectiveEndFuncletOrFunc(StringRef, SMLoc);
- bool parseSEHDirectiveStartChained(StringRef, SMLoc);
- bool parseSEHDirectiveEndChained(StringRef, SMLoc);
+ bool parseSEHDirectiveSplitChained(StringRef, SMLoc);
bool parseSEHDirectiveHandler(StringRef, SMLoc);
bool parseSEHDirectiveHandlerData(StringRef, SMLoc);
bool parseSEHDirectiveAllocStack(StringRef, SMLoc);
@@ -684,15 +681,9 @@ bool COFFAsmParser::parseSEHDirectiveEndFuncletOrFunc(StringRef, SMLoc Loc) {
return false;
}
-bool COFFAsmParser::parseSEHDirectiveStartChained(StringRef, SMLoc Loc) {
+bool COFFAsmParser::parseSEHDirectiveSplitChained(StringRef, SMLoc Loc) {
Lex();
- getStreamer().emitWinCFIStartChained(Loc);
- return false;
-}
-
-bool COFFAsmParser::parseSEHDirectiveEndChained(StringRef, SMLoc Loc) {
- Lex();
- getStreamer().emitWinCFIEndChained(Loc);
+ getStreamer().emitWinCFISplitChained(Loc);
return false;
}
diff --git a/llvm/lib/MC/MCStreamer.cpp b/llvm/lib/MC/MCStreamer.cpp
index bc7398120096e..db22382dbe86b 100644
--- a/llvm/lib/MC/MCStreamer.cpp
+++ b/llvm/lib/MC/MCStreamer.cpp
@@ -749,13 +749,14 @@ void MCStreamer::emitWinCFIEndProc(SMLoc Loc) {
WinEH::FrameInfo *CurFrame = EnsureValidWinFrameInfo(Loc);
if (!CurFrame)
return;
- if (CurFrame->ChainedParent)
- getContext().reportError(Loc, "Not all chained regions terminated!");
MCSymbol *Label = emitCFILabel();
CurFrame->End = Label;
- if (!CurFrame->FuncletOrFuncEnd)
- CurFrame->FuncletOrFuncEnd = CurFrame->End;
+ const MCSymbol **FuncletOrFuncEndPtr =
+ CurFrame->ChainedParent ? &CurFrame->ChainedParent->FuncletOrFuncEnd
+ : &CurFrame->FuncletOrFuncEnd;
+ if (!*FuncletOrFuncEndPtr)
+ *FuncletOrFuncEndPtr = CurFrame->End;
for (size_t I = CurrentProcWinFrameInfoStartIndex, E = WinFrameInfos.size();
I != E; ++I)
@@ -767,38 +768,38 @@ void MCStreamer::emitWinCFIFuncletOrFuncEnd(SMLoc Loc) {
WinEH::FrameInfo *CurFrame = EnsureValidWinFrameInfo(Loc);
if (!CurFrame)
return;
- if (CurFrame->ChainedParent)
- getContext().reportError(Loc, "Not all chained regions terminated!");
MCSymbol *Label = emitCFILabel();
- CurFrame->FuncletOrFuncEnd = Label;
+ const MCSymbol **FuncletOrFuncEndPtr =
+ CurFrame->ChainedParent ? &CurFrame->ChainedParent->FuncletOrFuncEnd
+ : &CurFrame->FuncletOrFuncEnd;
+ *FuncletOrFuncEndPtr = Label;
}
-void MCStreamer::emitWinCFIStartChained(SMLoc Loc) {
+void MCStreamer::emitWinCFISplitChained(SMLoc Loc) {
WinEH::FrameInfo *CurFrame = EnsureValidWinFrameInfo(Loc);
if (!CurFrame)
return;
- MCSymbol *StartProc = emitCFILabel();
-
- WinFrameInfos.emplace_back(std::make_unique<WinEH::FrameInfo>(
- CurFrame->Function, StartProc, CurFrame));
- CurrentWinFrameInfo = WinFrameInfos.back().get();
- CurrentWinFrameInfo->TextSection = getCurrentSectionOnly();
-}
-
-void MCStreamer::emitWinCFIEndChained(SMLoc Loc) {
- WinEH::FrameInfo *CurFrame = EnsureValidWinFrameInfo(Loc);
- if (!CurFrame)
- return;
- if (!CurFrame->ChainedParent)
+ if (!CurFrame->PrologEnd && !CurFrame->ChainedParent)
return getContext().reportError(
- Loc, "End of a chained region outside a chained region!");
+ Loc, "can't split into a new chained region (.seh_splitchained) in the "
+ "middle of a prolog in " +
+ CurFrame->Function->getName());
MCSymbol *Label = emitCFILabel();
+ // Complete the current frame before starting a new, chained one.
CurFrame->End = Label;
- CurrentWinFrameInfo = const_cast<WinEH::FrameInfo *>(CurFrame->ChainedParent);
+
+ // All chained frames point to the same parent.
+ WinEH::FrameInfo *ChainedParent =
+ CurFrame->ChainedParent ? CurFrame->ChainedParent : CurFrame;
+
+ WinFrameInfos.emplace_back(std::make_unique<WinEH::FrameInfo>(
+ CurFrame->Function, Label, ChainedParent));
+ CurrentWinFrameInfo = WinFrameInfos.back().get();
+ CurrentWinFrameInfo->TextSection = getCurrentSectionOnly();
}
void MCStreamer::emitWinEHHandler(const MCSymbol *Sym, bool Unwind, bool Except,
@@ -806,9 +807,10 @@ void MCStreamer::emitWinEHHandler(const MCSymbol *Sym, bool Unwind, bool Except,
WinEH::FrameInfo *CurFrame = EnsureValidWinFrameInfo(Loc);
if (!CurFrame)
return;
- if (CurFrame->ChainedParent)
- return getContext().reportError(
- Loc, "Chained unwind areas can't have handlers!");
+
+ // Handlers are always associated with the parent frame.
+ CurFrame = CurFrame->ChainedParent ? CurFrame->ChainedParent : CurFrame;
+
CurFrame->ExceptionHandler = Sym;
if (!Except && !Unwind)
getContext().reportError(Loc, "Don't know what kind of handler this is!");
@@ -822,8 +824,6 @@ void MCStreamer::emitWinEHHandlerData(SMLoc Loc) {
WinEH::FrameInfo *CurFrame = EnsureValidWinFrameInfo(Loc);
if (!CurFrame)
return;
- if (CurFrame->ChainedParent)
- getContext().reportError(Loc, "Chained unwind areas can't have handlers!");
}
void MCStreamer::emitCGProfileEntry(const MCSymbolRefExpr *From,
@@ -994,7 +994,8 @@ void MCStreamer::emitWinCFIBeginEpilogue(SMLoc Loc) {
if (!CurFrame)
return;
- if (!CurFrame->PrologEnd)
+ // Chained unwinds aren't guaranteed to have a prolog.
+ if (!CurFrame->PrologEnd && !CurFrame->ChainedParent)
return getContext().reportError(
Loc, "starting epilogue (.seh_startepilogue) before prologue has ended "
"(.seh_endprologue) in " +
diff --git a/llvm/test/MC/AsmParser/directive_seh.s b/llvm/test/MC/AsmParser/directive_seh.s
index 8eb50ed996dd3..0344e7ea3ad1f 100644
--- a/llvm/test/MC/AsmParser/directive_seh.s
+++ b/llvm/test/MC/AsmParser/directive_seh.s
@@ -42,13 +42,11 @@ func:
# CHECK: .seh_handlerdata
.long 0
.text
- .seh_startchained
.seh_endprologue
- .seh_endchained
+ .seh_splitchained
# CHECK: .text
-# CHECK: .seh_startchained
# CHECK: .seh_endprologue
-# CHECK: .seh_endchained
+# CHECK: .seh_splitchained
.seh_startepilogue
# CHECK: .seh_startepilogue
lea (%rbx), %rsp
diff --git a/llvm/test/MC/AsmParser/seh-directive-errors.s b/llvm/test/MC/AsmParser/seh-directive-errors.s
index d9dfe4b4182b9..5ed477b16c7f2 100644
--- a/llvm/test/MC/AsmParser/seh-directive-errors.s
+++ b/llvm/test/MC/AsmParser/seh-directive-errors.s
@@ -42,6 +42,8 @@ f: # @f
.seh_stackalloc 32
.seh_startepilogue
# CHECK: :[[@LINE-1]]:{{[0-9]+}}: error: starting epilogue (.seh_startepilogue) before prologue has ended (.seh_endprologue) in f
+ .seh_splitchained
+ # CHECK: :[[@LINE-1]]:{{[0-9]+}}: error: can't split into a new chained region (.seh_splitchained) in the middle of a prolog in f
.seh_endprologue
nop
.seh_endepilogue
diff --git a/llvm/test/MC/COFF/seh.s b/llvm/test/MC/COFF/seh.s
index 31e845397f2d0..18ad1d692838c 100644
--- a/llvm/test/MC/COFF/seh.s
+++ b/llvm/test/MC/COFF/seh.s
@@ -141,9 +141,8 @@ func:
.seh_handlerdata
.long 0
.text
- .seh_startchained
.seh_endprologue
- .seh_endchained
+ .seh_splitchained
.seh_startepilogue
lea (%rbx), %rsp
pop %rbx
diff --git a/llvm/test/tools/llvm-objdump/COFF/Inputs/win64-unwind.exe.coff-x86_64.asm b/llvm/test/tools/llvm-objdump/COFF/Inputs/win64-unwind.exe.coff-x86_64.asm
index c44d59b324036..9c301652f8ea4 100644
--- a/llvm/test/tools/llvm-objdump/COFF/Inputs/win64-unwind.exe.coff-x86_64.asm
+++ b/llvm/test/tools/llvm-objdump/COFF/Inputs/win64-unwind.exe.coff-x86_64.asm
@@ -19,9 +19,8 @@ func:
.seh_handlerdata
.long 0
.text
- .seh_startchained
.seh_endprologue
- .seh_endchained
+ .seh_splitchained
lea (%rbx), %rsp
pop %rbx
addq $24, %rsp
|
Collaborator
efriedma-quic
left a comment
efriedma-quic
left a comment
There was a problem hiding this comment.
Please add a testcase showing how interactions like the following work.
.seh_pushreg %rbx
.seh_endprologue
.seh_splitchained
.seh_pushreg %rsi
.seh_endprologue
Please add a testcase showing the interaction between unwind-v2 and seh_splitchained. (I think you already had a testcase, but just didn't add it here for some reason?)
dpaoliello
force-pushed
the
splitchained
branch
from
2b6228b to
0821179
Compare
dpaoliello
force-pushed
the
splitchained
branch
from
0821179 to
4d6f094
Compare
Contributor
Author
Done, added to
I wasn't sure how much of the unwindv2 stuff you wanted in this PR, but it makes sense that using |
llvm/lib/MC/MCStreamer.cpp
Outdated
| @@ -994,7 +994,8 @@ void MCStreamer::emitWinCFIBeginEpilogue(SMLoc Loc) { | |||
| if (!CurFrame) | |||
| return; | |||
|
|
|||
| if (!CurFrame->PrologEnd) | |||
| // Chained unwinds aren't guaranteed to have a prolog. | |||
| if (!CurFrame->PrologEnd && !CurFrame->ChainedParent) | |||
Collaborator
There was a problem hiding this comment.
Do we need to do something to prevent someone from writing:
.seh_endprologue
.seh_splitchained
pushq %rsi
.seh_pushreg %rsi
.seh_startepilogue
Contributor
Author
There was a problem hiding this comment.
Good catch - added tests for this, added errors for other cases and ensured that the AsmParser will "recover" and not assert later while emitting the tables.
dpaoliello
force-pushed
the
splitchained
branch
from
4d6f094 to
1c6223b
Compare
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
dpaoliello
force-pushed
the
splitchained
branch
from
1c6223b to
832c319
Compare
dpaoliello
force-pushed
the
splitchained
branch
from
832c319 to
1ce078b
Compare
…ith .seh_splitchained
dpaoliello
force-pushed
the
splitchained
branch
from
1ce078b to
095537d
Compare
Contributor
Author
|
Ping @efriedma-quic any other concerns? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
LLVM's existing support for chaining x64 Windows SEH frame infos is broken. Windows requires that each frame info (including the parent) is adjacent, but not overlapping, and the whole function is covered (i.e., each instruction maps to exactly one frame info). Therefore, having start/end chaining pseudo instructions doesn't make any sense, as every "end chain" pseudo would need a start pseudo immediately after it or be at the end of function.
This change switches having a "split chain" pseudo instruction that ends the current frame and starts a new chained frame.
Added a release note about the replacement - to my knowledge there is no one actually using frame info chaining, so it is highly unlikely that it will break any code.
Split off from #159206