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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1734
+      Name, Characteristics, SectionKind::getText(), COMDATSymName,
+      COFF::IMAGE_COMDAT_SELECT_NODUPLICATES, UniqueID);
+}
----------------
I wrote some comments about this on D99487, since there was discussion there about this.


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


================
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"
----------------
Can you include the .section directives in this test case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101421