On Tue, Aug 5, 2014 at 10:49 AM, Cary Coutant wrote: >> 2014-07-29 Jing Yu >> Han Shen >> >> * elfcpp/aarch64.h(enum): Replace withdrawn with R_AARCH64_withdrawn >> * gold/Makefile.am(HFILES): Add aarch64-reloc-property.h > > The ChangeLog entry needs to be split into two, one for > elfcpp/ChangeLog, and one for gold/ChangeLog. Remove the "elfcpp/" and > "gold/" from the file names on each line. Please put periods at the > end of each entry. Done. > >> (Output_data_plt_aarch64::do_write): Femove useless got_base. Set >> the got_pov entry to plt0. > > "Remove" Done. > >> diff --git a/gold/Makefile.in b/gold/Makefile.in > > No need to include generated files in the patch. > > + return (low_bound <= x && x < up_bound) > + && ((x & extra_alignment_requirement) == 0); > > Put another set of parens around the entire expression, and indent the > second line so the "&&" is aligned properly. Done. > > +template<> > +bool > +rvalue_checkup<0,0>(int64_t) {return true;} > > Spaces after the comma, and inside the braces. Done. > > +template<> > +uint64_t > +rvalue_bit_select<0,0>(uint64_t x) { return x; } > > Likewise. Done. > > + if(code == elfcpp::R_AARCH64_ABS64) > > Space between "if" and "(". Done. > > + typedef uint64_t (*rvalue_bit_select_func_p)(uint64_t); > > This typedef uses the "_p" convention for predicate functions, but the > function doesn't return bool. Done. > > + const AArch64_reloc_property* > + get_reloc_property(unsigned int code) const > > Bad indentation here. Done. > > + unsigned int idx = code_to_array_index(code); > + gold_assert(idx < Property_table_size); > > The assert here seems overcautious -- code_to_array_index() already > does the same assert. Done. > > What happens if you receive an object file with a bad relocation type? > It would be preferable to issue a regular error on bad input, rather > than an internal error. You should assert only when a logic error in > gold's own code could lead to a false condition. Yes, added gold_error when encountering an unexpected reloc type. > > + // The Parse_expression class is used to convert relocation operations in > + // aarch64-reloc.def into S-expression strings, which are parsed again to > + // build actual expression trees. We do not build the expression trees > + // directly because the parser for operations in arm-reloc.def is simpler > + // this way. Conversion from S-expressions to trees is simple. > > So you parse the "S + A" forms from aarch64-reloc.def into a tree form > (letting the compiler do the actual parsing), then convert that back > to a different string-based "( + S A )" form during initialization. > Then, when it's time to apply the relocation, you plan to parse the "( > + S A )" form into a tree form again, right? > > Don't take this as an objection, but why not just put the S-expression > form directly into the .def file, and save a bit of initialization > time? Alternatively, parsing the two forms doesn't seem appreciably > different, so why not just parse the original form when it's time to > apply relocations? Or, why not just save the tree form after the first > parse? > > In Relocate::relocate(), none of the relocations implemented so far > use these expressions -- are you planning to take advantage of these > expressions to simplify the logic later, or will they continue to go > unused? Ok, removed this part totally, including string s_expression member variable, since we probably won't use this approach. > > + // Map aarch64 rtypes into range(0,300) as following > + // 256 ~ 313 -> 0 ~ 57 > + // 512 ~ 573 -> 128 ~ 189 > + // 1024 ~ 1032 -> 256 ~ 264 > > Why bother to reserve space in your array for the dynamic relocations? Yeah, that's correct. Removed mapping from 1024-1032. > > +protected: > + void > + do_select_as_default_target() > > "protected" should be indented one space. Done. > > + offset = this->first_plt_entry_offset() + > + this->count_ * this->get_plt_entry_size(); > > Parenthesize the expression and indent the second line under the open paren. Done. > > +static const AArch64_howto aarch64_howto[AArch64_reloc_property::INST_NUM] = > +{ > + {0, -1, -1}, // DATA > + {0x1fffe0, 5, -1}, // MOVW [20:5]-imm16 > + {0xffffe0, 5, -1}, // LD [23:5]-imm19 > + {0x60ffffe0, 29, 5}, // ADR [30:29]-immlo [23:5]-immhi > + {0x60ffffe0, 29, 5}, // ADR [30:29]-immlo [23:5]-immhi > > Should this be ADRP? Yes. Done. > > + {0x3ffc00, 10, -1}, // ADD [21:10]-imm12 > + {0x3ffc00, 10, -1}, // LDST [21:10]-imm12 > + {0x7ffe0, 5, -1}, // TBZNZ [18:5]-imm14 > + {0xffffe0, 5, -1}, // CONDB [23:5]-imm19 > + {0x3ffffff, 0, -1}, // B [25:0]-imm26 > + {0x3ffffff, 0, -1}, // CALL [25:0]-imm26 > +}; > > + // Page(expr) = expr & ~0xFFF > + > + static inline typename elfcpp::Swap::Valtype > + Page(typename elfcpp::Elf_types::Elf_Addr address) > + { > + return (address >> 12) << 12; > + } > > Why write it as a mask operation in the comment, but use shifting in > the implementation? I'd prefer the mask operation, and the comment > ought to be in English. Changed the implementation and comment. > > + elfcpp::Swap::writeval(wv, > + (Valtype)(val | (immed << doffset))); > > + elfcpp::Swap::writeval(wv, > + (Valtype)(val | (immed1 << doffset1) | (immed2 << doffset2))); > > + elfcpp::Swap_unaligned::writeval(view, (Valtype)x); > > (... and several more places ...) > > Use static_cast(...). Done. > > + typename elfcpp::Elf_types::Elf_Addr x = > + psymval->value(object, addend); > > It's customary to indent lines by four spaces in cases like this > (e.g., when you begin the entire rhs of an assignment on a new line). Done. > > + int got_base = target->got_ != NULL > + ? target->got_->current_data_size() >= 0x8000 ? 0x8000 : 0 > + : 0; > > Parenthesize the expression and indent subsequent lines under the open paren. Done. > > + (size == 64 > + ? (big_endian ? "elf64-bigaarch64" > + : "elf64-littleaarch64") > + : (big_endian ? "elf32-bigaarch64" > + : "elf32-littleaarch64")), > + (size == 64 > + ? (big_endian ? "aarch64_elf64_be_vec" > + : "aarch64_elf64_le_vec") > + : (big_endian ? "aarch64_elf32_be_vec" > + : "aarch64_elf32_le_vec"))) > > I think this would be much easier to read if you specialized each of > the four constructors, like this: > Done. > template > class Target_selector_aarch64 : public Target_selector > { > public: > Target_selector_aarch64(); > > virtual Target* > do_instantiate_target() > { return new Target_aarch64(); } > }; > > template<> > Target_selector_aarch64<32, true>::Target_selector_aarch64() > : Target_selector(elfcpp::EM_AARCH64, 32, true, > "elf32-bigaarch64", "aarch64_elf32_be_vec") > { } > > template<> > Target_selector_aarch64<32, false>::Target_selector_aarch64() > : Target_selector(elfcpp::EM_AARCH64, 32, false, > "elf32-littleaarch64", "aarch64_elf32_le_vec") > { } > > template<> > Target_selector_aarch64<64, true>::Target_selector_aarch64() > : Target_selector(elfcpp::EM_AARCH64, 64, true, > "elf64-bigaarch64", "aarch64_elf64_be_vec") > { } > > template<> > Target_selector_aarch64<64, false>::Target_selector_aarch64() > : Target_selector(elfcpp::EM_AARCH64, 64, false, > "elf64-littleaarch64", "aarch64_elf64_le_vec") > { } > > -cary Thanks for the review! Attached the updated patch. elfcpp/Changelog: 2014-08-07 Jing Yu Han Shen * aarch64.h(enum): Replace withdrawn with R_AARCH64_withdrawn. gold/Changelog: 2014-08-07 Jing Yu Han Shen * Makefile.am(HFILES): Add aarch64-reloc-property.h. (DEFFILES): add aarch64-reloc.def. (TARGETSOURCES): Add aarch64-reloc-property.cc. (ALL_TARGETOBJS): Add aarch64-reloc-property.$(OBJEXT). * Makefile.in: Regenerate. * aarch64-reloc-property.cc: New file. * aarch64-reloc-property.h: New file. * aarch64-reloc.def: New file. * aarch64.cc: Include aarch64-reloc-property.h. Replace spaces with tab to make the format consistent. (Output_data_got_aarch64::symbol_table_): New method. (Target_aarch64::do_plt_address_for_global): New method. (Target_aarch64::do_plt_address_for_local): New method. (Target_aarch64::do_select_as_default_target): New method. (Target_aarch64::do_make_data_plt): New method. (Target_aarch64::make_data_plt): New method. (Output_data_plt_aarch64::has_irelative_section): New method. (Output_data_plt_aarch64::address_for_global): New method. (Output_data_plt_aarch64::address_for_local): New method. (Output_data_plt_aarch64::irelative_rel_): New parameter. (Output_data_plt_aarch64::add_entry): Implement contents. (Output_data_plt_aarch64::set_final_data_size): Fix typo. (Output_data_plt_aarch64::do_write): Remove useless got_base. Set the got_pov entry to plt0. (Output_data_plt_aarch64_standard::do_fill_first_plt_entry): Implement contents. (Output_data_plt_aarch64_standard::do_fill_plt_entry): Implement. (AArch64_howto): New struct. (aarch64_howto[]): New static const array. (AArch64_relocate_functions): New class (Target_aarch64::Scan::get_reference_flags): Remove method. (Target_aarch64::Scan::local): Implement to support a few relocations. (Target_aarch64::Scan::global): Implement to support a few relocations. (Target_aarch64::make_plt_section): Implement contents. (Target_aarch64::make_plt_entry): Implement contents. (Target_aarch64::do_finalize_sections): Implement contents. (Target_aarch64::Relocate::relocate): Implement a few relocations. (Target_aarch64::relocate_section): Implement contents.