[PATCH] D101421: [DebugInfo] Enable CodeView DebugInfo for basic block sections

[TaoPan via Phabricator via llvm-commits]

TaoPan added inline comments.


================
Comment at: llvm/lib/MC/MCStreamer.cpp:331
-        Loc,
-        "all .cv_loc directives for a function must be in the same section");
-    return false;
----------------
TaoPan wrote:
> rnk wrote:
> > TaoPan wrote:
> > > rnk wrote:
> > > > 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.
> > > 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?
> > 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.
> 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
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)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101421/new/

https://reviews.llvm.org/D101421