gold: addend issues in icf.cc:get_section_contents

[Cary Coutant]

> * 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()).

+    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