<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Tue, Feb 28, 2017 at 9:55 AM Victor Leschuk <<a href="mailto:vleschuk@accesssoftek.com">vleschuk@accesssoftek.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000" class="gmail_msg">
<p class="gmail_msg">Maybe we should just add one more constructor for DWARFFormValue
which would set both Form and Value fields?</p></div></blockquote><div>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).</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000" class="gmail_msg"><p class="gmail_msg">
</p></div><div bgcolor="#FFFFFF" text="#000000" class="gmail_msg">
<br class="gmail_msg">
<div class="m_-2245204638514289680moz-cite-prefix gmail_msg">On 02/28/2017 08:49 PM, David Blaikie
wrote:<br class="gmail_msg">
</div>
<blockquote type="cite" class="gmail_msg">
<div dir="ltr" class="gmail_msg"><br class="gmail_msg">
<br class="gmail_msg">
<div class="gmail_quote gmail_msg">
<div dir="ltr" class="gmail_msg">On Tue, Feb 28, 2017 at 9:39 AM Victor Leschuk
via Phabricator <<a href="mailto:reviews@reviews.llvm.org" class="gmail_msg" target="_blank">reviews@reviews.llvm.org</a>>
wrote:<br class="gmail_msg">
</div>
<blockquote class="gmail_quote gmail_msg" 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 class="gmail_msg"><br class="gmail_msg">
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 class="gmail_msg">
</div>
<blockquote class="gmail_quote gmail_msg" 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 class="gmail_msg"><br class="gmail_msg">
</div>
<div class="gmail_msg">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 class="gmail_msg">
<br class="gmail_msg">
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 class="gmail_msg">
<br class="gmail_msg">
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 class="gmail_msg">
<br class="gmail_msg">
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 class="gmail_msg">
<br class="gmail_msg">
- Dave</div>
<div class="gmail_msg"> </div>
<blockquote class="gmail_quote gmail_msg" 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>
</blockquote>
<br class="gmail_msg">
</div><div bgcolor="#FFFFFF" text="#000000" class="gmail_msg"><pre class="m_-2245204638514289680moz-signature gmail_msg" cols="72">--
Best Regards,
Victor</pre>
</div></blockquote></div></div>