This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Enable Windows exception handling for basic block sections
AcceptedPublic

Authored by TaoPan on Apr 18 2021, 6:18 PM.

Details

Summary

Windows x64 exception handling is quite different with Exception
Handling of Itanium C++ ABI for IA-64, no need to do EH pad operation.

Reference:
https://docs.microsoft.com/en-us/cpp/build/exception-handling-x64?view=msvc-160
https://github.com/itanium-cxx-abi/cxx-abi/blob/master/exceptions.pdf

Diff Detail

Event Timeline

TaoPan created this revision.Apr 18 2021, 6:18 PM
TaoPan requested review of this revision.Apr 18 2021, 6:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2021, 6:18 PM
TaoPan updated this revision to Diff 338434.Apr 18 2021, 11:39 PM

git rebase

TaoPan updated this revision to Diff 343811.May 7 2021, 10:25 PM

Remove content of parent D99487 for applying this patch

TaoPan updated this revision to Diff 343812.May 7 2021, 10:28 PM

Remove content of parent D99487 for applying this patch

TaoPan updated this revision to Diff 344659.May 11 2021, 10:03 PM

git rebase

TaoPan edited the summary of this revision. (Show Details)May 11 2021, 10:04 PM

Is there currently a difference in the output when built with -basic-block-sections=all -unique-basic-block-section-names vs. without? If so, should CHECK for both cases to highlight the difference and add some explanatory comments on why a particular transformation took place. What are considered landing pads for avoidZeroOffsetLandingPad under MSVC? I can double-check to make sure they're truly not needed.

Otherwise this looks good and works well as a first step.

Is there currently a difference in the output when built with -basic-block-sections=all -unique-basic-block-section-names vs. without? If so, should CHECK for both cases to highlight the difference and add some explanatory comments on why a particular transformation took place. What are considered landing pads for avoidZeroOffsetLandingPad under MSVC? I can double-check to make sure they're truly not needed.

Otherwise this looks good and works well as a first step.

Thanks for your comment!
No big difference. There are 12 more sections in Sections table, I checked asm file, there are 12 BB sections, so all added sections are BB sections. And as the test file CHECK, one relocation of .xdata section changed from ".text" to "?TestCPPEX@@YAXH@Z.__part.3". Is this difference need to add without -basic-block-sections=all -unique-basic-block-section-names case for highlight the difference?
FYI, about avoidZeroOffsetLandingPad under MSVC, isFuncletEHPersonality(Pers) check and return of SelectionDAGISel::PrepareEHLandingPad() prevent later addLandingPad(MBB) and adding TargetOpcode::EH_LABEL MachineInstr in the case of MSVC. while (!MI->isEHLabel()) ++MI; of avoidZeroOffsetLandingPad is dead loop as no EHLabel MI in the case of MSVC.

No big difference. There are 12 more sections in Sections table, I checked asm file, there are 12 BB sections, so all added sections are BB sections. And as the test file CHECK, one relocation of .xdata section changed from ".text" to "?TestCPPEX@@YAXH@Z.__part.3". Is this difference need to add without -basic-block-sections=all -unique-basic-block-section-names case for highlight the difference?

Yeah, check that the .xdata relocation changed and the size of the Sections table (CHECK-COUNT or looking at the last entry for each configuration). Also does the above test case run correctly when built with Clang if you define it as "main" and add something interesting to the __except clause (with and without BBS)?

FYI, about avoidZeroOffsetLandingPad under MSVC, isFuncletEHPersonality(Pers) check and return of SelectionDAGISel::PrepareEHLandingPad() prevent later addLandingPad(MBB) and adding TargetOpcode::EH_LABEL MachineInstr in the case of MSVC. while (!MI->isEHLabel()) ++MI; of avoidZeroOffsetLandingPad is dead loop as no EHLabel MI in the case of MSVC.

I'm not sure what you mean by dead loop for while (!MI->isEHLabel()) Can you elaborate?

TaoPan updated this revision to Diff 354175.Jun 24 2021, 2:25 AM

Add baseline case and check size of the Sections table

TaoPan added a comment.EditedJun 24 2021, 3:35 AM

No big difference. There are 12 more sections in Sections table, I checked asm file, there are 12 BB sections, so all added sections are BB sections. And as the test file CHECK, one relocation of .xdata section changed from ".text" to "?TestCPPEX@@YAXH@Z.__part.3". Is this difference need to add without -basic-block-sections=all -unique-basic-block-section-names case for highlight the difference?

Yeah, check that the .xdata relocation changed and the size of the Sections table (CHECK-COUNT or looking at the last entry for each configuration). Also does the above test case run correctly when built with Clang if you define it as "main" and add something interesting to the __except clause (with and without BBS)?

