Page MenuHomePhabricator

Fix ClangASTContext::CreateParameterDeclaration to not call addDecl
ClosedPublic

Authored by shafik on Jul 29 2019, 1:09 PM.

Details

Summary

The change D55575 modified ClangASTContext::CreateParameterDeclaration to call decl_ctx->addDecl(decl); this caused a regression since the existing code in DWARFASTParserClang::ParseChildParameters is called with the containing DeclContext:

m_ast.CreateParameterDeclaration(containing_decl_ctx, name,
                                 type->GetForwardCompilerType(),
                                 storage);

So when end up with cases where we are parsing a parameter for a member function and the parameter is added to the CXXRecordDecl as opposed to the CXXMethodDecl. This example is given in the regression test TestBreakpointInMemberFuncWNonPrimitiveParams.py which without this fix in a modules build leads to assert on setting a breakpoint in a member function with non primitive parameters. This scenario would be common when debugging LLDB or clang.

This leaves a the existing issue which D55571 tried to fix but apparently did not fix the issue or at least not totally, since there are no tests added with the PR it is hard to tell.

Diff Detail

Repository
rL LLVM

Event Timeline

shafik created this revision.Jul 29 2019, 1:09 PM

Looks fine to me. Be nice to clarify the test with comments a bit. I can't really tell what the test is doing or how it is verifying the fix in ClangASTContext.cpp

So basically this is reverting an untested and incomplete/incorrect fix for another bug, while fixing a regression. I'm fine with the change assuming that it won't break the Windows test suite. The Makefile can be simplified though.

packages/Python/lldbsuite/test/lang/cpp/breakpoint_in_member_func_w_non_primitive_params/Makefile
5 ↗(On Diff #212212)

This looks like it is copied and pasted form another testcase and not actually necessary.

What you want is

CFLAGS_EXTRAS = $(MANDATORY_CXXMODULE_BUILD_CFLAGS)

and then you should be able to delete all lines starting with $(CXX) because the default build rules should just work.

packages/Python/lldbsuite/test/lang/cpp/breakpoint_in_member_func_w_non_primitive_params/TestBreakpointInMemberFuncWNonPrimitiveParams.py
10 ↗(On Diff #212212)

This shouldn't be necessary. We want this everywhere where gmodules is supported, right?

shafik updated this revision to Diff 212231.Jul 29 2019, 2:39 PM

Addressing comments:

  • Simplifying Makefile
  • Adding comments to the test

@aprantl @clayborg Thank you for the comments, I think I have addressed your concerns.

shafik marked 2 inline comments as done.Jul 29 2019, 3:26 PM
shafik updated this revision to Diff 212243.Jul 29 2019, 3:50 PM

Simplifying Makefile even more

aprantl accepted this revision.Jul 29 2019, 3:54 PM
This revision is now accepted and ready to land.Jul 29 2019, 3:54 PM

@stella.stamenova this could potentially break the windows build, could you please verify before I land this change. Thank you in advance!

I'll run some tests and let you know the result.

It might be good to see if you can trigger the bug more directly (for instance by getting the SBType for the method and pulling out its arguments?) The reason that a breakpoint triggers this at present is that lldb reads the debug info for the function to find the prologue extents so it can more accurately push the breakpoint past the prologue. But that's not really desirable, it is a lot of work just for setting a breakpoint, and if I ever figure out how not to do that, I will. In which case, this test will no longer test what you think it tests.

One failure:

Error : no member named 'argc' in namespace '$__lldb_local_vars'

from LLDB :: SymbolFile/NativePDB/local-variables.cpp.

@stella.stamenova that is unfortunate but not surprising. I don't have a way to test a fix locally. Is there anyone who might be able to help me iterate over a fix or maybe a new maintainer of the PDB parsing?

I think @amccarth kind of inherited the PDB stuff from Zach, but if you just need to try out a patch on windows, I can do that too. I was hoping I would be able to create a core file which you could open up on your end and reproduce the problem yourself, but unfortunately it seems that the Native PDB reader does not interact very well with core (minidump) files. This is a use case that both I and Adrian are interested in, so we will make that work eventually, but that might take a while...

I am able to do some testing as well.

shafik updated this revision to Diff 212884.Aug 1 2019, 12:41 PM

Fix that should fix the failing PDB test.

@labath @stella.stamenova I updated the diff with a fix that I believe should address the test failure.

This is an unfortunate difference in how DWARF and PDB works. After spending some time digging into this I don't see the key difference that allows DWARF to work w/o addDecl. I believe further work along the lines of D55571 may help but my first attempts at fixing this for DWARF were not successful.

aprantl added inline comments.Aug 1 2019, 5:46 PM
source/Symbol/ClangASTContext.cpp
2229 ↗(On Diff #212884)

Please make sure to document why this flag is necessary and especially, what needs to be fixed in order to make it unnecessary again.

@labath @stella.stamenova I updated the diff with a fix that I believe should address the test failure.

This is an unfortunate difference in how DWARF and PDB works. After spending some time digging into this I don't see the key difference that allows DWARF to work w/o addDecl. I believe further work along the lines of D55571 may help but my first attempts at fixing this for DWARF were not successful.

@stella.stamenova, was the comment in <D55571#1611200> meant for this patch?

Yes, it was! Thanks for catching that :)

The tests pass with the latest update.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2019, 2:43 PM