This is an archive of the discontinued LLVM Phabricator instance.

[llvm-debuginfo-analyzer] 08 - ELF Reader
ClosedPublic

Authored by CarlosAlbertoEnciso on May 17 2022, 6:49 AM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
probinson added inline comments.Sep 20 2022, 3:13 PM
llvm/include/llvm/DebugInfo/LogicalView/Readers/LVELFReader.h
39

This should be a local variable in the place where prettyPrintRegisterOp is called; it doesn't need to be a class member.

112

It seems strange to have "get" methods that don't return anything. Are there better names?

llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp
231

Parameters should be re-aligned to conform to formatting standard.

Done with code files, still need to do tests. But my laptop is about to have a forced reboot so I am submitting these comments now.

llvm/include/llvm/DebugInfo/LogicalView/Readers/LVELFReader.h
44

DummyDie is a placeholder used in only two places. It should be a local variable in those places, not a class member.

llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp
95

StringRef has a str() method that returns std::string so you can eliminate the local variable TheFilename.

108

ConvertedPath is already a std::string, no need to make a copy.

155

Unexpected file type is success?

173

The for and first if can be swapped.

llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp
273
348
349
350
414

Maybe add a first line to the comment here, something like
// Here we add logical lines to the Instructions. Later on,
and then the comment about 'processLines()' has better context.

513
513

Have you thought about using std::merge to do this merging of two sorted containers? If that doesn't work, maybe refactor the three nested loops modeled on the example merge code sketched out here

602

Is TraverseLines really needed? If it is set to false only when you reach DebugLines->end() it seems like an unnecessary complication.

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

Maybe a comment here?

49

Isn't CurrentSymbol currently nullptr ?

441

FormValue and U are parameters, the lambda is using nothing else.

462

Any time lowpc == highpc it is an empty range. Linkers might fill in 1 for code that gets stripped, or they might fill in something else.

594

overrite should be override? or overwrite?

693

It looks like Upper will not include the size of the last child?

730

Maybe add a first line to the comment here, something like
// Here we collect logical debug lines in CULines. Later on,
and then the comment about 'processLines()' has better context.

796

If getFilename() returns a StringRef, you can do getFilename().str().c_str() and then don't need TheFilename.

863

"Deduct" means "subtract" which is not what you mean here.

959

This method doesn't "get" information in the usual sense (return it to the caller).
Maybe processLocationList ? Consistent with the lambda names in the implementation.

1050

Maybe processMemberLocation ?

1170

LVELFReader is used for MachO? I guess the differences are minimal because they both use DWARF. But probably should mention this in the comment at the top of the file.

1186

Is there a missing TODO here? The comment above talks about "unless you know" which implies there are cases where we could know. But this line is basically
if (IsSTAB) continue;
which could have been sooner, and simplify this bit here.

1216
1217

This implies that the tool can handle only linked executables? Not relocatable objects? maybe I knew that... but this requirement means it won't work for relocatable files compiled with -ffunction-sections.

Finished my review pass on this patch.

