<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Jun 23, 2017 at 5:47 PM Robinson, Paul <<a href="mailto:paul.robinson@sony.com">paul.robinson@sony.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div lang="EN-US" link="blue" vlink="purple">
<div class="m_-318128881778875500WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">I was thinking that getFixedByteSize could assert that version and address size are non-zero, whenever it was about to use the Params. Then getFixedSizeForAbbrev
could pre-emptively return None for those forms, and default to getFixedByteSize for the rest. </span></p></div></div></blockquote><div><br>Should it return None? If there are params-based forms in abbrev, is it not then necessary to retrieve the params from the unit & no longer possible to use it param-less?<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div class="m_-318128881778875500WordSection1"><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> Because getFixedSizeForAbbrev only passes along "safe" forms, the assertions won't be triggered.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><a name="m_-318128881778875500__MailEndCompose"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Knowledge about which forms depend on which parameters should be encapsulated in DWARFFormValue.</span></a> </p></div></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div class="m_-318128881778875500WordSection1"><p class="MsoNormal"><a name="m_-318128881778875500__MailEndCompose"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> I would not want to recode the abbreviation-table
scanner to know which forms didn't depend on the Params.</span></a></p></div></div></blockquote><div><br>Eh, perhaps. It's sort of part of the contract though at that point ("you can pass empty params for <these particular forms>") so it doesn't seem unreasonable that callers might check that they're conforming to the contract.<br><br>I'm still sort of uncertain how this is OK for abbrevs... <br><br>- Dave<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div class="m_-318128881778875500WordSection1"><p class="MsoNormal"><a name="m_-318128881778875500__MailEndCompose"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u><u></u></span></a></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">--paulr<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<div>
<div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> David Blaikie [mailto:<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>]
<br>
<b>Sent:</b> Friday, June 23, 2017 5:27 PM<br>
<b>To:</b> Robinson, Paul; <a href="mailto:reviews%2BD34570%2Bpublic%2B9d5e31ef9b3178e4@reviews.llvm.org" target="_blank">reviews+D34570+public+9d5e31ef9b3178e4@reviews.llvm.org</a>; <a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a><br>
<b>Cc:</b> <a href="mailto:clayborg@gmail.com" target="_blank">clayborg@gmail.com</a>; <a href="mailto:hiraditya@msn.com" target="_blank">hiraditya@msn.com</a>; <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<b>Subject:</b> Re: [PATCH] D34570: [DWARF] NFC: Collect info needed by DWARFFormValue into a helper<u></u><u></u></span></p>
</div>
</div></div></div></div><div lang="EN-US" link="blue" vlink="purple"><div class="m_-318128881778875500WordSection1"><div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><u></u> <u></u></p>
<div>
<div>
<p class="MsoNormal">On Fri, Jun 23, 2017 at 5:12 PM Robinson, Paul <<a href="mailto:paul.robinson@sony.com" target="_blank">paul.robinson@sony.com</a>> wrote:<u></u><u></u></p>
</div>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<div>
<div>
<p class="MsoNormal">================<br>
Comment at: llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp:64<br>
+DWARFFormValue::getFixedByteSize(dwarf::Form Form,<br>
+ const DWARFFormParams *Params) {<br>
switch (Form) {<br>
----------------<br>
dblaikie wrote:<br>
> Should this be optional? The only place that doesn't pass it looks to be DWARFAbbreviationDeclaration::extract - is that usage correct? Does the abbrev table implicit_const not support the extra features? Should there be some error path/check for any forms
that would require the params?<br>
There is a use-case for scanning the abbreviations table to determine which ones are fixed-size in all cases. And the abbreviations table can be shared by units/line-tables with different formats/versions. This was an important point for @clayborg and LLDB,
I think.<br>
So yes, I think it should remain optional. We can change our minds later if I'm wrong.<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal">Still might be worth moving the optionality to that one use case & adding some comments about why it's probably valid there so it doesn't get accidentally propagated to other users.<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
</div>
<div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">So we would have something like:</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> Optional<uint8_t> getFixedByteSize(dwarf::Form Form, const DWARFFormParams &Params);</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> Optional<uint8_t> getFixedByteSizeForAbbrev(dwarf::Form Form);</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">with commentary reinforcing the special case. I can do that.</span><u></u><u></u></p>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal"><br>
I was thinking abbrev call site could just call: getFixedByteSize(Form, DWARFFormParams()); <br>
<br>
But sure, a separate function that does that might be good. Dunno.<br>
<br>
It'd be good if in some way this was checked somehow to ensure that the forms used by callers not giving DWARFFormParams don't need those properties. I was figuring some assertions at the callsite (but if it has a separate function, the assertions could go
in that function)<br>
<u></u><u></u></p>
</div>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">--paulr</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<div>
<div>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-top:5.0pt;margin-right:0in;margin-bottom:5.0pt">
<p class="MsoNormal" style="margin-bottom:12.0pt"><a href="https://reviews.llvm.org/D34570" target="_blank">https://reviews.llvm.org/D34570</a><u></u><u></u></p>
</blockquote>
</div>
</div>
</div>
</div>
</div>
</blockquote>
</div>
</div>
</div></div></div></blockquote></div></div>