This is an archive of the discontinued LLVM Phabricator instance.

[llvm-debuginfo-analyzer] 07 - Compare elements
ClosedPublic

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

Diff Detail

Event Timeline

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

Good job! I have a few most stylistic only comments here. What I would like to discuss with the other reviewers: should an assert be used in every function that accepts an argument by pointer and dereferences the pointer or calls functions on it (so, dereferences it too)? My personal opinion: yes but there can be other opinions too, so I do not insist.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVCompare.h
29

Either the class should be marked as final or has a virtual destructor.

63

I see a call to this static member function in the LVCompare::execute only. Could the static member function be private or protected?

73

emplace_back creates the instance of the LVPassEntry class in place.

llvm/lib/DebugInfo/LogicalView/Core/LVCompare.cpp
68
213–215
288
llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp
870–871

Could the method just return the inverted if's condition instead of using the if statement?

877

Is Entry a reference to a pointer?

1055

I believe this is a good idea to always use assert on a pointer if there is a dereference of or call a method of in the function to be sure the pointer is not null. The LLVM Coding Standards guideline recommends to use asserts liberally: "Check all of your preconditions and assumptions, you never know when a bug (not necessarily even yours) might be caught early by an assertion, which reduces debugging time dramatically."

1777–1778

Just a nit. What about to combine the conditions together by &&? I personally think it would be more readable than nested if statements.

1802–1803

Just a nit. What about to combine the conditions together by &&? I personally think it would be more readable than nested if statements.

1933–1934

Just a nit. What about to combine the conditions together by &&? I personally think it would be more readable than nested if statements.

llvm/lib/DebugInfo/LogicalView/Core/LVSymbol.cpp
363–364

Just a nit. What about to combine the conditions together by &&? I personally think it would be more readable than nested if statements.

llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp
515–518

I believe this if statement can be converted to return of the inverted condition.

llvm/unittests/DebugInfo/LogicalView/CompareElementsTest.cpp
69
llvm/include/llvm/DebugInfo/LogicalView/Core/LVCompare.h
29

Added final.

63

Marking setInstance as private.

73

Replaced with emplace_back.

llvm/lib/DebugInfo/LogicalView/Core/LVCompare.cpp
68

Nice change.

213–215

Replaced with emplace.

288

Nice change.

llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp
1777–1778

Replaced the nested if with a combined conditions &&.

1802–1803

Replaced the nested if with a combined conditions &&.

1933–1934

Replaced the nested if with a combined conditions &&.

llvm/lib/DebugInfo/LogicalView/Core/LVSymbol.cpp
363–364

Replaced the nested if with a combined conditions &&.

llvm/unittests/DebugInfo/LogicalView/CompareElementsTest.cpp
69

Changed to use the non-throwing overload.

llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp
870–871

Eliminated the if and return an inverted condition.

877

Your change is correct.

1055

Very good point.
Added assert(Scopes && "Scopes must not be nullptr"); in few other places as well.

llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp
515–518

Very good simplification.

  • Addressed the reviewer’s feedback.
  • Tool renamed as: llvm-debuginfo-analyzer.
CarlosAlbertoEnciso retitled this revision from [llvm-dva] 07 - Compare elements to [llvm-debuginfo-analyzer] 07 - Compare elements.Jul 25 2022, 2:19 AM
CarlosAlbertoEnciso edited the summary of this revision. (Show Details)

For the problems described in https://github.com/llvm/llvm-project/issues/57040:

  • Add additional debugging traces.
probinson added inline comments.Sep 20 2022, 10:01 AM
llvm/include/llvm/DebugInfo/LogicalView/Core/LVCompare.h
36
37
76

Returning a std::vector by value seems inefficient.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVElement.h
345
llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h
300

Does this comment apply to all the methods in this block of code? It seems like not, in which case it would be good to put a blank line before the declarations that it does not apply to. (Or even better, comments for each of the other methods!)

307

No need to repeat public:

probinson added inline comments.Sep 20 2022, 10:01 AM
llvm/include/llvm/DebugInfo/LogicalView/Core/LVCompare.h
63

No need to repeat public:

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

The comment does not describe the second overload; put in a blank line, or preferably a comment for the second one.

652

Comment does not apply to the second overload.

692

Comment does not apply to the second overload.

730

Comment does not apply to the second overload.

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

Comment does not apply to the second overload.
Also curious that this time the second overload is static when for other classes it is not.

183

no need to keep repeating public:

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

Comment does not apply to the second overload.
Which is also static ?

llvm/lib/DebugInfo/LogicalView/Core/LVCompare.cpp
36

These appear to be global functions; constexpr implies inline but not static.
They should be static or inside the anonymous namespace.

49

CurrentComparator is a global variable, not even inside any namespace. Could it be a static member inside LVCompare?

