[Lldb-commits] [PATCH] D52689: [LLDB] - Add support for DW_FORM_implicit_const.

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

See inline comments.



================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h:51
+      uint32_t idx, dw_attr_t &attr, dw_form_t &form,
+      DWARFFormValue::ValueType *val = nullptr) const {
+    m_attributes[idx].get(attr, form, val);
----------------
Switch to using a "DWARFFormValue *form_value_ptr" so the form value can be filled in automatically, not just the       DWARFFormValue::ValueType. See comments below where this is called.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:386
 
+static void setUnsignedOrSigned(int &dest, DWARFFormValue &val) {
+  if (val.Form() == DW_FORM_implicit_const)
----------------
Remove this as it won't be needed if we do the work of filling in the form value in GetAttrAndFormByIndexUnchecked


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:435
     dw_form_t form;
+    DWARFFormValue::ValueType val;
     bool do_offset = false;
----------------
Remove? See inlined comment below.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:439-440
     for (i = 0; i < numAttributes; ++i) {
-      abbrevDecl->GetAttrAndFormByIndexUnchecked(i, attr, form);
-      DWARFFormValue form_value(cu, form);
+      abbrevDecl->GetAttrAndFormByIndexUnchecked(i, attr, form, &val);
+      DWARFFormValue form_value(cu, form, val);
+
----------------
Maybe switch the order here and pass "form_value" to GetAttrAndFormByIndexUnchecked?:

```
      DWARFFormValue form_value(cu, form);
      abbrevDecl->GetAttrAndFormByIndexUnchecked(i, attr, form, &form_value);
```



================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:508
           if (decl_file == 0)
-            decl_file = form_value.Unsigned();
+            setUnsignedOrSigned(decl_file, form_value);
           break;
----------------
Revert this if we do the work inside "GetAttrAndFormByIndexUnchecked" as form_value will be correct


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:512
         case DW_AT_decl_line:
-          if (decl_line == 0)
-            decl_line = form_value.Unsigned();
+          if (decl_line != 0)
+            setUnsignedOrSigned(decl_line, form_value);
----------------
You changed the logic here incorrectly from == to !=


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:513
+          if (decl_line != 0)
+            setUnsignedOrSigned(decl_line, form_value);
           break;
----------------
Revert this if we do the work inside "GetAttrAndFormByIndexUnchecked" as form_value will be correct


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:517
         case DW_AT_decl_column:
-          if (decl_column == 0)
-            decl_column = form_value.Unsigned();
+          if (decl_column != 0)
+            setUnsignedOrSigned(decl_column, form_value);
----------------
You changed the logic here incorrectly from == to !=


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:518
+          if (decl_column != 0)
+            setUnsignedOrSigned(decl_column, form_value);
           break;
----------------
Revert this if we do the work inside "GetAttrAndFormByIndexUnchecked" as form_value will be correct


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:522
         case DW_AT_call_file:
-          if (call_file == 0)
-            call_file = form_value.Unsigned();
+          if (call_file != 0)
+            setUnsignedOrSigned(call_file, form_value);
----------------
You changed the logic here incorrectly from == to !=


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:523
+          if (call_file != 0)
+            setUnsignedOrSigned(call_file, form_value);
           break;
----------------
Revert this if we do the work inside "GetAttrAndFormByIndexUnchecked" as form_value will be correct


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:528
           if (call_line == 0)
-            call_line = form_value.Unsigned();
+            setUnsignedOrSigned(call_line, form_value);
           break;
----------------
Revert this if we do the work inside "GetAttrAndFormByIndexUnchecked" as form_value will be correct


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp:157-159
+DWARFFormValue::DWARFFormValue(const DWARFUnit *cu, dw_form_t form,
+                               ValueType val)
+    : m_cu(cu), m_form(form), m_value(val) {}
----------------
Revert this if we do the work inside "GetAttrAndFormByIndexUnchecked" as form_value will be filled in by that call




================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFFormValue.h:59-60
   DWARFFormValue();
-  DWARFFormValue(const DWARFUnit *cu, dw_form_t form);
+  DWARFFormValue(const DWARFUnit *cu, dw_form_t form,
+                 ValueType val = ValueType());
   const DWARFUnit *GetCompileUnit() const { return m_cu; }
----------------
Revert this if we do the work inside "GetAttrAndFormByIndexUnchecked" as form_value will be filled in by that call


https://reviews.llvm.org/D52689