Skip to content

[LLDB] Export DWARF Parser symbols for external language plugins #67851

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

walter-erquinigo
Copy link
Member

@walter-erquinigo walter-erquinigo commented Sep 29, 2023

I'm developing support for the Mojo language as an out-of-tree plugin, and in order to implement the DWARF AST parser, I need to export the DWARF-related symbols that the TypeSystem expects me to use.
I have CMake configured to export all the lldb_private symbols, so, in order get access to these DWARF symbols, I need to move them to the lldb_private namespace.
This change is very NFC, but I also made two other changes:

  • remove the virtual destructor from DWARFExpression which doesn't have virtual methods but was giving me linking issues
  • remove the clang parser as friend of SymbolFileDWARF and simply make its protected methods public, as both the clang and the mojo parser need to access these functions.

After these changes I was able to do some basic dwarf processing in my plugin and make the type system happy.

@llvmbot
Copy link
Member

llvmbot commented Sep 29, 2023

@llvm/pr-subscribers-lldb

Changes

I'm developing support for the Mojo language as an out-of-tree plugin, and in order to implement the DWARF AST parser, I need to export the DWARF-related symbols that the TypeSystem expects me to use.
I have CMake configured to export all the lldb_private symbols, so, in order get access to these DWARF symbols, I need to move them to the lldb_private namespace.
This change is very NFC, but I also made two other changes:

  • remove the virtual destructor from DWARFExpression which doesn't have virtual methods but was giving me linking issues
  • remove the clang parser as friend of SymbolFileDWARF and simply make its protected methods public, as both the clang and the mojo parser need to access these functions.

After these changes I was able to do some basic dwarf processing in my plugin and make the type system happy.


Patch is 43.30 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/67851.diff

33 Files Affected:

  • (modified) lldb/include/lldb/Expression/DWARFExpression.h (+3-3)
  • (modified) lldb/include/lldb/Expression/DWARFExpressionList.h (+2-2)
  • (modified) lldb/include/lldb/Symbol/TypeSystem.h (+5-3)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DIERef.h (+5-2)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.h (+2-2)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h (+83-63)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFAttribute.cpp (+1)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFAttribute.h (+3)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFBaseDIE.cpp (+2)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFBaseDIE.h (+2)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h (+2)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.h (+2)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h (+2)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.h (+2)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAranges.h (+2)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h (+1-1)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h (+2)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.h (+1-3)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.h (+2-2)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFDeclContext.cpp (+1)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFDeclContext.h (+2)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.h (+3-1)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h (+1-1)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFTypeUnit.h (+3)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h (+9-5)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.h (+1-1)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/NameToDIE.h (+3-2)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h (+3-2)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp (+2)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h (+9-7)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h (+6-3)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/UniqueDWARFASTType.cpp (+1)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/UniqueDWARFASTType.h (+3-2)
diff --git a/lldb/include/lldb/Expression/DWARFExpression.h b/lldb/include/lldb/Expression/DWARFExpression.h
index 380910ba0ea3d61..c9d747c4a82ad88 100644
--- a/lldb/include/lldb/Expression/DWARFExpression.h
+++ b/lldb/include/lldb/Expression/DWARFExpression.h
@@ -18,10 +18,10 @@
 #include "llvm/DebugInfo/DWARF/DWARFLocationExpression.h"
 #include <functional>
 
