[Lldb-commits] [lldb] 8612e92 - [lldb][NFC] Remove GetASTContext call in ClangDeclVendor
Raphael “Teemperor” Isemann via lldb-commits
lldb-commits at lists.llvm.org
Fri Jan 3 00:50:10 PST 2020
We could use clang::Decl* etc. in these classes but that would defeat the whole concept that they are supposed to implement. CompilerType, CompilerDecl, etc.. are all classes that represent their respective equivalent in a language plugin. For Clang that is indeed clang::Type* and clang::Decl*, but Swift is storing swift::Type* and swift::Decl* in these void* pointers (and passes a SwiftASTContext* instead of a ClangASTContext* as a m_type_system). So if we change that to clang::Decl* then the all other language plugins like Swift are effectively broken. It would also mean that generic LLDB code using these classes would have a hard dependency on Clang.
And there shouldn’t be any place where we unconditionally cast this to clang::Decl* as this would directly crash swift-lldb. The m_type_system (which is either ClangASTContext or SwiftASTContext) can freely cast the void* to clang::Decl/swift::Decl as the checking happens through the virtual function call to the m_type_system interface (Swift types always have a SwiftASTContext as m_type_system, Clang types have a ClangASTContext as m_type_system).
Having said that, I’m not saying that the void* pointers in these classes are the best solution we could have here (there is a reason why I keep removing all these void* pointer conversions). If you see a way to get rid of them, then be my guest.
> On 3. Jan 2020, at 01:52, Shafik Yaghmour <syaghmour at apple.com> wrote:
>
> To further clarify all the member functions of CompilerDecl e.g.:
>
> GetMangledName(…)
> GetDeclContext(…)
>
> End up passing m_opaque_decl as an argument to a member function of m_type_system and in these member functions they unconditionally cast the argument to Decl* so why not make m_opaque_decl a Decl*
>
>> On Jan 2, 2020, at 4:20 PM, Shafik Yaghmour via lldb-commits <lldb-commits at lists.llvm.org> wrote:
>>
>> Apologies, m_opaque_decl member of CompilerDecl and operator < in CompilerDecl.h
>>
>>> On Jan 2, 2020, at 3:54 PM, Raphael “Teemperor” Isemann <teemperor at gmail.com> wrote:
>>>
>>> Not sure if I can follow. What variable should just be a Decl* and what operator<?
>>>
>>> (Also yeah that reinterpret_cast could also be a static_cast)
>>>
>>>> On Jan 3, 2020, at 12:38 AM, Shafik Yaghmour <syaghmour at apple.com> wrote:
>>>>
>>>> I am not crazy about the reinterpret_cast although if we look inside CompilerDecl impl we can see that basically it is always assuming it is a Decl* and just C-style casts it as such. So why not just make it a Decl*
>>>>
>>>> Also operator<(…) is super sketchy doing a < on void*
>>>>
>>>>> On Dec 28, 2019, at 6:52 AM, Raphael Isemann via lldb-commits <lldb-commits at lists.llvm.org> wrote:
>>>>>
>>>>>
>>>>> Author: Raphael Isemann
>>>>> Date: 2019-12-28T15:20:19+01:00
>>>>> New Revision: 8612e92ed590e615f9f56e4fb86a1fdaf3a39e15
>>>>>
>>>>> URL: https://github.com/llvm/llvm-project/commit/8612e92ed590e615f9f56e4fb86a1fdaf3a39e15
>>>>> DIFF: https://github.com/llvm/llvm-project/commit/8612e92ed590e615f9f56e4fb86a1fdaf3a39e15.diff
>>>>>
>>>>> LOG: [lldb][NFC] Remove GetASTContext call in ClangDeclVendor
>>>>>
>>>>> Instead of returning NamedDecls and then calling GetASTContext
>>>>> to find back the ClangASTContext we used can just implement the
>>>>> FindDecl variant that returns CompilerDecls (and implement the
>>>>> other function by throwing away the ClangASTContext part of the
>>>>> compiler decl).
>>>>>
>>>>> Added:
>>>>>
>>>>>
>>>>> Modified:
>>>>> lldb/source/Plugins/ExpressionParser/Clang/ClangDeclVendor.cpp
>>>>> lldb/source/Plugins/ExpressionParser/Clang/ClangDeclVendor.h
>>>>> lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
>>>>> lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.cpp
>>>>> lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.h
>>>>>
>>>>> Removed:
>>>>>
>>>>>
>>>>>
>>>>> ################################################################################
>>>>> diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangDeclVendor.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangDeclVendor.cpp
>>>>> index c59722b7b4f8..0c5796650d45 100644
>>>>> --- a/lldb/source/Plugins/ExpressionParser/Clang/ClangDeclVendor.cpp
>>>>> +++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangDeclVendor.cpp
>>>>> @@ -15,16 +15,17 @@ using namespace lldb_private;
>>>>>
>>>>> uint32_t ClangDeclVendor::FindDecls(ConstString name, bool append,
>>>>> uint32_t max_matches,
>>>>> - std::vector<CompilerDecl> &decls) {
>>>>> + std::vector<clang::NamedDecl *> &decls) {
>>>>> if (!append)
>>>>> decls.clear();
>>>>>
>>>>> - std::vector<clang::NamedDecl *> named_decls;
>>>>> - uint32_t ret = FindDecls(name, /*append*/ false, max_matches, named_decls);
>>>>> - for (auto *named_decl : named_decls) {
>>>>> - decls.push_back(CompilerDecl(
>>>>> - ClangASTContext::GetASTContext(&named_decl->getASTContext()),
>>>>> - named_decl));
>>>>> + std::vector<CompilerDecl> compiler_decls;
>>>>> + uint32_t ret = FindDecls(name, /*append*/ false, max_matches, compiler_decls);
>>>>> + for (CompilerDecl compiler_decl : compiler_decls) {
>>>>> + clang::Decl *d =
>>>>> + reinterpret_cast<clang::Decl *>(compiler_decl.GetOpaqueDecl());
>>>>> + clang::NamedDecl *nd = llvm::cast<clang::NamedDecl>(d);
>>>>> + decls.push_back(nd);
>>>>> }
>>>>> return ret;
>>>>> }
>>>>>
>>>>> diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangDeclVendor.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangDeclVendor.h
>>>>> index 85a10400a201..0c888de08841 100644
>>>>> --- a/lldb/source/Plugins/ExpressionParser/Clang/ClangDeclVendor.h
>>>>> +++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangDeclVendor.h
>>>>> @@ -21,12 +21,10 @@ class ClangDeclVendor : public DeclVendor {
>>>>>
>>>>> virtual ~ClangDeclVendor() {}
>>>>>
>>>>> - uint32_t FindDecls(ConstString name, bool append, uint32_t max_matches,
>>>>> - std::vector<CompilerDecl> &decls) override;
>>>>> + using DeclVendor::FindDecls;
>>>>>
>>>>> - virtual uint32_t FindDecls(ConstString name, bool append,
>>>>> - uint32_t max_matches,
>>>>> - std::vector<clang::NamedDecl *> &decls) = 0;
>>>>> + uint32_t FindDecls(ConstString name, bool append, uint32_t max_matches,
>>>>> + std::vector<clang::NamedDecl *> &decls);
>>>>>
>>>>> static bool classof(const DeclVendor *vendor) {
>>>>> return vendor->GetKind() >= eClangDeclVendor &&
>>>>>
>>>>> diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
>>>>> index ff0905dda4ef..0696c669f2e2 100644
>>>>> --- a/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
>>>>> +++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
>>>>> @@ -80,7 +80,7 @@ class ClangModulesDeclVendorImpl : public ClangModulesDeclVendor {
>>>>> Stream &error_stream) override;
>>>>>
>>>>> uint32_t FindDecls(ConstString name, bool append, uint32_t max_matches,
>>>>> - std::vector<clang::NamedDecl *> &decls) override;
>>>>> + std::vector<CompilerDecl> &decls) override;
>>>>>
>>>>> void ForEachMacro(const ModuleVector &modules,
>>>>> std::function<bool(const std::string &)> handler) override;
>>>>> @@ -356,7 +356,7 @@ bool ClangModulesDeclVendorImpl::AddModulesForCompileUnit(
>>>>> uint32_t
>>>>> ClangModulesDeclVendorImpl::FindDecls(ConstString name, bool append,
>>>>> uint32_t max_matches,
>>>>> - std::vector<clang::NamedDecl *> &decls) {
>>>>> + std::vector<CompilerDecl> &decls) {
>>>>> if (!m_enabled) {
>>>>> return 0;
>>>>> }
>>>>> @@ -382,7 +382,7 @@ ClangModulesDeclVendorImpl::FindDecls(ConstString name, bool append,
>>>>> if (num_matches >= max_matches)
>>>>> return num_matches;
>>>>>
>>>>> - decls.push_back(named_decl);
>>>>> + decls.push_back(CompilerDecl(m_ast_context.get(), named_decl));
>>>>> ++num_matches;
>>>>> }
>>>>>
>>>>>
>>>>> diff --git a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.cpp b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.cpp
>>>>> index 54f8397e1dad..29930c303b07 100644
>>>>> --- a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.cpp
>>>>> +++ b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.cpp
>>>>> @@ -537,10 +537,9 @@ bool AppleObjCDeclVendor::FinishDecl(clang::ObjCInterfaceDecl *interface_decl) {
>>>>> return true;
>>>>> }
>>>>>
>>>>> -uint32_t
>>>>> -AppleObjCDeclVendor::FindDecls(ConstString name, bool append,
>>>>> - uint32_t max_matches,
>>>>> - std::vector<clang::NamedDecl *> &decls) {
>>>>> +uint32_t AppleObjCDeclVendor::FindDecls(ConstString name, bool append,
>>>>> + uint32_t max_matches,
>>>>> + std::vector<CompilerDecl> &decls) {
>>>>> static unsigned int invocation_id = 0;
>>>>> unsigned int current_id = invocation_id++;
>>>>>
>>>>> @@ -587,7 +586,7 @@ AppleObjCDeclVendor::FindDecls(ConstString name, bool append,
>>>>> current_id, result_iface_type.getAsString(), isa_value);
>>>>> }
>>>>>
>>>>> - decls.push_back(result_iface_decl);
>>>>> + decls.push_back(CompilerDecl(&m_ast_ctx, result_iface_decl));
>>>>> ret++;
>>>>> break;
>>>>> } else {
>>>>> @@ -630,7 +629,7 @@ AppleObjCDeclVendor::FindDecls(ConstString name, bool append,
>>>>> new_iface_type.getAsString(), (uint64_t)isa);
>>>>> }
>>>>>
>>>>> - decls.push_back(iface_decl);
>>>>> + decls.push_back(CompilerDecl(&m_ast_ctx, iface_decl));
>>>>> ret++;
>>>>> break;
>>>>> } while (false);
>>>>>
>>>>> diff --git a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.h b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.h
>>>>> index 311113a27735..f49ca3540c2c 100644
>>>>> --- a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.h
>>>>> +++ b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.h
>>>>> @@ -28,7 +28,7 @@ class AppleObjCDeclVendor : public ClangDeclVendor {
>>>>> }
>>>>>
>>>>> uint32_t FindDecls(ConstString name, bool append, uint32_t max_matches,
>>>>> - std::vector<clang::NamedDecl *> &decls) override;
>>>>> + std::vector<CompilerDecl> &decls) override;
>>>>>
>>>>> friend class AppleObjCExternalASTSource;
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> lldb-commits mailing list
>>>>> lldb-commits at lists.llvm.org
>>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>>>>
>>>
>>
>> _______________________________________________
>> lldb-commits mailing list
>> lldb-commits at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
More information about the lldb-commits
mailing list