This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Move clang-based files out of Symbol
ClosedPublic

Authored by xiaobai on Jan 29 2020, 1:00 PM.

Details

Summary

This change represents the move of ClangASTImporter, ClangASTMetadata,
ClangExternalASTSourceCallbacks, ClangUtil, CxxModuleHandler, and
TypeSystemClang from lldbSource to lldbPluginExpressionParserClang.h

This explicitly removes knowledge of clang internals from lldbSymbol,
moving towards a more generic core implementation of lldb.

Diff Detail

Event Timeline

xiaobai created this revision.Jan 29 2020, 1:00 PM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

I think this is a big milestone. Thanks for working on this.

The main question I have is about the new location of this code. This patch puts it under ExpressionParser/Clang, which is not completely unreasonable, as that's where most of the clang stuff is. However, it does create some awkward-looking (to me) dependencies, or even loops (SymbolFileDWARF<->ExpressionParserClang). Also the dep from data formatters in Language/CPLusPlus on ExpressionParser/Clang is odd, because the data formatters don't actually use/need expressions do to their work.

So, as much as I hate proliferating plugins, I have to ask this question: Should this be a new plugin kind (TypeSystem/Clang) instead ? The name of the class, and the presence of the Initialize function already seem to indicate that. And it seems to me like this would create a reasonable dependency graph between the various plugins. TypeSystemClang would be at the bottom of this. The various SymbolFile plugins would depend on it, because they generate clang ASTs. ExpressionParserClang would depend on it because it pulls the generated ASTs that way. The same goes for the data formatters. But there would be no dependency between SymbolFiles and expression parsers or data formatters, because the former should just provide the ast (and not care about who the consumer is) and the latter should consume it (regardless of the source).

I think this ExpressionParser vs. TypeSystem would also make sense in terms of the outgoing dependencies. The type system should depend +/- only on clangAST, whereas the expression parser would need pretty much the whole clang.

WDYT?

Pretty much what Pavel said, otherwise this LGTM. Maybe also rename/move the unit test files.

Also this doesn't compile on Linux:

FAILED: tools/lldb/source/Plugins/ABI/SysV-ppc64/CMakeFiles/lldbPluginABISysV_ppc64.dir/ABISysV_ppc64.cpp.o                                                                                                                                      
/usr/lib/ccache/bin/clang++  -DGTEST_HAS_RTTI=0 -DHAVE_ROUND -DLLDB_CONFIGURATION_RELEASE -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/lldb/source/Plugins/ABI/SysV-ppc64 -I/home/teemperor/work/ci/llvm/lldb/source/Plugins/ABI/SysV-ppc64 -Itools/lldb/source -I/home/teemperor/work/ci/llvm/lldb/include -Itools/lldb/include -I/usr/include/libxml2 -Iinclude -I/home/teemperor/work/ci/llvm/llvm/include -I/usr/include/python3.8 -I/home/teemperor/work/ci/llvm/llvm/../clang/include -Itools/lldb/../clang/include -I/home/teemperor/work/ci/llvm/lldb/source/. -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -Wno-deprecated-declarations -Wno-unknown-pragmas -Wno-strict-aliasing -Wno-deprecated-register -Wno-vla-extension -O3    -UNDEBUG  -fno-exceptions -fno-rtti -std=c++14 -MD -MT tools/lldb/source/Plugins/ABI/SysV-ppc64/CMakeFiles/lldbPluginABISysV_ppc64.dir/ABISysV_ppc64.cpp.o -MF tools/lldb/source/Plugins/ABI/SysV-ppc64/CMakeFiles/lldbPluginABISysV_ppc64.dir/ABISysV_ppc64.cpp.o.d -o tools/lldb/source/Plugins/ABI/SysV-ppc64/CMakeFiles/lldbPluginABISysV_ppc64.dir/ABISysV_ppc64.cpp.o -c /home/teemperor/work/ci/llvm/lldb/source/Plugins/ABI/SysV-ppc64/ABISysV_ppc64.cpp                                                                                                                     
/home/teemperor/work/ci/llvm/lldb/source/Plugins/ABI/SysV-ppc64/ABISysV_ppc64.cpp:22:10: fatal error: 'lldb/Symbol/TypeSystemClang.h' file not found                                                                                             
#include "lldb/Symbol/TypeSystemClang.h"                    
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                    
1 error generated.                                          
ninja: build stopped: cannot make progress due to previous errors.

I vote to make this a plug-in as well!

I think this is a big milestone. Thanks for working on this.

The main question I have is about the new location of this code. This patch puts it under ExpressionParser/Clang, which is not completely unreasonable, as that's where most of the clang stuff is. However, it does create some awkward-looking (to me) dependencies, or even loops (SymbolFileDWARF<->ExpressionParserClang). Also the dep from data formatters in Language/CPLusPlus on ExpressionParser/Clang is odd, because the data formatters don't actually use/need expressions do to their work.

So, as much as I hate proliferating plugins, I have to ask this question: Should this be a new plugin kind (TypeSystem/Clang) instead ? The name of the class, and the presence of the Initialize function already seem to indicate that. And it seems to me like this would create a reasonable dependency graph between the various plugins. TypeSystemClang would be at the bottom of this. The various SymbolFile plugins would depend on it, because they generate clang ASTs. ExpressionParserClang would depend on it because it pulls the generated ASTs that way. The same goes for the data formatters. But there would be no dependency between SymbolFiles and expression parsers or data formatters, because the former should just provide the ast (and not care about who the consumer is) and the latter should consume it (regardless of the source).

I think this ExpressionParser vs. TypeSystem would also make sense in terms of the outgoing dependencies. The type system should depend +/- only on clangAST, whereas the expression parser would need pretty much the whole clang.

WDYT?

I think that creating a TypeSystem plugin is probably the better option here. After looking more closely at the dependency graph I think that what you pointed out is enough to justify a new plugin. I'll update this patch.

Also this doesn't compile on Linux:

Thanks for checking this.

xiaobai updated this revision to Diff 241584.Jan 30 2020, 2:43 PM

Move TypeSystemClang into its own plugin

Awesome, thanks for doing this Alex!

labath accepted this revision.Jan 31 2020, 12:38 AM

This looks good to me, and I believe everyone else also felt the same way. There are still some loops here (TypeSystemClang<->SymbolFileDWARF/PDB), but I think those could be broken fairly easily. This is not a problem of this patch, and it can be handled later..

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
37

remove one /

This revision is now accepted and ready to land.Jan 31 2020, 12:38 AM

Making this a plugin makes sense. Thank you!

This revision was automatically updated to reflect the committed changes.