Ping.

Ping.

Thanks MaskRay for your review comments!

Fix Windows gnu section name and test case

@rnk Thanks for your review comments! Could you please help to review my reply and new modification?
@MaskRay Could you please also help to review?

Change selection of entry block text section from IMAGE_COMDAT_SELECT_ANY to return value of getSelectionForCOFF()

@jpienaar Could you please have a review?

@rnk could you please have a review of IMAGE_COMDAT_SELECT_XXX modification?

Change select of BB sections to IMAGE_COMDAT_SELECT_NODUPLICATES

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

git rebase

Update dependent D99487

git rebase

In D100735#2860997, @modimo wrote:

In D100735#2846280, @TaoPan wrote:

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?

In D101421#2854680, @rnk wrote:

lgtm

Thanks for your big help!

Fix the typo of comment

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

In D100735#2845073, @modimo wrote:

In D100735#2843362, @TaoPan wrote:

In D100735#2842162, @modimo wrote:

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.

Fix comment format of test

In D100735#2842162, @modimo wrote:

In D100735#2838154, @TaoPan wrote:

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.

Add comment for difference of section number of BASELINE and BBS

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.

I reported bugs of using clang-cl and lld to test https://github.com/microsoft/compiler-tests/tree/master/seh

2. clang-cl.exe + lld linker

In D100735#2834985, @modimo wrote:

In D100735#2832513, @TaoPan wrote:

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.

Add baseline case and check size of the Sections table

In D99487#2832079, @modimo wrote:

In D99487#2821343, @TaoPan wrote:

I checked the microsoft SEH tests with

1. cl.exe
   
   a. x4ptcu.c: build error
2. clang-cl.exe + lld linker
   
   a. x4ptcu.c: build error
   
   b. seh0015.c, seh0017.c: build crash
   
   c. seh0034.c, seh0036.c, seh0041~0043.c, seh0048~0050.c, another build crash
   
   d. seh0020.c, seh0025, seh0026: build error
   
   e. sehframes.cpp: build pass, run dead loop

Please file bugs for these and if you can bucket the failures that would be even better. Thanks!

I'll file these bugs and try to bucket the failures in parallel.

Make clang option -fbasic-block-sections and -funique-basic-block-section-names available on Windows COFF.

In D100735#2832069, @modimo wrote:

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.

In D99487#2724701, @modimo wrote:

Thanks for guiding MSVC's SEH tests! I found the SEH tests use cl (not clang-cl) to compile the c test file https://github.com/microsoft/compiler-tests/blob/2fc7859d212d5fb59628eb21aec50dfb4b934146/seh/runtests.cmd#L8, I'll try to change cl to clang-cl and make sure non-regressed.

Sure, those are the tests run by MSVC for SEH verification which is why it uses cl. Clang-cl sounds good for testing.

Sorry for late feedback of this comment!
I checked the microsoft SEH tests with

1. cl.exe

Thanks for your review comments!

Change condition from MachineBasicBlock is begin section to MachineFunction has BB sections and add comment

Don't emit location of BB sections

Disable .cv_loc directive emission outside the primary function section.

git rebase

git rebase

git rebase

Remove same modification of D100735

Remove same modification of parent D99487 and add .cv_inline_linetable to test

The x64 debian failed was introduced by D101797 and is being fixed by https://reviews.llvm.org/rG72bd0116e3a1a70fb52fc47c056349b290ce2204

Remove content of parent D99487 for applying this patch

Remove content of parent D99487 for applying this patch

git rebase for triggering auto test

git rebase for triggering auto test

Remove if mingw conditionals of MBB sections.

Move test back to clang/test/CodeGen/

git rebase

git rebase

Change selection of the auxilliary MBB sections to IMAGE_COMDAT_SELECT_ASSOCIATIVE, git rebase, fix comdat name issue

Thanks Pengfei for your review comments!

Move test from clang/test to llvm/test, remove comma of the last enum item.

Could you please have a review?

Thanks all!
I'll take leave between 5.1~5.6 for national holiday and personal affair, I'll response @rnk 's review comment and other further review comment after then.

Thanks for your review comment!
I'll take leave between 5.1~5.6 for national holiday and personal affair, I'll response further review comment after then.

Forgot .section directives in the first CHECK of the last update

Add .section directives test

In D99487#2727370, @modimo wrote:

In D99487#2724706, @tmsriram wrote:

In D99487#2724702, @modimo wrote:

Agree with @MaskRay I'd like to see a holistics plan/RFC on all the pieces that are needed to get this functionality. The functionality here is only tested when exceptions are actually propagated which is not common in LLVM test suite. You'll want to run and likely write very specific tests stressing the test matrix here of BBS + MFS + EH. Given all that, a larger plan that details how we can be confident that all the pieces have been accounted for and will be well tested is (IMO) imperative in getting this change reviewed and accepted.

Since this (the test matrix) is already done for ELF, would it be complete if the author added equivalent tests for COFF?

Good point, that sounds like a good avenue to pursue. @snehasish mentions some of these exact tests here https://reviews.llvm.org/D95209#2523227. Looks like there was some movement for these tests here: https://reviews.llvm.org/D96393 although it appears BBS might need Windows debuginfo support for testing. I'm chasing through a bunch of these diffs to figure out what the state of this is and I'm not quite certain what it is. @TaoPan do you have a plan you can share about the progress here so we can follow along?

In D99487#2723758, @modimo wrote:

For some added testing you can run MSVC's SEH tests to see that this works properly: https://github.com/microsoft/compiler-tests/tree/master/seh. I wouldn't be surprised if not all of these pass in the baseline but we certainly want to make sure the ones that are remain non-regressed.

Hi tmsriram, MaskRay,

Could you please have a review?

In D99487#2698978, @tmsriram wrote:

Would you be able to paste the entire assembly with cv loc directives? I can try to take a look.

As the entire assembly has 500 lines, I sent to you through mail, I'll do if you think paste part of them or entire assembly to here is better.

git rebase

In D100612#2693949, @kadircet wrote:

thanks, lgtm! which compiler is this btw?

Hi sammccall,

git rebase

Simplify text section name code

In D99487#2690162, @tmsriram wrote:

This is quite important to validate the correctness of the generated assembly. If you could please check that CFI and DebugInfo are sane before we take this patch forward? Let's get a temperature on how good these are before we decide how to take it forward.

Thanks! I got the importance.
As for DebugInfo, I tried to add "--basic-block-sections=all" to RUN of llvm/test/DebugInfo/COFF/comdat.ll, text sections includes basic block text sections and CodeViewDebugInfo can be created correctly after

1. apply this patch
2. commenting out a Linux exception handling protocol dependent function which I plan to do in later basic block section Windows exception handling patch.

There are some non-blocking errors "error: all .cv_loc directives for a function must be in the same section" in llc stage, one error for each basic block, below is the first one.
f.__part.1: # %if.then
#DEBUG_VALUE: f:c <- $ecx
<unknown>:0: error: all .cv_loc directives for a function must be in the same section
.Ltmp3:

In D99487#2687506, @tmsriram wrote:

If it is easier for you to break this up, you can present exception handling in a separate patch with a clear "TODO:" at the appropriate places. That should be alright and is also easier to review.

I added "TODO:" for constructing exception section and cold section later.

Add TODO for constructing exception section and cold section later

Hi tmsriram, MaskRay,

git rebase

Simplify condition of setting COFF::IMAGE_SCN_LNK_COMDAT

Conditionally set COFF::IMAGE_SCN_LNK_COMDAT, add mtriple x86_64-windows-gnu support

git rebase