[llvm-debuginfo-analyzer] Fix memory leak reported by sanitizers.

[CarlosAlbertoEnciso]

Summary

After landing the 08-elf-reader patch the sanitizer reported
memory leak issues. The test causing the issue was disabled

https://reviews.llvm.org/rGd81725d2bc016b58cb9327910f7892a2d4ac53c1

The command line used in the test case is:

llvm-debuginfo-analyzer --attribute=level
                        --print=instructions
                        pr-incorrect-instructions-dwarf-clang.o

When dealing with logical instruction lines associated with
an artificial logical scope, skip the process of finding
their enclosing scope. Just add them to the scope.

Create logical debug lines only if the command line specifies:

--print=lines or --print=elements or --print=all

[jryans]

Thanks, this looks reasonable to me! 🙂

Were you able to reproduce the failure locally?

[CarlosAlbertoEnciso]

In D137156#3899106, @jryans wrote:

Thanks, this looks reasonable to me! 🙂

Were you able to reproduce the failure locally?

First of all, thanks very much for the review.

Building with sanitizers require extra setup and stages.
It was relatively fast to have a very basic memory tracking (new/delete) to find the places where the memory was allocated and reproduce the failure locally.

Log from @vitalybuka:

==1211325==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 1440 byte(s) in 12 object(s) allocated from:
    #0 0x560d26e1e8cd in operator new(unsigned long) /b/sanitizer-x86_64-linux-fast/build/llvm-project/compiler-rt/lib/asan/asan_new_delete.cpp:95:3
    #1 0x560d275ff029 in llvm::logicalview::LVBinaryReader::createInstructions(llvm::logicalview::LVScope*, unsigned long, std::__1::pair<unsigned long, unsigned long> const&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:424:31
    #2 0x560d27603fa6 in llvm::logicalview::LVBinaryReader::createInstructions() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:498:21

Log from the memory tracking:

ERROR: Unallocated 12 objects from:

    1: 'X:\diva-root\llvm-project\llvm\lib\DebugInfo\LogicalView\Readers\LVBinaryReader.cpp':485
    2: 'X:\diva-root\llvm-project\llvm\lib\DebugInfo\LogicalView\Readers\LVBinaryReader.cpp':485
    3: 'X:\diva-root\llvm-project\llvm\lib\DebugInfo\LogicalView\Readers\LVBinaryReader.cpp':485
    4: 'X:\diva-root\llvm-project\llvm\lib\DebugInfo\LogicalView\Readers\LVBinaryReader.cpp':485
    5: 'X:\diva-root\llvm-project\llvm\lib\DebugInfo\LogicalView\Readers\LVBinaryReader.cpp':485
    6: 'X:\diva-root\llvm-project\llvm\lib\DebugInfo\LogicalView\Readers\LVBinaryReader.cpp':485
    7: 'X:\diva-root\llvm-project\llvm\lib\DebugInfo\LogicalView\Readers\LVBinaryReader.cpp':485
    8: 'X:\diva-root\llvm-project\llvm\lib\DebugInfo\LogicalView\Readers\LVBinaryReader.cpp':485
    9: 'X:\diva-root\llvm-project\llvm\lib\DebugInfo\LogicalView\Readers\LVBinaryReader.cpp':485
   10: 'X:\diva-root\llvm-project\llvm\lib\DebugInfo\LogicalView\Readers\LVBinaryReader.cpp':485
   11: 'X:\diva-root\llvm-project\llvm\lib\DebugInfo\LogicalView\Readers\LVBinaryReader.cpp':485
   12: 'X:\diva-root\llvm-project\llvm\lib\DebugInfo\LogicalView\Readers\LVBinaryReader.cpp':485

SUMMARY: Allocated 122, Deleted 110
ERROR: Detected memory leaks

