This is an archive of the discontinued LLVM Phabricator instance.

[4/5] [MC] [Win64EH] Fill in FuncletOrFuncEnd if missing
ClosedPublic

Authored by mstorsjo on Aug 25 2020, 4:38 AM.

Details

Summary

This can happen e.g. for code that declare .seh_proc/.seh_endproc in assembly, or for code that use .seh_handlerdata (which triggers the unwind info to be emitted before the end of the function).

The TextSection field must be made non-const to be able to use it with Streamer.SwitchSection().

Diff Detail

Event Timeline

mstorsjo created this revision.Aug 25 2020, 4:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2020, 4:38 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
efriedma added inline comments.Aug 25 2020, 1:51 PM
llvm/lib/MC/MCWin64EH.cpp
531

I don't understand why you're dropping the FIXME; clearly, if we're emitting unwind info that doesn't cover anything, something has gone very wrong.

681

It seems like you're setting FuncletOrFuncEnd in too many places; there should be exactly one directive that corresponds to the end of the function, I think.

mstorsjo added inline comments.Aug 26 2020, 1:48 AM
llvm/lib/MC/MCWin64EH.cpp
531

Right, the FIXME indeed should stay, if we still hit that case, even though it's much less probable after this change.

681

There's actually two different codepaths; either in MCStreamer::EmitWinCFIEndProc when running into .seh_endproc`, or in ARM64UnwindEmitter::EmitUnwindInfo when .seh_handlerdata forces emitting it while halfway through the function. But this one can be left out, as far as I see with the cases I have in mind.

The case with .seh_handlerdata is tricky, because it forces writing the xdata entry possibly before the full function has been generated. On x86 this is not an issue, because the xdata entry doesn't contain the full function length but that's referenced in the .pdata via relocations to the function start/end. But on arm, the xdata entry needs to know the full length. So for cases with .seh_handlerdata, we can place FuncletOrFuncEnd at the place where we are, and hope that it covers the parts of the function that unwinding will occur in.

mstorsjo updated this revision to Diff 287867.Aug 26 2020, 1:49 AM

Wrote a bit longer comment explaining the issue with potentially truncated function length on .seh_handlerdata, left out the redundant place for filling in FuncletOrFuncEnd, and kept the fixme comment.

efriedma added inline comments.Aug 26 2020, 5:26 PM
llvm/lib/MC/MCWin64EH.cpp
710

Instead of checking Streamer.getCurrentSectionOnly() == info->TextSection, can we just Streamer.SwitchSection(info->TextSection)?

mstorsjo added inline comments.Aug 26 2020, 10:59 PM
llvm/lib/MC/MCWin64EH.cpp
710

Oh, yeah I guess that'd work just as well.

mstorsjo updated this revision to Diff 288216.Aug 27 2020, 12:42 AM
mstorsjo edited the summary of this revision. (Show Details)

Using SwitchSection() to always emit the label if missing. Had to make TextSection non-const to be able to use it with SwitchSection().

mstorsjo updated this revision to Diff 288270.Aug 27 2020, 4:23 AM

Updated the testcase after the new version of 3/5.

efriedma added inline comments.Aug 27 2020, 2:09 PM
llvm/lib/MC/MCStreamer.cpp
696

Instead of setting FuncletOrFuncEnd here, should we be calling EmitWinCFIFuncletOrFuncEnd somewhere?

llvm/lib/MC/MCWin64EH.cpp
529

With the other changes in this patch, is it still possible for FuncletOrFuncEnd to be null?

mstorsjo added inline comments.Aug 27 2020, 11:46 PM
llvm/lib/MC/MCStreamer.cpp
696

I don't think so. EmitWinCFIEndProc is essentially called from two places; either from lib/CodeGen/AsmPrinter/WinException.cpp, which also does call EmitWinCFIFuncletOrFuncEnd properly, so there it shouldn't be possible to reach EndProc without FuncletOrFuncEnd being set. The only other case is MC/MCParser/COFFAsmParser.cpp, which has a 1:1 mapping between directives and calling these methods.

llvm/lib/MC/MCWin64EH.cpp
529

Ah, right, yes, it shouldn't be possible. Even if we'd trigger .seh_handlerdata immediately, we would have a FuncletOrFuncEnd symbol (with a zero distance from the start), and we'd skip writing this due to it being empty anyway. So we should be able to make this a report_fatal_error, as it shouldn't be triggerable even with specially crafted input.

mstorsjo updated this revision to Diff 288536.Aug 27 2020, 11:51 PM
mstorsjo edited the summary of this revision. (Show Details)

Made a missing FuncletOrFuncEnd a hard error.

This revision is now accepted and ready to land.Aug 28 2020, 4:27 PM
This revision was automatically updated to reflect the committed changes.