Re: FDE with 0 PC range filtering for gold

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Fri, Feb 26, 2016 at 8:05 AM, Cary Coutant <ccoutant@gmail.com> wrote:
> Thanks for the patch!
>
>> gold/ChangeLog:
>>
>> Egor Kochetov <egor.kochetov@intel.com>
>>       * ehframe.h: updated function prototypes to enable discarding
>> FDEs with 0 PC range
>>       * ehframe.cc: added filtering of FDEs to discard those with 0 PC range.
>
> The ChangeLog entry needs more detail -- there should be a separate
> item for each function modified.
>
> +  int fde_pc_encoding;
>
> This is uninitialized, and may not be initialized in read_cie() if
> there is no 'R' augmentation.
>
> No matter, because it's actually unnecessary....
>
>           if (!this->read_cie(object, shndx, symbols, symbols_size,
>                               symbol_names, symbol_names_size,
>                               pcontents, p, pentend, &relocs, &cies,
> -                             new_cies))
> +                             new_cies, fde_pc_encoding))
>             return false;
>         }
>        else
>         {
>           // FDE.
>           if (!this->read_fde(object, shndx, symbols, symbols_size,
> -                             pcontents, id, p, pentend, &relocs, &cies))
> +                             pcontents, id, p, pentend, &relocs, &cies,
> +                  fde_pc_encoding))
>
> Indentation is off.
>
> Saving the encoding when reading the CIE, and using it when reading
> the next FDE is not only unnecessary, but wrong. Each FDE begins with
> a pointer to the CIE that it is based on. (While the x86 psABI says
> that the CIE pointer points the the "nearest preceding CIE", neither
> the LSB nor the DWARF spec for call frame information say that, so it
> is dangerous to assume that an FDE will always refer to the last CIE
> we saw. I hope the x86 psABI doesn't actually mean that, because that
> would make the CIE optimizations we do invalid.)
>
> -                  New_cies* new_cies)
> +                  New_cies* new_cies,
> +                  int& fde_pc_encoding)
>
> This should be a pointer, not a reference. We only use reference
> parameters when they can be const.
>
> -         switch (fde_encoding & 7)
> +         switch ((fde_pc_encoding = fde_encoding & 7))
>
> I'd prefer not to embed assignments in the middle of a switch
> expression. This objection is moot, though, since we don't need to
> save it here.
>
> +  switch (fde_pc_encoding) {
>
> Here, we can use the FDE encoding stored in the CIE, which we already
> have a pointer to.
>
> +      case elfcpp::DW_EH_PE_udata2: is_zero_range = *(uint16_t*)(pfde
> + 2) == 0; break;
> +      case elfcpp::DW_EH_PE_udata4: is_zero_range = *(uint32_t*)(pfde
> + 4) == 0; break;
> +      case elfcpp::DW_EH_PE_udata8: is_zero_range = *(uint64_t*)(pfde
> + 8) == 0; break;
>
> These lines are too long.
>
> In gold, we use static_cast<>() instead of old-style casts.
>
> Since we're only testing for zero here, we don't strictly need to
> honor the byte order, but I'd still prefer to use
> elfcpp::Swap::readval() for consistency and safety (e.g., in case
> someone later modifies this code or copies it somewhere else where
> we're not just testing for 0).
>
> +      default:
> +        // All other cases were rejected in Eh_frame::read_cie.
> +        gold_unreachable();
>
> Actually, read_cie() doesn't reject DW_EH_PE_absptr, which you haven't
> handled here.
>
> +  if (is_zero_range
> +      || (is_ordinary
>        && fde_shndx != elfcpp::SHN_UNDEF
>        && fde_shndx < object->shnum()
> -      && !object->is_section_included(fde_shndx))
> +      && !object->is_section_included(fde_shndx)))
>
> Indentation needs adjusting.
>
> I've committed the attached patch.
>
> -cary
>
>
> 2016-02-26  Egor Kochetov  <egor.kochetov@intel.com>
>             Cary Coutant  <ccoutant@gmail.com>
>
>         PR gold/19735
>         * ehframe.h (Cie::fde_encoding): New method.
>         * ehframe.cc (Eh_frame::read_fde): Discard FDEs for zero-length
>         address ranges.

It would be nice to include an assembly testcase and verify
frame info is correct.

-- 
H.J.