[PATCH gold/21066] Consider C++ exception handling metadata during ICF

[Joshua Oreman]

On Wed, May 8, 2019 at 7:04 PM Cary Coutant <ccoutant@gmail.com> wrote:
>
> > Checking in several months later -- should I still be expecting this patch to eventually get reviewed?
>
> Sorry for dropping the ball on this -- I've been busy with other
> projects lately, and only just got back to reviewing your patch. I
> wanted to be sure that I understood the performance implications and
> the correctness of the changes you made to the file locking, and I
> needed to be in a mood where I could wrap my head around it all.

No worries -- thank you for the review!

> Anyway, it looks to me like the patch is good. I only have a couple of comments:
>
> +  // Maps section offset to the length of the CIE defined at that offset.
> +  std::map<section_offset_type, section_size_type> cies;
>
> Should use Unordered_map here. The defines in system.h will select the
> best implementation of those available.

Ah great, somehow didn't realize that was available. Done.

> +   std::map<section_offset_type, section_size_type>::const_iterator it;
>
> Likewise. To avoid repeating the type, we should probably define a
> typedef for "Unordered_map<section_offset_type, section_size_type>":
>
> typedef Unordered_map<section_offset_type, section_size_type> Cie_map;

Done.

> +  typedef std::multimap<Section_id, Extra_identity_info> Extra_identity_list;
>
> Should use Unordered_multimap.

I'm less sure about this one. Unordered_multimap does provide
equal_range(), but it doesn't guarantee the order of the equal
elements, while std::multimap preserves insertion order among equal
elements. If the iteration order isn't consistent when iterating the
extra_identity_list entries for two functions with identical code and
identical EH metadata, they'll fail to be folded when they could
otherwise safely be.

> +bool
> +Icf::add_ehframe_links(Relobj* object, unsigned int ehframe_shndx,
> +        Reloc_info& relocs)
>
> I'd like to see a comment above this function.

Added:
// Iterate through the .eh_frame section that has index
// `ehframe_shndx` in `object`, adding entries to extra_identity_list_
// that will cause the contents of each FDE and its CIE to be included
// in the logical ICF identity of the function that the FDE refers to.

> If you agree with the use of Unordered_map and Unordered_multimap
> above, let me know and I'll apply the patch for you with those
> changes. And if you can write a brief comment about
> Icf::add_ehframe_links(), I'll add it to the patch.

Thanks! Feel free to update the patch from my notes above if that's
easiest, or I've attached an updated version against current binutils
master.

Josh


> Apologies again for the delays! I really do appreciate your work on this.
>
> -cary
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr21066-gold-icf-exception-metadata-v2.patch
Type: text/x-patch
Size: 28839 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20190510/3d6d930f/attachment.bin>