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

[Victor Leschuk via llvm-commits]

Maybe we should just add one more constructor for DWARFFormValue which 
would set both Form and Value fields?


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 <mailto: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/b6adc160/attachment.html>