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

Robinson, Paul via llvm-commits llvm-commits at lists.llvm.org
Fri Jun 23 17:47:06 PDT 2017


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.  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.
--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<mailto: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/e5da4df7/attachment.html>


More information about the llvm-commits mailing list