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
38

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

111

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
43

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
94

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

107

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

154

Unexpected file type is success?

172

The for and first if can be swapped.

llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp
272
347
348
349
413

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.

512
512

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

601

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

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

Maybe a comment here?

48

Isn't CurrentSymbol currently nullptr ?

440

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

461

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.

593

overrite should be override? or overwrite?

692

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

729

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.

795

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

862

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

958

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.

1049

Maybe processMemberLocation ?

1169

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.

1185

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.

1215
1216

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
llvm/test/tools/llvm-debuginfo-analyzer/DWARF/05-dwarf-incorrect-lexical-scope-variable.test
25

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

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

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
40

Changed to Mach-O.

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

Changed.

347

Changed.

348

Changed.

349

Changed.

413

Good point. Added your suggested comment.

623

Changed.

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

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

Yes. To simplify changed to

return nullptr;
593

Changed to override.

729

Added your suggested comment.

759

Changed.

760

Changed.

762

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

Changed.

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

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

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

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
81

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
152

You are correct. Marked LVBinaryReader destructor as virtual.

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

Renamed as processLocationMember.

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

Applied correct alignment.

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

Nice suggestion. Removed TheFilename.

107

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
795

Nice suggestion. Removed TheFilename.

862

Renamed as DeduceIncrementFileIndex.

958

Renamed as processLocationList.

1049

Renamed as processMemberLocation.

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

Changed DIDumpOptions DumpOpts; to a local variable.

43

Changed DWARFDie DummyDie; to a local variable.

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

Changed.

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

Good point. Changed.

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

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
154

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());
172

Nice point. Swapped.

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

You are correct. Changed.

1216

Changed.

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

Changed prettyPrintRegisterOp as a DWARFExpression static public member.

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

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
403

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
221

Good point. Added to list for later refactor.

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

Good point. Added to list for later refactor.

152

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

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

Removed else after return.

1435

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
692

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
764

Good point. Removed if.

Please upload the revised patch for final approval.

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

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
601
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
1169

Added at the top of the file:

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

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.

1216

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
290

This comment hasn't been answered.

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

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
290

@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
692

Thanks

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

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
290

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
290

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
181
==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
181

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.