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

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

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



================
Comment at: llvm/lib/MC/MCDwarf.cpp:148
+  // which may directly use .loc/.file directives.
+  if (MCLineDivisions.count(Sec)) {
+    auto &Entries = MCLineDivisions[Sec];
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108261