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

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
62

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

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

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

452

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

470

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.

490

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

502

I see no modifications of Dispatch in the function.

503
515

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.

531

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

532

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?)

537

The previous comment for checkPattern could be applied here too.

561

A typo? = delete should be?

597

Would const LVMatchInfo &MatchInfo work?

602

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

646

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
53

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.

92

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.

276–277

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

368
llvm/include/llvm/DebugInfo/LogicalView/Core/LVElement.h
62

Included <map> and <vector>.

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

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

452

Added final.

470

Renamed to LVMatchOffsets.

490

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

502

Added const.

503

Added const.

515

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

515

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

531

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

532

Thanks for the explanation.

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

537

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

561

Good catch. Changed to = delete.

597

Added const.

602

Changed.

646

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
53

Changed to use the non-throwing overload.

92

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

276–277

Replaced with EXPECT_TRUE and EXPECT_FALSE.

368

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
555

No need to repeat public:

633

No need to repeat public:

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

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
555

Removed public:.

633

Removed public:.

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

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
166

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
166

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