Page MenuHomePhabricator

[llvm-dva] 08 - ELF Reader
Needs ReviewPublic

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

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2022, 6:49 AM
CarlosAlbertoEnciso requested review of this revision.May 17 2022, 6:49 AM
Herald added a project: Restricted Project. · View Herald Transcript
CarlosAlbertoEnciso edited the summary of this revision. (Show Details)May 17 2022, 7:12 AM
CarlosAlbertoEnciso removed a reviewer: jdoerfert.
CarlosAlbertoEnciso removed a project: Restricted Project.
Herald added a project: Restricted Project. · View Herald Transcript
CarlosAlbertoEnciso edited the summary of this revision. (Show Details)May 18 2022, 1:35 AM

Here are a series of comments to the new patch. The most comments are just nits (replacing map#insert with map#emplace but sometimes a piecewise construct could make sense however; and manually constructing by [const] Map::value_type & references to a map entry in foreach loops with the already defined in the std::map class template reference and const_reference aliases. Also a couple of possible memory leaks and bugs related to handling Map.end() iterator have been found, have a look, please.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h
90
149–150

Should both member functions be const?

154

This function looks like const too.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h
489

These pairs: Scope/LVNameInfo and LVNameInfo (LVAddress/uint64_t) are relatively small but it could be more effective do not copy at least the first pair: PublicNames.emplace(Scope, LVNameInfo(LowPC, HightPC - LowPC)); or do not copy all the pairs:

PublicNames.emplace(std::piecewise_construct,
  std::forward_as_tuple(Scope),
  std::forward_as_tuple(LowPC, HighPC - LowPC));
llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h
164
190
197
247

Just a nit. return Path; should work too but I haven't checked.

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

The struct should either be marked as final or have a virtual destructor.

48

The class should either be marked as final or have a virtual destructor.

87
154
156

delete a null pointer is not an issue, so the assert could be superfluous.

llvm/include/llvm/DebugInfo/LogicalView/Readers/LVELFReader.h
101
132–133

Just a nit. In the copy constructor a west-const is used while in the copy assignment operation an east-const. Also there are some east-const for return values (in LVSymbols const &GetSymbolsWithLocations() const member for example). Could a single style be chosen?

llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp
1269–1271

Here could be a bug: if Map->upper_bound(Address) returns Map->end(), Map->end() is definitely not equal to Map->begin(), Iter changes it's value to an element previous to the end and the check Iter != Map->end() is forever be true.

1435
1438
llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp
133

Should here be a check that ArchiveOrErr is an error? Without the check, all the code on lines 135-137 looks like a dead code.

165

Why not just TheReaders.push_back? Readers will always have zero or one element as long as the createReader function creates exactly one reader.

llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp
28–29

Just a recommendation, I'm not sure the proposed code is more readable or gives a large optimization but from another point of view it eliminates copying of a struct of 18-24 bytes (LVSymbolTableEntry).

47

The previous comment about a std::picewise_construct construction could be applied here as well.

159
178
191
289

Should the Iter be checked that it is not SectionAddresses.end()? If the Iter is the end() iterator, --Iter just returns it to a "normal" value and all the checks for end in the callers (if these checks are there) will positive (sometimes false positive).

313
421

An allocated via new line - Line - is added to the Instructions vector. Then, the Instructions vector is moved into the ScopeInstructions class member but elements of the ScopeInstructions has never been deleted using delete (or I just miss it). This looks like a memory leak.

441
465
710
718
734
llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp
250

It looks like the lambda captures nothing, so [&] could be replaced with [].

251
254

It looks like the lambda captures nothing.

463
541

Just a nit, if you find your variant more readable, no problem.

724

An allocated in the heap LVLineDebug is appended into the CULines vector. The vector then goes into the processLines function and is cleared in the LVELFReader::createScopes() member function. I cannot find any place where the allocated Line is deleted.

856

It this correct that CULines (a class member) is cleared after each iteration over CompileUnits? If there are numerous CompileUnits, CULines will just be cleared after the first iteration.

868
1020
1028
llvm/unittests/DebugInfo/LogicalView/ELFReaderTest.cpp
42–43

It could be better to check that the iterator Iter is not null before to dereference it on the previous line.

70
80

It could make sense to check pointers on the nullptr in ASSERT_NE instead of EXPECT_NE. If a pointer is null, the test will just crash on the first dereference of the pointer.

142–143

It could be better to swap these lines.

147–148

It could be better to swap these lines.

243

I believe this should work as well as for the SmallVector<N>/SmallVectorImpl pair.

267
298
331

SmallStringImpl InputDir also should works but I haven't checked.