llvm/test/tools/llvm-debuginfo-analyzer/DWARF/04-dwarf-missing-nested-enumerators.test
24 ↗(On Diff #458404)
llvm/test/tools/llvm-debuginfo-analyzer/DWARF/05-dwarf-incorrect-lexical-scope-variable.test
25 ↗(On Diff #458404)

I'm not seeing a CodeView display in this test.

llvm/unittests/DebugInfo/LogicalView/ELFReaderTest.cpp
80

There are other ASSERT_* macros used in this function, so I don't think your concern is correct.

Update patch due to changes in previous patches.

Updated patch due to changes in previous patches.

Updated patch due to changes in previous patches.

llvm/include/llvm/DebugInfo/LogicalView/LVReaderHandler.h
41

Changed to Mach-O.

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

Changed.

348

Changed.

349

Changed.

350

Changed.

414

Good point. Added your suggested comment.

624

Changed.

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

Added

// As the command line options did not specify a request to print
// logical symbols (--print=symbols or --print=all or --print=elements),
// skip its creation.
49

Yes. To simplify changed to

return nullptr;
594

Changed to override.

730

Added your suggested comment.

760

Changed.

761

Changed.

763

Changed.

// The 'prettyPrintRegisterOp' function uses the DWARFUnit to support
// DW_OP_regval_type. At this point we are operating on a logical view
// item, with no access to the underlying DWARF data used by LLVM.
// We do not support DW_OP_regval_type here.
1216

Changed.

llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h
82

LVSectionIndex is the value returned by getIndex and it is the object section number (id).

https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Object/ObjectFile.h#L470

llvm/test/tools/llvm-debuginfo-analyzer/DWARF/04-dwarf-missing-nested-enumerators.test
24 ↗(On Diff #458404)

Changed.

; In the following logical views, we can see that the DWARF debug
; information generated by the Clang compiler does not include any
; references to the enumerators 'RED' and 'BLUE'. The DWARF generated
; by GCC, does include such references.
llvm/test/tools/llvm-debuginfo-analyzer/DWARF/05-dwarf-incorrect-lexical-scope-variable.test
25 ↗(On Diff #458404)

Changed

; In the following logical views, we can see that the DWARF debug
; information generated by the Clang compiler shows the variables
; 'Var_1' and 'Var_2' are at the same lexical scope (4) in the function
; 'InlineFuction'.
; The DWARF generated by GCC/Clang show those variables at the correct
; lexical scope: '3' and '4' respectively.
llvm/unittests/DebugInfo/LogicalView/ELFReaderTest.cpp
80

You are correct. Very good point.
I was confused with functions that return a value.
Changed other instances to use ASSERT_NE(cond1, nullptr);

llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h
153

You are correct. Marked LVBinaryReader destructor as virtual.

llvm/include/llvm/DebugInfo/LogicalView/Readers/LVELFReader.h
112

Renamed as processLocationMember.

llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp
231

Applied correct alignment.

llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp
95

Nice suggestion. Removed TheFilename.

108

Removed TheFilename and changed to

return createStringError(errc::bad_file_descriptor,
                         "File '%s' does not exist.",
                         ConvertedPath.c_str());
llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp
796

Nice suggestion. Removed TheFilename.

863

Renamed as DeduceIncrementFileIndex.

959

Renamed as processLocationList.

1050

Renamed as processMemberLocation.

llvm/include/llvm/DebugInfo/LogicalView/Readers/LVELFReader.h
39

Changed DIDumpOptions DumpOpts; to a local variable.

44

Changed DWARFDie DummyDie; to a local variable.

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

Changed.

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

Good point. Changed.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h
239

The method is used only within the logical reader and always the parameter To corresponds to the binary file being analyzed, which already has any Windows backlashes converted for forward slashes. Moved the method to LVReader:

class LVReader {
  ...
protected:
  ...
  std::string createAlternativePath(StringRef From) {
    // During the reader initialization, any backslashes in 'InputFilename'
    // are converted to forward slashes.
    SmallString<128> Path;
    sys::path::append(Path, sys::path::Style::posix,
                      sys::path::parent_path(InputFilename),
                      sys::path::filename(sys::path::convert_to_slash(
                          From, sys::path::Style::windows)));
    return std::string(Path);
  }
  ...
};
llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp
155

Good finding. That should be an Error. For example:

llvm-debuginfo-analyzer resource-file.res --print=scopes

The file_magic::windows_resource is supported by LLVM but not by llvm-debuginfo-analyzer.
Adding an Error condition:

return createStringError(errc::not_supported,
                         "Binary object format in '%s' is not supported.",
                         Filename.str().c_str());
173

Nice point. Swapped.

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

You are correct. Changed.

1217

Changed.

llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h
173

Changed prettyPrintRegisterOp as a DWARFExpression static public member.

llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
527

Added:

/// The \p AlternativeLocation specifies an alternative location to get
/// the DWARF context for the DWO object; this is the case when it has
/// been moved from its original location.
llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h
398

Short answer is no.

Both readers (ELF/CodeView) use the public names information to create the instructions (LVLineAssembler). Instead of relying on differents DWARF section names (.debug_pubnames, .debug_names) and formats (CodeView S_PUB32), the reader collects the needed information in LVELFReader::processOneDie for DWARF and LVSymbolVisitor::visitKnownRecord(CVSymbol &Record, ProcSym &Proc) for CodeView S_GPROC32, S_LPROC32, S_LPROC32_ID, S_GPROC32_ID records.

A future task can be to process the respective sections for DWARF (.debug_pubnames, .debug_names) and CodeView public symbol stream.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h
214

Good point. Added to list for later refactor.

  • Use Optional<ValueType>.
llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h
50

Good point. Added to list for later refactor.

153

Moved destructor to .cpp file close to createInstructions and getSectionRanges where the allocations are done.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
1427

Removed else after return.

1433

The problem is that both returned true values must be in the same scope as the Directory setting and there a missing return false for the case of DWARF >= 5.

bool DWARFDebugLine::LineTable::getDirectoryForEntry(
    const FileNameEntry &Entry, std::string &Directory) const {
  if (Prologue.getVersion() >= 5) {
    if (Entry.DirIdx < Prologue.IncludeDirectories.size()) {
      Directory =
          dwarf::toString(Prologue.IncludeDirectories[Entry.DirIdx], "");
      return true;
    }
    return false;
  }
  if (0 < Entry.DirIdx && Entry.DirIdx <= Prologue.IncludeDirectories.size()) {
    Directory =
        dwarf::toString(Prologue.IncludeDirectories[Entry.DirIdx - 1], "");
    return true;
  }
  return false;
}
llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp
693

I debugged the code using the following DWARF test case:

0x0000000b: DW_TAG_compile_unit
0x0000002d:   DW_TAG_variable
0x00000043:   DW_TAG_base_type
0x0000004a:   DW_TAG_variable
0x00000060:   DW_TAG_subprogram
0x00000077:   DW_TAG_subprogram
0x00000092:     DW_TAG_formal_parameter
0x000000a1:     DW_TAG_formal_parameter
0x000000b0:     NULL
0x000000b1:   DW_TAG_subprogram
0x000000cf:   DW_TAG_subprogram
0x000000f5:     DW_TAG_formal_parameter
0x00000103:     DW_TAG_formal_parameter
0x00000111:     DW_TAG_variable
0x00000120:     DW_TAG_variable
0x0000012f:     DW_TAG_lexical_block
0x00000140:       DW_TAG_variable
0x0000014f:       DW_TAG_variable
0x0000015e:       NULL
0x0000015f:     NULL
0x00000160:   DW_TAG_subprogram
0x00000186:     DW_TAG_formal_parameter
0x00000195:     NULL
0x00000196:   DW_TAG_subprogram
0x000001b8:     DW_TAG_formal_parameter
0x000001c7:     NULL
0x000001c8:   NULL

These are the Lower and Upper values for each logical scope, in the order they are recorded in the compile unit.

Offset                                  Lower   Upper
0x00000060:   DW_TAG_subprogram         0x060   0x077
0x00000077:   DW_TAG_subprogram         0x077   0x0b0
0x000000b1:   DW_TAG_subprogram         0x0b1   0x0cf
0x0000012f:     DW_TAG_lexical_block    0x12f   0x15e
0x000000cf:   DW_TAG_subprogram         0x0cf   0x15f
0x00000160:   DW_TAG_subprogram         0x160   0x195
0x00000196:   DW_TAG_subprogram         0x196   0x1c7
0x0000000b: DW_TAG_compile_unit         0x00b   0x1c8

Looking at that table, it seems that Upper is including the size of the last child.
May be I am missing something?

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

Good point. Removed if.

Please upload the revised patch for final approval.

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

Thanks for mentioning std::merge and a possible model to refactor this method.

If is OK with you, I will differ the factoring for a later patch.
These are the cases to be considered during refactoring.

  • Merge instructions for objects with no comdat functions.
  • Merge instructions for objects with comdat functions.
  • Merge instructions for a fully linked binary.
  • Merge instructions when -ffunction-sections
602
for (LVLine *InstructionLine : InstructionLines) {
  if (TraverseLines) {
    while (Iter != DebugLines->end()) {
      if (InstructionAddress < DebugAddress) {
        ...
      }
    }
    if (Iter == DebugLines->end()) {
      TraverseLines = false;
      DebugLines->push_back(InstructionLine);
    }
  } else {
    DebugLines->push_back(InstructionLine);
  }
}

We are traversing the instructions and comparing the current InstructionAddress and DebugAddress. When we reach the end of the current DebugLines, we should stop any further comparison.
TraverseLines will indicate that the remaining Instructions must be added to the DebugLines.

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

Added at the top of the file:

// This implements the LVELFReader class.
// It supports ELF and Mach-O formats.
1186

Reprased the comment to:

// In the case of a Mach-O STAB symbol, get its section only if
// the STAB symbol's section field refers to a valid section index.
// Otherwise the symbol may error trying to load a section that
// does not exist.

As the method is used by ELF and Mach-O

if (Section == Obj.section_end())
      continue;

cover both cases.

1217

You are correct. The tool does not work for relocatable files compiled with -ffunction-sections.
I just tried that option and the tool generates incorrect instructions LVLineAssembler.
Added that support for a later patch.

Updated patch to address reviews from @probinson.

probinson accepted this revision.Oct 12 2022, 7:02 AM

LGTM, although I did notice one comment from @psamolysov that had not been answered.

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

This comment hasn't been answered.

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

No, my misunderstanding.

This revision is now accepted and ready to land.Oct 12 2022, 7:02 AM
llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp
289

@psamolysov I am sorry for missing this comment. I will answer it once I finish with the 09-codeview-reader patch.

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

Thanks

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

The best way to explain this code is by an example:

Using the LLVM 'count' testing tool executable
llvm-debuginfo-analyzer --print=lines,instructions count.exe

We traverse the collected public names to generate assembler instructions by calling createInstructions.

Collected public names: CompileUnit PublicNames

Range: [0x01400014c0:0x0140001760] Name: 'main' / ''
Range: [0x0140001820:0x0140001865] Name: '_vfprintf_l' / ''
Range: [0x0140001810:0x014000181a] Name: '__local_stdio_printf_options' / ''
Range: [0x0140001880:0x01400018f9] Name: 'fprintf' / ''

Code section information: SectionAddresses

[0x0140001000:0x0140008cdf] Size: 0x0000007cdf

While generating instructions for the public name main

'main' / 'main' Range: [0x01400014c0:0x0140001760]

Find an entry in SectionAddresses that contains its lower Address=0x01400014c0

Iter = SectionAddresses.lower_bound(Address);
if (Iter != SectionAddresses.begin())
  --Iter;

Iter is the end() iterator.
--Iter points to the entry: 0x0140001000 which is the object section that contains the code for main.

@psamolysov do you have any additional comments on this patch? I would like to land it. Thanks.

psamolysov added inline comments.Oct 25 2022, 4:15 AM
llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp
289

Thank you for the great explanation. I've checked from some sources (answers on SO but not the standard, unfortunately) that doing -- for end() is ok for bidirectional iterators. If the container is empty, end() will be equal to begin() and your code won't do -- for the iterator. So, I have no concerns. Thank you very much for the application.

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

Thanks for your very valuable feedback.

This revision was landed with ongoing or failed builds.Oct 26 2022, 9:39 PM
This revision was automatically updated to reflect the committed changes.

@psamolysov , @probinson, @MaskRay Thanks very much for all your valuable reviews.

This appears to have broken LLVM buildbot clang-armv8-quick https://lab.llvm.org/buildbot/#/builders/245/builds/146

This patch has also broken LLVM buildbot clang-aarch64-quick https://lab.llvm.org/buildbot/#/builders/188/builds/21397

I have not reverted it yet as it is part of large patch series.

This patch has also broken LLVM buildbot clang-aarch64-quick https://lab.llvm.org/buildbot/#/builders/188/builds/21397

I have not reverted it yet as it is part of large patch series.

@omjavaid Basically the test cases are platform dependent (requires a x86-registered-target).
There are 12 cases that failed:
I have a fix for 11: include the line REQUIRES: x86-registered-target
Working in the fix for the remaining one. If in the next hour I can't fix it, I will disable the test cases.
I hope it is OK with you.
Thanks

Landed a patch to check for the specific target.
https://reviews.llvm.org/D136825

thakis added a subscriber: thakis.Oct 27 2022, 5:48 AM
thakis added inline comments.
llvm/unittests/DebugInfo/LogicalView/CMakeLists.txt
9

See:

# Use for test binaries that call llvm::getInputFileDirectory(). Use of this
# is discouraged.
function(add_unittest_with_input_files test_suite test_name)

Any chance you could write this test without needing getInputFileDirectory()?

thakis added inline comments.Oct 27 2022, 5:49 AM
llvm/unittests/DebugInfo/LogicalView/CMakeLists.txt
3

(alphabetize?)

llvm/unittests/DebugInfo/LogicalView/CMakeLists.txt
3

There is only one remaining patch (09-codeview-reader) for the llvm-debuginfo-analyzer tool; it changes this file to include the CodeViewReaderTest.cpp.

I will address the (alphabetize?) issue in that patch.

9

There is only one remaining patch (09-codeview-reader) for the llvm-debuginfo-analyzer tool; it changes this file to include the CodeViewReaderTest.cpp which uses the getInputFileDirectory call.

I will explore the option to remove the call to getInputFileDirectory in that patch.

@jryans Thanks very much for reviewing the patches that fixed the test cases failures.

==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
    #3 0x560d275e05d8 in llvm::logicalview::LVELFReader::createScopes() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:936:21
    #4 0x560d27522372 in llvm::logicalview::LVReader::doLoad() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/DebugInfo/LogicalView/Core/LVReader.cpp:233:19
    #5 0x560d275be45e in llvm::logicalview::LVReaderHandler::createReader(llvm::StringRef, std::__1::vector<llvm::logicalview::LVReader*, std::__1::allocator<llvm::logicalview::LVReader*>>&, llvm::PointerUnion<llvm::object::ObjectFile*, llvm::pdb::PDBFile*>&, llvm::StringRef, llvm::StringRef) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:60:18
    #6 0x560d275c36fe in llvm::logicalview::LVReaderHandler::handleObject(std::__1::vector<llvm::logicalview::LVReader*, std::__1::allocator<llvm::logicalview::LVReader*>>&, llvm::StringRef, llvm::object::Binary&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:141:12
    #7 0x560d275c2951 in llvm::logicalview::LVReaderHandler::handleBuffer(std::__1::vector<llvm::logicalview::LVReader*, std::__1::allocator<llvm::logicalview::LVReader*>>&, llvm::StringRef, llvm::MemoryBufferRef, llvm::StringRef) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:92:10
    #8 0x560d275c42fb in llvm::logicalview::LVReaderHandler::handleFile(std::__1::vector<llvm::logicalview::LVReader*, std::__1::allocator<llvm::logicalview::LVReader*>>&, llvm::StringRef, llvm::StringRef) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:108:10
    #9 0x560d275bcf40 in createReader /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/DebugInfo/LogicalView/LVReaderHandler.h:78:12
    #10 0x560d275bcf40 in llvm::logicalview::LVReaderHandler::createReaders() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:159:21
    #11 0x560d275bcad4 in llvm::logicalview::LVReaderHandler::process() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:25:19
    #12 0x560d26e22f6c in main /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/tools/llvm-debuginfo-analyzer/llvm-debuginfo-analyzer.cpp:137:33
    #13 0x7fadb1ca7d8f  (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)

SUMMARY: AddressSanitizer: 1440 byte(s) leaked in 12 allocation(s).
vitalybuka reopened this revision.Oct 27 2022, 9:26 PM
This revision is now accepted and ready to land.Oct 27 2022, 9:26 PM
vitalybuka added inline comments.Oct 27 2022, 9:40 PM
llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp
384

Why this code does not use unique_ptr<> ?

vitalybuka added inline comments.Oct 27 2022, 9:45 PM
llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h
174
==1211325==ERROR: LeakSanitizer: detected memory leaks
SUMMARY: AddressSanitizer: 1440 byte(s) leaked in 12 allocation(s).

@vitalybuka Thanks very for the additional information for the failure.

CarlosAlbertoEnciso added a comment.EditedOct 27 2022, 11:33 PM

The following tests have been disabled for further investigation:

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

https://reviews.llvm.org/rGd81725d2bc016b58cb9327910f7892a2d4ac53c1

sanitizer-x86_64-linux-fast
https://lab.llvm.org/buildbot/#/builders/5/builds/28595

sanitizer-x86_64-linux-bootstrap-asan
https://lab.llvm.org/buildbot/#/builders/168/builds/9731

ERROR: LeakSanitizer: detected memory leaks
  • tools/llvm-debuginfo-analyzer/DWARF/06-dwarf-full-logical-view.test

https://reviews.llvm.org/rG5193d0a4a305d0dc8d611ade9f059fe904cf6e99

clang-armv7-vfpv3-2stage
https://lab.llvm.org/buildbot/#/builders/182/builds/4232

clang-armv7-global-isel
https://lab.llvm.org/buildbot/#/builders/186/builds/9519

clang-armv7-2stage
https://lab.llvm.org/buildbot/#/builders/187/builds/9483

Unexpected test output

Expected:
 189 (100.00%) : [0x000000000b][001]    {CompileUnit}
 110 ( 58.20%) : [0x000000002a][002] 2    {Function}
  27 ( 14.29%) : [0x0000000071][003]        {Block}

Generated:
3432 (  0.00%) : [0x000000000b][001]    {CompileUnit}
3351 (  0.00%) : [0x000000002a][002] 2    {Function}
3234 (  0.00%) : [0x0000000071][003]        {Block}
llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h
174

Thanks for the link. Added to the list of improvements.
The list will be added as 'README.txt' in llvm\tools\llvm-debuginfo-analyzer

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

The memory management issue has been already discussed in the reviews and it would be addressed once the series of patches are landed.

A bit concerned about two things that happened here (though, apologies as I haven't otherwise been following the review):

  1. disabling tests - generally code is reviewed with the tests, disabling the tests renders the review approval non-standing because the code is now not covered by tests. Usually the solution is to revert the patch while addressing issues like this.
  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?)

I understand this is a complex/large patch series, but these are the sort of reasons they are generally discourageed - because it's expensive for both reviewers and authors to do quality review when lots of code is already written on top of the patches being reviewed (you get that pushback of "but if I fix this, all the other patches are broken/need fixes, and that's expensive").

This is also broken on i386 (so perhaps on all 32-bit platforms?):

FAIL: LLVM :: tools/llvm-debuginfo-analyzer/DWARF/06-dwarf-full-logical-view.test (40612 of 44599)
******************** TEST 'LLVM :: tools/llvm-debuginfo-analyzer/DWARF/06-dwarf-full-logical-view.test' FAILED ********************
Script:
--
: 'RUN: at line 24';   /var/tmp/portage/sys-devel/llvm-16.0.0_pre20221029/work/llvm_build-abi_x86_32.x86/bin/llvm-debuginfo-analyzer --attribute=all                          --print=all                          /var/tmp/portage/sys-devel/llvm-16.0.0_pre20221029/work/llvm/test/tools/llvm-debuginfo-analyzer/DWARF/Inputs/test-dwarf-clang.o 2>&1 |  /var/tmp/portage/sys-devel/llvm-16.0.0_pre20221029/work/llvm_build-abi_x86_32.x86/bin/FileCheck --strict-whitespace -check-prefix=ONE /var/tmp/portage/sys-devel/llvm-16.0.0_pre20221029/work/llvm/test/tools/llvm-debuginfo-analyzer/DWARF/06-dwarf-full-logical-view.test
--
Exit Code: 1

Command Output (stderr):
--
+ : 'RUN: at line 24'
+ /var/tmp/portage/sys-devel/llvm-16.0.0_pre20221029/work/llvm_build-abi_x86_32.x86/bin/FileCheck --strict-whitespace -check-prefix=ONE /var/tmp/portage/sys-devel/llvm-16.0.0_pre20221029/work/llvm/test/tools/llvm-debuginfo-analyzer/DWARF/06-dwarf-full-logical-view.test
+ /var/tmp/portage/sys-devel/llvm-16.0.0_pre20221029/work/llvm_build-abi_x86_32.x86/bin/llvm-debuginfo-analyzer --attribute=all --print=all /var/tmp/portage/sys-devel/llvm-16.0.0_pre20221029/work/llvm/test/tools/llvm-debuginfo-analyzer/DWARF/Inputs/test-dwarf-clang.o
/var/tmp/portage/sys-devel/llvm-16.0.0_pre20221029/work/llvm/test/tools/llvm-debuginfo-analyzer/DWARF/06-dwarf-full-logical-view.test:103:20: error: ONE-NEXT: expected string not found in input
; ONE-NEXT:        189 (100.00%) : [0x000000000b][001]              {CompileUnit} 'test.cpp'
                   ^
<stdin>:75:13: note: scanning from here
Scope Sizes:
            ^
<stdin>:76:8: note: possible intended match here
       189 (  0.00%) : [0x000000000b][001]              {CompileUnit} 'test.cpp'
       ^

Input file: <stdin>
Check file: /var/tmp/portage/sys-devel/llvm-16.0.0_pre20221029/work/llvm/test/tools/llvm-debuginfo-analyzer/DWARF/06-dwarf-full-logical-view.test

-dump-input=help explains the following input dump.

Input was:
<<<<<<
            .
            .
            .
           70: Types            5          5 
           71: Lines           25         25 
           72: ----------------------------- 
           73: Total           37         37 
           74:  
           75: Scope Sizes: 
next:103'0                 X error: no match found
           76:        189 (  0.00%) : [0x000000000b][001]              {CompileUnit} 'test.cpp' 
next:103'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
next:103'1            ?                                                                          possible intended match
           77:        110 ( -0.00%) : [0x000000002a][002]      2         {Function} extern not_inlined 'foo' -> [0x0000000099]'int' 
next:103'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           78:         27 (-26815615859885194199148049996411692254958731641184786755447122887443528060147093953603748596333806855380063716372972101707507765623893139892867298012168192.00%) : [0x0000000071][003]                  {Block} 
next:103'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           79:  
next:103'0     ~
           80: Totals by lexical level: 
next:103'0     ~~~~~~~~~~~~~~~~~~~~~~~~~
           81: [001]:        189 (100.00%) 
next:103'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
            .
            .
            .
>>>>>>

--

********************

This is also broken on i386 (so perhaps on all 32-bit platforms?):

Input was:
           76:        189 (  0.00%) : [0x000000000b][001]              {CompileUnit} 'test.cpp' 
           77:        110 ( -0.00%) : [0x000000002a][002]      2         {Function} extern not_inlined 'foo' -> [0x0000000099]'int' 
           78:         27 (-26815615859885194199148049996411692254958731641184786755447122887443528060147093953603748596333806855380063716372972101707507765623893139892867298012168192.00%) : [0x0000000071][003]                  {Block}

That failure is the same as the one detected on ARM: https://reviews.llvm.org/rG5193d0a4a305d0dc8d611ade9f059fe904cf6e99.

As you are building on i386, can I have the CMake configuration?
Locally I build on Ubuntu 20.04.5 LTS (64-bit). I can try to build a 32-bit version (LLVM_BUILD_32_BITS=on).
Thanks

A bit concerned about two things that happened here (though, apologies as I haven't otherwise been following the review):

Very much appreciated your input and I fully understand your concerns about code reviews and large/complex patches.

  1. disabling tests - generally code is reviewed with the tests, disabling the tests renders the review approval non-standing because the code is now not covered by tests. Usually the solution is to revert the patch while addressing issues like this.

There are 2 specific cases that are failing:

  • Memory leak detected by the sanitizers: tools/llvm-debuginfo-analyzer/DWARF/pr-incorrect-logical-instructions.test

Currently I am working on a fix which I hope to submit in the next couple of days.

  • unexpected output: tools/llvm-debuginfo-analyzer/DWARF/06-dwarf-full-logical-view.test

The test fails on some ARM configurations and i386 (32-bit) as reported by @mgorny.
From an early analysis based on the error, the code associated with --print=sizes fails.
I can enable the test and change --print=all which is equivalent to --print=elements,warnings,summary,sizes to elements,warnings,summary and open a GitHub issue.

  1. 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.

I understand this is a complex/large patch series, but these are the sort of reasons they are generally discourageed - because it's expensive for both reviewers and authors to do quality review when lots of code is already written on top of the patches being reviewed (you get that pushback of "but if I fix this, all the other patches are broken/need fixes, and that's expensive").

Thanks for your understanding on the nature of the patches.

As you are building on i386, can I have the CMake configuration?

Just to be clear, I'm building 32-bit multilib on amd64. The full config we're using is:

cmake -C /var/tmp/portage/sys-devel/llvm-16.0.0_pre20221029/work/llvm_build-abi_x86_32.x86/gentoo_common_config.cmake -G Ninja -DCMAKE_INSTALL_PREFIX=/usr -DLLVM_APPEND_VC_REV=OFF -DCMAKE_INSTALL_PREFIX=/usr/lib/llvm/16 -DLLVM_LIBDIR_SUFFIX= -DBUILD_SHARED_LIBS=OFF -DLLVM_BUILD_LLVM_DYLIB=ON -DLLVM_LINK_LLVM_DYLIB=ON -DLLVM_DISTRIBUTION_COMPONENTS=LLVM;LTO;Remarks;llvm-config;cmake-exports;llvm-headers;LLVMDemangle;LLVMSupport;LLVMTableGen; -DLLVM_TARGETS_TO_BUILD= -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD=SPIRV;X86;ARM;MSP430;SystemZ;Mips;AMDGPU;PowerPC;Sparc;M68k;CSKY;BPF;Hexagon;XCore;AVR;VE;WebAssembly;NVPTX;DirectX;Lanai;ARC;LoongArch;RISCV;AArch64 -DLLVM_BUILD_TESTS=yes -DLLVM_ENABLE_FFI=yes -DLLVM_ENABLE_LIBEDIT=yes -DLLVM_ENABLE_TERMINFO=yes -DLLVM_ENABLE_LIBXML2=yes -DLLVM_ENABLE_ASSERTIONS=no -DLLVM_ENABLE_LIBPFM=yes -DLLVM_ENABLE_EH=ON -DLLVM_ENABLE_RTTI=ON -DLLVM_ENABLE_Z3_SOLVER=yes -DLLVM_ENABLE_ZSTD=yes -DLLVM_HOST_TRIPLE=i686-pc-linux-gnu -DFFI_INCLUDE_DIR=/usr/lib/libffi/include  -DFFI_LIBRARY_DIR= -DLLVM_HAVE_LIBXAR=0 -DPython3_EXECUTABLE=/usr/bin/python3.11 -DOCAMLFIND=NO -DLLVM_VERSION_SUFFIX=gitc4a8f9ab -DLLVM_LIT_ARGS=-vv;-j;12 -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_TOOLCHAIN_FILE=/var/tmp/portage/sys-devel/llvm-16.0.0_pre20221029/work/llvm_build-abi_x86_32.x86/gentoo_toolchain.cmake /var/tmp/portage/sys-devel/llvm-16.0.0_pre20221029/work/llvm

gentoo_common_config is:

set(CMAKE_GENTOO_BUILD ON CACHE BOOL "Indicate Gentoo package build")
set(LIB_SUFFIX  CACHE STRING "library path suffix" FORCE)
set(CMAKE_INSTALL_LIBDIR lib CACHE PATH "Output directory for libraries")
set(CMAKE_INSTALL_INFODIR "/usr/share/info" CACHE PATH "")
set(CMAKE_INSTALL_MANDIR "/usr/share/man" CACHE PATH "")
set(CMAKE_USER_MAKE_RULES_OVERRIDE "/var/tmp/portage/sys-devel/llvm-16.0.0_pre20221023/work/llvm_build-abi_x86_32.x86/gentoo_rules.cmake" CACHE FILEPATH "Gentoo override rules")
set(CMAKE_INSTALL_DOCDIR "/usr/share/doc/llvm-16.0.0_pre20221023" CACHE PATH "")
set(BUILD_SHARED_LIBS ON CACHE BOOL "")
set(CMAKE_INSTALL_ALWAYS 1)
set(CMAKE_ASM_FLAGS_RELWITHDEBINFO "" CACHE STRING "")
set(CMAKE_ASM-ATT_FLAGS_RELWITHDEBINFO "" CACHE STRING "")
set(CMAKE_C_FLAGS_RELWITHDEBINFO "" CACHE STRING "")
set(CMAKE_CXX_FLAGS_RELWITHDEBINFO "" CACHE STRING "")
set(CMAKE_Fortran_FLAGS_RELWITHDEBINFO "" CACHE STRING "")
set(CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO "" CACHE STRING "")
set(CMAKE_MODULE_LINKER_FLAGS_RELWITHDEBINFO "" CACHE STRING "")
set(CMAKE_SHARED_LINKER_FLAGS_RELWITHDEBINFO "" CACHE STRING "")
set(CMAKE_STATIC_LINKER_FLAGS_RELWITHDEBINFO "" CACHE STRING "")

gentoo_toolchain.cmake is:

set(CMAKE_ASM_COMPILER "x86_64-pc-linux-gnu-gcc;-m32 -mfpmath=sse")
set(CMAKE_ASM-ATT_COMPILER "x86_64-pc-linux-gnu-gcc;-m32 -mfpmath=sse")
set(CMAKE_C_COMPILER "x86_64-pc-linux-gnu-gcc;-m32 -mfpmath=sse")
set(CMAKE_CXX_COMPILER "x86_64-pc-linux-gnu-g++;-m32 -mfpmath=sse")
set(CMAKE_Fortran_COMPILER "x86_64-pc-linux-gnu-gfortran;-m32 -mfpmath=sse")
set(CMAKE_AR /usr/bin/x86_64-pc-linux-gnu-ar CACHE FILEPATH "Archive manager" FORCE)
set(CMAKE_RANLIB /usr/bin/x86_64-pc-linux-gnu-ranlib CACHE FILEPATH "Archive index generator" FORCE)
set(CMAKE_SYSTEM_PROCESSOR "i686")

But I can also reproduce in my working copy that's configured as:

CC=i686-pc-linux-gnu-gcc CXX=i686-pc-linux-gnu-g++ cmake ../llvm -G Ninja -DBUILD_SHARED_LIBS=ON -DCMAKE_BUILD_TYPE=MinSizeRel -DLLVM_TARGETS_TO_BUILD=X86 -DCLANG_ENABLE_ARCMT=OFF -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_CCACHE_BUILD=ON -DLLVM_ENABLE_PROJECTS='llvm' -DLLVM_BUILD_TESTS=OFF -DLLVM_HOST_TRIPLE=i686-pc-linux-gnu

where i686-pc-linux-gnu-* wrappers simply call the respective GCC executables with -m32.

As you are building on i386, can I have the CMake configuration?

Just to be clear, I'm building 32-bit multilib on amd64.
But I can also reproduce in my working copy that's configured as:

CC=i686-pc-linux-gnu-gcc CXX=i686-pc-linux-gnu-g++ cmake ../llvm -G Ninja -DBUILD_SHARED_LIBS=ON -DCMAKE_BUILD_TYPE=MinSizeRel -DLLVM_TARGETS_TO_BUILD=X86 -DCLANG_ENABLE_ARCMT=OFF -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_CCACHE_BUILD=ON -DLLVM_ENABLE_PROJECTS='llvm' -DLLVM_BUILD_TESTS=OFF -DLLVM_HOST_TRIPLE=i686-pc-linux-gnu

where i686-pc-linux-gnu-* wrappers simply call the respective GCC executables with -m32.

@mgorny Thanks very much for the command lines.

After installing gcc-i686-linux-gnu and using

CC='/usr/bin/i686-linux-gnu-gcc -m32' CXX='/usr/bin/i686-linux-gnu-g++ -m32' cmake ../llvm -GNinja -DBUILD_SHARED_LIBS=OFF -DCMAKE_BUILD_TYPE=MinSizeRel -DLLVM_TARGETS_TO_BUILD=X86 -DCLANG_ENABLE_ARCMT=OFF -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_CCACHE_BUILD=ON -DLLVM_ENABLE_PROJECTS="llvm" -DLLVM_BUILD_TESTS=OFF -DLLVM_HOST_TRIPLE=i686-pc-linux-gnu

I can reproduce the problem with

llvm-debuginfo-analyzer --attribute=offset,level --print=scopes,sizes ../llvm/test/tools/llvm-debuginfo-analyzer/DWARF/Inputs/test-dwarf-clang.o
Logical View:
[0x0000000000][000]           {File} 'test-dwarf-clang.o'

[0x000000000b][001]             {CompileUnit} 'test.cpp'
[0x000000002a][002]     2         {Function} extern not_inlined 'foo' -> [0x0000000099]'int'
[0x0000000071][003]                 {Block}

Scope Sizes:
       189 (  0.00%) : [0x000000000b][001]             {CompileUnit} 'test.cpp'
       110 ( -0.00%) : [0x000000002a][002]     2         {Function} extern not_inlined 'foo' -> [0x0000000099]'int'
        27 (-26815615859885194199148049996411692254958731641184786755447122887443528060147093953603748596333806855380063716372972101707507765623893139892867298012168192.00%) : [0x0000000071][003]                 {Block}

Totals by lexical level:
[001]:        189 (100.00%)
[002]:        110 ( 58.20%)
[003]:         27 ( 14.29%)

Expect output

Logical View:
[0x0000000000][000]           {File} 'test-dwarf-clang.o'

[0x000000000b][001]             {CompileUnit} 'test.cpp'
[0x000000002a][002]     2         {Function} extern not_inlined 'foo' -> [0x0000000099]'int'
[0x0000000071][003]                 {Block}

Scope Sizes:
       189 (100.00%) : [0x000000000b][001]             {CompileUnit} 'test.cpp'
       110 ( 58.20%) : [0x000000002a][002]     2         {Function} extern not_inlined 'foo' -> [0x0000000099]'int'
        27 ( 14.29%) : [0x0000000071][003]                 {Block}

Totals by lexical level:
[001]:        189 (100.00%)
[002]:        110 ( 58.20%)
[003]:         27 ( 14.29%)
CarlosAlbertoEnciso added a comment.EditedNov 1 2022, 6:23 AM

The test case tools/llvm-debuginfo-analyzer/DWARF/pr-incorrect-logical-instructions.test was disabled due to memory leak detected by the sanitizers.

Uploaded patch for review: https://reviews.llvm.org/D137156 that fixes that issue and enables the test.

Failures before the patch:

sanitizer-x86_64-linux-fast: https://lab.llvm.org/buildbot/#/builders/5/builds/28595
sanitizer-x86_64-linux-bootstrap-asan: https://lab.llvm.org/buildbot/#/builders/168/builds/9731

Successful builds after the patch:
sanitizer-x86_64-linux-fast: https://lab.llvm.org/buildbot/#/builders/5/builds/28776
sanitizer-x86_64-linux-bootstrap-asan: https://lab.llvm.org/buildbot/#/builders/168/builds/9850

The test case tools/llvm-debuginfo-analyzer/DWARF/06-dwarf-full-logical-view.test was disabled due to unexpected output:

Uploaded patch for review: https://reviews.llvm.org/D137242 that removes the option to print the sizes.

llvm/unittests/DebugInfo/LogicalView/CMakeLists.txt
3

An updated patch for 09-codeview-reader https://reviews.llvm.org/D125784 includes this change.

9

I was not able to rewrite the test without using getInputFileDirectory. Looked at other test cases that include a binary file and they use the same approach.

thakis added inline comments.Nov 2 2022, 10:59 AM
llvm/unittests/DebugInfo/LogicalView/CMakeLists.txt
9

Any chance you could use a lit test instead of a unit test for this then?

dblaikie added inline comments.Nov 2 2022, 11:12 AM
llvm/unittests/DebugInfo/LogicalView/CMakeLists.txt
9

Yeah, seems like either a lit test (if this library is predominantly/sufficiently exposed via a command line utility - we generally test via lit tests on that corresponding command line utility (eg: other libraries in lib/DebugInfo are tested via llvm-dwarfdump and llvm-symbolizer, mostly))

Alternatively, if there's nuanced uses of the library not readily accessible via the command line (or easier to test once at a lower layer) - like some parts of libDebugInfoDWARF - are tested via unit tests that generate the DWARF they're testing within the unit test, rather than depending on a checked in binary. (see DWARFGenerator usage in the unit tests fo rexamples of this)

mgorny added a comment.Nov 4 2022, 1:29 AM

Submitted D137400 as the correct fix for 32-bit platforms. However, the code seems to mix integer types freely, so I suspect there could be more subtle breakage there.

CarlosAlbertoEnciso edited the summary of this revision. (Show Details)Nov 14 2022, 4:03 AM
llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp
384

@vitalybuka: A new patch has been uploaded that addresses the memory management issues: https://reviews.llvm.org/D137933 (10 Smart pointers). With that patch the tool uses only smart pointers.

llvm/unittests/DebugInfo/LogicalView/CMakeLists.txt
9

Created https://reviews.llvm.org/D144857 that contains the README for the tool.

It contains notes collected during the development, review and test.
It describes limitations, know issues and future work.

The next patch will address the issue for the unit test.

https://reviews.llvm.org/D144857 added the README for the tool.

  • Contains notes collected during the development, review and test.
  • Describes limitations, know issues and future work.