TaoPan added a comment. In D99487#2832079 <https://reviews.llvm.org/D99487#2832079>, @modimo wrote:
> In D99487#2821343 <https://reviews.llvm.org/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. ================ Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1712 + COFF::IMAGE_SCN_MEM_READ | COFF::IMAGE_SCN_LNK_COMDAT, + SectionKind::getText(), COMDATSymName, + COFF::IMAGE_COMDAT_SELECT_NODUPLICATES, UniqueID); ---------------- TaoPan wrote: > mstorsjo wrote: > > rnk wrote: > > > mstorsjo wrote: > > > > TaoPan wrote: > > > > > rnk wrote: > > > > > > tmsriram wrote: > > > > > > > MaskRay wrote: > > > > > > > > TaoPan wrote: > > > > > > > > > tmsriram wrote: > > > > > > > > > > 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.*"? > > > > > > > > > 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. > > > > > > > > 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. > > > > > > > The basic block should just mimic the COMDAT type of its > > > > > > > containing function, is there a reason to do anything more with > > > > > > > it here? > > > > > > 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? > > > > > 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. > > > > @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.) > > > 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. > > Thanks for the clarification! Leaving the feature as LLD-only (or in other > > terms, requiring a conforming COMDAT implementation) sounds good to me. > Thanks for discussion between @mstorsjo and @rnk ! > I removed "if mingw" conditionals. @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? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99487/new/ https://reviews.llvm.org/D99487 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
