[CodeGen] Port basic block sections from ELF to COFF

[TaoPan]

Summary

Ignore size check as COFF assembly doesn't supprt .size directive.
The override function getSectionForMachineBasicBlock will construct
cold section and general basic block section. This patch constructs
general basic block section only, the cold section will be constructed
in later machine function splitter on Windows patch.

Signed-off-by: Pan, Tao <[e-mail]>

[TaoPan]

git rebase

[tmsriram]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1711

Why is this set to always comdat? This should be comdat only if the function is comdat, right?

1712

COMDATSymName can be folded to be equal to MBB.getSymbol()->getName() here? Plus, you are not preserving the .text.hot prefix that the original function might get here. Is this future work? If the original function is named .text.hot.foo, the basic block will still be named .text.foo.__part.1 which is not right.

Plus, what about exception handling sections like ".eh.*"?

[TaoPan]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1711

Thanks for pointing out comdat or not depending on the specific situation! I'll add comdat test to basic-block-sections.ll for making sure both comdat and not comdat work correctly. Then I'll answer your second question.

1712

Thanks! I'll redesign section name and comdat symbol name.
The text section with prefix "hot" and "unlikely" won't be constructed here, I added COFF text section prefix "hot" and "unlikely" in D92073. In ELF override function, also not handling text section with prefix "hot" and "unlikely".
The text section with prefix "split" will be constructed here, I plan to add related code in MFS COFF patch.
Also, exception handling section is future work that support basic block sections Windows COFF exception handling.

[MaskRay]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1712

This is complex. PE-COFF has multiple COMDAT seletion kinds. I want to see a holistic plan how every component is going to be implemented.

[TaoPan]

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

[tmsriram]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1712

The basic block should just mimic the COMDAT type of its containing function, is there a reason to do anything more with it here?

[TaoPan]

Simplify condition of setting COFF::IMAGE_SCN_LNK_COMDAT

[TaoPan]

git rebase

[TaoPan]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1711

I referred to the existing COFF code (TargetLoweringObjectFileCOFF::SelectSectionForGlobal()), COFF::IMAGE_SCN_LNK_COMDAT of text section is set when setting -function-sections and the function is comdat.

[TaoPan]

Hi tmsriram, MaskRay,

I updated implementation and test. Could you please have a look?
As for multiple COMDAT seletion kinds of PE-COFF, is IMAGE_COMDAT_SELECT_NODUPLICATES wrong? Or need to set different COMDAT selection kind in some case?
As for a holistic plan how every component is going to be implemented, I'm developing

1. Basic block sections Windows exception handling, I'm not sure whether need to handle exception handling sections like ELF sections ".eh.*" here, anyway, I'll make sure Windows exception handling work on basic block sections.
2. MFS on Windows, I submitted D95209, it includes text section with prefix ".text$split" handling, it will be updated after landing this patch.

in turn.

[tmsriram]

In D99487#2670028, @TaoPan wrote:

Hi tmsriram, MaskRay,

I updated implementation and test. Could you please have a look?
As for multiple COMDAT seletion kinds of PE-COFF, is IMAGE_COMDAT_SELECT_NODUPLICATES wrong? Or need to set different COMDAT selection kind in some case?

I am not familiar with PE-COFF and I don't know the answer. FOR ELF, the basic block COMDAT type should be the same as its parent function and should also be in the same group.

As for a holistic plan how every component is going to be implemented, I'm developing

1. Basic block sections Windows exception handling, I'm not sure whether need to handle exception handling sections like ELF sections ".eh.*" here, anyway, I'll make sure Windows exception handling work on basic block sections.

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.

2. MFS on Windows, I submitted D95209, it includes text section with prefix ".text$split" handling, it will be updated after landing this patch.

in turn.

Great, please use the parent-child relationships to mark these patches.

• Could you also please expand the summary of this patch? I would write what extra was needed to port this from ELF to PE-COFF.
• Could you also talk about DebugInfo handling and CFI with PE-COFF and basic block sections? Are these format agnostic and have you verified that they do the right thing? These were the biggest challenges for us when doing this for ELF.

Appreciate your efforts towards porting this to PE-COFF and making it more generally available. Thanks!

[TaoPan]

Add TODO for constructing exception section and cold section later

