Page MenuHomePhabricator

Fix GetDIEForDeclContext so it only returns entries matching the provided context
ClosedPublic

Authored by guiandrade on Aug 16 2019, 11:05 AM.

Details

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.

Event Timeline

guiandrade created this revision.Aug 16 2019, 11:05 AM
guiandrade added a comment.EditedAug 16 2019, 11:06 AM

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 requested changes to this revision.Aug 16 2019, 11:23 AM

Needs a test, but looks good.

This revision now requires changes to proceed.Aug 16 2019, 11:23 AM
guiandrade added a comment.EditedAug 17 2019, 11:48 AM

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!

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.

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

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!

Pavel, any comments on the testing code? LGTM.

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 marked 3 inline comments as done.

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 accepted this revision.Aug 22 2019, 3:17 PM

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.

This revision is now accepted and ready to land.Aug 22 2019, 3:17 PM

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
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 29 2019, 8:31 AM