[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
+  // which may directly use .loc/.file directives.
+  if (MCLineDivisions.count(Sec)) {
+    auto &Entries = MCLineDivisions[Sec];
----------------
dblaikie wrote:
> kyulee wrote:
> > 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.
> Ah, OK! Right - the solution to that would be to have a virtual function in MCStreamer that's differently implemented (a no-op in the asm streamer case - maybe someone'll eventually extend the assembly syntax to support terminating line contributions - so nodebug could be properly done in assembly) this would be side-by-side with the MCStreamer::emitDwarfLocDirective - or, perhaps it could use that specific function, with an extra parameter or flag value for "end entry"?
Yea. I tried to create `emitTerminateLineTable` in MCStreamer and differently extends it for MCAsmStreamer, which resolved the most cases.
But I'm still seeing dozens of failures in unit tests which are probably because of incomplete debug data in the test cases that are manually synthesized.
For instance, ` DebugInfo/X86/multiple-at-const-val.ll`, it was like
```
define i32 @main() !dbg !960 {
entry:
  %call1.i = tail call %"class.std::basic_ostream"* @test(%"class.std::basic_ostream"* @_ZSt4cout, i8* getelementptr inbounds ([6 x i8], [6 x i8]* @.str, i64 0, i64 0), i64 5)
  ret i32 0
}
```
This means the function appears to have a debug info, but there is no tag like `DILocation` for each line.
I presume although the above is unlikely the real case from the compiler, but I think this seems a valid case (opt or any transformation may drop debug tag) so we should cope with. 
Having said that, I think this check seems worth being kept -- then I don't see the reason above to specialize streamer just for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108261