[TaoPan]

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.

Great, please use the parent-child relationships to mark these patches.

I marked parent-child relationships.

• Could you also please expand the summary of this patch? I would write what extra was needed to port this from ELF to PE-COFF.

I expanded the summary, could you please have a review?

• Could you also talk about DebugInfo handling and CFI with PE-COFF and basic block sections? Are these format agnostic and have you verified that they do the right thing? These were the biggest challenges for us when doing this for ELF.

No, I didn't verify DebugInfo handling and CFI with PE-COFF and basic block sections.

[tmsriram]

In D99487#2688224, @TaoPan wrote:

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.

Great, please use the parent-child relationships to mark these patches.

I marked parent-child relationships.

• Could you also please expand the summary of this patch? I would write what extra was needed to port this from ELF to PE-COFF.

I expanded the summary, could you please have a review?

• Could you also talk about DebugInfo handling and CFI with PE-COFF and basic block sections? Are these format agnostic and have you verified that they do the right thing? These were the biggest challenges for us when doing this for ELF.

No, I didn't verify DebugInfo handling and CFI with PE-COFF and basic block sections.

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.

[TaoPan]

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:

As for CFI, does it need be verified addition to llvm/test/DebugInfo/ tests? If so, could you please guide me how CFI of basic block sections is verified on Linux?

[TaoPan]

Simplify text section name code

[TaoPan]

git rebase

[tmsriram]

In D99487#2690528, @TaoPan wrote:

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:

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

As for CFI, does it need be verified addition to llvm/test/DebugInfo/ tests? If so, could you please guide me how CFI of basic block sections is verified on Linux?

Basically, this means your code won't assemble with CFI and that is a serious limitation. This is similar to what we did for ELF. All CFI directives should be within a cfi_startproc cfi_endproc directive as part of a single section. When you fragment a function into multiple sections, this breaks. We solved this by adding a separate startproc endproc directive for each section.

I have some suggestions for you to better understand the problem:

• GCC supports function splitting with -freorder-blocks-and-partition. You can use GCC to split a function and see what it does to cv_loc directives.
• D79978 is what we did with ELF, my guess is it is very similar. You just need to add a startproc endproc to every function and duplicate all the information to compute Call Frame Address.

[tmsriram]

In D99487#2697789, @tmsriram wrote:

In D99487#2690528, @TaoPan wrote:

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:

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

As for CFI, does it need be verified addition to llvm/test/DebugInfo/ tests? If so, could you please guide me how CFI of basic block sections is verified on Linux?

Basically, this means your code won't assemble with CFI and that is a serious limitation. This is similar to what we did for ELF. All CFI directives should be within a cfi_startproc cfi_endproc directive as part of a single section. When you fragment a function into multiple sections, this breaks. We solved this by adding a separate startproc endproc directive for each section.

I have some suggestions for you to better understand the problem:

• GCC supports function splitting with -freorder-blocks-and-partition. You can use GCC to split a function and see what it does to cv_loc directives.
• D79978 is what we did with ELF, my guess is it is very similar. You just need to add a startproc endproc to every function and duplicate all the information to compute Call Frame Address.

https://llvm.org/docs/Extensions.html#codeview-dependent

These aren't the CFI directives and I falsely assumed that. Stems from my misunderstanding of COFF, please take a note of this.

[TaoPan]

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.

Basically, this means your code won't assemble with CFI and that is a serious limitation. This is similar to what we did for ELF. All CFI directives should be within a cfi_startproc cfi_endproc directive as part of a single section. When you fragment a function into multiple sections, this breaks. We solved this by adding a separate startproc endproc directive for each section.

I have some suggestions for you to better understand the problem:

• GCC supports function splitting with -freorder-blocks-and-partition. You can use GCC to split a function and see what it does to cv_loc directives.
• D79978 is what we did with ELF, my guess is it is very similar. You just need to add a startproc endproc to every function and duplicate all the information to compute Call Frame Address.

https://llvm.org/docs/Extensions.html#codeview-dependent

These aren't the CFI directives and I falsely assumed that. Stems from my misunderstanding of COFF, please take a note of this.

Thanks for your detail guidance! It's very helpful. I'll study what you did with ELF and codeview format.

