[lldb] Set the access property on member function decls

[amccarth]

Summary

This fixes two failures in the PDB tests. LLVM has a "sanity" check on function decls. One of the requirements of member functions is that they have the access property (public, protected, private) set if the function is a member function. The check is an assert, so you'll see the failure only if you're running with assertions enabled.

This sets the access property to public to match how the existing code handles member function templates.

[aganea]

LGTM.

[labath]

Dwarf parser uses TypeSystemClang::AddMethodToCXXRecordType instead of this function to create methods (which is why there are no assertions like this when using dwarf). Maybe it would be better to change the pdb parser to use that function instead (as it allows the user to specify not only accessibility, but also other potentially useful properties of the created method -- static, virtual, etc.).

And this function could assert that it is _only_ used for creating free functions ?

[teemperor]

+1 for AddMethodToCXXRecordType. Calling CreateFunctionDeclaration creates a FunctionDecl which can't be inside a record. All (static/non-static) member functions in a record are always CXXMethodDecls.

[amccarth]

In D85993#2220724, @labath wrote:

Dwarf parser uses TypeSystemClang::AddMethodToCXXRecordType instead of this function to create methods (which is why there are no assertions like this when using dwarf). Maybe it would be better to change the pdb parser to use that function instead (as it allows the user to specify not only accessibility, but also other potentially useful properties of the created method -- static, virtual, etc.).

The NativePDB AST parser also uses AddMethodToCXXRecordType. It's in udtcompleter.cpp.

But the bug is triggered while the plugin is trying to synthesize the function declaration during a ResolveSymbolContext call. It looks like the DWARF implementation of ResolveSymbolContext relies on its AST parser, but the NativePDB one does not. I'm trying to figure out how to do that.

Also note that AddMethodToCXXRecordType just sets whatever access type it's asked to set. Before calling AddMethodToCXXRecordType, the DWARF parser has this gem:

// Neither GCC 4.2 nor clang++ currently set a valid
// accessibility in the DWARF for C++ methods...
// Default to public for now...
if (attrs.accessibility == eAccessNone)
  attrs.accessibility = eAccessPublic;

That bit of logic is what prevents the assertion from failing for DWARF. I'll can make the NativePDB AST parser do the same thing.

But that won't fix the bug, since the NativePDB AST parser isn't involved (at least, my breakpoints in the parser never hit during the course of the test). In the mean time, I'll try to figure out how to get the NativePDB plugin's ResolveSymbolContext implementation to use AddMethodToCXXRecord.

And this function could assert that it is _only_ used for creating free functions ?

Sure. Sounds like a good idea.

I'll also assume that, since you don't want my access-fixup in TypeSystemClang::CreateFunctionDeclaration, then you also would want me to remove the already-existing such fixup from TypeSystemClang::CreateFunctionTemplateDecl. Does that make sense?

[teemperor]

In D85993#2253465, @amccarth wrote:

In D85993#2220724, @labath wrote:

Dwarf parser uses TypeSystemClang::AddMethodToCXXRecordType instead of this function to create methods (which is why there are no assertions like this when using dwarf). Maybe it would be better to change the pdb parser to use that function instead (as it allows the user to specify not only accessibility, but also other potentially useful properties of the created method -- static, virtual, etc.).

The NativePDB AST parser also uses AddMethodToCXXRecordType. It's in udtcompleter.cpp.

But the bug is triggered while the plugin is trying to synthesize the function declaration during a ResolveSymbolContext call. It looks like the DWARF implementation of ResolveSymbolContext relies on its AST parser, but the NativePDB one does not. I'm trying to figure out how to do that.

Also note that AddMethodToCXXRecordType just sets whatever access type it's asked to set. Before calling AddMethodToCXXRecordType, the DWARF parser has this gem:

// Neither GCC 4.2 nor clang++ currently set a valid
// accessibility in the DWARF for C++ methods...
// Default to public for now...
if (attrs.accessibility == eAccessNone)
  attrs.accessibility = eAccessPublic;

That bit of logic is what prevents the assertion from failing for DWARF. I'll can make the NativePDB AST parser do the same thing.

But that won't fix the bug, since the NativePDB AST parser isn't involved (at least, my breakpoints in the parser never hit during the course of the test). In the mean time, I'll try to figure out how to get the NativePDB plugin's ResolveSymbolContext implementation to use AddMethodToCXXRecord.

No idea about the PDB parsing code, but PdbAstBuilder::GetOrCreateFunctionDecl seems to be the right place. It already has a check for when the context is a TagDecl or a namespace, so doing the same thing for the CreateFunctionDecl call below should do the trick (namespace -> CreateFunctionDecl, TagDecl -> AddMethodToCXXRecordType).

I'll also assume that, since you don't want my access-fixup in TypeSystemClang::CreateFunctionDeclaration, then you also would want me to remove the already-existing such fixup from TypeSystemClang::CreateFunctionTemplateDecl. Does that make sense?

Confusingly FunctionTemplateDecls *can* be inside a record and the name is just misleading. The actual specialisations of a FunctionTemplateDecl in a record are again CXXMethodDecls, so CreateFunctionTemplateDecl is fine:

struct Foo {                                                                    
  template<typename T>                                                          
  int x() {                                                                     
    return 3;                                                                   
  }                                                                             
  int foo() {                                                                   
    return x<double>();                                                         
  }                                                                             
};
$ clang -fsyntax-only -Xclang -ast-dump  foo.cpp
...
`-CXXRecordDecl 0x7f9849077f50 <method.cpp:1:1, line:9:1> line:1:8 struct Foo definition
  |-FunctionTemplateDecl 0x7f98490782d0 <line:2:3, line:5:3> line:3:7 x
  | |-TemplateTypeParmDecl 0x7f98490780f8 <line:2:12, col:21> col:21 typename depth 0 index 0 T
  | |-CXXMethodDecl 0x7f9849078230 <line:3:3, line:5:3> line:3:7 x 'int ()'
  | `-CXXMethodDecl 0x7f98490785e0 <line:3:3, line:5:3> line:3:7 used x 'int ()'
...

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

FunctionDecl *TypeSystemClang::CreateFunctionDeclaration(
   func_decl->setDeclContext(decl_ctx);
   func_decl->setDeclName(declarationName);
   func_decl->setType(ClangUtil::GetQualType(function_clang_type));
   func_decl->setStorageClass(static_cast<clang::StorageClass>(storage));
   func_decl->setInlineSpecified(is_inline);
   func_decl->setHasWrittenPrototype(hasWrittenPrototype);
   func_decl->setConstexprKind(isConstexprSpecified ? CSK_constexpr
                                                    : CSK_unspecified);
+  // Functions inside a record need to have an access specifier.  It doesn't
+  // matter what access specifier we give the function as LLDB allows
+  // accessing everything inside a record.
+  if (decl_ctx->isRecord())
+    func_decl->setAccess(clang::AccessSpecifier::AS_public);
+
   SetOwningModule(func_decl, owning_module);
   if (func_decl)
     decl_ctx->addDecl(func_decl);
 
   VerifyDecl(func_decl);
 
   return func_decl;
 }