-class DWARFUnit;
-
 namespace lldb_private {
 
+class DWARFUnit;
+
 /// \class DWARFExpression DWARFExpression.h
 /// "lldb/Expression/DWARFExpression.h" Encapsulates a DWARF location
 /// expression and interprets it.
@@ -45,7 +45,7 @@ class DWARFExpression {
   DWARFExpression(const DataExtractor &data);
 
   /// Destructor
-  virtual ~DWARFExpression();
+  ~DWARFExpression();
 
   /// Return true if the location expression contains data
   bool IsValid() const;
diff --git a/lldb/include/lldb/Expression/DWARFExpressionList.h b/lldb/include/lldb/Expression/DWARFExpressionList.h
index c0939647056dcbf..2bb83e05c8f304e 100644
--- a/lldb/include/lldb/Expression/DWARFExpressionList.h
+++ b/lldb/include/lldb/Expression/DWARFExpressionList.h
@@ -13,10 +13,10 @@
 #include "lldb/Utility/RangeMap.h"
 #include "lldb/lldb-private.h"
 
-class DWARFUnit;
-
 namespace lldb_private {
 
+class DWARFUnit;
+
 /// \class DWARFExpressionList DWARFExpressionList.h
 /// "lldb/Expression/DWARFExpressionList.h" Encapsulates a range map from file
 /// address range to a single DWARF location expression.
diff --git a/lldb/include/lldb/Symbol/TypeSystem.h b/lldb/include/lldb/Symbol/TypeSystem.h
index eb6e453e1aec0d0..377fc51000b89cd 100644
--- a/lldb/include/lldb/Symbol/TypeSystem.h
+++ b/lldb/include/lldb/Symbol/TypeSystem.h
@@ -28,11 +28,13 @@
 #include "lldb/Symbol/CompilerDeclContext.h"
 #include "lldb/lldb-private.h"
 
-class DWARFDIE;
-class DWARFASTParser;
 class PDBASTParser;
 
 namespace lldb_private {
+
+class DWARFDIE;
+class DWARFASTParser;
+
 namespace npdb {
   class PdbAstBuilder;
 } // namespace npdb
@@ -579,7 +581,7 @@ class TypeSystemMap {
   llvm::Expected<lldb::TypeSystemSP> GetTypeSystemForLanguage(
       lldb::LanguageType language,
       std::optional<CreateCallback> create_callback = std::nullopt);
-  };
+};
 
 } // namespace lldb_private
 
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DIERef.h b/lldb/source/Plugins/SymbolFile/DWARF/DIERef.h
index b5a5cfe263f7804..7864189503a9666 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DIERef.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DIERef.h
@@ -14,6 +14,7 @@
 #include <cassert>
 #include <optional>
 
+namespace lldb_private {
 /// Identifies a DWARF debug info entry within a given Module. It contains three
 /// "coordinates":
 /// - file_index: identifies the separate stand alone debug info file
@@ -131,10 +132,12 @@ class DIERef {
 static_assert(sizeof(DIERef) == 8);
 
 typedef std::vector<DIERef> DIEArray;
+} // namespace lldb_private
 
 namespace llvm {
-template<> struct format_provider<DIERef> {
-  static void format(const DIERef &ref, raw_ostream &OS, StringRef Style);
+template <> struct format_provider<lldb_private::DIERef> {
+  static void format(const lldb_private::DIERef &ref, raw_ostream &OS,
+                     StringRef Style);
 };
 } // namespace llvm
 
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.h
index 18825ae060b12fe..8361953750461a4 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.h
@@ -17,11 +17,10 @@
 #include "lldb/lldb-enumerations.h"
 #include <optional>
 
-class DWARFDIE;
 namespace lldb_private {
+class DWARFDIE;
 class CompileUnit;
 class ExecutionContext;
-}
 class SymbolFileDWARF;
 
 class DWARFASTParser {
@@ -65,5 +64,6 @@ class DWARFASTParser {
 
   static lldb::AccessType GetAccessTypeFromDWARF(uint32_t dwarf_accessibility);
 };
+} // namespace lldb_private
 
 #endif // LLDB_SOURCE_PLUGINS_SYMBOLFILE_DWARF_DWARFASTPARSER_H
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
index 88bfc490e890744..c7b451d5b638f00 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
@@ -30,13 +30,13 @@
 
 namespace lldb_private {
 class CompileUnit;
-}
 class DWARFDebugInfoEntry;
 class SymbolFileDWARF;
+} // namespace lldb_private
 
 struct ParsedDWARFTypeAttributes;
 
-class DWARFASTParserClang : public DWARFASTParser {
+class DWARFASTParserClang : public lldb_private::DWARFASTParser {
 public:
   DWARFASTParserClang(lldb_private::TypeSystemClang &ast);
 
@@ -44,32 +44,33 @@ class DWARFASTParserClang : public DWARFASTParser {
 
   // DWARFASTParser interface.
   lldb::TypeSP ParseTypeFromDWARF(const lldb_private::SymbolContext &sc,
-                                  const DWARFDIE &die,
+                                  const lldb_private::DWARFDIE &die,
                                   bool *type_is_new_ptr) override;
 
   lldb_private::ConstString
-  ConstructDemangledNameFromDWARF(const DWARFDIE &die) override;
+  ConstructDemangledNameFromDWARF(const lldb_private::DWARFDIE &die) override;
 
   lldb_private::Function *
   ParseFunctionFromDWARF(lldb_private::CompileUnit &comp_unit,
-                         const DWARFDIE &die,
+                         const lldb_private::DWARFDIE &die,
                          const lldb_private::AddressRange &func_range) override;
 
   bool
-  CompleteTypeFromDWARF(const DWARFDIE &die, lldb_private::Type *type,
+  CompleteTypeFromDWARF(const lldb_private::DWARFDIE &die,
+                        lldb_private::Type *type,
                         lldb_private::CompilerType &compiler_type) override;
 
   lldb_private::CompilerDecl
-  GetDeclForUIDFromDWARF(const DWARFDIE &die) override;
+  GetDeclForUIDFromDWARF(const lldb_private::DWARFDIE &die) override;
 
   void EnsureAllDIEsInDeclContextHaveBeenParsed(
       lldb_private::CompilerDeclContext decl_context) override;
 
   lldb_private::CompilerDeclContext
-  GetDeclContextForUIDFromDWARF(const DWARFDIE &die) override;
+  GetDeclContextForUIDFromDWARF(const lldb_private::DWARFDIE &die) override;
 
-  lldb_private::CompilerDeclContext
-  GetDeclContextContainingUIDFromDWARF(const DWARFDIE &die) override;
+  lldb_private::CompilerDeclContext GetDeclContextContainingUIDFromDWARF(
+      const lldb_private::DWARFDIE &die) override;
 
   lldb_private::ClangASTImporter &GetClangASTImporter();
 
@@ -87,7 +88,7 @@ class DWARFASTParserClang : public DWARFASTParser {
   ///         into the given integer type or the integer type isn't supported.
   llvm::Expected<llvm::APInt>
   ExtractIntFromFormValue(const lldb_private::CompilerType &int_type,
-                          const DWARFFormValue &form_value) const;
+                          const lldb_private::DWARFFormValue &form_value) const;
 
   /// Returns the template parameters of a class DWARFDIE as a string.
   ///
@@ -100,7 +101,7 @@ class DWARFASTParserClang : public DWARFASTParser {
   /// If the DIE's name already has '<>', returns an empty ConstString because
   /// it's assumed that the caller is using the DIE name anyway.
   lldb_private::ConstString
-  GetDIEClassTemplateParams(const DWARFDIE &die) override;
+  GetDIEClassTemplateParams(const lldb_private::DWARFDIE &die) override;
 
 protected:
   /// Protected typedefs and members.
@@ -108,14 +109,17 @@ class DWARFASTParserClang : public DWARFASTParser {
   class DelayedAddObjCClassProperty;
   typedef std::vector<DelayedAddObjCClassProperty> DelayedPropertyList;
 
-  typedef llvm::DenseMap<const DWARFDebugInfoEntry *, clang::DeclContext *>
+  typedef llvm::DenseMap<const lldb_private::DWARFDebugInfoEntry *,
+                         clang::DeclContext *>
       DIEToDeclContextMap;
-  typedef std::multimap<const clang::DeclContext *, const DWARFDIE>
+  typedef std::multimap<const clang::DeclContext *,
+                        const lldb_private::DWARFDIE>
       DeclContextToDIEMap;
-  typedef llvm::DenseMap<const DWARFDebugInfoEntry *,
+  typedef llvm::DenseMap<const lldb_private::DWARFDebugInfoEntry *,
                          lldb_private::OptionalClangModuleID>
       DIEToModuleMap;
-  typedef llvm::DenseMap<const DWARFDebugInfoEntry *, clang::Decl *>
+  typedef llvm::DenseMap<const lldb_private::DWARFDebugInfoEntry *,
+                         clang::Decl *>
       DIEToDeclMap;
 
   lldb_private::TypeSystemClang &m_ast;
@@ -126,11 +130,11 @@ class DWARFASTParserClang : public DWARFASTParser {
   std::unique_ptr<lldb_private::ClangASTImporter> m_clang_ast_importer_up;
   /// @}
 
-  clang::DeclContext *GetDeclContextForBlock(const DWARFDIE &die);
+  clang::DeclContext *GetDeclContextForBlock(const lldb_private::DWARFDIE &die);
 
-  clang::BlockDecl *ResolveBlockDIE(const DWARFDIE &die);
+  clang::BlockDecl *ResolveBlockDIE(const lldb_private::DWARFDIE &die);
 
-  clang::NamespaceDecl *ResolveNamespaceDIE(const DWARFDIE &die);
+  clang::NamespaceDecl *ResolveNamespaceDIE(const lldb_private::DWARFDIE &die);
 
   /// Returns the namespace decl that a DW_TAG_imported_declaration imports.
   ///
@@ -141,31 +145,33 @@ class DWARFASTParserClang : public DWARFASTParser {
   ///          'die' imports. If the imported entity is not a namespace
   ///          or another import declaration, returns nullptr. If an error
   ///          occurs, returns nullptr.
-  clang::NamespaceDecl *ResolveImportedDeclarationDIE(const DWARFDIE &die);
+  clang::NamespaceDecl *
+  ResolveImportedDeclarationDIE(const lldb_private::DWARFDIE &die);
 
-  bool ParseTemplateDIE(const DWARFDIE &die,
+  bool ParseTemplateDIE(const lldb_private::DWARFDIE &die,
                         lldb_private::TypeSystemClang::TemplateParameterInfos
                             &template_param_infos);
 
   bool ParseTemplateParameterInfos(
-      const DWARFDIE &parent_die,
+      const lldb_private::DWARFDIE &parent_die,
       lldb_private::TypeSystemClang::TemplateParameterInfos
           &template_param_infos);
 
-  std::string GetCPlusPlusQualifiedName(const DWARFDIE &die);
+  std::string GetCPlusPlusQualifiedName(const lldb_private::DWARFDIE &die);
 
   bool ParseChildMembers(
-      const DWARFDIE &die, lldb_private::CompilerType &class_compiler_type,
+      const lldb_private::DWARFDIE &die,
+      lldb_private::CompilerType &class_compiler_type,
       std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> &base_classes,
-      std::vector<DWARFDIE> &member_function_dies,
+      std::vector<lldb_private::DWARFDIE> &member_function_dies,
       DelayedPropertyList &delayed_properties,
       const lldb::AccessType default_accessibility,
       lldb_private::ClangASTImporter::LayoutInfo &layout_info);
 
   size_t
   ParseChildParameters(clang::DeclContext *containing_decl_ctx,
-                       const DWARFDIE &parent_die, bool skip_artificial,
-                       bool &is_static, bool &is_variadic,
+                       const lldb_private::DWARFDIE &parent_die,
+                       bool skip_artificial, bool &is_static, bool &is_variadic,
                        bool &has_template_params,
                        std::vector<lldb_private::CompilerType> &function_args,
                        std::vector<clang::ParmVarDecl *> &function_param_decls,
@@ -173,33 +179,39 @@ class DWARFASTParserClang : public DWARFASTParser {
 
   size_t ParseChildEnumerators(lldb_private::CompilerType &compiler_type,
                                bool is_signed, uint32_t enumerator_byte_size,
-                               const DWARFDIE &parent_die);
+                               const lldb_private::DWARFDIE &parent_die);
 
   /// Parse a structure, class, or union type DIE.
   lldb::TypeSP ParseStructureLikeDIE(const lldb_private::SymbolContext &sc,
-                                     const DWARFDIE &die,
+                                     const lldb_private::DWARFDIE &die,
                                      ParsedDWARFTypeAttributes &attrs);
 
-  lldb_private::Type *GetTypeForDIE(const DWARFDIE &die);
+  lldb_private::Type *GetTypeForDIE(const lldb_private::DWARFDIE &die);
 
-  clang::Decl *GetClangDeclForDIE(const DWARFDIE &die);
+  clang::Decl *GetClangDeclForDIE(const lldb_private::DWARFDIE &die);
 
-  clang::DeclContext *GetClangDeclContextForDIE(const DWARFDIE &die);
+  clang::DeclContext *
+  GetClangDeclContextForDIE(const lldb_private::DWARFDIE &die);
 
-  clang::DeclContext *GetClangDeclContextContainingDIE(const DWARFDIE &die,
-                                                       DWARFDIE *decl_ctx_die);
-  lldb_private::OptionalClangModuleID GetOwningClangModule(const DWARFDIE &die);
+  clang::DeclContext *
+  GetClangDeclContextContainingDIE(const lldb_private::DWARFDIE &die,
+                                   lldb_private::DWARFDIE *decl_ctx_die);
+  lldb_private::OptionalClangModuleID
+  GetOwningClangModule(const lldb_private::DWARFDIE &die);
 
-  bool CopyUniqueClassMethodTypes(const DWARFDIE &src_class_die,
-                                  const DWARFDIE &dst_class_die,
-                                  lldb_private::Type *class_type,
-                                  std::vector<DWARFDIE> &failures);
+  bool
+  CopyUniqueClassMethodTypes(const lldb_private::DWARFDIE &src_class_die,
+                             const lldb_private::DWARFDIE &dst_class_die,
+                             lldb_private::Type *class_type,
+                             std::vector<lldb_private::DWARFDIE> &failures);
 
-  clang::DeclContext *GetCachedClangDeclContextForDIE(const DWARFDIE &die);
+  clang::DeclContext *
+  GetCachedClangDeclContextForDIE(const lldb_private::DWARFDIE &die);
 
-  void LinkDeclContextToDIE(clang::DeclContext *decl_ctx, const DWARFDIE &die);
+  void LinkDeclContextToDIE(clang::DeclContext *decl_ctx,
+                            const lldb_private::DWARFDIE &die);
 
-  void LinkDeclToDIE(clang::Decl *decl, const DWARFDIE &die);
+  void LinkDeclToDIE(clang::Decl *decl, const lldb_private::DWARFDIE &die);
 
   /// If \p type_sp is valid, calculate and set its symbol context scope, and
   /// update the type list for its backing symbol file.
@@ -207,16 +219,17 @@ class DWARFASTParserClang : public DWARFASTParser {
   /// Returns \p type_sp.
   lldb::TypeSP
   UpdateSymbolContextScopeForType(const lldb_private::SymbolContext &sc,
-                                  const DWARFDIE &die, lldb::TypeSP type_sp);
+                                  const lldb_private::DWARFDIE &die,
+                                  lldb::TypeSP type_sp);
 
   /// Follow Clang Module Skeleton CU references to find a type definition.
   lldb::TypeSP ParseTypeFromClangModule(const lldb_private::SymbolContext &sc,
-                                        const DWARFDIE &die,
+                                        const lldb_private::DWARFDIE &die,
                                         lldb_private::Log *log);
 
   // Return true if this type is a declaration to a type in an external
   // module.
-  lldb::ModuleSP GetModuleForType(const DWARFDIE &die);
+  lldb::ModuleSP GetModuleForType(const lldb_private::DWARFDIE &die);
 
 private:
   struct FieldInfo {
@@ -268,32 +281,37 @@ class DWARFASTParserClang : public DWARFASTParser {
   /// created property.
   /// \param delayed_properties The list of delayed properties that the result
   /// will be appended to.
-  void ParseObjCProperty(const DWARFDIE &die, const DWARFDIE &parent_die,
+  void ParseObjCProperty(const lldb_private::DWARFDIE &die,
+                         const lldb_private::DWARFDIE &parent_die,
                          const lldb_private::CompilerType &class_clang_type,
                          DelayedPropertyList &delayed_properties);
 
   void
-  ParseSingleMember(const DWARFDIE &die, const DWARFDIE &parent_die,
+  ParseSingleMember(const lldb_private::DWARFDIE &die,
+                    const lldb_private::DWARFDIE &parent_die,
                     const lldb_private::CompilerType &class_clang_type,
                     lldb::AccessType default_accessibility,
                     lldb_private::ClangASTImporter::LayoutInfo &layout_info,
                     FieldInfo &last_field_info);
 
-  bool CompleteRecordType(const DWARFDIE &die, lldb_private::Type *type,
+  bool CompleteRecordType(const lldb_private::DWARFDIE &die,
+                          lldb_private::Type *type,
                           lldb_private::CompilerType &clang_type);
-  bool CompleteEnumType(const DWARFDIE &die, lldb_private::Type *type,
+  bool CompleteEnumType(const lldb_private::DWARFDIE &die,
+                        lldb_private::Type *type,
                         lldb_private::CompilerType &clang_type);
 
   lldb::TypeSP ParseTypeModifier(const lldb_private::SymbolContext &sc,
-                                 const DWARFDIE &die,
+                                 const lldb_private::DWARFDIE &die,
                                  ParsedDWARFTypeAttributes &attrs);
   lldb::TypeSP ParseEnum(const lldb_private::SymbolContext &sc,
-                         const DWARFDIE &die, ParsedDWARFTypeAttributes &attrs);
-  lldb::TypeSP ParseSubroutine(const DWARFDIE &die,
+                         const lldb_private::DWARFDIE &die,
+                         ParsedDWARFTypeAttributes &attrs);
+  lldb::TypeSP ParseSubroutine(const lldb_private::DWARFDIE &die,
                                ParsedDWARFTypeAttributes &attrs);
-  lldb::TypeSP ParseArrayType(const DWARFDIE &die,
+  lldb::TypeSP ParseArrayType(const lldb_private::DWARFDIE &die,
                               const ParsedDWARFTypeAttributes &attrs);
-  lldb::TypeSP ParsePointerToMemberType(const DWARFDIE &die,
+  lldb::TypeSP ParsePointerToMemberType(const lldb_private::DWARFDIE &die,
                                         const ParsedDWARFTypeAttributes &attrs);
 
   /// Parses a DW_TAG_inheritance DIE into a base/super class.
@@ -311,7 +329,8 @@ class DWARFASTParserClang : public DWARFASTParser {
   /// \param layout_info The layout information that will be updated for C++
   /// base classes with the base offset.
   void ParseInheritance(
-      const DWARFDIE &die, const DWARFDIE &parent_die,
+      const lldb_private::DWARFDIE &die,
+      const lldb_private::DWARFDIE &parent_die,
       const lldb_private::CompilerType class_clang_type,
       const lldb::AccessType default_accessibility,
       const lldb::ModuleSP &module_sp,
@@ -328,7 +347,8 @@ class DWARFASTParserClang : public DWARFASTParser {
   /// \param layout_info The layout information that will be updated for
   //   base classes with the base offset
   void
-  ParseRustVariantPart(DWARFDIE &die, const DWARFDIE &parent_die,
+  ParseRustVariantPart(lldb_private::DWARFDIE &die,
+                       const lldb_private::DWARFDIE &parent_die,
                        lldb_private::CompilerType &class_clang_type,
                        const lldb::AccessType default_accesibility,
                        lldb_private::ClangASTImporter::LayoutInfo &layout_info);
@@ -338,7 +358,7 @@ class DWARFASTParserClang : public DWARFASTParser {
 /// Some attributes are relevant for all kinds of types (declaration), while
 /// others are only meaningful to a specific type (is_virtual)
 struct ParsedDWARFTypeAttributes {
-  explicit ParsedDWARFTypeAttributes(const DWARFDIE &die);
+  explicit ParsedDWARFTypeAttributes(const lldb_private::DWARFDIE &die);
 
   lldb::AccessType accessibility = lldb::eAccessNone;
   bool is_artificial = false;
@@ -355,12 +375,12 @@ struct ParsedDWARFTypeAttributes {
   const char *mangled_name = nullptr;
   lldb_private::ConstString name;
   lldb_private::Declaration decl;
-  DWARFDIE object_pointer;
-  DWARFFormValue abstract_origin;
-  DWARFFormValue containing_type;
-  DWARFFormValue signature;
-  DWARFFormValue specification;
-  DWARFFormValue type;
+  lldb_private::DWARFDIE object_pointer;
+  lldb_private::DWARFFormValue abstract_origin;
+  lldb_private::DWARFFormValue containing_type;
+  lldb_private::DWARFFormValue signature;
+  lldb_private::DWARFFormValue specification;
+  lldb_private::DWARFFormValue type;
   lldb::LanguageType class_language = lldb::eLanguageTypeUnknown;
   std::optional<uint64_t> byte_size;
   size_t calling_convention = llvm::dwarf::DW_CC_normal;
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFAttribute.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFAttri...
[truncated]

Copy link
Collaborator

@clayborg clayborg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most plug-ins inside of lldb either have their own namespace or none. If there is a larger patch that will go into open source that exposes the lldb_private namespace, then this would be applicable. In that case we would need a liblldb_private.so which contained all of the lldb_private APIs, and then liblldb.so would need to link against that. For Apple systems we would need a LLDBPrivate.framework that LLDB.framework would link against. This would need to all be optional to enable via CMake. So unless this patch also attempts this, I don't see a reason to change these files in open source as you could just make this change in your branch.

@clayborg
Copy link
Collaborator

So no plug-ins currently exist in the lldb_private namespace. I would rather not have all plug-in code end up in the lldb_private namespace either if people want access to these unsafe APIs outside of lldb. Maybe we should be creating a lldb_plugins namespace, and each plugin would have its own namespace inside this, so the DWARF stuff may be in:

namespace lldb_plugins {
namespace dwarf {
...
}
}

Then add new cmake flags as needed to export only certain plugins if/when needed. Don't need to do all plug-ins in this new namespace with this patch, just the DWARF code. We can then allow users to specify exactly what they want to export and it can limit it to just what is needed.

But I am still not a fan of exporting private APIs that are going to change all of the time.

@clayborg clayborg requested a review from jimingham September 29, 2023 22:17
@clayborg
Copy link
Collaborator

I didn't realize there was already a cmake option for exporting the lldb_private namespace when I made my first comments. For this option to truly be useful, I would prefer to have a liblldb_private.so/LLDBPrivate.framework that we would create and then have the liblldb.so/LLDB.framework link against these libraries. We went through great lengths to ensure that we vend a backward compatible API from liblldb.so/LLDB.framework and I would like to keep it that way. If nothing from lldb_private or other internal APIs are exported by enabling a special cmake config variable, then we should only have liblldb.so/LLDB.framework with a backward compatible API. If we enable a flag that exports unsafe APIs we have both a liblldb.so/LLDB.framework whichs links to the unsafe API libraries liblldb_private.so/LLDBPrivate.framework.

@clayborg
Copy link
Collaborator

But the above comments are my personal opinion. I would love to hear from other developers on what they would like to see.

@walter-erquinigo
Copy link
Member Author

walter-erquinigo commented Sep 29, 2023

@clayborg , LLDB has the cmake flag LLDB_EXPORT_ALL_SYMBOLS (see https://github.com/llvm/llvm-project/blob/main/lldb/cmake/modules/AddLLDB.cmake#L177) that is used to export lldb_private as well as lldb symbols. This shows that there's been a precedent for this kind of features and it seems to be well maintained.

Besides that, I do like your idea of exporting specific namespaces for plugins, however for this specific case, I do think that lldb_private is the namespace to use. The main reason is that lldb_private::TypeSystem forces to implement DWARFASTParser, which is a base class with its implementation in the plugin folder. A cleaner API would have all of the necessary bits to implement the DWARFASTParser child in the lldb_private namespace outside of a plugin, because lldb_private::TypeSystem is already outside of any plugin folder, just like other DWARF-related classes like DWARFExpression are outside of plugin folders. I chatted with @bulbazord about moving some DWARF files from the symbol plugin to be at the same level as the type system, but that's particularly challenging because of some coupling between the DWARF parser code and clang/clang typesystem. So this patch at least puts some code in the correct namespace, so to speak.

@walter-erquinigo
Copy link
Member Author

After chatting with @clayborg , I'm going to try a different approach:

  • putting all of these dwarf-related files from the plugin in a lldb_plugins::dwarf namespace.
  • adding a new cmake flag that operates along with LLDB_EXPORT_ALL_SYMBOLS to specify a custom exports file, which would default to lldb/source/API/liblldb-private.exports. That way external plugins would have the option to decide what gets exported and what not.

In the meantime, I'm open to more feedback

@bulbazord
Copy link
Member

I think I am on the same page with Greg for not exposing these symbols for not shoving everything into lldb_private. Perhaps instead of lldb_plugin we can name it something like lldb_private::plugin instead? Not a huge difference, but keeping the top-level private namespace still indicates that it is still a "private" API. Even if most folks aren't going to export lldb_private symbols in their distribution of LLDB, perhaps the folks at Modular would like to do that for the lldb they ship. The fact that the only stable interface is the SBAPI will not change as a result of doing that.

Changing the architecture or the way we build LLDB (having some lldb_private.so/LLDBPrivate.framework) is a pretty big decision that I think will require more buy-in from the community. Whether or not that's the right direction to go in, I'm not sure. But that discussion is much more suitable for the LLVM Discourse.

I am a little curious though, it looks like this change is very DWARF centric. Are you also generating DWARF for Mojo on Windows? Will you be doing something similar for PDBs?

@bulbazord
Copy link
Member

Adding Jonas and Adrian for visibility

@walter-erquinigo
Copy link
Member Author

Perhaps instead of lldb_plugin we can name it something like lldb_private::plugin instead?

Well, that would definitely work for me, as these symbols would be automatically exported alongside all the other lldb_private symbols. What I'm not completely sure about is if we want to go that route for plugins because then all of their symbols would be exported with this flag. @clayborg what do you think?

@clayborg
Copy link
Collaborator

I think I am on the same page with Greg for not exposing these symbols for not shoving everything into lldb_private. Perhaps instead of lldb_plugin we can name it something like lldb_private::plugin instead? Not a huge difference, but keeping the top-level private namespace still indicates that it is still a "private" API.

I think of the lldb_private being the internal LLDB specific code, not plugin code. Right now most plugin code isn't in any namespace and I don't think it belongs in lldb_private.

Even if most folks aren't going to export lldb_private symbols in their distribution of LLDB, perhaps the folks at Modular would like to do that for the lldb they ship. The fact that the only stable interface is the SBAPI will not change as a result of doing that.

This would allow them access to the lldb specific code mostly needed to implement core functionality. But for plug-ins, it would be nice to be able to only export the specific plug-ins needed by folks to keep the number of exported functions down. If the plugin code is in "lldb_private::plugin", then if you export "lldb_private::" you end up exporting all plug-ins whether you want them or not. Keeping them in a different namespace like "lldb_plugins" would allow you to export all of them "lldb_plugins::", or just one with "lldb_plugins::dwarf*".

Changing the architecture or the way we build LLDB (having some lldb_private.so/LLDBPrivate.framework) is a pretty big decision that I think will require more buy-in from the community. Whether or not that's the right direction to go in, I'm not sure. But that discussion is much more suitable for the LLVM Discourse.

Agreed

@jimingham
Copy link
Collaborator

So first off, the lldb_private API is not a stable API and will likely never be. After all, it vends lots of llvm ADT types and other llvm & clang API's which are also not stable API's... So anything which tends to making these API's seem like we're vending them as such, or that it's safe to distribute shared libraries that depend on them is a supported mode is going in the wrong direction. If we want to make the plugin API a stable API for external customers, we'll have to do a bunch more work (including either getting by-in to make ABI stable versions of the underlying llvm/clang API's...)

If you are not planning to make an actual loadable plugin, which I am pretty sure we don't want to support with the lldb_private API's at this time, then I'm not clear why all this work is necessary, as opposed to just linking to the lldb internal headers & building against the .a files.

@walter-erquinigo
Copy link
Member Author

walter-erquinigo commented Oct 2, 2023

@jimingham , indeed, I don't want to have a plugin that could be loadable by any build of lldb. The Mojo SDK is distributing already its own build of vanilla lldb along with the mojo plugin that is loaded at runtime and that links correctly with that lldb.
I initially tried linking the .a files, but that lead to a not short list of dependencies that included clang and other libraries that I also needed to link against, which was not very clean and could lead to potential issues because of the number of symbols to link against becomes huge. In the end after all the discussion, I think that the cleanest solution that could benefit other plugin developers is to allow specifying a custom exports file that can be used instead of third-party/llvm-project/lldb/source/API/liblldb-private.exports offering more control on what to export.

walter-erquinigo pushed a commit to walter-erquinigo/llvm-project that referenced this pull request Oct 5, 2023
LLDB has the cmake flag `LLDB_EXPORT_ALL_SYMBOLS` that exports the lldb, lldb_private namespaces, as well as other symbols like python and lua (see `third-party/llvm-project/lldb/source/API/liblldb-private.exports`). However, not all symbols in lldb fall into these categories and in order to get access to some symbols that live in plugin folders (like dwarf parsing symbols), it's useful to be able to specify a custom exports file giving more control to the developer using lldb as a library.

This adds the new cmake flag `LLDB_EXPORT_ALL_SYMBOLS_EXPORTS_FILE` that is used when `LLDB_EXPORT_ALL_SYMBOLS` is enabled to specify that custom exports file.

This is a follow up of llvm#67851
walter-erquinigo added a commit that referenced this pull request Oct 6, 2023
LLDB has the cmake flag `LLDB_EXPORT_ALL_SYMBOLS` that exports the lldb,
lldb_private namespaces, as well as other symbols like python and lua
(see `lldb/source/API/liblldb-private.exports`). However, not all
symbols in lldb fall into these categories and in order to get access to
some symbols that live in plugin folders (like dwarf parsing symbols),
it's useful to be able to specify a custom exports file giving more
control to the developer using lldb as a library.

This adds the new cmake flag `LLDB_EXPORT_ALL_SYMBOLS_EXPORTS_FILE` that
is used when `LLDB_EXPORT_ALL_SYMBOLS` is enabled to specify that custom
exports file.

This is a follow up of #67851
walter-erquinigo pushed a commit to walter-erquinigo/llvm-project that referenced this pull request Oct 13, 2023
As a followup of llvm#67851, I'm defining a new namespace `lldb_plugin::dwarf` for the classes in this Plugins/SymbolFile/DWARF folder. This change is very NFC and helped me with exporting the necessary symbols for my out-of-tree language plugin. The only two classes that I didn't change are DWARFDataExtractor, because that's being explicitly exported as part of lldb_private in `lldb-forward.h` , and the ClangDWARFASTParser, because that shouldn't be in the same namespace as the generic language-agnostic dwarf parser, but I'm okay with changing that. In any case, even if I didn't need this for my work, adding this namespace could be considered a good practice.
walter-erquinigo added a commit that referenced this pull request Oct 13, 2023
As a followup of #67851, I'm
defining a new namespace `lldb_plugin::dwarf` for the classes in this
Plugins/SymbolFile/DWARF folder. This change is very NFC and helped me
with exporting the necessary symbols for my out-of-tree language plugin.
The only class that I didn't change is ClangDWARFASTParser, because that
shouldn't be in the same namespace as the generic language-agnostic
dwarf parser.
It would be a good idea if other plugins follow the same namespace
scheme.
adrian-prantl added a commit to adrian-prantl/llvm-project that referenced this pull request Oct 25, 2023
As a followup of llvm#67851, I'm
defining a new namespace `lldb_plugin::dwarf` for the classes in this
Plugins/SymbolFile/DWARF folder. This change is very NFC and helped me
with exporting the necessary symbols for my out-of-tree language plugin.
The only class that I didn't change is ClangDWARFASTParser, because that
shouldn't be in the same namespace as the generic language-agnostic
dwarf parser.
It would be a good idea if other plugins follow the same namespace
scheme.

(cherry picked from commit 1673a1b)

 Conflicts:
	lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
	lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants