[PATCH] D82084: [DebugInfo] Refactored out `debug_line.dwo` emission from `DwarfTypeUnit` to `DwarfDebug`
Sourabh Singh Tomar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 19 01:02:19 PDT 2020
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
More information about the llvm-commits
mailing list