llvm/lib/DebugInfo/LogicalView/Core/LVLine.cpp
78
if (!Targets)
  return nullptr;

allows the rest of the function to be less indented.

103

Could this method be marked const ?

llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp
826

Can reduce indentation by starting with

if (!Targets)
  return nullptr;
861
900

Could simplify slightly by starting with

if (!(References && Targets))
  return;
917

If you don't simplify as suggested above, this if needs braces around the for per LLVM style.

966

Could this method be marked const ?

1048

This sequence looks a bit strange; if neither one is named, compare the filenames? If that's correct maybe the comment should say "file name" instead of "full name"?

1054

The name equals implies a straightforward equality test, which is not really what this method does. In fact it's a little hard for me to describe what it does... looks through a given set of Scopes to find one that equals the current Scope? So maybe findEqualScope would be a better name?

1055

When pointers cannot be null it's usually better practice to pass a reference instead. I understand that this would be a fairly extensive API change, so I won't insist.

1309–1325

Not sure what "Notify the Reader if element comparison" means. It looks like what the code does is "Notify the Reader of the new element."

1807

Same comment as LVScopeAggregate::equals

1876

Same comment as LVScopeAggregate::equals

1939

Same comment as LVScopeAggregate::equals

llvm/lib/DebugInfo/LogicalView/Core/LVSymbol.cpp
285

Could simplify/reduce indentation by starting with

if (!(References && Targets))
  return;
311

Could reduce indentation by starting with

if (!Targets)
  return nullptr;
llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp
175

Could simplify by doing this first:

if (!(References && Targets))
  return;
201

Reduce indentation with

if (!Targets)
  return nullptr;
llvm/unittests/DebugInfo/LogicalView/CompareElementsTest.cpp
70

Use ASSERT_NE so the test will abort if allocation fails.

Update patch due to changes in previous patches.

Updated patch due to changes in previous patches.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVCompare.h
36

Changed.

37

Changed.

63

Removed redundant public:.

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

Changed to virtual void report(LVComparePass Pass) {}

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

Added comments to each method.

307

Removed redundant public:.

351

Added comment to second method.

652

Added comment to second method.

692

Added comment to second method.

730

Added comment to second method.

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

Remove redundand public:.

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

Added comments to second method.

All bool equals(const LVAbc *References, const LVAbc *Targets) methods in all the classes are static.

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

Added comment to second method.

All bool equals(const LVAbc *References, const LVAbc *Targets) methods in all the classes are static.

llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp
861

Moved the comment to the include file and changed the comment.

1054
  • Renamed method to findEqualScope.
  • Added comment to method declarations.
// For the given 'Scopes' returns a scope that is logically equal
// to the current scope; otherwise 'nullptr'.
1807
  • Renamed method to findEqualScope.
  • Added comment to method declarations.
// For the given 'Scopes' returns a scope that is logically equal
// to the current scope; otherwise 'nullptr'.
1876
  • Renamed method to findEqualScope.
  • Added comment to method declarations.
// For the given 'Scopes' returns a scope that is logically equal
// to the current scope; otherwise 'nullptr'.
1939
  • Renamed method to findEqualScope.
  • Added comment to method declarations.
// For the given 'Scopes' returns a scope that is logically equal
// to the current scope; otherwise 'nullptr'.
llvm/lib/DebugInfo/LogicalView/Core/LVLine.cpp
78

Changed.

llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp
826

Changed.

900

Changed to your suggestion.
Removed the extra check condition.

917

Changed to the previous suggestion.

llvm/lib/DebugInfo/LogicalView/Core/LVSymbol.cpp
285

Changed to your suggestion.
Removed the extra check condition.

311

Changed.

llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp
175

Changed to your suggestion.
Removed the extra check condition.

201

Changed.

llvm/lib/DebugInfo/LogicalView/Core/LVCompare.cpp
36

Moved to the anonymous namespace.

49

Moved to the anonymous namespace.

llvm/lib/DebugInfo/LogicalView/Core/LVLine.cpp
103

The method is static.

llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp
966

The method is static.

1055

Good point. Added to the improvements list.

1309–1325

