[NativePDB] Make GetOrCreateDeclForUid return an lldb CompilerDecl

[lanza]

Summary

We intend to make PdbAstBuilder abstract and implement
PdbAstBuilderClang along with any other languages that wish to use
PDBs. Thus, change GetOrCreateDeclForUid from returning a clang decl
to a lldb_private::CompilerDecl.

[aleksandr.urakov]

LGTM! So the other functions (e.g. GetOrCreateDeclContextForUid, GetParentDeclContext etc.) will be also replaced in the future in a corresponding manner?

[amccarth]

Was there a proposal or discussion (e.g., a thread on lldb-dev) about making PdbAstBuilder abstract in order to handle other languages? It sounds like a good idea, but I'd like to catch up on any context I may have missed.

source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp

470

Should the return value be an Expected<lldb_private::CompilerDecl>?

473

auto decl = ...; return decl; seems unnecessarily verbose. The name decl doesn't add any value beyond CompilerDecl, so consider just returning the expression.

490

Again, I would just return the expression directly.

source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

1509–1512

I'm a little confused here. It looks like we took a CompilerDecl apart, made an assumption about the language, and put it back together. That has no net effect, right? Is this in anticipation of future changes or should this just return decl?

[lanza]

Was there a proposal or discussion (e.g., a thread on lldb-dev) about making PdbAstBuilder abstract in order to handle other languages? It sounds like a good idea, but I'd like to catch up on any context I may have missed.

Nope. We're considering using PDBs for Swift on Windows and this is the first of a long list of steps that would need to be done for it. I submitted similar things in the past to SymbolFilePDB and Zach redirected me to the native implementation, but I got pulled away from this work for some time after that. That was the limit of the discussion outside of my team.

To summarize -- PdbAstBuilder, SymbolFileNativePDB and maybe one other file need the word clang ripped out of them and replaced with the corresponding types. At that point PdbAstBuilder becomes abstract and PdbAstBuilderClang is created followed by PdbAstBuilderSwift.

[lanza]

LGTM! So the other functions (e.g. GetOrCreateDeclContextForUid, GetParentDeclContext etc.) will be also replaced in the future in a corresponding manner?

Yup. I was asked to split it up last time, so just sticking with that request.

[lanza]

update

[lanza]

source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp

470

Similar to before, I plan on doing a pass like this once all the types are done changing.

[amccarth]

LGTM, but please reconsider whether the return type of GetOrCreateDeclForUid should be changed in this patch rather than in a future one.

source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp

470

This case is a bit different, though. The original code returns pointers (and a nullptr for the failure case).
Your always returning an object, and the failure cases are now default constructed objects. That has an impact on the callers, so it seems (to me) a waste to have to change the callers twice, especially since the Expected<> version would be handled very similarly to the possibly-null pointer version.

Your call. If you're planning to do this in a future patch, I'm satisfied.

[amccarth]

Oh, and thanks for the background and context on the motivation for this change!

[lanza]

source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp

470

Sure, I'll go ahead and do them as I change the function signatures.. Though this is more fitting for Optional<> given that not having debug info is not an error.

[lanza]

optional

[amccarth]

Thanks for the change. I was thinking Expected based on the name of the function: GetOrCreateDeclForUid. I was thinking that it would be odd to have a UID if you don't have debug info. Anyway, Optional is fine. Thanks again.

