[lldb][NFC] Move generic DWARFASTParser code out of Clang-specific code

[ljmf00]

Summary

This patch moves ParseChildArrayInfo out of DWARFASTParserClang in order to decouple Clang-specific logic from DWARFASTParser.

[ljmf00]

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.cpp

78

Why we are including just these specific attributes? Maybe we should add a comment explaining it. According to the DWARF standard, any attribute is eligible for any tag.

[bulbazord]

I think breaking it out of the Clang-specific class makes sense if we want LLDB to be more language-agnostic. Do you have an idea of what bits of DWARFASTParserClang can be moved out other than ParseChildArrayInfo and GetAccessTypeFromDWARF (from the patch on top of this)? What is your end-goal with this decoupling? I assume you want to work towards supporting languages non-clang-based languages but I'm curious about the motivation.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.cpp

2

This file will need a license header.

78

I'm not sure why. Possibly they were added to make sure the switch was fully covered (potentially to silence a warning)? You could add a FIXME or a TODO if you feel that these attributes should have functionality associated with them like the ones above.

[ljmf00]

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.cpp

2

Yes, my bad. Thanks for noticing it.

78

I don't think it is to mark it as fully covered since there are much more attributes, the default label will address it anyway, and according to the DWARF standard, any attribute can be in a type tag (realistically, any tag). We can take the example of DW_AT_description which is just a description associated with the symbol. I feel like this can be safely deleted but I'm afraid to do it in favour of some other rationale I'm not seeing.

[shafik]

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.cpp

78

@clayborg it looks like this has been this way since you put this in: https://github.com/llvm/llvm-project/commit/261ac3f4b5b98d02dd8718078015a92cf07df736

Do you agree this is dead code or is there something we are missing?

[clayborg]

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.cpp

78

Fine to remove these extra attributes that are in the default case as we definitely don't have all of the attributes listed here.

[ljmf00]

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.cpp

2

Added.

78

Done.

[ljmf00]

In D114668#3159640, @bulbazord wrote:

I think breaking it out of the Clang-specific class makes sense if we want LLDB to be more language-agnostic. Do you have an idea of what bits of DWARFASTParserClang can be moved out other than ParseChildArrayInfo and GetAccessTypeFromDWARF (from the patch on top of this)? What is your end-goal with this decoupling? I assume you want to work towards supporting languages non-clang-based languages but I'm curious about the motivation.

@bulbazord Yes, my plan is to make LLDB interfaces more language-agnostic, to accommodate D programming language DWARFASTParser and TypeSystem. I've seen other language plugins such as Go that simply copy and paste this method, but I want to make D addition clearer and avoid such duplication. You can see more similar changes on Clang-specific code decoupling on the stacked changes.

I have made the requested changes, can you re-review, please? Also pinging @shafik .

[ljmf00]

Ping @shafik @bulbazord . Can you re-review?

[ljmf00]

Sorry for leaving this hanging for a long time. Feel free to leave feedback if I'm missing something, in the meantime.

I'll try to push the other patches too. I ran the test suite again and didn't break anything.

lldb/source/Plugins/SymbolFile/DWARF/CMakeLists.txt

 lldb_tablegen(SymbolFileDWARFProperties.inc -gen-lldb-property-defs
   SOURCE SymbolFileDWARFProperties.td
   TARGET LLDBPluginSymbolFileDWARFPropertiesGen)
 
 lldb_tablegen(SymbolFileDWARFPropertiesEnum.inc -gen-lldb-property-enum-defs
   SOURCE SymbolFileDWARFProperties.td
   TARGET LLDBPluginSymbolFileDWARFPropertiesEnumGen)
 
 add_lldb_library(lldbPluginSymbolFileDWARF PLUGIN
   AppleDWARFIndex.cpp
   DebugNamesDWARFIndex.cpp
   DIERef.cpp
   DWARFAbbreviationDeclaration.cpp
+  DWARFASTParser.cpp
   DWARFASTParserClang.cpp
   DWARFAttribute.cpp
   DWARFBaseDIE.cpp
   DWARFCompileUnit.cpp
   DWARFContext.cpp
   DWARFDataExtractor.cpp
   DWARFDebugAbbrev.cpp
   DWARFDebugAranges.cpp

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.cpp

+//===-- DWARFASTParser.cpp ------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "DWARFASTParser.h"
+#include "DWARFAttribute.h"
+#include "DWARFDIE.h"
+
+#include "lldb/Core/ValueObject.h"
+#include "lldb/Symbol/SymbolFile.h"
+#include "lldb/Target/StackFrame.h"
+
+using namespace lldb;
+using namespace lldb_private;
+using namespace lldb_private::dwarf;
+
+llvm::Optional<SymbolFile::ArrayInfo>
+DWARFASTParser::ParseChildArrayInfo(const DWARFDIE &parent_die,
+                                    const ExecutionContext *exe_ctx) {
+  SymbolFile::ArrayInfo array_info;
+  if (!parent_die)
+    return llvm::None;
+
+  for (DWARFDIE die : parent_die.children()) {
+    const dw_tag_t tag = die.Tag();
+    if (tag != DW_TAG_subrange_type)
+      continue;
+
+    DWARFAttributes attributes;
+    const size_t num_child_attributes = die.GetAttributes(attributes);
+    if (num_child_attributes > 0) {
+      uint64_t num_elements = 0;
+      uint64_t lower_bound = 0;
+      uint64_t upper_bound = 0;
+      bool upper_bound_valid = false;
+      uint32_t i;
+      for (i = 0; i < num_child_attributes; ++i) {
+        const dw_attr_t attr = attributes.AttributeAtIndex(i);
+        DWARFFormValue form_value;
+        if (attributes.ExtractFormValueAtIndex(i, form_value)) {
+          switch (attr) {
+          case DW_AT_name:
+            break;
+
+          case DW_AT_count:
+            if (DWARFDIE var_die = die.GetReferencedDIE(DW_AT_count)) {
+              if (var_die.Tag() == DW_TAG_variable)
+                if (exe_ctx) {
+                  if (auto frame = exe_ctx->GetFrameSP()) {
+                    Status error;
+                    lldb::VariableSP var_sp;
+                    auto valobj_sp = frame->GetValueForVariableExpressionPath(
+                        var_die.GetName(), eNoDynamicValues, 0, var_sp, error);
+                    if (valobj_sp) {
+                      num_elements = valobj_sp->GetValueAsUnsigned(0);
+                      break;
+                    }
+                  }
+                }
+            } else
+              num_elements = form_value.Unsigned();
+            break;
+
+          case DW_AT_bit_stride:
+            array_info.bit_stride = form_value.Unsigned();
+            break;
+
+          case DW_AT_byte_stride:
+            array_info.byte_stride = form_value.Unsigned();
+            break;
+
+          case DW_AT_lower_bound:
+            lower_bound = form_value.Unsigned();
+            break;
+
+          case DW_AT_upper_bound:
+            upper_bound_valid = true;
+            upper_bound = form_value.Unsigned();
+            break;
+
+          default:
+            break;
+          }
+        }
+      }
+
+      if (num_elements == 0) {
+        if (upper_bound_valid && upper_bound >= lower_bound)
+          num_elements = upper_bound - lower_bound + 1;
+      }
+
+      array_info.element_orders.push_back(num_elements);
+    }
+  }
+  return array_info;
+}

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

for (DWARFDIE die : parent_die.children()) {
 
     default:
       break;
     }
   }
   return arg_idx;
 }
 
