Page MenuHomePhabricator

[DebugInfo] Refactored out `debug_line.dwo` emission from `DwarfTypeUnit` to `DwarfDebug`
Needs ReviewPublic

Authored by SouraVX on Thu, Jun 18, 5:16 AM.

Details

Summary

Emission of debug_line.dwo was restricted by presence of
DwarfTypeUnits and the associated code was also hard-wired just
to satisfy this requirement.

This patch pulls out the emission code of debug_line.dwo from
DwarfTypeUnits to DwarfDebug so that, if required(for instance
debug_macro.dwo needs this), it can be leveraged easily.

This patch is based on @dblaikie feedback on D81476. Thanks for this!

Diff Detail

Event Timeline

SouraVX created this revision.Thu, Jun 18, 5:16 AM
dblaikie added inline comments.Thu, Jun 18, 12:37 PM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
405

What's the motivation for this change to use unique_ptr here?

SouraVX added inline comments.Thu, Jun 18, 12:52 PM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
405

The intent/motivation here is that, we don't want to leak memory and simplify memory management(at least for pointers).
Does this introduces any overhead(to whole thing) ?? or may introduce other potential bugs/undesired behavior that I'm not aware of ?
Happy to roll back.

dblaikie added inline comments.Thu, Jun 18, 5:14 PM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
405

Not sure I follow why there would be memory leaks or complications if this remained as it was before the patch - what concerns did you have about the way the code was phrased previously (using an MCDwarfDwoLineTable value, without unique_ptr or other indirection)?

SouraVX marked an inline comment as done.Fri, Jun 19, 12:33 AM
SouraVX added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
405

Sorry I misunderstood the context of your question(I thought "Why unique_ptr" over raw ptr), and hence the confusion.

what concerns did you have about the way the code was phrased previously (using an MCDwarfDwoLineTable value, without unique_ptr or other indirection)?

I first tried patching-up this at very minimal(Following value semantics as it was earlier), later I realized it can't(looks to me) be done that way, due to limitation in existing design.

SplitTypeUnitFileTable was used only for Emission and some initialization effort later will end up initializing MCDwarfDwoLineTable *SplitLineTable( in DwarfTypeUnit) which was used to do the real work.

Emission of debug_line is also mixed in it, for instance: consider this snippet(before patch) from DwarfUnit.

 unsigned DwarfTypeUnit::getOrCreateSourceID(const DIFile *File) {
   // Qucik boucne check whether we need a split unit line table.
   if (!SplitLineTable) // SplitLineTable member of `DwarfTypeUnit` initialized in           
                                // constructor using `SplitTypeUnitFileTable`
     return getCU().getOrCreateSourceID(File);
...
code for `debug_line.dwo` emission

So let's say, we go by value semantics, code will look like(mostly):

 unsigned DwarfTypeUnit::getOrCreateSourceID(const DIFile *File) {
      // Qucik boucne check whether we need a split unit line table.
   if (!DD.getSplitUnitLineTable().HasSplitLineTable)
                      // `HasSplitLineTable` is private member of `MCDwarfDwoLineTable`.
     return getCU().getOrCreateSourceID(File);
...
code for `debug_line.dwo` emission

If you look close enough, you'll realize that's also not going to work and you won't have debug_line.dwo in any case(Since HasSplitLineTable is initialized to false by default and it can be only changed by the code below(which will never be executed in first place)). + This also demands to expose private member HasSplitLineTable to public.

Overall this path doesn't looks promising(at least to me). And using pointer semantics(used carefully) solves the problem cleanly and doesn't degenerate anything, more flexible/maintainable.

Conclusion: I've a patch using existing semantics(Can share if you want to have a look :) ). It is causing following test case failure(due to above described problem) in regression suite.

Failing Tests (3):
  LLVM :: CodeGen/X86/dwarf-headers.ll
  LLVM :: CodeGen/X86/dwarf-split-line-2.ll
  LLVM :: DebugInfo/X86/generate-odr-hash.ll

I hope this make sense, Thank You for bearing with me.

@dblaikie Are you Convinced/Okay with explanation and the overall changes.

@dblaikie Are you Convinced/Okay with explanation and the overall changes.

It's on my list of things to look at

Gentle Ping :)