IMO, I think this naming is very clear as this pass exists to carve basic block sections.

In D85085#2194130, @dblaikie wrote:

In D85085#2192340, @tmsriram wrote:

In D85085#2191679, @dblaikie wrote:

Trying to reproduce this (by taking the .ll test case and compiling it with clang -c) I don't get any debug_loc section or DW_AT_locations. Is there something I'm missing about how to reproduce this?

(trying to see what the non-bb-sections location description looks like - if it depends on basic block ordering, it's probably incorrect anyway, and might be worth fixing so it doesn't depend on that non-guaranteed property & then there'll be less divergence between bb-sections and not)

I see why debug_loc is not produced in the default case. This is what happens:

+ For the default case (no bbsections), DebugLoc in https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp#L1744 , it has exactly one value that contains the whole function scope as the start label is lfunc_begin and the end label is the end of the function.
+ Such location lists are handled specially here when the scope is the whole function : https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp#L1841 and the debug_loc is not generated.

OK ^ and with bb sections do we see the same behavior? If not, why not?

This patch is simple enough and LGTM.

In D85085#2191679, @dblaikie wrote:

Trying to reproduce this (by taking the .ll test case and compiling it with clang -c) I don't get any debug_loc section or DW_AT_locations. Is there something I'm missing about how to reproduce this?

(trying to see what the non-bb-sections location description looks like - if it depends on basic block ordering, it's probably incorrect anyway, and might be worth fixing so it doesn't depend on that non-guaranteed property & then there'll be less divergence between bb-sections and not)

In D85085#2191679, @dblaikie wrote:

Trying to reproduce this (by taking the .ll test case and compiling it with clang -c) I don't get any debug_loc section or DW_AT_locations. Is there something I'm missing about how to reproduce this?

(trying to see what the non-bb-sections location description looks like - if it depends on basic block ordering, it's probably incorrect anyway, and might be worth fixing so it doesn't depend on that non-guaranteed property & then there'll be less divergence between bb-sections and not)

In D84462#2183765, @MaskRay wrote:

lld/test/ELF/lto/basic-block-sections.ll should be placed in another patch. Adding a new test is unrelated to renaming the whole bunch of test files.

In D84462#2183755, @tmsriram wrote:

In D84462#2183752, @tmsriram wrote:

Add extra test without long section names and other nits.

Looks like I messsed up the renaming somehow. Let me fix that.

In D84462#2183752, @tmsriram wrote:

Add extra test without long section names and other nits.

Add extra test without long section names and other nits.

Rename test files too.

Some inital comments. Expanding the summary to provide a bit more detail on why this symbol naming scheme is better might be useful. Thanks for doing this.

Address reviewer comments.

Ping, is this alright?

In D79978#2146366, @dblaikie wrote:

In D79978#2146131, @tmsriram wrote:

Let me try a slightly different approach here. It is not clear to us what more is needed to land the patch. In the interests of resolving conflict :

1. I will also explicitly test assembly too for callee saved registers with bb sections when they are being pushed and popped.

Just on this point - it's not clear to me the motivation for the difference in testing between the two tests (one testing assembly, the other testing LLVM's MIR) - was there some particular difference/detail that these different strategies enabled testing? Otherwise, yeah, I'd probably just go with only testing assembly in both cases, for consistency/simplicity? (any variation in tests always makes me wonder "what's the reason for this difference? Must be something important or they'd be the same and so I'm not understanding some important difference")

• Change the callee saved register test to use a simple clobber on all callee saved registers and test cfi_offset for all the callee-save registers including rbp.
• The tests now cover the 2 issues that need special handling with sections: CFA tracking with and without frame pointer and cfi directives for callee saved registers.
• Other reviewer mentioned changes.

Let me try a slightly different approach here. It is not clear to us what more is needed to land the patch. In the interests of resolving conflict :

In D79978#2146011, @MaskRay wrote:

In D79978#2146009, @amharc wrote:

In D79978#2145979, @MaskRay wrote:

I cannot find a test with .cfa_offset or describing a non-rbp register, so the implementation is under-tested. How about leveraging inline asm to clobber callee-saved registers?

I'm not exactly sure which CFI instruction this comment refers to (there is no .cfa_offset directive, see e.g. https://sourceware.org/binutils/docs/as/CFI-directives.html).

If to .cfi_def_cfa_offset - the question was answered before: we emit a full CFA definition (.cfi_def_cfa - setting both the offset and the register) which is equivalent to a .cfi_def_cfa_offset (only setting the offset) plus a .cfi_def_cfa_register (only setting the register). Moreover, for code following the SysV ABI the register used in the .cfi_def_cfa_register/.cfi_def_cfa instructions is conventionally always set to either rbp (if the frame pointer is kept explicitly) or in rsp (when it's omitted). Both cases are covered by the existing tests. We are not aware of any easy way to force a different register to be the frame pointer.

If the review comment above refers to .cfi_offset instead, please note that the cfi-basic-block-sections-1.ll test checks that .cfi_offset is emitted properly in the generated assembly and cfi-inserter-basic-block-sections-callee-save-registers.ll checks that the CFI Inserter Pass actually inserts the corresponding CFI_INSTRUCTION offset instructions for clobbered callee-saved registers. I'm not exactly sure what would be gained by using inline assembly instead.

I ran llc for both tests. Neither has a section with CFI describing a non-rbp register at the top, thus I say I am not sure the implementation is properly tested.

In D79978#2145979, @MaskRay wrote:

In D79978#2145413, @tmsriram wrote:

...
Now, what happens when we start placing individual basic blocks in unique sections:

• Basic block sections allow the linker to randomly reorder basic blocks in the address space such that a given basic block can become non-contiguous with the original function.
• The different basic block sections can no longer share the cfi_startproc and cfi_endproc directives. So, each basic block section should emit this independently.
• Each (cfi_startproc, cfi_endproc) directive will result in a new FDE that caters to that basic block section.
• Now, this basic block section needs to duplicate the information from the entry block to compute the CFA as it is an independent entity. It cannot refer to the FDE of the original function and hence must duplicate all the stuff that is needed to compute the CFA on its own.

...

Thanks for the detailed write-up! I I've learned a bit from it. I think at least the quoted items should be placed in the description.

In D79978#2145189, @amharc wrote:

Note also that only rbp is described. I think we need another register to demonstrate the effect.

rbp is the usual frame pointer register for the x86 architecture and I'm not really sure we can easily force the compiler to choose a different register to hold the frame pointer. If you know how to force a different register to be the frame pointer, please let us know - we will add a corresponding test.

I cannot find a test with .cfa_offset or describing a non-rbp register, so the implementation is under-tested. How about leveraging inline asm to clobber callee-saved registers?

Enhance test cfi-basic-block-sections-1.ll to also check for frame-pointer=none.

Honestly I am not a CFI expert but I have read enough bits of LLVM libunwind and am not completely CFI illiterate (I have fixed a very subtle negative cfiDefCfa bug). The description of the patch is still puzzling me.
I think it lacks a summary about what the patch intends to do.

In D79978#2145063, @MaskRay wrote:

In D79978#2141992, @wmi wrote:

In D79978#2141959, @MaskRay wrote:

Ack. Then what instructions should be placed at the top of these basic blocks? Should .cfi_def_cfa_register %rbp be placed as well? If you move these basic blocks around, .cfi_def_cfa_register %rbp is currently not tracked.

That is because .cfi_def_cfa %rbp, 16 is identical to the following:
.cfi_def_cfa_register %rbp
.cfi_def_cfa_offset 16

Honestly I am not a CFI expert but I have read enough bits of LLVM libunwind and am not completely CFI illiterate (I have fixed a very subtle negative cfiDefCfa bug). The description of the patch is still puzzling me.

Ping, Is this alright now?

Move tests to CodeGen/X86/cfi*.

In D79978#2141959, @MaskRay wrote:

In D79978#2141768, @wmi wrote:

In D79978#2140849, @MaskRay wrote:

I haven't looked into the details, but the test suggests that the patch is wrong:

# basic-block-sections-cfi-1.ll
        .section        .text,"ax",@progbits,unique,2
_Z2f3b.2:                               # %if.end
        .cfi_startproc
        .cfi_def_cfa %rbp, 16    # this should be inserted after addq $16, %rsp
        .cfi_offset %rbp, -16    # this should be after .cfi_def_cfa %rbp, 16
        addq    $16, %rsp
        popq    %rbp
        .cfi_def_cfa %rsp, 8
        retq

I think the position where the cfi directives are currently inserted is correct. Those directives at the beginning of BB are not to maintain call frame information for instructions inside of BB like "addq $16, %rsp" and "popq %rbp", but to setup the call frame information correctly at the beginning of BB because the BB could be moved around.

Ack. Then what instructions should be placed at the top of these basic blocks? Should .cfi_def_cfa_register %rbp be placed as well? If you move these basic blocks around, .cfi_def_cfa_register %rbp is currently not tracked.

For basic-block-sections-cfiinstr_1.ll, have you considered places like CodeGen/X86/cfi-inserter-*? You may even create a subdirectory there.

This question still stands. I haven't seen other CFI tests in`DebugInfo/X86/`

In D79978#2141768, @wmi wrote:

In D79978#2140849, @MaskRay wrote:

I haven't looked into the details, but the test suggests that the patch is wrong:

# basic-block-sections-cfi-1.ll
        .section        .text,"ax",@progbits,unique,2
_Z2f3b.2:                               # %if.end
        .cfi_startproc
        .cfi_def_cfa %rbp, 16    # this should be inserted after addq $16, %rsp
        .cfi_offset %rbp, -16    # this should be after .cfi_def_cfa %rbp, 16
        addq    $16, %rsp
        popq    %rbp
        .cfi_def_cfa %rsp, 8
        retq

I think the position where the cfi directives are currently inserted is correct. Those directives at the beginning of BB are not to maintain call frame information for instructions inside of BB like "addq $16, %rsp" and "popq %rbp", but to setup the call frame information correctly at the beginning of BB because the BB could be moved around.

Address reviewer comments:

In D79978#2138208, @MaskRay wrote:

I have studied CFIInstrInserter in May. If you don't mind, please give me some time to review as well.

For basic-block-sections-cfiinstr_1.ll, have you considered places like CodeGen/X86/cfi-inserter-*? You may even create a subdirectory there.
_1 is not very common. -1 is more common.

Fix CFIInstr test and address other reviewer comments.

Simplify CFI instructions test.

Remove check for skipping debug info in DebugHandlerBase.cpp as it is not needed.

Simplify cfiinstr test .ll file

Simplify CFI test.

Split deduplication out of base CFI patch.

Use MapVector for MBBSectionRanges instead of IndexedMap.

Address reviewer comments :

Rebase and address reviewer comments which are code refactoring and enhancing the test case.

In D68049#2066937, @MaskRay wrote:

LLD side changes look good. Are you waiting on an explicit approval from @rmisth ?

Ping.

Ping.

Ping.

Code refactoring in CFIInstrInserter.cpp

Address reviewer comments:

Ping.

Rebase

Rebase and add a test for fission.

Address reviewer comments:

Rebase.

As far as I can tell, this LGTM. I am not an expert on section names and how this affects assumptions in GNU land, so please do seek an expert's review if necessary.

@MaskRay Could you please elaborate more on the problem being solved as it is not still fully clear to me? Is it that only the "exit" function has this problem? If you could also mention why the linker is not the right place to handle this problem, it would be useful. Thanks.

Update patch with changes to BackendUtil.cpp (forgot this file).

Rebase.

LGTM. You still need an approval from a LLD owner.

@rsmith Ping.

@rnk Ping.

LGTM, but please wait for Eli's comments before pushing.

Ping.

Update patch with missing file (MachineBasicBlock.h) and simplify test.

Ping.