<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Tue, Feb 28, 2017 at 9:39 AM Victor Leschuk via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">vleschuk marked an inline comment as done.<br class="gmail_msg">
vleschuk added inline comments.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:94-95<br class="gmail_msg">
<br class="gmail_msg">
- if (!formValue.extractValue(U->getDebugInfoExtractor(), OffsetPtr, U))<br class="gmail_msg">
+ if (AS.isImplicitConst())<br class="gmail_msg">
+ formValue.setSValue(*AS.ByteSizeOrValue);<br class="gmail_msg">
+ else if (!formValue.extractValue(U->getDebugInfoExtractor(), OffsetPtr, U))<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> 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).<br class="gmail_msg">
><br class="gmail_msg">
> 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.<br class="gmail_msg">
><br class="gmail_msg">
> So DWARFFormValue should take the AttributeSpec... oh, but that has the attribute in it.<br class="gmail_msg">
><br class="gmail_msg">
> Maybe what's needed is a thing that represents the form +ByteSizeOrValue. Maybe it could be called "FormSpec" to match "AttributeSpec"? Not sure.<br class="gmail_msg">
> 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.<br class="gmail_msg">
<br class="gmail_msg">
Actually DWARFFormValue holds not only enum but actual value too:<br class="gmail_msg"></blockquote><div><br>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)).<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
```<br class="gmail_msg">
struct ValueType {<br class="gmail_msg">
ValueType() {<br class="gmail_msg">
uval = 0;<br class="gmail_msg">
}<br class="gmail_msg">
<br class="gmail_msg">
union {<br class="gmail_msg">
uint64_t uval;<br class="gmail_msg">
int64_t sval;<br class="gmail_msg">
const char* cstr;<br class="gmail_msg">
};<br class="gmail_msg">
const uint8_t* data = nullptr;<br class="gmail_msg">
};<br class="gmail_msg">
<br class="gmail_msg">
dwarf::Form Form; // Form for this value.<br class="gmail_msg">
ValueType Value; // Contains all data for the form. struct ValueType {<br class="gmail_msg">
ValueType() {<br class="gmail_msg">
uval = 0;<br class="gmail_msg">
}<br class="gmail_msg">
<br class="gmail_msg">
union {<br class="gmail_msg">
uint64_t uval;<br class="gmail_msg">
int64_t sval;<br class="gmail_msg">
const char* cstr;<br class="gmail_msg">
};<br class="gmail_msg">
const uint8_t* data = nullptr;<br class="gmail_msg">
};<br class="gmail_msg">
<br class="gmail_msg">
dwarf::Form Form; // Form for this value.<br class="gmail_msg">
ValueType Value; // Contains all data for the form.<br class="gmail_msg">
struct ValueType {<br class="gmail_msg">
ValueType() {<br class="gmail_msg">
uval = 0;<br class="gmail_msg">
}<br class="gmail_msg">
<br class="gmail_msg">
union {<br class="gmail_msg">
uint64_t uval;<br class="gmail_msg">
int64_t sval;<br class="gmail_msg">
const char* cstr;<br class="gmail_msg">
};<br class="gmail_msg">
const uint8_t* data = nullptr;<br class="gmail_msg">
};<br class="gmail_msg">
<br class="gmail_msg">
dwarf::Form Form; // Form for this value.<br class="gmail_msg">
ValueType Value; // Contains all data for the form.<br class="gmail_msg">
```<br class="gmail_msg">
<br class="gmail_msg">
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:<br class="gmail_msg">
<br class="gmail_msg">
```<br class="gmail_msg">
formValue.extractValue(/* ... */, int64_t actualValue);<br class="gmail_msg">
// ...<br class="gmail_msg">
bool DWARFFormValue::extractValue(/* ... */, int64_t actualValue) {<br class="gmail_msg">
// ....<br class="gmail_msg">
case DW_FORM_implicit_const:<br class="gmail_msg">
Value.sdata = actualValue;<br class="gmail_msg">
break;<br class="gmail_msg">
// ...<br class="gmail_msg">
```<br class="gmail_msg">
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.<br class="gmail_msg"></blockquote><div><br></div><div>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.<br><br>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.<br><br>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).<br><br>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.<br><br>- Dave</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D30448" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D30448</a><br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div></div>