[PATCH] D108261: [DebugInfo] Fix end_sequence of debug_line in LTO Object

[Kyungwoo Lee via Phabricator via llvm-commits]

kyulee added inline comments.


================
Comment at: llvm/lib/MC/MCDwarf.cpp:148
+  auto &Entries = MCLineDivisions[Sec];
+  if (!Entries.empty()) {
+    auto EndEntry = Entries.back();
----------------
dblaikie wrote:
> kyulee wrote:
> > dblaikie wrote:
> > > When does this situation come up? (I think this question got lost from its previous place here: https://reviews.llvm.org/D108261#inline-1086075 during refactoring)
> > > 
> > > (if it is necessary, it might be suitable to use `find` on the `MCLineDivisions` rather than `operator[]` (so this call doesn't create an entry when it's empty/unused))
> > Even though we have a range, we may not have any debug line entry in unit tests.
> > 
> > Makes sense using `find`. Will update it with additional assembly test.
> Seems like we shouldn't be adding extra API surface area only for unit tests, though? But it's possible this comes up in real cases of empty functions - if that's the case (worth testing/validating), leaving a FIXME for when we remove/fix the existence of empty functions would be good.
It turned out the MCAsmStreamer path (as opposed to MCObjectStreamer) typically doesn't populate the line entry.
So, I keep this check to bail-out cloning/adding the end entry in that case. Or, hundreds of LIT tests failed. 


================
Comment at: llvm/lib/MC/MCDwarf.cpp:148
+  // which may directly use .loc/.file directives.
+  if (MCLineDivisions.count(Sec)) {
+    auto &Entries = MCLineDivisions[Sec];
----------------
dblaikie wrote:
> find is preferred over count - so that the result can be used rather than a duplicate lookup:
> ```
> auto I = MCLineDivisions.find(Sec);
> if (I != MCLineDivisions.end()) {
>   auto &Entries = I->second;
>   ...
> }
> ```
> 
> Though I'm still curious to better understand under what conditions this is needed.
Yea. `find` seems better since I need to use the map anyhow inside. Will update it.

As commented above in the code, the assembly output path (MCAsmStreamer as opposed to MCObjectStreamer), the line entry is not added instead .loc directives are emitted during the function emission. So, the line entries are often irrelevant in the assembly output except a certain target -- I guess XCOFF shown below in the tests.
I was thinking to check streamer and specialize this logic outside this, but not sure exactly how to do so.


================
Comment at: llvm/test/DebugInfo/XCOFF/empty.ll:230
 ; ASM32-NEXT:          .byte   1
-; ASM32-NEXT:          .byte   0                               # Set address to L..sec_end0
+; ASM32-NEXT:          .byte   0                               # Set address to L..func_end0
 ; ASM32-NEXT:          .byte   5
----------------
dblaikie wrote:
> Hmm, what aspect of the change caused these labels to change name?
I think this XCOFF seems a unique path that generates the line table for the assembly output in DwarfDebug.
So, the new logic is still kicked in, which adds an entry based on the range end label (instead of the section end label in the fall-back path).
I think the range for function uses function labels, so that's why the change happens.
Although this is the same in this unit tests, in theory, I think this new change is more precise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108261