[TaoPan]

Hi tmsriram, MaskRay,

I submitted D100735 and D101421 for enabling exception handling and CodeView DebugInfo for basic block sections on Windows, they are all depend on the override functions getSectionForMachineBasicBlock and getUniqueSectionForFunction in this patch, I added these dependent functions in those patches for passing arc unit test. I'll rebase those patches after the dependent functions are merged into llvm. Could you please review these 3 patches?

[modimo]

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.

[TaoPan]

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.

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.

[modimo]

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.

[modimo]

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.

[tmsriram]

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?

[rnk]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1712

After thinking about it a bit, I think the entry block should use the regular selection kind, and all of the auxilliary MBB sections should use IMAGE_COMDAT_SELECT_ASSOCIATIVE. They should be associated with the main function symbol, unless the function itself is associated with something else, in which case the BBs use that symbol.

This will ensure that if the main function section prevails, they are included, and if it does not prevail, they are discarded. Does that make sense?

[modimo]

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?

[TaoPan]

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?

Sorry for confusion! Let me summarize the history.

1. I submitted D95209 for porting MFS to Windows COFF. @tmsriram and @snehasish mentioned CFI, DebugInfo and exception handling should be guaranteed, thanks for their help and guidance!
2. I submitted D96393 for adding MFS DebugInfo test, as MFS COFF implementation code is in 1, it includes only Linux ELF test. I planed to extend MFS DebugInfo COFF test in 1 at that time. @snehasish guided me that add (MFS dependent) BBS not MFS DebugInfo test on Windows COFF, just like Linux ELF.
3. I submitted this patch D99487 for supporting basic function of BBS on Windows COFF.
4. I submitted D100735 for making sure Windows exception handling for BBS. As Linux ELF exception handling test llvm/test/CodeGen/X86/basic-block-sections-eh.ll is mainly testing throw & catch exception, so I referred https://docs.microsoft.com/en-us/cpp/cpp/structured-exception-handling-c-cpp?view=msvc-160 to generate ll test file.
5. I submitted D101421 for making sure Windows CodeView DebugInfo for BBS. I used same C code of llvm/test/CodeGen/X86/cfi-basic-block-sections-1.ll introduced by D79978 that is CFI DebugInfo for BBS on Linux ELF to generate ll test file.

So the dependence chain is: 1 depends on 3 depends on 4 and 5. 2 will be abandoned.
Could you please review the upper progress? Any question is welcome.

[TaoPan]

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.

[TaoPan]

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

[TaoPan]

git rebase

[TaoPan]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1712

Thanks! I think set entry block sections as regular IMAGE_COMDAT_SELECT_NODUPLICATES and set the auxilliary MBB sections as IMAGE_COMDAT_SELECT_ASSOCIATIVE is fine. I changed.

[mstorsjo]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1712

@rnk - I'm not quite familiar with the concrete implications of this work here, but would this need to be mapped to the GNU style comdats, for compatibility with ld.bfd for mingw targets? (LLD itself works fine with proper associative comdats though.)

[rnk]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1712

I think basic block sections are kind of in the same category as LTO: it's a very sophisticated optimization feature, and it's probably OK if it's LLD-only. I think it also requires special linker support that might make it LLD-only, but I've forgotten the details.

It also has potentially very high object file size overhead, and it will be important to do everything possible to minimize that object file size overhead. Long gnu-style section names for every basic block section are likely to make the feature unusably slow, so maybe it's worth removing these "if mingw" conditionals. We can leave behind a comment by the use of IMAGE_COMDAT_SELECT_ASSOCIATIVE indicating that gnu-style section names are not needed because the feature is only designed to work with LLD.

[mstorsjo]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1712

Thanks for the clarification! Leaving the feature as LLD-only (or in other terms, requiring a conforming COMDAT implementation) sounds good to me.

[TaoPan]

Remove if mingw conditionals of MBB sections.

[TaoPan]

git rebase for triggering auto test

[TaoPan]

git rebase for triggering auto test

[TaoPan]

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

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1712

Thanks for discussion between @mstorsjo and @rnk !
I removed "if mingw" conditionals.

[TaoPan]

git rebase

[TaoPan]

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
   
   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
3. 2 + BBS related patches
   
   Test results are same with 2.
