Page MenuHomePhabricator

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

Authored by SouraVX on Jun 18 2020, 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

Unit TestsFailed

TimeTest
9,460 mslinux > libomp.env::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/env/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/env/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic
1,520 mslinux > libomp.worksharing/for::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/worksharing/for/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic && /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp 7
150 mslinux > lld.ELF::Unknown Unit Message ("")
Script: -- : 'RUN: at line 6'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-mc -filetype=obj -triple=x86_64 /mnt/disks/ssd0/agent/llvm-project/lld/test/ELF/text-section-prefix.s -o /mnt/disks/ssd0/agent/llvm-project/build/tools/lld/test/ELF/Output/text-section-prefix.s.tmp.o
0 mslinux > lld.ELF::Unknown Unit Message ("")
Exception during script execution: OSError: [Errno 28] No space left on device

Event Timeline

SouraVX created this revision.Jun 18 2020, 5:16 AM
dblaikie added inline comments.Jun 18 2020, 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.Jun 18 2020, 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.Jun 18 2020, 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.Jun 19 2020, 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 :)

Apologies for If I'm pinging too frequently :) . Gentle Ping to all reviewers again! :)

@dblaikie and other reviewers, these changes seems Okay to you folks ?

@dblaikie and other reviewers, these changes seems Okay to you folks ?

Still looking, and yeah - I'm not sure this is the right direction in general, might be easier for me to just write up the change.(for this and D81476). Working on it.

SouraVX updated this revision to Diff 278980.Jul 18 2020, 3:18 AM

Tried one more time disregarding both the approach followed previously.
This time finally worked out(in a much cleaner way).
It exposes getDwoLineTable private method. But IMO that's needed
anyways, since it will used/needed subsequently in D81476.

@dblaikie, Could you please share your thoughts on this.
I hope this should avoid the rework from your side.

Thanks!

Tried one more time disregarding both the approach followed previously.
This time finally worked out(in a much cleaner way).
It exposes getDwoLineTable private method. But IMO that's needed
anyways, since it will used/needed subsequently in D81476.

@dblaikie, Could you please share your thoughts on this.
I hope this should avoid the rework from your side.

Thanks!

I've posted this as a possible alternative solution/direction - seems simple enough to me, but perhaps I'm missing some cases/understanding? D84278