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

Sun Nov 14 19:31:09 PST 2021

kyulee added a comment.

In D108261#3130364 <https://reviews.llvm.org/D108261#3130364>, @dblaikie wrote:

> Thanks for all the work/iteration/research here - sorry it was a bit fussy.

Thanks for the thorough review! I had a chance to dive a bit in this area.
Indeed, this debug world seems subtle to make it right.

================
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:
> > > 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.
> Ah, hmm - yeah, there's certainly ways we could arrive at a function without any instructions having a location. Though I wonder what the line table should look like for that? Not having the line table cover those instructions seems a bit weird too.
> 
> But if that's the state of things today, might as well leave ti as-is. Could you include a comment describing how that occurs with the MCObjectStreamer use case as well as the one outlined with MCAsmStreamer?
For the above case where instructions have no location at all, the line entries in the table didn't appear in dwarf although other contents appeared.

Yeah. Will update the comments to include two cases (MCAsmStreamer and MCObjectStreamer).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108261