I added baseline (without BBS) case and check the size of the Sections table.
I tried to build with clang, define it as "main" and add printf to the __except clause

without BBS, the exception can be captured, printf log in __except clause was printed.
with BBS,  build fail with "error: Cannot represent this expression", I'm investigating this error.

FYI, about avoidZeroOffsetLandingPad under MSVC, isFuncletEHPersonality(Pers) check and return of SelectionDAGISel::PrepareEHLandingPad() prevent later addLandingPad(MBB) and adding TargetOpcode::EH_LABEL MachineInstr in the case of MSVC. while (!MI->isEHLabel()) ++MI; of avoidZeroOffsetLandingPad is dead loop as no EHLabel MI in the case of MSVC.

I'm not sure what you mean by dead loop for while (!MI->isEHLabel()) Can you elaborate?

The dead loop occurs if "MBB->setIsEHPad() but all MIs of the MBB are not isEHLabel()", as condition !MI->isEHLabel() of while loop is always true.
Adding "&& MI != MBB.end()" to while (!MI->isEHLabel()) will resolve this dead loop.
The iterator is rolling loop, begin() is next to end(), the '++' operation change the iterator in loop turn, e.g. if size() is 2, the turn of '++' operation is:

begin() (1st node) -> 2nd node -> end() -> begin() (1st node) -> 2nd node -> ....

I don't know the reason MBB->setIsEHPad() without any MI of the MBB setIsEHLabel() in the case of Windows COFF.

I added baseline (without BBS) case and check the size of the Sections table.

Nice thanks. One comment on the format.

I tried to build with clang, define it as "main" and add printf to the __except clause

without BBS, the exception can be captured, printf log in __except clause was printed.
with BBS,  build fail with "error: Cannot represent this expression", I'm investigating this error.

Cool, feel free to file a bug and link it here to track progress.

The iterator is rolling loop, begin() is next to end(), the '++' operation change the iterator in loop turn, e.g. if size() is 2, the turn of '++' operation is:

begin() (1st node) -> 2nd node -> end() -> begin() (1st node) -> 2nd node -> ....

Ah it becomes infinite, thanks for clarifying.

I don't know the reason MBB->setIsEHPad() without any MI of the MBB setIsEHLabel() in the case of Windows COFF.

My cursory understanding is that Windows COFF in LLVM uses a different set of intrinsics to represent EH which probably don't fall under EHLabel. There's probably a good reason for that. If this is the case then probably better to guard it like you're currently doing from scanning wastefully. I would guard with if (!<triple for windows>) rather specifically checking for ELF. Also, put the check inside avoidZeroOffsetLandingPad rather than the call to it so the call site doesn't have to be aware of this.

llvm/test/MC/COFF/seh-bbs.ll
88

I would interleave these with the baseline so its easy to see how exactly the codegen differs, for this example:

FileCheck --check-prefix=COMMON,BASELINE %s
FileCheck --check-prefix=COMMON,BBS %s
...
; COMMON:          Relocations [
; COMMON-NEXT:       0xC IMAGE_REL_AMD64_ADDR32NB __C_specific_handler
; COMMON-NEXT:       0x14 IMAGE_REL_AMD64_ADDR32NB .text
; COMMON-NEXT:       0x18 IMAGE_REL_AMD64_ADDR32NB .text
; BASELINE:       0x20 IMAGE_REL_AMD64_ADDR32NB .text
; BBS:            0x20 IMAGE_REL_AMD64_ADDR32NB ?TestCPPEX@@YAXH@Z.__part.3

It's also good to add short comments for these diff spots on why this is expected behavior.

TaoPan updated this revision to Diff 354788.Jun 27 2021, 10:43 PM

Change CHECK format of test to COMMON, BASELINE, BBS for highlighting difference of BASELINE and BBS.
Change triple condition from COFF to Windows.
Move check of Windows inside avoidZeroOffsetLandingPad.
Add iterator end check for avoiding infinite loop.

TaoPan updated this revision to Diff 354790.Jun 27 2021, 11:05 PM

Add comment for difference of section number of BASELINE and BBS

I tried to build with clang, define it as "main" and add printf to the __except clause

without BBS, the exception can be captured, printf log in __except clause was printed.
with BBS,  build fail with "error: Cannot represent this expression", I'm investigating this error.

Cool, feel free to file a bug and link it here to track progress.

This bug depends on #D99487 and this commit, can't be reproduced without these two commits. Is it ok file the bug after landing these two commits?

I don't know the reason MBB->setIsEHPad() without any MI of the MBB setIsEHLabel() in the case of Windows COFF.

My cursory understanding is that Windows COFF in LLVM uses a different set of intrinsics to represent EH which probably don't fall under EHLabel. There's probably a good reason for that. If this is the case then probably better to guard it like you're currently doing from scanning wastefully. I would guard with if (!<triple for windows>) rather specifically checking for ELF. Also, put the check inside avoidZeroOffsetLandingPad rather than the call to it so the call site doesn't have to be aware of this.

Thanks for your analysis! I changed code according to your guidance.

llvm/test/MC/COFF/seh-bbs.ll
88

Good idea! I changed check format as you guided.

Cool, feel free to file a bug and link it here to track progress.

This bug depends on #D99487 and this commit, can't be reproduced without these two commits. Is it ok file the bug after landing these two commits?

In that case it would make sense to fix this in D99487. I pulled this down locally and noticed that baseline testing with BBS hits:

llc: /home/modimo/llvm-project/llvm/lib/MC/MCStreamer.cpp:1207: virtual void llvm::MCStreamer::SwitchSection(llvm::MCSection *, const llvm::MCExpr *): Assertion Section && "Cannot switch to a null section!"' failed.`

If you plan on checking this test in early (which I would recommend) before all the other pieces are in place you can replace RUN for BBS with a TODO D99487.

llvm/test/MC/COFF/seh-bbs.ll
51

";" for comment lines

TaoPan updated this revision to Diff 355087.Jun 28 2021, 6:53 PM

Fix comment format of test

Cool, feel free to file a bug and link it here to track progress.

This bug depends on #D99487 and this commit, can't be reproduced without these two commits. Is it ok file the bug after landing these two commits?

In that case it would make sense to fix this in D99487. I pulled this down locally and noticed that baseline testing with BBS hits:

llc: /home/modimo/llvm-project/llvm/lib/MC/MCStreamer.cpp:1207: virtual void llvm::MCStreamer::SwitchSection(llvm::MCSection *, const llvm::MCExpr *): Assertion Section && "Cannot switch to a null section!"' failed.`

If you plan on checking this test in early (which I would recommend) before all the other pieces are in place you can replace RUN for BBS with a TODO D99487.

I set D99487 as this patch's parent according to @tmsriram 's comment, it's the reason why the auto test passed but you pull only this down locally and run this test crash. The section of SwitchSection() is created by D99487.
I submitted BBS patches according to purpose:

D99487 (parent of the below two patches): BBS basic function on Windows
D100735 (this patch):  BBS exception handling on Windows
D101421: BBS DebugInfo on Windows

So the upper mentioned bug "with BBS, build fail with "error: Cannot represent this expression", I'm investigating this error" belongs to this patch.

llvm/test/MC/COFF/seh-bbs.ll
51

Thanks! I changed to ";"

TaoPan updated this revision to Diff 356080.Jul 1 2021, 6:58 PM

Update test for reproducing build error of "<unknown>:0: error: Cannot represent this expression" and fix the error

modimo added a comment.Jul 6 2021, 7:35 PM
D99487 (parent of the below two patches): BBS basic function on Windows
D100735 (this patch):  BBS exception handling on Windows
D101421: BBS DebugInfo on Windows

Is the plan to commit all of this at once?

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
3202 ↗(On Diff #356080)

How does this change interact with the ELF beginFunclet/endFunclet?

TaoPan added a comment.Jul 6 2021, 8:42 PM
D99487 (parent of the below two patches): BBS basic function on Windows
D100735 (this patch):  BBS exception handling on Windows
D101421: BBS DebugInfo on Windows

Is the plan to commit all of this at once?

Yes.
Could you please review this commit further?

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
3202 ↗(On Diff #356080)

No impact.
The ELF child classes of class EHStream don't override beginFunclet/endFunclet, refer to llvm/lib/CodeGen/AsmPrinter/DwarfException.h, class DwarfCFIExceptionBase, DwarfCFIException, ARMException.
Only class WinException override beginFunclet/endFunclet of class EHStream, refer to llvm/lib/CodeGen/AsmPrinter/WinException.h, class WinException.

TaoPan updated this revision to Diff 356859.Jul 6 2021, 8:46 PM

git rebase

modimo accepted this revision.Jul 9 2021, 9:59 AM

LGTM

This revision is now accepted and ready to land.Jul 9 2021, 9:59 AM

Thanks @modimo for big help!
Any other review comment?

modimo added a comment.Aug 2 2021, 1:56 PM

Thanks @modimo for big help!
Any other review comment?

Not on this change no.

snehasish resigned from this revision.Jan 30 2023, 8:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2023, 8:58 AM