[ast] CreateParameterDeclaration should use an appropriate DeclContext.

[zturner]

Summary

Previously CreateParameterDeclaration was always using the translation unit DeclContext. We would later go and add parameters to the FunctionDecl, but internally clang makes a copy when you do this, and we'd end up with ParmVarDecl's at the global scope as well as in the function scope.

This fixes the issue. It's hard to say whether this will introduce a behavioral change in name lookup, but I know there have been several hacks introduced in previous years to deal with collisions between various types of variables, so there's a chance that this patch could obviate one of those hacks.

[clayborg]

Looks good to me. Jim should ok as well.

[jingham]

This change looks good to me too.

I don't think this will fix the parameter/local variable lookup inversion that we've been hacking around. The problem comes because to make an JITted object we can easily call we make a wrapper function with a simple argument list - that makes running it easy but means it doesn't share the original function's context. Then we smuggle all the local Decls into the wrapper function using the FindExternalLexicalDecls from our ClangASTSource. Unfortunately, that lookup gets consulted after the namespace/class lookup is done. So names in the current class or namespace context shadow local variables/arguments.

This will clean up the AST Context's we make but sadly I don't think it will affect the expression parser name lookup issues. If you wanted to test that proposition, you can do:

settings set target.experimental.inject-local-vars false

then try some tests where a local variable or parameter name shadows an ivar or type name in the current context. That will find the wrong variable when this is false. I'm sure there are also tests for this, but I don't remember where they are off-hand.

[friss]

Zachary, how did you figure out this can be an issue? Does it fix something we should be testing?

[zturner]

In D55571#1330354, @friss wrote:

Zachary, how did you figure out this can be an issue? Does it fix something we should be testing?

Recently I added a target modules dump ast command and I've been using it to write tests against (see lldb/lit/SymbolFile/NativePDB for an example of what some of these tests look like). While experimenting with this command, I noticed that I had some ParmVarDecls under the translation unit decl (e.g. global scope), which obviously is something that isn't possible. I only tested this with the PDB parser, so I can't confirm whether the same situation could occur int he DWARF parser, but since this is strictly building the AST *after* we've parsed the debug info, I definitely think it would be a problem for DWARF.

So the million dollar question is: What happens when you have function parameters in the AST at global scope? I think it falls into "undefined behavior" territory - i.e. there's no way to know what clang will do, since you're not supposed to have that situation.

As for as testing, I've found this command extremely useful for writing ast reconstruction tests -- Do a few things in the debugger, run the command, FileCheck the output. I think "did we construct a valid AST from the debug info?" is a largely unexplored testing surface in LLDB, so this definitely opens up a lot of possibilities for testing on this front.

[stella.stamenova]

Test passes now. Thanks!

include/lldb/Symbol/ClangASTContext.h

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

source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

case DW_TAG_formal_parameter: {
         }
 
         if (!skip) {
           Type *type = die.ResolveTypeUID(DIERef(param_type_die_form));
           if (type) {
             function_param_types.push_back(type->GetForwardCompilerType());
 
             clang::ParmVarDecl *param_var_decl =
-                m_ast.CreateParameterDeclaration(
-                    name, type->GetForwardCompilerType(), storage);
+                m_ast.CreateParameterDeclaration(containing_decl_ctx, name,
+                                                 type->GetForwardCompilerType(),
+                                                 storage);
             assert(param_var_decl);
             function_param_decls.push_back(param_var_decl);
 
             m_ast.SetMetadataAsUserID(param_var_decl, die.GetID());
           }
         }
       }
       arg_idx++;

source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

case S_BLOCK32:
       continue;
     default:
       continue;
     }
 
     PdbCompilandSymId param_uid(func_id.modi, record_offset);
     TypeSP type_sp = GetOrCreateType(param_type);
     clang::ParmVarDecl *param = m_clang->CreateParameterDeclaration(
-        param_name.str().c_str(), type_sp->GetForwardCompilerType(),
-        clang::SC_None);
+        function_decl, param_name.str().c_str(),
+        type_sp->GetForwardCompilerType(), clang::SC_None);
     lldbassert(m_uid_to_decl.count(toOpaqueUid(param_uid)) == 0);
 
     m_uid_to_decl[toOpaqueUid(param_uid)] = param;
     params.push_back(param);
     --params_remaining;
   }
 
   if (!params.empty())

source/Plugins/SymbolFile/PDB/PDBASTParser.cpp

if (std::unique_ptr<PDBSymbolTypeFunctionSig> sig = func->getSignature()) {
               arg_enum = sig->findAllChildren<PDBSymbolTypeFunctionArg>()) {
         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(
-              nullptr, arg_type->GetForwardCompilerType(), clang::SC_None);
+              decl, nullptr, arg_type->GetForwardCompilerType(),
+              clang::SC_None);
           if (param)
             params.push_back(param);
         }
       }
     }
     if (params.size())
       m_ast.SetFunctionParameters(decl, params.data(), params.size());
 

source/Symbol/ClangASTContext.cpp

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