source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.h

 
 class PdbAstBuilder {
 public:
   // Constructors and Destructors
   PdbAstBuilder(ObjectFile &obj, PdbIndex &index);
 
   lldb_private::CompilerDeclContext GetTranslationUnitDecl();
 
-  clang::Decl *GetOrCreateDeclForUid(PdbSymUid uid);
+  llvm::Optional<lldb_private::CompilerDecl>
+  GetOrCreateDeclForUid(PdbSymUid uid);
   clang::DeclContext *GetOrCreateDeclContextForUid(PdbSymUid uid);
   clang::DeclContext *GetParentDeclContext(PdbSymUid uid);
 
   clang::FunctionDecl *GetOrCreateFunctionDecl(PdbCompilandSymId func_id);
   clang::BlockDecl *GetOrCreateBlockDecl(PdbCompilandSymId block_id);
   clang::VarDecl *GetOrCreateVariableDecl(PdbCompilandSymId scope_id,
                                           PdbCompilandSymId var_id);
   clang::VarDecl *GetOrCreateVariableDecl(PdbGlobalSymId var_id);
   clang::TypedefNameDecl *GetOrCreateTypedefDecl(PdbGlobalSymId id);
   void ParseDeclsForContext(clang::DeclContext &context);
 
   clang::QualType GetBasicType(lldb::BasicType type);
   clang::QualType GetOrCreateType(PdbTypeSymId type);
 
   bool CompleteTagDecl(clang::TagDecl &tag);
   bool CompleteType(clang::QualType qt);
 
   CompilerDecl ToCompilerDecl(clang::Decl &decl);
   CompilerType ToCompilerType(clang::QualType qt);
   CompilerDeclContext ToCompilerDeclContext(clang::DeclContext &context);
-  clang::Decl * FromCompilerDecl(CompilerDecl decl);
+  clang::Decl *FromCompilerDecl(CompilerDecl decl);
   clang::DeclContext *FromCompilerDeclContext(CompilerDeclContext context);
 
   ClangASTContext &clang() { return m_clang; }
   ClangASTImporter &importer() { return m_importer; }
 
   void Dump(Stream &stream);
 
 private:

source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp

case S_CONSTANT:
     return nullptr;
   case S_BLOCK32:
     return GetOrCreateBlockDecl(id);
   default:
     return nullptr;
   }
 }
 
-clang::Decl *PdbAstBuilder::GetOrCreateDeclForUid(PdbSymUid uid) {
+llvm::Optional<CompilerDecl> PdbAstBuilder::GetOrCreateDeclForUid(PdbSymUid uid) {
   if (clang::Decl *result = TryGetDecl(uid))
-    return result;
+    return ToCompilerDecl(*result);
 
   clang::Decl *result = nullptr;
   switch (uid.kind()) {
   case PdbSymUidKind::CompilandSym:
     result = GetOrCreateSymbolForId(uid.asCompilandSym());
     break;
   case PdbSymUidKind::Type: {
     clang::QualType qt = GetOrCreateType(uid.asTypeSym());
     if (auto *tag = qt->getAsTagDecl()) {
       result = tag;
       break;
     }
-    return nullptr;
+    return llvm::None;
   }
   default:
-    return nullptr;
+    return llvm::None;
   }
   m_uid_to_decl[toOpaqueUid(uid)] = result;
-  return result;
+  return ToCompilerDecl(*result);
 }
 
 clang::DeclContext *PdbAstBuilder::GetOrCreateDeclContextForUid(PdbSymUid uid) {
   if (uid.kind() == PdbSymUidKind::CompilandSym) {
     if (uid.asCompilandSym().offset == 0)
       return FromCompilerDeclContext(GetTranslationUnitDecl());
   }
-
-  clang::Decl *decl = GetOrCreateDeclForUid(uid);
+  auto option = GetOrCreateDeclForUid(uid);
+  if (!option)
+    return nullptr;
+  clang::Decl *decl = FromCompilerDecl(option.getValue());
   if (!decl)
     return nullptr;
 
   return clang::Decl::castToDeclContext(decl);
 }
 
 std::pair<clang::DeclContext *, std::string>
 PdbAstBuilder::CreateDeclInfoForUndecoratedName(llvm::StringRef name) {

source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

if (sc.comp_unit) {
     }
     return ParseVariablesForCompileUnit(*sc.comp_unit, *variables);
   }
 
   llvm_unreachable("Unreachable!");
 }
 
 CompilerDecl SymbolFileNativePDB::GetDeclForUID(lldb::user_id_t uid) {
-  clang::Decl *decl = m_ast->GetOrCreateDeclForUid(PdbSymUid(uid));
-
-  return m_ast->ToCompilerDecl(*decl);
+  if (auto decl = m_ast->GetOrCreateDeclForUid(uid))
+    return decl.getValue();
+  else
+    return CompilerDecl();
 }
 
 CompilerDeclContext
 SymbolFileNativePDB::GetDeclContextForUID(lldb::user_id_t uid) {
   clang::DeclContext *context =
       m_ast->GetOrCreateDeclContextForUid(PdbSymUid(uid));
   if (!context)
     return {};