4. 3 + option -Xclang -fbasic-block-sections=all -Xclang -funique-basic-block-section-names
   
   a. All tests build crash, report lld link error
   
   lld/COFF/InputFiles.cpp, ObjFile::readAssociativeDefinition(), error(toString(this) + ": associative comdat " + name + " (sec " + Twine(sectionNumber) + ") has invalid reference to section " + parentName + " (sec " + Twine(parentIndex) + ")");

I'm investigating 4.a.

[modimo]

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!

[TaoPan]

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

[TaoPan]

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.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1712

@rnk about the issue 4.a of https://reviews.llvm.org/D99487#2821343, I tried to use clang-cl.exe with bbs option and lld to build helloworld c program, meet IMAGE_COMDAT_SELECT_XXX related problem.
helloworld c program is general c demo program as below
$ cat helloworld.c
#include <stdio.h>

int main() {

printf("hello world!\n");
return 0;

}
$ clang-cl.exe helloworld.c -Xclang -fbasic-block-sections=all -Xclang -funique-basic-block-section-names -fuse-ld=lld
lld-link: error: C:\Users\taopan\AppData\Local\Temp\helloworld-0f57a4.obj: associative comdat .text (sec 25) has invalid reference to section .text (sec 25)
lld-link: error: C:\Users\taopan\AppData\Local\Temp\helloworld-0f57a4.obj: associative comdat .text (sec 26) has invalid reference to section .text (sec 26)
lld-link: error: C:\Users\taopan\AppData\Local\Temp\helloworld-0f57a4.obj: associative comdat .text (sec 27) has invalid reference to section .text (sec 27)
clang-cl: error: linker command failed with exit code 1 (use -v to see invocation)

These errors can be fixed by change selection of getSectionForMachineBasicBlock() from IMAGE_COMDAT_SELECT_ASSOCIATIVE to IMAGE_COMDAT_SELECT_ANY.

Then new error occurs.
$ clang-cl.exe helloworld.c -Xclang -fbasic-block-sections=all -Xclang -funique-basic-block-section-names
libcmt.lib(default_local_stdio_options.obj) : error LNK2005: local_stdio_printf_options already defined in helloworld-1f7490.obj
libvcruntime.lib(undname.obj) : error LNK2005: local_stdio_printf_options already defined in helloworld-1f7490.obj
helloworld.exe : fatal error LNK1169: one or more multiply defined symbols found
clang-cl: error: linker command failed with exit code 1169 (use -v to see invocation)

This error can be fixed by change selection of getUniqueSectionForFunction() from IMAGE_COMDAT_SELECT_NODUPLICATES to IMAGE_COMDAT_SELECT_ANY.
Is it acceptable change selection of getSectionForMachineBasicBlock() and getUniqueSectionForFunctio() to IMAGE_COMDAT_SELECT_ANY?

[TaoPan]

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
   
   a. x4ptcu.c: build error https://bugs.llvm.org/show_bug.cgi?id=50859
   
   b. seh0015.c, seh0017.c: build crash https://bugs.llvm.org/show_bug.cgi?id=50854
   
   c. seh0034.c, seh0036.c, seh0041~0043.c, seh0048~0050.c, another build crash https://bugs.llvm.org/show_bug.cgi?id=50855
   
   d. seh0020.c, seh0025, seh0026: build error https://bugs.llvm.org/show_bug.cgi?id=50856
   
   e. sehframes.cpp: build pass, run dead loop https://bugs.llvm.org/show_bug.cgi?id=50858

[TaoPan]

Update dependent D99487

[TaoPan]

git rebase

[TaoPan]

Change select of BB sections to IMAGE_COMDAT_SELECT_NODUPLICATES

[TaoPan]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1712

@rnk The last sentence of IMAGE_COMDAT_SELECT_ASSOCIATIVE's Description: The section association chain must eventually come to a COMDAT section that doesn't have IMAGE_COMDAT_SELECT_ASSOCIATIVE set. The upper "lld-link: error: C:\Users\taopan\AppData\Local\Temp\helloworld-0f57a4.obj: associative comdat .text (sec 25) has invalid reference to section .text (sec 25)" is caused by the association chain (only has BB text section) doesn't have a COMDAT section that doesn't have IMAGE_COMDAT_SELECT_ASSOCIATIVE set. I think the association chain should be per section chain, not per function chain, like:

entry block text section (default one, not IMAGE_COMDAT_SELECT_ASSOCIATIVE) -> data/debug/.. section if has (IMAGE_COMDAT_SELECT_ASSOCIATIVE)
BB section 1 (default one, not IMAGE_COMDAT_SELECT_ASSOCIATIVE) -> data/debug/... section if has (IMAGE_COMDAT_SELECT_ASSOCIATIVE)
BB section 2 (default one, not IMAGE_COMDAT_SELECT_ASSOCIATIVE) -> data/debug/... section if has (IMAGE_COMDAT_SELECT_ASSOCIATIVE)
...

So I changed BB text section's select to IMAGE_COMDAT_SELECT_NODUPLICATES.

As for the upper "clang-cl: error: linker command failed with exit code 1169 (use -v to see invocation)", I tried again with lld linker.
$ clang-cl.exe helloworld.c -Xclang -fbasic-block-sections=all -Xclang -funique-basic-block-section-names -fuse-ld=lld
lld-link: error: duplicate symbol: __local_stdio_printf_options

defined at C:\Users\taopan\AppData\Local\Temp\helloworld-b851d7.obj
defined at libcmt.lib(default_local_stdio_options.obj)

lld-link: error: duplicate symbol: __local_stdio_printf_options

defined at C:\Users\taopan\AppData\Local\Temp\helloworld-b851d7.obj
defined at libvcruntime.lib(undname.obj)

clang-cl: error: linker command failed with exit code 1 (use -v to see invocation)

Duplication is between helloworld obj and system libcmt.lib(default_local_stdio_options.obj), so I changed select of entry block text section to IMAGE_COMDAT_SELECT_ANY.
How do you think of these two modifications?

[TaoPan]

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

[rnk]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1712

IIUC this error looks like the entry block was mistakenly associated with itself:
"lld-link: error: C:\Users\taopan\AppData\Local\Temp\helloworld-0f57a4.obj: associative comdat .text (sec 25) has invalid reference to section .text (sec 25)

So, section 25 is associated with section 25, a self-reference, creating a loop, not a chain. As I said earlier, the entry block must use the "regular" selection type. Whatever LLVM would use today. The cold blocks should be associated with the entry block.

Alternatively, if the function itself is associative, it's equivalent to associate the cold blocks directly with the global that the function is associated with.

I don't like the idea of BBS arbitrarily making the entry block use IMAGE_COMDAT_SELECT_ANY. It should be using whatever selection LLVM would have chosen for it without BBS enabled.

[TaoPan]

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

[TaoPan]

@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?

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1712

The section association chain consists of multiple sections with same comdat name. In the case of BBS and -unique-basic-block-section-names, comdat names of entry block and BB sections are different (BB sections have suffix ".__part.x"), so BB sections can't be associated with the entry block.
Previous issue "section 25 is associated with section 25" is caused by no COMDAT section (with same name of BB section) that doesn't have IMAGE_COMDAT_SELECT_ASSOCIATIVE set.
I changed selection of entry block to same as without BBS enabled.

[MaskRay]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1712

I think UniqueID = NextUniqueID++; is not needed.

1716

Can you fix the TODO in this patch?

1746

Is isOSBinFormatCOFF() more appropriate? Can windows-gnu use the optimization?

[TaoPan]

Fix Windows gnu section name and test case

[TaoPan]

Thanks MaskRay for your review comments!

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1712

I added comment (line 1752) to explain why add this line, this line referred to ELF implementation, please refer to https://github.com/llvm/llvm-project/blob/cb2a2ba8d64da5be3fac0ea90e406270e8a615bd/llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp#L993

1716

It's indeed TODO. MMBSectionID::ColdSectionID will be handled in MFS on Windows COFF patch. I plan to re-base my previous MFS on Windows COFF patch (D95209) after this patch. I also explained this TODO in Summary.

1746