During comparison notify the Reader of the new element.
`

llvm/unittests/DebugInfo/LogicalView/CompareElementsTest.cpp
70

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.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVCompare.h
76

Good point. Changed.

llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp
1048

The sequence is correct. That covers the case of unnamed union/struct/class, We use the filename index to determine if they are the same.
Added // In the case of unnamed union/structure/class compare the file name.

Updated patch to address reviews from @probinson.

This revision is now accepted and ready to land.Oct 5 2022, 7:34 AM
psamolysov added inline comments.Oct 5 2022, 7:50 AM
llvm/include/llvm/DebugInfo/LogicalView/Core/LVCompare.h
76

What about to add & after const in method declaration to make the method callable on lvalues only? If a method that returns a reference is callable on an rvalue ("temporary"), a returned reference can become dangling immediately after destroying the temporary.

llvm/include/llvm/DebugInfo/LogicalView/Core/LVCompare.h
76

Nice suggestion. Changed to

const LVPassTable &getPassTable() const & { return PassTable; }
This revision was landed with ongoing or failed builds.Oct 24 2022, 12:29 AM
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/D136593

kda added a subscriber: kda.Oct 24 2022, 9:19 AM

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

It does not seem to have fixed them all...

https://lab.llvm.org/buildbot/#/builders/168
https://lab.llvm.org/buildbot/#/builders/5/builds/28512

These are still broken claiming this target is the trouble:

Failed Tests (1):
  LLVM-Unit :: DebugInfo/LogicalView/./DebugInfoLogicalViewTests/8/15

Appears to be a leak:

==1565271==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 208 byte(s) in 1 object(s) allocated from:
    #0 0x56055b3457fd in operator new(unsigned long, std::nothrow_t const&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/compiler-rt/lib/asan/asan_new_delete.cpp:101:3
    #1 0x56055b38aaf1 in llvm::logicalview::LVScopeFunction* (anonymous namespace)::ReaderTestCompare::create<llvm::logicalview::LVScopeFunction, void (llvm::logicalview::LVScope::*)()>(void (llvm::logicalview::LVScope::*)()) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/unittests/DebugInfo/LogicalView/CompareElementsTest.cpp:68:18
    #2 0x56055b3812fa in (anonymous namespace)::ReaderTestCompare::createElements() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/unittests/DebugInfo/LogicalView/CompareElementsTest.cpp:142:7
    #3 0x56055b3794ae in (anonymous namespace)::LogicalViewTest_CompareElements_Test::TestBody() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/unittests/DebugInfo/LogicalView/CompareElementsTest.cpp:437:21
    #4 0x56055b63dd6c in HandleExceptionsInMethodIfSupported<testing::Test, void> /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc
    #5 0x56055b63dd6c in testing::Test::Run() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2508:5
    #6 0x56055b64052c in testing::TestInfo::Run() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2684:11
    #7 0x56055b64192f in testing::TestSuite::Run() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2816:28
    #8 0x56055b66c6b1 in testing::internal::UnitTestImpl::RunAllTests() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:5338:44
    #9 0x56055b66b3c1 in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc
    #10 0x56055b66b3c1 in testing::UnitTest::Run() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4925:10
    #11 0x56055b62164a in RUN_ALL_TESTS /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:2472:46
    #12 0x56055b62164a in main /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/UnitTestMain/TestMain.cpp:55:10
    #13 0x7f066136ed8f  (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)
CarlosAlbertoEnciso added a comment.EditedOct 24 2022, 11:12 AM

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

It does not seem to have fixed them all...

https://lab.llvm.org/buildbot/#/builders/168
https://lab.llvm.org/buildbot/#/builders/5/builds/28512

@kda Commited the fix. Sorry for any inconvenience.

kda added a comment.Oct 24 2022, 11:38 AM

These are still broken claiming this target is the trouble:

Failed Tests (1):
  LLVM-Unit :: DebugInfo/LogicalView/./DebugInfoLogicalViewTests/8/15

Appears to be a leak:

Some of it can be cleaned up by deleting CompileUnit, but there is other issues: see comment in test source.

The memory ownership model is pretty unusual here. It seems that this would benefit from more use of unique_ptr to automate ownership (destruction).
Without a fix, this will probably need to be rolled back.

llvm/unittests/DebugInfo/LogicalView/CompareElementsTest.cpp
212

In the case of line 437 (below), FunctionTwo is unowned and is not deleted.
I don't know if it is even used when IsTarget is false, but it is allocated and is a leak.

I see this behavior repeated for IsReference.

CarlosAlbertoEnciso added a comment.EditedOct 24 2022, 12:28 PM

Some of it can be cleaned up by deleting CompileUnit, but there is other issues: see comment in test source.

The memory leak is caused by those Child that are not added to a Parent. The other allocated items are removed when the Reader is destroyed.

The memory ownership model is pretty unusual here. It seems that this would benefit from more use of unique_ptr to automate ownership (destruction).

One of the areas discussed in the reviews for the tool llvm-debuginfo-analyzer is the use unique_ptr, which will be addressed once all the patches are commited.

Without a fix, this will probably need to be rolled back.

For these builds
https://lab.llvm.org/buildbot/#/builders/168
https://lab.llvm.org/buildbot/#/builders/5/builds/28512
the issues are in stage2/asan check

With the fix
https://lab.llvm.org/buildbot/#/builders/168/builds/9681
https://lab.llvm.org/buildbot/#/builders/5/builds/28517
the step stage2/asan check passed.

https://lab.llvm.org/buildbot/#/builders/5/builds/28517 - SUCCESS.

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