[PATCH] D30448: [DebugInfo] Show implicit_const values when dumping .debug_info section

[David Blaikie via llvm-commits]

On Tue, Feb 28, 2017 at 9:55 AM Victor Leschuk <vleschuk at accesssoftek.com>
wrote:

> Maybe we should just add one more constructor for DWARFFormValue which
> would set both Form and Value fields?
>
That's /sort/ of what I'm suggesting, but providing a wrapper around that
data so the code here doesn't have to have explicit support for values. It
should be part of some 'form' abstraction/concept (that carries both the
form enumerator and any other data is needed - the value).

>
> On 02/28/2017 08:49 PM, David Blaikie wrote:
>
>
>
> On Tue, Feb 28, 2017 at 9:39 AM Victor Leschuk via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
> vleschuk marked an inline comment as done.
> vleschuk added inline comments.
>
>
> ================
> Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:94-95
>
> -  if (!formValue.extractValue(U->getDebugInfoExtractor(), OffsetPtr, U))
> +  if (AS.isImplicitConst())
> +    formValue.setSValue(*AS.ByteSizeOrValue);
> +  else if (!formValue.extractValue(U->getDebugInfoExtractor(), OffsetPtr,
> U))
> ----------------
> dblaikie wrote:
> > I wouldn't expect to see a special case here. Is it necessary? I'd
> expect extractValue to do the work necessary for whatever the attribute
> kind is (as it already does for form_present, I assume).
> >
> > I guess the issue is that DWARFFormValue only holds the form enum, not
> the rest - I think that probably should be changed so that extractValue can
> still behave uniformly across all forms.
> >
> > So DWARFFormValue should take the AttributeSpec... oh, but that has the
> attribute in it.
> >
> > Maybe what's needed is a thing that represents the form
> +ByteSizeOrValue. Maybe it could be called "FormSpec" to match
> "AttributeSpec"? Not sure.
> > I guess the issue is that DWARFFormValue only holds the form enum, not
> the rest - I think that probably should be changed so that extractValue can
> still behave uniformly across all forms.
>
> Actually DWARFFormValue holds not only enum but actual value too:
>
>
> Right, sorry, I meant it doesn't include the other parts of the form
> representation (the stuff that's in AttributeSpec, like the value (or
> size)).
>
>
>
> ```
>   struct ValueType {
>     ValueType() {
>       uval = 0;
>     }
>
>     union {
>       uint64_t uval;
>       int64_t sval;
>       const char* cstr;
>     };
>     const uint8_t* data = nullptr;
>   };
>
>   dwarf::Form Form; // Form for this value.
>   ValueType Value; // Contains all data for the form.
>        struct ValueType {
>     ValueType() {
>       uval = 0;
>     }
>
>     union {
>       uint64_t uval;
>       int64_t sval;
>       const char* cstr;
>     };
>     const uint8_t* data = nullptr;
>   };
>
>   dwarf::Form Form; // Form for this value.
>   ValueType Value; // Contains all data for the form.
>   struct ValueType {
>     ValueType() {
>       uval = 0;
>     }
>
>     union {
>       uint64_t uval;
>       int64_t sval;
>       const char* cstr;
>     };
>     const uint8_t* data = nullptr;
>   };
>
>   dwarf::Form Form; // Form for this value.
>   ValueType Value; // Contains all data for the form.
> ```
>
> What extractValue() does - it actually fills the ValueType field of
> DWARFFormValue, for flag_present it just sets Value to "true" and doesn't
> need additional data. To fill in implicit_const from inside of
> extractValue() we need to pass the actual value to it and than assign it
> from within:
>
> ```
> formValue.extractValue(/* ... */, int64_t actualValue);
> // ...
> bool DWARFFormValue::extractValue(/* ... */, int64_t actualValue) {
>   // ....
>   case DW_FORM_implicit_const:
>   Value.sdata = actualValue;
>   break;
>   // ...
> ```
> That looks a little bit weird for me, as lots of useless actions are
> performed like passing data back and forth. I think having exceptional case
> for implicit_const in upper stack frame is better way here.
>
>
> Right - I'm not suggesting passing in the value only to 'return' it again.
> I'm wondering if the abstraction could be a bit higher than that.
> Fundamentally implicit_const changed the semantics of mapping form +
> attribute value -> value. Now that mapping involves more than only the form
> kind, but the associated value as well. So I'm wondering if the API
> can/should be changed to reflect that generalization, rather than with a
> special case here.
>
> What I'm imagining/roughly picturing is grouping the
> {form+byteSizeOrValue} into a FormSpec and passing the FormSpec to
> DWARFFormValue, rather than passing only the form enumerator.
>
> One could imagine future forms that work like implicit_value - that have
> ways of encoding some or all of their bytes (granted, if it were only
> 'some', more API changes would be needed to support a form with some part
> of its value in the abbreviation, and some part of its value in the DIE
> itself - I'm not suggesting providing /that/ generality until it's needed)
> in the abbreviation (eg: there's sdata and udata abbreviations - but no way
> to designate sign/unsigned on an implicit_value, so perhaps that may one
> day be needed).
>
> Avoiding special cases & keeping the form -> value mapping in one place
> (by widening the API a bit to pass through all the necessary data in a
> general manner) seems desirable to me.
>
> - Dave
>
>
>
>
> https://reviews.llvm.org/D30448
>
>
>
>
> --
> Best Regards,
> Victor
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170228/c5a55017/attachment.html>