Fix GetDIEForDeclContext so it only returns entries matching the provided context

[guiandrade]

Summary

Currently, we return all the entries such that their decl_ctx pointer >= decl_ctx provided.
Instead, we should return only the ones that decl_ctx pointer == decl_ctx provided.

[guiandrade]

This is a follow up on the investigation I mentioned in http://lists.llvm.org/pipermail/lldb-dev/2019-August/015324.html.
Please let me know if you guys think this makes sense. Thanks!

[clayborg]

Needs a test, but looks good.

[guiandrade]

In D66357#1633461, @clayborg wrote:

Needs a test, but looks good.

Sure, I agree. Though, it's not total clear to me how we could do that.

I imagined something like the following.

TEST_F(DWARFASTParserClangTests,
       TestGetDIEForDeclContextReturnsOnlyMatchingEntries) {
  auto ast = ClangASTContext(...);
  auto ast_parser = DWARFASTParserClang(ast);
  CompilerDeclContext decl1(nullptr, (void *)1LL);
  CompilerDeclContext decl2(nullptr, (void *)2LL);
  CompilerDeclContext decl3(nullptr, (void *)2LL);
  CompilerDeclContext decl4(nullptr, (void *)3LL);
  ast_parser->LinkDeclContextToDIE(decl1, DWARFDIE());
  ast_parser->LinkDeclContextToDIE(decl2, DWARFDIE());
  ast_parser->LinkDeclContextToDIE(decl3, DWARFDIE());
  ast_parser->LinkDeclContextToDIE(decl4, DWARFDIE());

  auto decl_ctx_die_list = ast_parser->GetDIEForDeclContext(decl2);
  EXPECT_EQ(2u, decl_ctx_die_list.size());
  EXPECT_EQ(decl2.GetOpaqueDeclContext(),
            decl_ctx_die_list[0].GetDeclContext().GetOpaqueDeclContext());
  EXPECT_EQ(decl3.GetOpaqueDeclContext(),
            decl_ctx_die_list[1].GetDeclContext().GetOpaqueDeclContext());
}

But DWARFASTParserClang::LinkDeclContextToDIE is protected. Do you have any ideas to overcome that?

Thanks!

[clayborg]

In D66357#1634175, @guiandrade wrote:

In D66357#1633461, @clayborg wrote:

Needs a test, but looks good.

Sure, I agree. Though, it's not total clear to me how we could do that.

I imagined something like the following.

TEST_F(DWARFASTParserClangTests,
       TestGetDIEForDeclContextReturnsOnlyMatchingEntries) {
  auto ast = ClangASTContext(...);
  auto ast_parser = DWARFASTParserClang(ast);
  CompilerDeclContext decl1(nullptr, (void *)1LL);
  CompilerDeclContext decl2(nullptr, (void *)2LL);
  CompilerDeclContext decl3(nullptr, (void *)2LL);
  CompilerDeclContext decl4(nullptr, (void *)3LL);
  ast_parser->LinkDeclContextToDIE(decl1, DWARFDIE());
  ast_parser->LinkDeclContextToDIE(decl2, DWARFDIE());
  ast_parser->LinkDeclContextToDIE(decl3, DWARFDIE());
  ast_parser->LinkDeclContextToDIE(decl4, DWARFDIE());

  auto decl_ctx_die_list = ast_parser->GetDIEForDeclContext(decl2);
  EXPECT_EQ(2u, decl_ctx_die_list.size());
  EXPECT_EQ(decl2.GetOpaqueDeclContext(),
            decl_ctx_die_list[0].GetDeclContext().GetOpaqueDeclContext());
  EXPECT_EQ(decl3.GetOpaqueDeclContext(),
            decl_ctx_die_list[1].GetDeclContext().GetOpaqueDeclContext());
}

But DWARFASTParserClang::LinkDeclContextToDIE is protected. Do you have any ideas to overcome that?

Feel free to relax that function to be public or to add a testing helper function that is public if needed. If you make it public, please put a comment on the function to indicate it was done for testing purposes.

[labath]

If you're in a position to control how the class gets created, you can just make a new test class which inherits from DWARFASTParserClang and increases the visibility of that method. (Though I am somewhat doubtful you'll be able to construct this class without any of the other machinery around.)

[guiandrade]

I tried to write a unit test following @labath's suggestion of creating a test class that inherits from DWARFASTParserClang. Please let me know what you guys think.

Thanks!

[clayborg]

Pavel, any comments on the testing code? LGTM.

[labath]

In D66357#1635851, @clayborg wrote:

Pavel, any comments on the testing code?

I am worried that this approach of using pointers into thin air instead of real objects (which admittedly, would be hard to create/mock) is going to make this test fragile in face of future modifications of this code (if, e.g., something suddenly decides it needs to dereference these pointers).

However, given that this patch is (if I understand correctly) fixing a performance bug, and not an actual correctness bug, and we don't really have a framework for testing performance, I would be fine with accepting the patch without a test, and so I suppose I would be also fine with just deleting this test if/when it starts to become a maintenance burden...

lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp

18–19 ↗

(On Diff #215966)

If you're not going to be putting any code here, you can just drop the fixture and use TEST instead of TEST_F below.

32–36 ↗

(On Diff #215966)

This seems to be written in the "always use auto" style. That is definitely not consistent with the llvm coding style http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable, as there's a shorter, more traditional way to write this, and the style guide explicitly rejects "always use auto".

44 ↗

(On Diff #215966)

s/EXPECT/ASSERT

[guiandrade]

I'm fixing the style issues pointed by @labath, but I acknowledge the flakiness of this test the way it is right now and am open to deleting it. I can also try to spend some time trying to implement it in a more proper manner. It's your call, @clayborg.

Thanks!

[clayborg]

Now that obj2yaml is a library, we might be able to make some real DWARF to exercise this? I can custom make any DWARF you want as I have a DWARF generator.

Though this test does verify that the searching process works correctly. Not sure how maintenance free this will be over time though. But I am fine with this for now and we can fix it later if it becomes an issue.

[guiandrade]

Sounds good, thank you!

Can someone please merge this for me?

ninja check-lldb
Testing Time: 163.56s
  Expected Passes    : 1603
  Expected Failures  : 4
  Unsupported Tests  : 85

lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

#endif
   }
 
   return false;
 }
 
 std::vector<DWARFDIE> DWARFASTParserClang::GetDIEForDeclContext(
     lldb_private::CompilerDeclContext decl_context) {
   std::vector<DWARFDIE> result;
-  for (auto it = m_decl_ctx_to_die.find(
-           (clang::DeclContext *)decl_context.GetOpaqueDeclContext());
-       it != m_decl_ctx_to_die.end(); it++)
+  auto opaque_decl_ctx =
+      (clang::DeclContext *)decl_context.GetOpaqueDeclContext();
+  for (auto it = m_decl_ctx_to_die.find(opaque_decl_ctx);
+       it != m_decl_ctx_to_die.end() && it->first == opaque_decl_ctx; it++)
     result.push_back(it->second);
   return result;
 }
 
 CompilerDecl DWARFASTParserClang::GetDeclForUIDFromDWARF(const DWARFDIE &die) {
   clang::Decl *clang_decl = GetClangDeclForDIE(die);
   if (clang_decl != nullptr)
     return CompilerDecl(&m_ast, clang_decl);

lldb/trunk/unittests/SymbolFile/DWARF/CMakeLists.txt

 add_lldb_unittest(SymbolFileDWARFTests
+  DWARFASTParserClangTests.cpp
   SymbolFileDWARFTests.cpp
 
   LINK_LIBS
     lldbCore
     lldbHost
     lldbSymbol
     lldbPluginObjectFilePECOFF
     lldbPluginSymbolFileDWARF

lldb/trunk/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp

+//===-- DWARFASTParserClangTests.cpp ----------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "gtest/gtest.h"
+
+#include "Plugins/SymbolFile/DWARF/DWARFASTParserClang.h"
+#include "Plugins/SymbolFile/DWARF/DWARFDIE.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+namespace {
+class DWARFASTParserClangStub : public DWARFASTParserClang {
+public:
+  using DWARFASTParserClang::DWARFASTParserClang;
+  using DWARFASTParserClang::LinkDeclContextToDIE;
+};
+} // namespace
+
+// If your implementation needs to dereference the dummy pointers we are
+// defining here, causing this test to fail, feel free to delete it.
+TEST(DWARFASTParserClangTests,
+     TestGetDIEForDeclContextReturnsOnlyMatchingEntries) {
+  ClangASTContext ast_ctx;
+  DWARFASTParserClangStub ast_parser(ast_ctx);
+
+  DWARFUnit *unit = nullptr;
+  DWARFDIE die1(unit, (DWARFDebugInfoEntry *)1LL);
+  DWARFDIE die2(unit, (DWARFDebugInfoEntry *)2LL);
+  DWARFDIE die3(unit, (DWARFDebugInfoEntry *)3LL);
+  DWARFDIE die4(unit, (DWARFDebugInfoEntry *)4LL);
+  ast_parser.LinkDeclContextToDIE((clang::DeclContext *)1LL, die1);
+  ast_parser.LinkDeclContextToDIE((clang::DeclContext *)2LL, die2);
+  ast_parser.LinkDeclContextToDIE((clang::DeclContext *)2LL, die3);
+  ast_parser.LinkDeclContextToDIE((clang::DeclContext *)3LL, die4);
+
+  auto die_list = ast_parser.GetDIEForDeclContext(
+      CompilerDeclContext(nullptr, (clang::DeclContext *)2LL));
+  ASSERT_EQ(2u, die_list.size());
+  ASSERT_EQ(die2, die_list[0]);
+  ASSERT_EQ(die3, die_list[1]);
+}