Windows-gnu can use the optimization. In the case of Windows-msvc, isOSBinFormatCOFF() is true, but the section name should not include COMDATSymName suffix.
This part referred to existing code https://github.com/llvm/llvm-project/blob/cb2a2ba8d64da5be3fac0ea90e406270e8a615bd/llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp#L1663
Thanks for this Windows-gnu comment! I reviewed Window-gnu related implementation and fixed Windows-gnu problem of TargetLoweringObjectFileCOFF::getSectionForMachineBasicBlock().

[TaoPan]

Ping.

[TaoPan]

Ping.

llvm/include/llvm/CodeGen/TargetLoweringObjectFileImpl.h

MCSection *SelectSectionForGlobal(const GlobalObject *GO, SectionKind Kind,
                                     const TargetMachine &TM) const override;
 
   void getNameWithPrefix(SmallVectorImpl<char> &OutName, const GlobalValue *GV,
                          const TargetMachine &TM) const override;
 
   MCSection *getSectionForJumpTable(const Function &F,
                                     const TargetMachine &TM) const override;
 
+  MCSection *
+  getSectionForMachineBasicBlock(const Function &F,
+                                 const MachineBasicBlock &MBB,
+                                 const TargetMachine &TM) const override;
+
+  MCSection *
+  getUniqueSectionForFunction(const Function &F,
+                              const TargetMachine &TM) const override;
+
   /// Emit Obj-C garbage collection and linker options.
   void emitModuleMetadata(MCStreamer &Streamer, Module &M) const override;
 
   MCSection *getStaticCtorSection(unsigned Priority,
                                   const MCSymbol *KeySym) const override;
   MCSection *getStaticDtorSection(unsigned Priority,
                                   const MCSymbol *KeySym) const override;
 

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

MCSection *TargetLoweringObjectFileCOFF::getSectionForJumpTable(
   Characteristics |= COFF::IMAGE_SCN_LNK_COMDAT;
   unsigned UniqueID = NextUniqueID++;
 
   return getContext().getCOFFSection(
       SecName, Characteristics, Kind, COMDATSymName,
       COFF::IMAGE_COMDAT_SELECT_ASSOCIATIVE, UniqueID);
 }
 