UNREACHABLE executed at X:\diva-root\llvm-project\llvm\include\llvm/DebugInfo/LogicalView/Core/LVSupport.h:84!
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Exception Code: 0x80000003
 #0 0x00007ff6a5fc7d7c HandleAbort X:\diva-root\llvm-project\llvm\lib\Support\Windows\Signals.inc:414:0
 #1 0x00007ffd991cbc31 (C:\WINDOWS\SYSTEM32\ucrtbased.dll+0x6bc31)
 #2 0x00007ffd991cd889 (C:\WINDOWS\SYSTEM32\ucrtbased.dll+0x6d889)
 #3 0x00007ff6a5e7566a llvm::llvm_unreachable_internal(char const *, char const *, unsigned int) X:\diva-root\llvm-project\llvm\lib\Support\ErrorHandling.cpp:218:0
 #4 0x00007ff6a559f249 llvm::logicalview::LVAllocations::dump(void) X:\diva-root\llvm-project\llvm\include\llvm\DebugInfo\LogicalView\Core\LVSupport.h:86:0
 #5 0x00007ff6a5596e44 llvm::logicalview::LVAllocations::~LVAllocations(void) X:\diva-root\llvm-project\llvm\include\llvm\DebugInfo\LogicalView\Core\LVSupport.h:51:0
 #6 0x00007ff6a7e78662 `dynamic atexit destructor for '`public: static class logicalview::LVAllocations::getInstance & __cdecl llvm::logicalview::LVAllocations::getInstance(void)'::`2'::Instance''(void) (e:\projects\diva-root\builds\win\debug\Debug\bin\llvm-debuginfo-analyzer.exe+0x3568662)
 #7 0x00007ffd991d4957 (C:\WINDOWS\SYSTEM32\ucrtbased.dll+0x74957)
 #8 0x00007ffd991d4365 (C:\WINDOWS\SYSTEM32\ucrtb

Just as historical note: diva was the tool original name.

The lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:424:31 is the same line as lib\DebugInfo\LogicalView\Readers\LVBinaryReader.cpp':485.

[vitalybuka]

LGTM if this fixes the bug
Still the same questing as before: Can this code avoid using directly new/delete and rely on unique_ptrs?

[CarlosAlbertoEnciso]

In D137156#3899661, @vitalybuka wrote:

LGTM if this fixes the bug

Thanks very much for your review.

Still the same questing as before: Can this code avoid using directly new/delete and rely on unique_ptrs?

As per my response to @dblaikie in https://reviews.llvm.org/D125783:

In D125783#3893003, @dblaikie wrote:

2. code review feedback (such as memory allocation handling) being pushed back to some much later stage isn't usually the way we do things if it's reasonable observations about how things are done - postcommit (or, it sounds like precommit) feedback is to be addressed "There is a strong expectation that authors respond promptly to post-commit feedback and address it." (perhaps you could link to the other discussion on the raw new usage issue? Might help provide explanation for why it's not already addressed?)

We are fully aware of the issues related with the current memoy allocation and changing it to use smart pointers is a priority.
Based on your suggestion, I can create a GitHub issue to include all the information and add that link to each of the entries in the reviews that refer to the memory handling.

We can't change the new/delete to use unique_ptrs in just a single place.
The change must be done across the tool, as the logical elements (scopes, types, symbols, lines) are used in different modules:

• creation (debug info parsing)
• completion (add extra information)
• comparison/selection
• logical view generation

[vitalybuka]

Based on your suggestion, I can create a GitHub issue to include all the information and add that link to each of the entries in the reviews that refer to the memory handling.

Sorry, I missed that comment on the original patch.
Thank you!

[jryans]

In D137156#3901310, @CarlosAlbertoEnciso wrote:

Based on your suggestion, I can create a GitHub issue to include all the information and add that link to each of the entries in the reviews that refer to the memory handling.

I think it would be best to prioritise creating this issue quite soon (in the next few days), as it would be good to have on hand when discussing the current status of any other memory-related issues that arise with the tool.

[CarlosAlbertoEnciso]

In D137156#3902189, @jryans wrote:

In D137156#3901310, @CarlosAlbertoEnciso wrote:

Based on your suggestion, I can create a GitHub issue to include all the information and add that link to each of the entries in the reviews that refer to the memory handling.

I think it would be best to prioritise creating this issue quite soon (in the next few days), as it would be good to have on hand when discussing the current status of any other memory-related issues that arise with the tool.

You are right. The creation of that issue is the next item on my list and I am hoping to finish it by Friday.
Currently I am writing a README.txt for the tool that contains:

• Know issues
• Limitations
• Improvements
• etc

[jryans]

In D137156#3902237, @CarlosAlbertoEnciso wrote:

The creation of that issue is the next item on my list and I am hoping to finish it by Friday.
Currently I am writing a README.txt for the tool that contains:

• Know issues
• Limitations
• Improvements
• etc

Sounds great, thanks for all of your work on this tool! 🙂

llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp

while (TraverseLines && Iter != DebugLines->end()) {
     LVLines *Lines = ScopeInstructions.find(SectionIndex, Scope);
     if (Lines)
       InstructionLines = std::move(*Lines);
 
     LLVM_DEBUG({
       size_t Index = 0;
       dbgs() << "\nSectionIndex: " << format_decimal(SectionIndex, 3)
              << " Scope DIE: " << hexValue(Scope->getOffset()) << "\n"
-             << format("Process instructions lines: %d\n",
+             << format("Process instruction lines: %d\n",
                        InstructionLines.size());
       for (const LVLine *Line : InstructionLines)
         dbgs() << format_decimal(++Index, 5) << ": "
                << hexValue(Line->getOffset()) << ", (" << Line->getName()
                << ")\n";
     });
 
     // Continue with next debug line if there are not instructions lines.
void LVBinaryReader::processLines(LVLines *DebugLines,
   });
 
   // If this compilation unit does not have line records, traverse its scopes
   // and take any collected instruction lines as the working set in order
   // to move them to their associated scope.
   if (DebugLines->empty()) {
     if (const LVScopes *Scopes = CompileUnit->getScopes())
       for (LVScope *Scope : *Scopes) {
-        if (Scope->getIsArtificial())
-          continue;
         LVLines *Lines = ScopeInstructions.find(Scope);
         if (Lines) {
 
           LLVM_DEBUG({
             size_t Index = 0;
             dbgs() << "\nSectionIndex: " << format_decimal(SectionIndex, 3)
                    << " Scope DIE: " << hexValue(Scope->getOffset()) << "\n"
-                   << format("Instructions lines: %d\n", Lines->size());
+                   << format("Instruction lines: %d\n", Lines->size());
             for (const LVLine *Line : *Lines)
               dbgs() << format_decimal(++Index, 5) << ": "
                      << hexValue(Line->getOffset()) << ", (" << Line->getName()
                      << ")\n";
           });
 
+          if (Scope->getIsArtificial()) {
+            // Add the instruction lines to their artificial scope.
+            for (LVLine *Line : *Lines)
+              Scope->addElement(Line);
+          } else {
             DebugLines->append(*Lines);
+          }
           Lines->clear();
         }
       }
   }
 
   LVRange *ScopesWithRanges = getSectionRanges(SectionIndex);
   ScopesWithRanges->startSearch();
 

llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp

for (const DWARFDebugLine::FileNameEntry &Entry :
       std::string String;
       raw_string_ostream(String) << Directory << "/" << File;
       CompileUnit->addFilename(String);
     }
 
   // In DWARF5 the file indexes start at 0;
   bool IncrementIndex = Lines->Prologue.getVersion() >= 5;
 
-  // Get the source lines.
-  if ((options().getAttributeRange() || options().getPrintLines()) &&
-      Lines->Rows.size())
+  // Get the source lines if requested by command line option.
+  if (options().getPrintLines() && Lines->Rows.size())
     for (const DWARFDebugLine::Row &Row : Lines->Rows) {
       // Here we collect logical debug lines in CULines. Later on,
       // the 'processLines()' function will move each created logical line
       // to its enclosing logical scope, using the debug ranges information
       // and they will be released when its scope parent is deleted.
       LVLineDebug *Line = new LVLineDebug();
       CULines.push_back(Line);
       Line->setAddress(Row.Address.Address);

llvm/test/tools/llvm-debuginfo-analyzer/DWARF/pr-incorrect-logical-instructions.test

 ; REQUIRES: x86-registered-target
 
-; FIXME: Memory leak https://reviews.llvm.org/D125783
-; UNSUPPORTED: asan
-
 ; * Added incorrect logical instructions for: --print=lines,instructions
 ;   'bar' and 'foo' showing extra instruction from compiler generated functions:
 ;   '_cxx_global_var_init' and '_GLOBAL_sub_l_suite_lexical_01.cpp'
 ;
 ; * Missing logical instructions for: --print=instructions
 ;   Only 'foo' showing logical instructions.
 
 ; pr-incorrect-instructions-dwarf-clang.cpp

llvm/unittests/DebugInfo/LogicalView/ELFReaderTest.cpp

 TEST(LogicalViewTest, ELFReader) {
   // Initialize targets and assembly printers/parsers.
   llvm::InitializeAllTargetInfos();
   llvm::InitializeAllTargetMCs();
   InitializeAllDisassemblers();
 
   llvm::sys::InitializeCOMRAII COM(llvm::sys::COMThreadingMode::MultiThreaded);
 
-  SmallString<128> InputsDir = unittest::getInputFileDirectory(TestMainArgv0);
-
-  // This test requires a x86-registered-target
+  // This test requires a x86-registered-target.
   Triple TT;
   TT.setArch(Triple::x86_64);
   TT.setVendor(Triple::UnknownVendor);
   TT.setOS(Triple::UnknownOS);
 
   std::string TargetLookupError;
   if (!TargetRegistry::lookupTarget(std::string(TT.str()), TargetLookupError))
     return;
 
+  SmallString<128> InputsDir = unittest::getInputFileDirectory(TestMainArgv0);
+
   // Logical elements general properties and selection.
   elementProperties(InputsDir);
   elementSelection(InputsDir);
 
   // Compare logical elements.
   compareElements(InputsDir);
 }
 
 } // namespace