gold: addend issues in icf.cc:get_section_contents
Sriraman Tallam
tmsriram@google.com
Fri Feb 5 21:24:00 GMT 2016
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
-------------- next part --------------
* icf.cc (get_rel_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.
diff --git a/gold/icf.cc b/gold/icf.cc
index 96b7f2d..4242a14 100644
--- a/gold/icf.cc
+++ b/gold/icf.cc
@@ -213,6 +213,45 @@ preprocess_for_unique_sections(const std::vector<Section_id>& id_section,
}
}
+// 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.
+
+inline void
+get_rel_addend(const unsigned char* reloc_addend_ptr,
+ const unsigned int addend_size,
+ uint64_t* reloc_addend_value)
+{
+ switch (addend_size)
+ {
+ case 0:
+ break;
+ 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;
+ 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();
+ }
+}
+
// 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 +436,36 @@ get_section_contents(bool first_iteration,
uint64_t entsize =
(it_v->first)->section_entsize(it_v->second);
long long offset = it_a->first;
-
- 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;
-
- // For SHT_REL relocation sections, the addend is stored in the
- // text section at the relocation offset.
- uint64_t reloc_addend_value = 0;
+ // 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;
+
+ // Handle SHT_REL addends.
+ // For REL relocations, we need to fetch the addend from the
+ // section contents.
const unsigned char* reloc_addend_ptr =
contents + static_cast<unsigned long long>(*it_o);
- switch(*it_addend_size)
- {
- case 0:
- {
- break;
- }
- 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;
- }
- 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();
- }
- offset = offset + reloc_addend_value;
+
+ // Update the addend value with the SHT_REL addend if
+ // available.
+ get_rel_addend(reloc_addend_ptr, *it_addend_size,
+ &reloc_addend_value);
+
+ // Ignore the addend when it is a negative value. See the
+ // comments in Merged_symbol_value::value in object.h.
+ if (reloc_addend_value < 0xffffff00)
+ offset = offset + reloc_addend_value;
section_size_type secn_len;
+
const unsigned char* str_contents =
(it_v->first)->section_contents(it_v->second,
&secn_len,
false) + offset;
+ gold_assert (offset < (long long) secn_len);
+
if ((secn_flags & elfcpp::SHF_STRINGS) != 0)
{
// String merge section.
@@ -489,10 +506,14 @@ get_section_contents(bool first_iteration,
}
else
{
- // Use the entsize to determine the length.
- buffer.append(reinterpret_cast<const
+ // Use the entsize to determine the length to copy.
+ uint64_t bufsize = entsize;
+ // If entsize is too big, copy all the remaining bytes.
+ if ((offset + entsize) > secn_len)
+ bufsize = secn_len - offset;
+ buffer.append(reinterpret_cast<const
char*>(str_contents),
- entsize);
+ bufsize);
}
buffer.append("@");
}
More information about the Binutils
mailing list