[PATCH] D26514: Get rid of DWARFFormValue::getFixedFormSizes(uint8_t AddrSize, uint16_t Version).

[David Blaikie via llvm-commits]

On Thu, Nov 10, 2016 at 11:53 AM Greg Clayton <clayborg at gmail.com> wrote:

> clayborg added a comment.
>
> Will update according to comments.
>
>
>
> ================
> Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:57
>
> +Optional<uint8_t> DWARFFormValue::getFixedByteSize(dwarf::Form form,
> +                                                   const DWARFUnit *U) {
> ----------------
> aprantl wrote:
> > Would it make sense / be possible to encode this tabular data (at least
> for the majority of values) in the macros in Dwarf.def?
> Not sure how you would do this since many don't have a byte size. Zero is
> a valid DW_FORM size, so you can't use zero for items that don't have byte
> sizes. I wouldn't want to have a signed integer as the size either where -1
> would indicate that there is no size.


Why would this (signed values with -1 as a flag value, or unsigned with
MAX_INT as the flag) be particularly problematic?


> So since there is no easy way to encode an optional value with no value
> into the table, I would vote against this.


'None' would be the empty Optional value, which could be in the table o'
macros.


> Agains, this is the only function that needs to be updated.


>
> ================
> Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:364
>
> -bool
> -DWARFFormValue::skipValue(DataExtractor debug_info_data, uint32_t*
> offset_ptr,
> -                          const DWARFUnit *U) const {
> -  return DWARFFormValue::skipValue(Form, debug_info_data, offset_ptr, U);
> -}
> -
> -bool
> -DWARFFormValue::skipValue(dwarf::Form form, DataExtractor debug_info_data,
> -                          uint32_t *offset_ptr, const DWARFUnit *cu) {
> -  return skipValue(form, debug_info_data, offset_ptr, cu->getVersion(),
> -                   cu->getAddressByteSize());
> -}
> -bool
> -DWARFFormValue::skipValue(dwarf::Form form, DataExtractor debug_info_data,
> -                          uint32_t *offset_ptr, uint16_t Version,
> -                          uint8_t AddrSize) {
> +bool skipVariableLengthValue(dwarf::Form form,
> +                             const DataExtractor &debug_info_data,
> ----------------
> aprantl wrote:
> > I know it was like this in the original source, but while you are
> editing it, we could use the opportunity fix the variable names to start
> with uppercase (
> http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly).
> Up to you.
> I desire to keep this patch small so the extra diffs don't cloud our
> ability to see changes. I will do a follow up patch where I do nothing but
> case changing.
>
>
> ================
> Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:424
> +      } else {
> +        assert(!"only variable length Form can be passed to this
> function");
> +      }
> ----------------
> aprantl wrote:
> > Should this really be an assertion, in the sense of that it indicates a
> programming error in the caller? Or can this situation be triggered by
> malformed DWARF input and should we return an Error instead?
> This function is internal to this file and is designed to only be called
> for variable length forms. The callers of this function both do:
>
> ```
>   if (Optional<uint8_t> FixedByteSize = getFixedByteSize(form, U)) {
>     *offset_ptr += *FixedByteSize;
>     return true;
>   }
>   return skipVariableLengthValue(...);
> ```
>
> I turned this into an llvm_unreachable().
>
>
> https://reviews.llvm.org/D26514
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161110/9b3f7857/attachment.html>