<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<p>Maybe we should just add one more constructor for DWARFFormValue
which would set both Form and Value fields?<br>
</p>
<br>
<div class="moz-cite-prefix">On 02/28/2017 08:49 PM, David Blaikie
wrote:<br>
</div>
<blockquote
cite="mid:CAENS6EsFr9b88UJfVyqQN3zxo6SiPj_UTLC2ZjJiN8AbnjX7Lg@mail.gmail.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<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 moz-do-not-send="true"
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 moz-do-not-send="true"
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>
<pre class="moz-signature" cols="72">--
Best Regards,
Victor</pre>
</body>
</html>