This is an archive of the discontinued LLVM Phabricator instance.

[llvm-debuginfo-analyzer] 05 - Select elements
ClosedPublic

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

Diff Detail

Build Status
Buildable 188372

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2022, 6:45 AM
CarlosAlbertoEnciso requested review of this revision.May 17 2022, 6:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2022, 6:45 AM
CarlosAlbertoEnciso edited the summary of this revision. (Show Details)May 17 2022, 7:09 AM
CarlosAlbertoEnciso removed a project: Restricted Project.
CarlosAlbertoEnciso removed subscribers: mgorny, hiraditya.
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2022, 7:09 AM
CarlosAlbertoEnciso edited the summary of this revision. (Show Details)May 18 2022, 1:34 AM

Good job! Here is a series of comments, please have a look if you don't mind.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVElement.h
65

Could you add #include <map> and #include <vector> to make the header more self-contained?

llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h
18–19

Do you mean LVObject.h? Currently it looks like a self-inclusion.

453

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

471

Maybe it makes sense to rename the alias into LVMatchOffsets with s because the code already has LVOffset alias for just uint64_t and it could get the reader in doubt why an instance of LVMatchOffset is a heavy object.

491

What about to add an assert to check that Element is not null?

503

I see no modifications of Dispatch in the function.

504
516

What about to add an assert to check that Element is not null? Also it looks like the Element could have the const T * type.

532

What about to add an assert to check that Element is not null?

533

The lambda captures this and a pointer (Line) only, it makes sense to capture this by reference (implicitly) and Line by value: auto checkPattern = [=]() -> bool { (will this ought to be replaced with [=, this] when LLVM switches on C++ 20?)

538

The previous comment for checkPattern could be applied here too.

562

A typo? = delete should be?

598

Would const LVMatchInfo &MatchInfo work?

603

The LVObject.h header defines an alias LVOffset for uint64_t.

647

I believe this pair of brackets is not needed.

llvm/lib/DebugInfo/LogicalView/Core/LVOptions.cpp
447

Patterns has not been changed in the method.

448
509
llvm/unittests/DebugInfo/LogicalView/SelectElementsTest.cpp
54

An std::bad_alloc will be thrown if the test is out of memory. It makes sense to wrap the line in the try-catch block or use the non-throwing overload of the new operator.

93

It would be better to use EXPECT_EQ on the objects of Element->getName() and Name because the .data() member of StringRef doesn't guarantee that the returned char array will be null-terminated.

277–278

If I get it right, getIsMatched() returns a bool value. GTest's EXPECT_TRUE and EXPECT_FALSE can handle bool values better.

369
llvm/include/llvm/DebugInfo/LogicalView/Core/LVElement.h
65

Included <map> and <vector>.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h
18–19

Very good catch.
LVObject.h it is not needed as it is included by any of the logical elements.

453

Added final.

471

Renamed to LVMatchOffsets.

491

Added assert(Element && "Element must not be nullptr");

503

Added const.

504

Added const.

516

Added assert(Element && "Element must not be nullptr");

516

I can add the const qualifier, but as side effect a lot of functions needs to be modified as they are not expected a const. They are indirectly called by addElement(Element).

532

Added assert(Line && "Line must not be nullptr");

533

Thanks for the explanation.

Changed to auto CheckPattern = [=]() -> bool {

538

Changed to auto CheckOffset = [=]() -> bool {

562

Good catch. Changed to = delete.

598

Added const.

603

Changed.

647

Removed the wrapping brackets.

llvm/lib/DebugInfo/LogicalView/Core/LVOptions.cpp
447

Added const.

448

Replaced.

509

Added const.

llvm/unittests/DebugInfo/LogicalView/SelectElementsTest.cpp
54

Changed to use the non-throwing overload.

93

Changed to EXPECT_EQ(Element->getName(), Name);

277–278

Replaced with EXPECT_TRUE and EXPECT_FALSE.

369

Fix typo.

  • Addressed the reviewer’s feedback.
  • Tool renamed as: llvm-debuginfo-analyzer.
CarlosAlbertoEnciso retitled this revision from [llvm-dva] 05 - Select elements to [llvm-debuginfo-analyzer] 05 - Select elements.Jul 25 2022, 2:18 AM
CarlosAlbertoEnciso edited the summary of this revision. (Show Details)
probinson added inline comments.Sep 19 2022, 11:28 AM
llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h
556

No need to repeat public:

634

No need to repeat public:

llvm/include/llvm/DebugInfo/LogicalView/Core/LVSymbol.h
162

No need to repeat public:

llvm/unittests/DebugInfo/LogicalView/SelectElementsTest.cpp
55

Use ASSERT_NE to bail out of the test if the allocation fails.

Update patch due to changes in previous patches.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h
556

Removed public:.

634

Removed public:.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVSymbol.h
162

Removed public:.

llvm/unittests/DebugInfo/LogicalView/SelectElementsTest.cpp
55

ASSERT_NE can be used only inside functions that don't return a value.

See https://google.github.io/googletest/advanced.html#assertion-placement

The one constraint is that assertions that generate a fatal failure (FAIL* and ASSERT_*) can only be used in void-returning functions.

Looks like the patch needs to be updated with the latest changes.

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

No need to repeat public:

llvm/include/llvm/DebugInfo/LogicalView/Core/LVType.h
116

No need to repeat public:

Address points raised by @probinson.

probinson accepted this revision.Sep 30 2022, 7:32 AM

LGTM, with just a few remaining nits about public:

llvm/include/llvm/DebugInfo/LogicalView/Core/LVLine.h
88

Repeated public:

llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h
168

Here and above, more duplicate public:

This revision is now accepted and ready to land.Sep 30 2022, 7:32 AM
llvm/include/llvm/DebugInfo/LogicalView/Core/LVLine.h
88

Removed the repeated public:.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h
168

Removed the repeated public:.

Updated patch:

  • Removes some redundant public: modifiers.
This revision was landed with ongoing or failed builds.Oct 20 2022, 9:55 PM
This revision was automatically updated to reflect the committed changes.

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

The following patch fixed some buildbot sanitizer issues: https://reviews.llvm.org/D136444

CarlosAlbertoEnciso edited the summary of this revision. (Show Details)Nov 14 2022, 4:02 AM