[Lldb-commits] [lldb] 8612e92 - [lldb][NFC] Remove GetASTContext call in ClangDeclVendor
Greg Clayton via lldb-commits
lldb-commits at lists.llvm.org
Mon Jan 6 14:09:55 PST 2020
Raphael is correct that the idea behind all of the "CompilerXXXX" classes is to abstract us from the TypeSystem that is being used, so they should stay "void *". I would rather not have each CompilerXXXX class make a union of pointers. CompilerType looks like:
class CompilerType {
...
private:
lldb::opaque_compiler_type_t m_type = nullptr;
TypeSystem *m_type_system = nullptr;
};
If would rather not see:
class CompilerType {
...
private:
union {
clang::Type*clang;
swift::Type *swift;
};
TypeSystem *m_type_system = nullptr;
};
Because then every type systems that gets installed has to modify this file and add forward declarations. It is very nice that if we don't compile in the Swift support that we don't need to change CompilerType and anyone that comes along (rust, go, etc) can have their own type systems that are completely abstracted without any need to modify CompilerType.h, CompilerDecl.h, etc...
> On Jan 3, 2020, at 12:50 AM, Raphael “Teemperor” Isemann via lldb-commits <lldb-commits at lists.llvm.org> wrote:
>
> 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
>>
>
> _______________________________________________
> 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