Can we add a test with the =list option to verify that functions not requiring basic block sections are not emitted in unique sections?
Or maybe we can use the existing basic-block-sections-list.ll by adding a new invocation without -function-sections.

Please also replace bb in the description of the patch with part.

In D93082#2450638, @tmsriram wrote:

In D93082#2449369, @rahmanl wrote:

It may be worthwhile to think about removing the second dot from the naming, i.e., foo.__bb.N -> foo.__bbN . The reason is that this further distinguishes the symbol name from symbols like __cxx_global_var_init.N plus it saves one character.

The -funique-internal-linkage-names option uses ".__uniq." so I wanted to stick to that convention. I am not sure what you mean when you say further distinguishing.

It may be worthwhile to think about removing the second dot from the naming, i.e., foo.__bb.N -> foo.__bbN . The reason is that this further distinguishes the symbol name from symbols like __cxx_global_var_init.N plus it saves one character.

rebase.

Thank you for the comments @MaskRay . Are there any style guidelines for the test files? I wasn't able to find anything online.

• Add {{$}} to the checks.

• Add {{$}} to the checks.

Thank you @grimar and @jhenderson for the thorough feedback. I will start on llvm-readobj support once I submit this patch.

• Typos/Style fix.

• Refine tests.

In D83655#2369636, @MaskRay wrote:

@rahmanl: @dblaikie and @jrtc27 prefer having a suffix to be consistent with .text.foo and .rodata.foo . On a second thought I think this indeed makes more sense. People who want to save .strtab space can use -fno-unique-section-names. Are you fine with the resolution?

In D83655#2366222, @MaskRay wrote:

In D83655#2366206, @rahmanl wrote:

In D83655#2366091, @jrtc27 wrote:

I wonder if it'd be nicer to push this logic into getLSDASection by passing an (optional?) Function given that's where the normal LSDASection gets created? Having the check for ELF here seems to indicate that the logic isn't quite in the right place and belongs in the MCOFI subclass.

I agree with @jrtc27 here. How about we implement this like MCObjectFileInfo::getStackSizeSection(const MCSection&) or MCObjectFileInfo::getBBAddrMapSection(const MCSection&). The intended logic seems quite similar.

It would be inconvenient.

1. getStackSizeSection and getBBAddrMapSection do not use different section names. They will need to know the suffix
2. MCObjectFileInfo does not know whether function sections is enabled

MCObjectFileInfo is a good place for getStackSizeSection and getBBAddrMapSection because they need just information from MCObjectFileInfo. However, it is different for .gcc_except_table

In D83655#2366091, @jrtc27 wrote:

I wonder if it'd be nicer to push this logic into getLSDASection by passing an (optional?) Function given that's where the normal LSDASection gets created? Having the check for ELF here seems to indicate that the logic isn't quite in the right place and belongs in the MCOFI subclass.

+1,
Maybe worth mentioning in the description that the issue with -fbasic-block-sections is the revocations associated with @LPStart (Relocations are needed both in PIC and non-PIC mode). The no-basic-block-sections case is not affected since LPStart is omitted.

• Remove function comment for AsmPrinter::shouldEmitLabelForBasicBlock.

• Remove excessive empty line.

Thanks for your helpful comments @grimar.

• Remove NumBlocks from the YAML key.
• Add yaml2obj test.

Thanks again for the comments @tmsriram

• Better comments.

Thank you for your helpful comments @tmsriram and @snehasish

• Add a private member function shouldEmitLabelForBasicBlock and call it to decide if basic blocks require symbols.

Thanks for the comments @tmsriram.

• Remove redundant CHECK check-prefix.

• Fix the condition for emitting basic block label.

Thanks everyone for the review. I'll go ahead and commit this and start working on D88717.

• Remove extra '#'s from Extensions.rst.

• Format Extensions.rst.

Thanks @grimar.
I added the entry in the Extensions doc. I am not sure if my formatting is correct. I appreciate if you could please take a look.

• Add entry in docs/Extensions.rst.
• Address @grimar's comments.

• Rename test, and fix indentation.

ping.

In D88199#2312785, @MaskRay wrote:

In D88199#2302543, @jhenderson wrote:

I noticed that there are other LLVM sections which use __llvm_* as a prefix for their section name, instead of .llvm_* or .llvm.* (see e.g. __llvm_prf_cnts). What do you think about using that style for the name? It's not necessary, and it looks like we have a bit of inconsistency going on here, but it's something worth considering.

I am not sure it will be a good idea. The start/stop symbols of __llvm_prf_cnts is __start___llvm_prf_cnts and __stop___llvm_prf_cnts. The 3 _ look strange.

Right. We won't be using the __start and __stop symbols.

If the optimization eventually will be ported to Mach-O: strlen("__llvm_prf_cnts") <= 16 so the section name can be encoded in Mach-O. __llvm_bb_addr_map is not.

