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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
