Re: gold: addend issues in icf.cc:get_section_contents

[Sriraman Tallam <tmsriram at google dot com>]

Hi Cary,

   Thanks for reviewing.

On Mon, Feb 1, 2016 at 10:56 AM, Cary Coutant <ccoutant@gmail.com> wrote:
>> * icf.cc (get_sht_rel_merge_section_reloc_addend): New function.
>> (get_section_contents):  Move merge section addend computation to a
>> new function.  Ignore negative values for SHT_REL and SHT_RELA addends.
>> Fix bug to not read past the length of the section.
>
> +// For SHT_REL relocation sections that are SHF_MERGE sections, the addend is
>
> "For SHF_MERGE sections that use REL relocations, the addend is..."
>
> +// stored in the text section at the relocation offset.  Read  the addend value
> +// given the pointer to the addend in the text section and the addend size.
> +// Update the addend value if a valid addend is found.
> +// Parameters:
> +// RELOC_ADDEND_PTR   : Pointer to the addend in the text section.
> +// ADDEND_SIZE        : The size of the addend.
> +// RELOC_ADDEND_VALUE : Pointer to the addend that is updated.
> +
> +static uint64_t
> +get_sht_rel_merge_section_reloc_addend(const unsigned char* reloc_addend_ptr,
> +                                      const unsigned int addend_size,
> +                                      uint64_t* reloc_addend_value)
>
> How about shortening the name of the function to something like
> "get_rel_addend"?
>
> You've made this function return a uint64_t, but you never return anything
> but 0. Let's get rid of the reloc_addend_value pointer parameter, and just
> return the value. To avoid overwriting the value in the RELA case, let's
> call this function only for REL sections.
>
> It would probably be good to declare this function as inline.
>
> +{
> +  switch(addend_size)
>
> Please add a space before the '('.
>
> +    {
> +    case 0:
> +      break;
>
> You can eliminate this case (or make it gold_unreachable()).

I made all the changes mentioned except this one where I feel the
value of 0 is not unreachable code.  We could find 0  and we do
nothing, so I will keep this as it is. I will go ahead and submit this
patch.

Thanks
Sri



>
> +    case 1:
> +      *reloc_addend_value =
> +        read_from_pointer<8>(reloc_addend_ptr);
> +      break;
> +    case 2:
> +      *reloc_addend_value =
> +          read_from_pointer<16>(reloc_addend_ptr);
> +      break;
>
> The indentation here is inconsistent. When breaking an assignment after
> the '=', our usual convention is to indent by 4 spaces.
>
> +    case 4:
> +      *reloc_addend_value =
> +        read_from_pointer<32>(reloc_addend_ptr);
> +      break;
> +    case 8:
> +      *reloc_addend_value =
> +        read_from_pointer<64>(reloc_addend_ptr);
> +      break;
> +    default:
> +      gold_unreachable();
> +    }
> +
> +  return 0;
> +}
> +
>  // This returns the buffer containing the section's contents, both
>  // text and relocs.  Relocs are differentiated as those pointing to
>  // sections that could be folded and those that cannot.  Only relocs
> @@ -397,58 +438,35 @@ get_section_contents(bool first_iteration,
>                    uint64_t entsize =
>                      (it_v->first)->section_entsize(it_v->second);
>                   long long offset = it_a->first;
> +                 // Handle SHT_RELA and SHT_REL addends, only one of
> these addends
> +                 // exists.
> +                 // Get the SHT_RELA addend.
>
> "For RELA relocations, we have the addend from the relocation."
>
> +                 uint64_t reloc_addend_value = it_a->second;
>
> -                  unsigned long long addend = it_a->second;
> -                  // Ignoring the addend when it is a negative value.  See the
> -                  // comments in Merged_symbol_value::Value in object.h.
> -                  if (addend < 0xffffff00)
> -                    offset = offset + addend;
> -
> +                 // Handle SHT_REL addends.
>                   // For SHT_REL relocation sections, the addend is
> stored in the
>                   // text section at the relocation offset.
>
> "For REL relocations, we need to fetch the addend from the section contents."
>
>                   if (*it_addend_size > 0)
>                     {
>                       ...
>                     }
>
> This is OK with those changes.
>
> Thanks!
>
> -cary

Attachment: gold_patch_icf_merge_section.txt
Description: Text document