Re: FDE with 0 PC range filtering for gold

[Cary Coutant <ccoutant at gmail dot com>]

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.

Attachment: pr19735.patch
Description: Binary data