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

[George Rimar via Phabricator via lldb-commits]

grimar added 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);
----------------
clayborg wrote:
> 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.
Answered below.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:386
 
+static void setUnsignedOrSigned(int &dest, DWARFFormValue &val) {
+  if (val.Form() == DW_FORM_implicit_const)
----------------
clayborg wrote:
> Remove this as it won't be needed if we do the work of filling in the form value in GetAttrAndFormByIndexUnchecked
Answered 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);
+
----------------
clayborg wrote:
> Maybe switch the order here and pass "form_value" to GetAttrAndFormByIndexUnchecked?:
> 
> ```
>       DWARFFormValue form_value(cu, form);
>       abbrevDecl->GetAttrAndFormByIndexUnchecked(i, attr, form, &form_value);
> ```
> 
> DWARFFormValue form_value(cu, form);

We can not do this because at this point `form` is not yet known.
I changed the code in a bit different way though.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:508
           if (decl_file == 0)
-            decl_file = form_value.Unsigned();
+            setUnsignedOrSigned(decl_file, form_value);
           break;
----------------
clayborg wrote:
> Revert this if we do the work inside "GetAttrAndFormByIndexUnchecked" as form_value will be correct
The problem I see here is the following:

DWARF5 spec says (2.14 Declaration Coordinates, p50):
"Any debugging information entry representing the declaration of an object,
module, subprogram or type may have DW_AT_decl_file, DW_AT_decl_line and
DW_AT_decl_column attributes, each of whose value is an **unsigned integer
constant**."

But about `DW_FORM_implicit_const` it says (p207):

"The attribute form DW_FORM_implicit_const **is another special case**. For
attributes with this form, the attribute specification contains a third part, which is
a **signed LEB128 number**. The value of this number is used as the value of the
attribute, and no value is stored in the .debug_info section."

So I read `DW_FORM_implicit_const` to `value.sval` and I think we can not
use `form_value.Unsigned();` in that case because it reads from `uval`.
Writing to one union field and reading from the another is undefined behavior in C++ I believe.
Though it works with the modern compilers I think.

DWARFFormValue.h contains dead enum (it is unused)
https://github.com/llvm-mirror/lldb/blob/master/source/Plugins/SymbolFile/DWARF/DWARFFormValue.h#L51
which intention seems was about to keep the value type information.
Should we start to set and check the type of form value?

I removed the helper for now, but this UB should be addressed somehow I think?

Should we create`DWARFFormValue::GetSignedOrUnsigned` may be? It perhaps will be consistent
with `DWARFFormValue::Address`/`AsCString`which check the form type and returned value depends
on that.


https://reviews.llvm.org/D52689