This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Enable CodeView DebugInfo for basic block sections
AcceptedPublic

Authored by TaoPan on Apr 27 2021, 7:05 PM.

Details

Summary

With basic block sections, there are multiple flexible text sections for
a function, allow multiple .cv_loc directives for a function.

Signed-off-by: Pan, Tao <tao.pan@intel.com>

Diff Detail

Event Timeline

TaoPan created this revision.Apr 27 2021, 7:05 PM
TaoPan requested review of this revision.Apr 27 2021, 7:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2021, 7:05 PM

Could you please have a review?

rnk added inline comments.Apr 29 2021, 12:59 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1734 ↗(On Diff #341053)

I wrote some comments about this on D99487, since there was discussion there about this.

llvm/lib/MC/MCStreamer.cpp
331 ↗(On Diff #341053)

This check prevents us from crashing later in CodeViewContext::emitLineTableForFunction when it calls:

OS.emitAbsoluteSymbolDiff(J->getLabel(), FuncBegin, 4);

If those two labels are in different sections, this just moves the crash later. Which makes me wonder if any BB section splitting happens in the test case you added. Does it exercise that yet, or does that come later?

The codeview line table format assumes functions are contiguous to avoid extra relocations. There is a relocation to the function start at the beginning, and the rest is a sequence of code offsets and line numbers. I wonder if it would be better to plan to produce a line table per section instead of trying to make one holistic line table. Then we could keep this check. So, if that is the long term plan, maybe we can leave this code here in this change.

llvm/test/DebugInfo/COFF/basic-block-sections.ll
14

Can you include the .section directives in this test case?

TaoPan updated this revision to Diff 341801.Apr 30 2021, 1:29 AM

Add .section directives test

TaoPan updated this revision to Diff 341803.Apr 30 2021, 1:43 AM

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

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.

llvm/lib/MC/MCStreamer.cpp
331 ↗(On Diff #341053)

Both removing and restoring this check, OS.emitAbsoluteSymbolDiff(J->getLabel(), FuncBegin, 4); of CodeViewContext::emitLineTableForFunction won't be called in my test.
I compared to llvm/test/DebugInfo/X86/basic-block-sections_1.ll (Linux ELF test), it's also not per section line table, it's per file line table, the format is as below, no function and section information.
Address Line Column File ISA Discriminator Flags
0x0000000000000000 1 0 1 0 0 is_stmt
Is there any reason that need per section line table?

llvm/test/DebugInfo/COFF/basic-block-sections.ll
14

I added .section directives

rnk added inline comments.May 7 2021, 2:39 PM
llvm/lib/MC/MCStreamer.cpp
331 ↗(On Diff #341053)

I'm pretty confident that we do reach emitAbsoluteSymbolDiff when emitting emitLineTableForFunction, but I discovered that it doesn't assert, it produces a relocation instead. I haven't reasoned through what the relocation does, but I see it in your test case. I confused emitAbsoluteSymbolDiff with the computeLabelDiff helper in the same file, see here:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/MC/MCCodeView.cpp#L454

So, I agree, removing this assertion doesn't cause a crash now, but it will if you introduce inlining into the test case and attempt to produce an inline line table. (.cv_inline_linetable).

Even though we don't crash in the non-inline line table case, it's not clear to me what a debugger would do with a an unsorted, non-ascending, possibly negative line table, which is what you would get after the linker applies relocations.

TaoPan updated this revision to Diff 343992.May 10 2021, 1:13 AM

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

TaoPan edited the summary of this revision. (Show Details)May 10 2021, 1:15 AM
TaoPan updated this revision to Diff 343995.May 10 2021, 1:19 AM

Remove same modification of D100735

TaoPan updated this revision to Diff 344648.May 11 2021, 8:41 PM

git rebase

TaoPan added inline comments.May 11 2021, 10:16 PM
llvm/lib/MC/MCStreamer.cpp
331 ↗(On Diff #341053)

Thanks for your guidance!
I added inline function to test, I can produce an inline line table (.cv_inline_linetable) with llc to asm then llvm-mc to obj (line 2 and 4 in my test). I can reproduce your mentioned problem if llc directly to obj, I'm investigating the problem that Success is false in https://github.com/llvm/llvm-project/blob/main/llvm/lib/MC/MCCodeView.cpp#L454

TaoPan added inline comments.May 19 2021, 1:22 AM
llvm/lib/MC/MCStreamer.cpp
331 ↗(On Diff #341053)

Thanks Rnk!
We are facing two problems in turn.

  1. At first, with BB section splitting, relocations at the beginning of all BB sections are needed, so this check should be removed, it blocks emit later relocations after the first one.
  2. Then emit line table, I understood the codeview line table format depends on beginning relocation and contiguous text. Before enabling BB section splitting, function is the smallest unit satisfying the condition, after enabling BB section splitting, section become the smallest unit satisfying the condition. Your design that produce a line table per section is good.

I think it’s ok fix 1 and 2 at the same time.
As you know, 2 is not easy handling, it should be a long term plan.
How do you think of decoupling D99487 BB sections on Windows COFF(release mode, is controlled by default off option) and BB sections CodeView DebugInfo on Windows COFF (debug mode)?

TaoPan updated this revision to Diff 349178.Jun 1 2021, 10:52 PM

Disable .cv_loc directive emission outside the primary function section.

rnk added inline comments.Jun 2 2021, 1:59 PM
llvm/lib/MC/MCStreamer.cpp
328 ↗(On Diff #349178)

Please make this change in the AsmPrinter, not the assembler. The assembler should still emit an error: if the user directly uses cv_loc directives, the assembler should produce an error. The compiler needs to avoid asking the assembler to do the impossible.

ormris removed a subscriber: ormris.Jun 3 2021, 10:11 AM
TaoPan updated this revision to Diff 350198.Jun 7 2021, 1:51 AM

Don't emit location of BB sections

TaoPan added inline comments.Jun 7 2021, 2:03 AM
llvm/lib/MC/MCStreamer.cpp
328 ↗(On Diff #349178)

Thanks for your review comments!
I moved change from assembler to AsmPrinter, the new added condition of not emitting location is same as the condition of creating BB section (calling to getSectionForMachineBasicBlock()) in AsmPrinter.cpp.

rnk added inline comments.Jun 7 2021, 4:18 PM
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
2947

Please add a comment here.

Also, are you sure this is the right condition? Aren't there MBBs that do not begin sections but belong to a non-primary section?

TaoPan updated this revision to Diff 350516.Jun 8 2021, 12:41 AM

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

TaoPan added inline comments.Jun 8 2021, 12:45 AM
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
2947

Thanks!
I added comment and changed condition to the MachineFunction which has the MachineInstr has BB sections and the MachineBasicBlock which has the MachineInstr is not entry block, how do you think?

rnk added a comment.Jun 15 2021, 11:21 AM

Sorry for the delay.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
2952

Instead of just the entry block, can this be MI->getParent()->getSectionID() == MBBSectionId::Default? That would cover all blocks in the default section, right?

tmsriram added inline comments.Jun 15 2021, 11:32 AM
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
2949

Typo, "section" and "primary". Similar comment rnk@. The condition below only checks whether it is not the entry block but the comment says it is all non-primary basic blocks.

Thanks for your review comments!

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
2949

Thanks for pointing out the confuse comment! How about "The codeview line table format requires functions are contiguous, if function has BB sections, all MBBs have their own section, all sections are not contiguous, emit codeview relocation only for entry block MBB in this case."?

2952

I think MBBSectionId can not be used for distinguishing basic blocks in the default section or not. The enum values of MBBSectionId are Default = 0, Exception, Cold. Section ID of all MBBs are MBBSectionId::Default in BB Sections case.
I tried C function like below:
int bazb(...) {

...           // Unique section for function bazb, the MBB is entry block: .text,"xr",one_only,"?bazb@@YAHH@Z"
if (...) {
  ...         // BB section 1: .text,"xr",associative,"?bazb@@YAHH@Z.__part.1"
} else {
  ...         // BB section 2: .text,"xr",associative,"?bazb@@YAHH@Z.__part.2"
}
...           // BB section 3: .text,"xr",associative,"?bazb@@YAHH@Z.__part.3"
if (...) {
  ...         // BB section 4: .text,"xr",associative,"?bazb@@YAHH@Z.__part.4"
} else {
  ...         // BB section 5: .text,"xr",associative,"?bazb@@YAHH@Z.__part.5"
}
...           // BB section 6: .text,"xr",associative,"?bazb@@YAHH@Z.__part.6"

}
I think if function has BB sections, all basic blocks have their own section, all non-EntryBlock sections are relocatable BB Sections. In other words, entry block is unique block in the default section.

rnk accepted this revision.Jul 1 2021, 8:12 PM

lgtm

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
2949

Please fix the typo before committing.

2952

I see. I guess that's me being confused between intra-function hot cold splitting and BB sections. The condition is correct, then.

This revision is now accepted and ready to land.Jul 1 2021, 8:12 PM
TaoPan updated this revision to Diff 356092.Jul 1 2021, 8:55 PM

Fix the typo of comment

TaoPan added a comment.Jul 1 2021, 8:58 PM

lgtm

Thanks for your big help!

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
2949

I updated the comments.

snehasish resigned from this revision.Jan 30 2023, 8:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2023, 8:58 AM