Good point. Actually this shows the importance of using the section type as opposed to the section name. If necessary, we can always change the section name since we make sure we never rely on the actual name.

Revert back to using ".llvm_bb_addr_map" for the section name.

@MaskRay Do you think this is fine now?

In D73739#2307463, @modimo wrote:

Glad to see this landing! I'm testing locally with my change to enable this purely for landing blocks.

Me too. Thanks.

As is, MachineFunctionSplitter can utilize this without basic-block-sections which means the "nop" handling doesn't get invoked and the runtime fails to find the landing pads. This would certainly be a separate change and I'd be happy to drive that.

That is expected. Thanks for signing up.

Diff reupload.

• Rename section to __llvm_bb_addr_map.

In D88199#2302543, @jhenderson wrote:

I noticed that there are other LLVM sections which use __llvm_* as a prefix for their section name, instead of .llvm_* or .llvm.* (see e.g. __llvm_prf_cnts). What do you think about using that style for the name? It's not necessary, and it looks like we have a bit of inconsistency going on here, but it's something worth considering.

• Use 'action_table_base' instead of 'cst_end' with more than one call-site range.
• Rebase.

In D73739#2299885, @MaskRay wrote:

Let's take gcc_except_table_bb_sections.ll as the example:

Call site sizes are incorrect

.uleb128 .Lcst_end0-.Lcst_begin0  # Call Site Size

.uleb128 .Lcst_end0-.Lcst_begin1  # Call Site Size

.uleb128 .Lcst_end0-.Lcst_begin2  # Call Site Size

I think the Call Site Sizes are incorrect. A pair of .Lcst_end0 and .Lcst_begin0 should exactly delimiter the call sites table, but the current implements uses .Lcst_end0 for all 3 basic block sections and places .Lcst_end0 after the last Call Site Table.

In D73739#2299339, @MaskRay wrote:

There is a compilation error:

llvm/lib/Target/ARM/ARMAsmPrinter.cpp:906:13: error: use of undeclared identifier 'getCurExceptionSym'
    MCSym = getCurExceptionSym();

• Fix ARM AsmPrinter code.
• Use DenseMap for MBBSectionExceptionSyms.

In D88199#2298841, @MaskRay wrote:

In D88199#2297495, @jhenderson wrote:

In D88199#2295596, @MaskRay wrote:

In D88199#2292193, @jhenderson wrote:

Mostly looks okay. I think there is no testing for the assembly parser being able to parse the new section type?

I'm also keen to hear what others think (e.g. @MaskRay).

This patch lets the bb_addr_map (renamed to llvm_bb_addr_map) section use a special section type (SHT_LLVM_BB_ADDR_MAP) instead of SHT_PROGBITS. This would help linker and other tools to use the sh_type ELF field to identify this section rather than relying on string comparison on the section name.

This description implies that some special processing is needed in the linker. (As you can see, when llvm_call_graph_profile and llvm_dependent_libraries were proposed, they had clear patches on the linker side.)

Maybe pivot the statement to say "dumpers and other tools"? Dumpers will need to know the section type for simpler processing of the code, even if the linker has no special handling currently.

xray_instr_map and xray_fn_idx are also an LLVM specific section types but they do not get their own section types. They use a dedicated dumper (llvm-xray), instead of llvm-readobj.

If the eventual behavior is to let LLD interpret .bb_addr_map differently (like .deplibs and .llvm_sympart*) differently (not "Rules for Linking Unrecognized Sections" on http://www.sco.com/developers/gabi/latest/ch4.sheader.html), having a dedicated section type seems fine. Though, I'd really like to know whether auto-defined __start_bb_addr_map and __stop_bb_addr_map are needed. If yes, the section name should not start with a period. And I'd like to know how the section is differently interpreted. xray_instr_map and xray_fn_idx only use generic ELF features.

• Rebase.
• Add test comment about LSDA and personality encodings.
• Make avoidZeroOffsetLandingPad static.

Thanks @MaskRay and @jhenderson.
Changed the description and the test according to your comments.

• Rename test to llvm_section_types.

• Remove non-llvm section types from the test.
• Rebase.

Thanks @jhenderson, turned out I did need a one-line change for asmparser to work.

• Add new test for MCParser.
• Clang-format.

• clang-format.

Thanks for the comments @MaskRay

• Rebase.
• Unify access to exception symbols for basic-block-sections and no basic-block-sections.
• other nits.

LGTM.
Please just keep in mind that call-site-table is not rigorously tested in LLVM.

In D73739#2278247, @modimo wrote:

@rahmanl Have you had a chance to run your added test on ARM64 as another itanium C++ ABI target to make sure it works properly? Having testing there is fine as a follow-up but wanted to make sure we're in agreement here. Otherwise changes LGTM, please let @MaskRay have a chance to provide any additional feedback.