+/// Returns a unique section for the given machine basic block.
+MCSection *TargetLoweringObjectFileCOFF::getSectionForMachineBasicBlock(
+    const Function &F, const MachineBasicBlock &MBB,
+    const TargetMachine &TM) const {
+  assert(MBB.isBeginSection() && "Basic block does not start a section!");
+  unsigned UniqueID = MCContext::GenericSectionID;
+  SmallString<128> COMDATSymName;
+  if (TM.getUniqueBasicBlockSectionNames())
+    COMDATSymName += MBB.getSymbol()->getName();
+  else
+    UniqueID = NextUniqueID++;
+
+  return getContext().getCOFFSection(
+      getCOFFSectionNameForUniqueGlobal(SectionKind::getText()),
+      COFF::IMAGE_SCN_CNT_CODE | COFF::IMAGE_SCN_MEM_EXECUTE |
+          COFF::IMAGE_SCN_MEM_READ | COFF::IMAGE_SCN_LNK_COMDAT,
+      SectionKind::getText(), COMDATSymName,
+      COFF::IMAGE_COMDAT_SELECT_NODUPLICATES, UniqueID);
+}
+
+MCSection *TargetLoweringObjectFileCOFF::getUniqueSectionForFunction(
+    const Function &F, const TargetMachine &TM) const {
+  SectionKind Kind = SectionKind::getText();
+  return getContext().getCOFFSection(
+      getCOFFSectionNameForUniqueGlobal(Kind),
+      getCOFFSectionFlags(Kind, TM) | COFF::IMAGE_SCN_LNK_COMDAT, Kind,
+      TM.getSymbol(&F)->getName(), COFF::IMAGE_COMDAT_SELECT_NODUPLICATES,
+      NextUniqueID++);
+}
+
 void TargetLoweringObjectFileCOFF::emitModuleMetadata(MCStreamer &Streamer,
                                                       Module &M) const {
   emitLinkerDirectives(Streamer, M);
 
   unsigned Version = 0;
   unsigned Flags = 0;
   StringRef Section;
 
   GetObjCImageInfo(M, Version, Flags, Section);
   if (!Section.empty()) {
     auto &C = getContext();
     auto *S = C.getCOFFSection(Section,
                                COFF::IMAGE_SCN_CNT_INITIALIZED_DATA |
                                    COFF::IMAGE_SCN_MEM_READ,
                                SectionKind::getReadOnly());
     Streamer.SwitchSection(S);
     Streamer.emitLabel(C.getOrCreateSymbol(StringRef("OBJC_IMAGE_INFO")));
     Streamer.emitInt32(Version);
     Streamer.emitInt32(Flags);
     Streamer.AddBlankLine();
   }
 
   emitCGProfileMetadata(Streamer, M);
 }
 
 void TargetLoweringObjectFileCOFF::emitLinkerDirectives(
     MCStreamer &Streamer, Module &M) const {
   if (NamedMDNode *LinkerOptions = M.getNamedMetadata("llvm.linker.options")) {
     // Emit the linker options to the linker .drectve section.  According to the

llvm/test/CodeGen/X86/basic-block-sections.ll

 ; RUN: llc < %s -mtriple=x86_64-pc-linux -function-sections -basic-block-sections=all -unique-basic-block-section-names | FileCheck %s -check-prefix=LINUX-SECTIONS
 ; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=all -unique-basic-block-section-names | FileCheck %s -check-prefix=LINUX-SECTIONS
 ; RUN: llc < %s -mtriple=x86_64-pc-linux -function-sections -basic-block-sections=all -unique-basic-block-section-names -split-machine-functions | FileCheck %s -check-prefix=LINUX-SECTIONS
 ; RUN: llc < %s -mtriple=i386-unknown-linux-gnu  -function-sections -basic-block-sections=all -unique-basic-block-section-names | FileCheck %s -check-prefix=LINUX-SECTIONS
 ; RUN: llc < %s -mtriple=i386-unknown-linux-gnu  -basic-block-sections=all -unique-basic-block-section-names | FileCheck %s -check-prefix=LINUX-SECTIONS
+; RUN: llc < %s -mtriple=x86_64-windows-msvc -function-sections -basic-block-sections=all -unique-basic-block-section-names | FileCheck %s -check-prefix=WINDOWS-SECTIONS
+; RUN: llc < %s -mtriple=x86_64-windows-msvc -basic-block-sections=all -unique-basic-block-section-names | FileCheck %s -check-prefix=WINDOWS-SECTIONS
 
 define void @_Z3bazb(i1 zeroext) nounwind {
   %2 = alloca i8, align 1
   %3 = zext i1 %0 to i8
   store i8 %3, i8* %2, align 1
   %4 = load i8, i8* %2, align 1
   %5 = trunc i8 %4 to i1
   br i1 %5, label %6, label %8

 ; LINUX-SECTIONS: .section        .text._Z3bazb.[[SECTION_LABEL_1:_Z3bazb.__part.[0-9]+]],"ax",@progbits
 ; LINUX-SECTIONS: [[SECTION_LABEL_1]]:
 ; LINUX-SECTIONS: .LBB_END0_1:
 ; LINUX-SECTIONS-NEXT: .size  [[SECTION_LABEL_1]], .LBB_END0_1-[[SECTION_LABEL_1]]
 ; LINUX-SECTIONS: .section        .text._Z3bazb.[[SECTION_LABEL_2:_Z3bazb.__part.[0-9]+]],"ax",@progbits
 ; LINUX-SECTIONS: [[SECTION_LABEL_2]]:
 ; LINUX-SECTIONS: .LBB_END0_2:
 ; LINUX-SECTIONS-NEXT: .size   [[SECTION_LABEL_2]], .LBB_END0_2-[[SECTION_LABEL_2]]
+; WINDOWS-SECTIONS: .section        .text,"xr",one_only,_Z3bazb
+; WINDOWS-SECTIONS: _Z3bazb:
+; WINDOWS-SECTIONS: .section        .text,"xr",one_only,[[SECTION_LABEL_1:_Z3bazb.__part.[0-9]+]]
+; WINDOWS-SECTIONS: [[SECTION_LABEL_1]]:
+; WINDOWS-SECTIONS: .section        .text,"xr",one_only,[[SECTION_LABEL_2:_Z3bazb.__part.[0-9]+]]
+; WINDOWS-SECTIONS: [[SECTION_LABEL_2]]: