This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
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
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |