[PATCH] D82084: [DebugInfo] Refactored out `debug_line.dwo` emission from `DwarfTypeUnit` to `DwarfDebug`

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX marked an inline comment as done.
SouraVX added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h:405
   // 0, referencing the comp_dir of all the type units that use it.
-  MCDwarfDwoLineTable SplitTypeUnitFileTable;
+  std::unique_ptr<MCDwarfDwoLineTable> SplitUnitLineTable = nullptr;
   /// @}
----------------
dblaikie wrote:
> SouraVX wrote:
> > dblaikie wrote:
> > > What's the motivation for this change to use unique_ptr here?
> > 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.
> 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)?
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.







Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82084