[PATCH] D34570: [DWARF] NFC: Collect info needed by DWARFFormValue into a helper

[David Blaikie via llvm-commits]

On Fri, Jun 23, 2017 at 5:47 PM Robinson, Paul <paul.robinson at sony.com>
wrote:

> 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.
>

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?


> Because getFixedSizeForAbbrev only passes along "safe" forms, the
> assertions won't be triggered.
>
>
>
> Knowledge about which forms depend on which parameters should be
> encapsulated in DWARFFormValue.
>
  I would not want to recode the abbreviation-table scanner to know which
> forms didn't depend on the Params.
>

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.

I'm still sort of uncertain how this is OK for abbrevs...

- Dave


> --paulr
>
>
>
> *From:* David Blaikie [mailto:dblaikie at gmail.com]
> *Sent:* Friday, June 23, 2017 5:27 PM
> *To:* Robinson, Paul;
> reviews+D34570+public+9d5e31ef9b3178e4 at reviews.llvm.org; aprantl at apple.com
> *Cc:* clayborg at gmail.com; hiraditya at msn.com; llvm-commits at lists.llvm.org
> *Subject:* Re: [PATCH] D34570: [DWARF] NFC: Collect info needed by
> DWARFFormValue into a helper
>
>
>
>
>
> On Fri, Jun 23, 2017 at 5:12 PM Robinson, Paul <paul.robinson at sony.com>
> wrote:
>
> ================
> Comment at: llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp:64
> +DWARFFormValue::getFixedByteSize(dwarf::Form Form,
> +                                 const DWARFFormParams *Params) {
>    switch (Form) {
> ----------------
> dblaikie wrote:
> > 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?
> 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.
> So yes, I think it should remain optional.  We can change our minds later
> if I'm wrong.
>
>
>
> 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.
>
>
>
> So we would have something like:
>
>     Optional<uint8_t> getFixedByteSize(dwarf::Form Form, const
> DWARFFormParams &Params);
>
>     Optional<uint8_t> getFixedByteSizeForAbbrev(dwarf::Form Form);
>
> with commentary reinforcing the special case.  I can do that.
>
>
> I was thinking abbrev call site could just call: getFixedByteSize(Form,
> DWARFFormParams());
>
> But sure, a separate function that does that might be good. Dunno.
>
> 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)
>
>
> --paulr
>
>
>
> https://reviews.llvm.org/D34570
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170624/882c5fbc/attachment.html>