[Lldb-commits] [PATCH] D85993: [lldb] Set the access property on member function decls

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

In D85993#2253465 <https://reviews.llvm.org/D85993#2253465>, @amccarth wrote:

> In D85993#2220724 <https://reviews.llvm.org/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 ()'
  ...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85993/new/

https://reviews.llvm.org/D85993