-llvm::Optional<SymbolFile::ArrayInfo>
-DWARFASTParser::ParseChildArrayInfo(const DWARFDIE &parent_die,
-                                    const ExecutionContext *exe_ctx) {
-  SymbolFile::ArrayInfo array_info;
-  if (!parent_die)
-    return llvm::None;
-
-  for (DWARFDIE die : parent_die.children()) {
-    const dw_tag_t tag = die.Tag();
-    if (tag != DW_TAG_subrange_type)
-      continue;
-
-    DWARFAttributes attributes;
-    const size_t num_child_attributes = die.GetAttributes(attributes);
-    if (num_child_attributes > 0) {
-      uint64_t num_elements = 0;
-      uint64_t lower_bound = 0;
-      uint64_t upper_bound = 0;
-      bool upper_bound_valid = false;
-      uint32_t i;
-      for (i = 0; i < num_child_attributes; ++i) {
-        const dw_attr_t attr = attributes.AttributeAtIndex(i);
-        DWARFFormValue form_value;
-        if (attributes.ExtractFormValueAtIndex(i, form_value)) {
-          switch (attr) {
-          case DW_AT_name:
-            break;
-
-          case DW_AT_count:
-            if (DWARFDIE var_die = die.GetReferencedDIE(DW_AT_count)) {
-              if (var_die.Tag() == DW_TAG_variable)
-                if (exe_ctx) {
-                  if (auto frame = exe_ctx->GetFrameSP()) {
-                    Status error;
-                    lldb::VariableSP var_sp;
-                    auto valobj_sp = frame->GetValueForVariableExpressionPath(
-                        var_die.GetName(), eNoDynamicValues, 0, var_sp,
-                        error);
-                    if (valobj_sp) {
-                      num_elements = valobj_sp->GetValueAsUnsigned(0);
-                      break;
-                    }
-                  }
-                }
-            } else
-              num_elements = form_value.Unsigned();
-            break;
-
-          case DW_AT_bit_stride:
-            array_info.bit_stride = form_value.Unsigned();
-            break;
-
-          case DW_AT_byte_stride:
-            array_info.byte_stride = form_value.Unsigned();
-            break;
-
-          case DW_AT_lower_bound:
-            lower_bound = form_value.Unsigned();
-            break;
-
-          case DW_AT_upper_bound:
-            upper_bound_valid = true;
-            upper_bound = form_value.Unsigned();
-            break;
-
-          default:
-          case DW_AT_abstract_origin:
-          case DW_AT_accessibility:
-          case DW_AT_allocated:
-          case DW_AT_associated:
-          case DW_AT_data_location:
-          case DW_AT_declaration:
-          case DW_AT_description:
-          case DW_AT_sibling:
-          case DW_AT_threads_scaled:
-          case DW_AT_type:
-          case DW_AT_visibility:
-            break;
-          }
-        }
-      }
-
-      if (num_elements == 0) {
-        if (upper_bound_valid && upper_bound >= lower_bound)
-          num_elements = upper_bound - lower_bound + 1;
-      }
-
-      array_info.element_orders.push_back(num_elements);
-    }
-  }
-  return array_info;
-}
-
 Type *DWARFASTParserClang::GetTypeForDIE(const DWARFDIE &die) {
   if (die) {
     SymbolFileDWARF *dwarf = die.GetDWARF();
     DWARFAttributes attributes;
     const size_t num_attributes = die.GetAttributes(attributes);
     if (num_attributes > 0) {
       DWARFFormValue type_die_form;
       for (size_t i = 0; i < num_attributes; ++i) {