Fix ClangASTContext::CreateParameterDeclaration to not call addDecl

[shafik]

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.

[clayborg]

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

[aprantl]

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]

Addressing comments:

• Simplifying Makefile
• Adding comments to the test

[shafik]

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

[shafik]

Simplifying Makefile even more

[shafik]

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

[stella.stamenova]

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

[jingham]

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.

[stella.stamenova]

One failure:

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

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

[shafik]

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

[labath]

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

[stella.stamenova]

I am able to do some testing as well.

[shafik]

Fix that should fix the failing PDB test.

[shafik]

@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]

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]

In D65414#1610923, @shafik wrote:

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

[stella.stamenova]

Yes, it was! Thanks for catching that :)

The tests pass with the latest update.

lldb/trunk/include/lldb/Symbol/ClangASTContext.h

CompilerType CreateFunctionType(const CompilerType &result_type,
     return ClangASTContext::CreateFunctionType(getASTContext(), result_type,
                                                args, num_args, is_variadic,
                                                type_quals, cc);
   }
 
   clang::ParmVarDecl *CreateParameterDeclaration(clang::DeclContext *decl_ctx,
                                                  const char *name,
                                                  const CompilerType &param_type,
-                                                 int storage);
+                                                 int storage,
+                                                 bool add_decl=false);
 
   void SetFunctionParameters(clang::FunctionDecl *function_decl,
                              clang::ParmVarDecl **params, unsigned num_params);
 
   CompilerType CreateBlockPointerType(const CompilerType &function_type);
 
   // Array Types
 

lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/breakpoint_in_member_func_w_non_primitive_params/Makefile

+LEVEL = ../../../make
+
+CXX_SOURCES = main.cpp a.cpp
+CFLAGS_EXTRAS = $(MANDATORY_CXXMODULE_BUILD_CFLAGS)
+
+include $(LEVEL)/Makefile.rules

lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/breakpoint_in_member_func_w_non_primitive_params/TestBreakpointInMemberFuncWNonPrimitiveParams.py

+"""
+This is a regression test for an assert that happens while setting a breakpoint.
+The root cause of the assert was attempting to add a ParmVarDecl to a CXXRecordDecl
+when it should have been added to a CXXMethodDecl.
+
+We can reproduce with a module build and setting a breakpoint in a member function
+of a class with a non-primitive type as a parameter.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestBreakpointInMemberFuncWNonPrimitiveParams(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @add_test_categories(["gmodules"])
+    def test_breakpint_in_member_func_w_non_primitie_params(self):
+        self.build()
+
+        (self.target, self.process, _, bkpt) = lldbutil.run_to_source_breakpoint(self, '// break here',
+                lldb.SBFileSpec("main.cpp", False))
+
+        self.runCmd("b a.cpp:11");

lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/breakpoint_in_member_func_w_non_primitive_params/a.h

+struct A {
+  bool b(int x);
+};
+
+struct B {
+  bool member_func_a(A a);
+};

lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/breakpoint_in_member_func_w_non_primitive_params/a.cpp

+#include "a.h"
+
+bool A::b(int x) {
+  if (x)
+    return true;
+
+  return false;
+}
+
+bool B::member_func_a(A a) {
+  return a.b(10); // We will try and add a breakpoint here which
+                  // trigger an assert since we will attempt to
+                  // to add ParamVarDecl a to CXXRecordDecl A
+};

lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/breakpoint_in_member_func_w_non_primitive_params/main.cpp

+#include "a.h"
+#include <cstdio>
+
+bool foo() {
+  A a1;
+  B b1;
+
+  return b1.member_func_a(a1); // break here
+}
+
+int main() {
+  int x = 0;
+
+  return foo();
+}

lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/breakpoint_in_member_func_w_non_primitive_params/module.modulemap

+module A {
+  header "a.h"
+}

lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp

while (begin != end && param_count > 0) {
     }
 
     PdbCompilandSymId param_uid(func_id.modi, record_offset);
     clang::QualType qt = GetOrCreateType(param_type);
 
     CompilerType param_type_ct(&m_clang, qt.getAsOpaquePtr());
     clang::ParmVarDecl *param = m_clang.CreateParameterDeclaration(
         &function_decl, param_name.str().c_str(), param_type_ct,
-        clang::SC_None);
+        clang::SC_None, true);
     lldbassert(m_uid_to_decl.count(toOpaqueUid(param_uid)) == 0);
 
     m_uid_to_decl[toOpaqueUid(param_uid)] = param;
     params.push_back(param);
     --param_count;
   }
 
   if (!params.empty())

lldb/trunk/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp

if (std::unique_ptr<PDBSymbolTypeFunctionSig> sig = func->getSignature()) {
         while (std::unique_ptr<PDBSymbolTypeFunctionArg> arg =
                    arg_enum->getNext()) {
           Type *arg_type = symbol_file->ResolveTypeUID(arg->getTypeId());
           if (!arg_type)
             continue;
 
           clang::ParmVarDecl *param = m_ast.CreateParameterDeclaration(
               decl, nullptr, arg_type->GetForwardCompilerType(),
-              clang::SC_None);
+              clang::SC_None, true);
           if (param)
             params.push_back(param);
         }
       }
     }
     if (params.size())
       m_ast.SetFunctionParameters(decl, params.data(), params.size());
 

lldb/trunk/source/Symbol/ClangASTContext.cpp

CompilerType ClangASTContext::CreateFunctionType(
 
   return CompilerType(ast,
                       ast->getFunctionType(ClangUtil::GetQualType(result_type),
                                            qual_type_args, proto_info));
 }
 
 ParmVarDecl *ClangASTContext::CreateParameterDeclaration(
     clang::DeclContext *decl_ctx, const char *name,
-    const CompilerType &param_type, int storage) {
+    const CompilerType &param_type, int storage, bool add_decl) {
   ASTContext *ast = getASTContext();
   assert(ast != nullptr);
   auto *decl =
       ParmVarDecl::Create(*ast, decl_ctx, SourceLocation(), SourceLocation(),
                           name && name[0] ? &ast->Idents.get(name) : nullptr,
                           ClangUtil::GetQualType(param_type), nullptr,
                           (clang::StorageClass)storage, nullptr);
+  if (add_decl)
     decl_ctx->addDecl(decl);
+
   return decl;
 }
 
 void ClangASTContext::SetFunctionParameters(FunctionDecl *function_decl,
                                             ParmVarDecl **params,
                                             unsigned num_params) {
   if (function_decl)
     function_decl->setParams(ArrayRef<ParmVarDecl *>(params, num_params));