From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3951 invoked by alias); 5 Aug 2014 17:49:37 -0000 Mailing-List: contact binutils-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: binutils-owner@sourceware.org Received: (qmail 3941 invoked by uid 89); 5 Aug 2014 17:49:36 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-qa0-f45.google.com Received: from mail-qa0-f45.google.com (HELO mail-qa0-f45.google.com) (209.85.216.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Tue, 05 Aug 2014 17:49:35 +0000 Received: by mail-qa0-f45.google.com with SMTP id cm18so1258719qab.32 for ; Tue, 05 Aug 2014 10:49:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=2PW1Y2Uc3pQZfDABMtKZxVwx6z0fJHMeL2tn9iVa06E=; b=NMESTQg9mo2M6G2NfzFIRl+e7uy0VWNex8eQZj7Lr8VevESHCTTD/9UczQqYWY1eaA TecDEkgf2xXzBmBfBxeA4Li03Hfvp/QILejDxRJB93bdfzFALvfiAj1K6t+n7bVqRbbx WJ51cvVYfkzwQcqw0SIYvfGyqNnf44Um+E5AzfbUJKbfJy3Ps/VUQuQkBAPCWpvFFr7n qN3XhWJrRKIhxgumMGqE7AeDirH0pqhweZyYYwnSJZ5pkplZkBR8RGqrPybrGvpjyWtU UuypuOemTkNXjm29mBtQNZGrLpsS1jqHOnVJqf8SmggwTcllFvdT5UjXrZdfyuIEwjrO 0HHA== X-Gm-Message-State: ALoCoQkJvUV5VoqwhwdaaTYuZ5vmbqqjbNQEXqhoNYAcm3hAo82C/tomKnQ8zUgRR8OSdx3SCkF6 MIME-Version: 1.0 X-Received: by 10.140.94.197 with SMTP id g63mr7645552qge.90.1407260971930; Tue, 05 Aug 2014 10:49:31 -0700 (PDT) Received: by 10.140.85.113 with HTTP; Tue, 5 Aug 2014 10:49:31 -0700 (PDT) In-Reply-To: References: Date: Tue, 05 Aug 2014 17:49:00 -0000 Message-ID: Subject: Re: [gold][aarch64]patch2: link helloworld From: Cary Coutant To: Jing Yu Cc: binutils , Doug Kwan , Han Shen Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-08/txt/msg00038.txt.bz2 > 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. > (Output_data_plt_aarch64::do_write): Femove useless got_base. Set > the got_pov entry to plt0. "Remove" > 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. +template<> +bool +rvalue_checkup<0,0>(int64_t) {return true;} Spaces after the comma, and inside the braces. +template<> +uint64_t +rvalue_bit_select<0,0>(uint64_t x) { return x; } Likewise. + if(code == elfcpp::R_AARCH64_ABS64) Space between "if" and "(". + 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. + const AArch64_reloc_property* + get_reloc_property(unsigned int code) const Bad indentation here. + 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. 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. + // 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? + // 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? +protected: + void + do_select_as_default_target() "protected" should be indented one space. + 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. +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? + {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. + 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(...). + 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). + 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. + (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: 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