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

[TaoPan via Phabricator via llvm-commits]

TaoPan added a comment.

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.



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


================
Comment at: llvm/test/DebugInfo/COFF/basic-block-sections.ll:13
+
+; X86-LABEL:  "?f3@@YAX_N at Z":
+; X86:        .cv_file 1 "c:\\work\\basic-block-sections.cpp"
----------------
rnk wrote:
> Can you include the .section directives in this test case?
I added .section directives


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101421