<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Oct 23, 2017 at 10:01 AM, Rafael Avila de Espindola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>Rui Ueyama via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> writes:<br>
<br>
> RelExpr AArch64::getRelExpr(RelType Type, const SymbolBody &S,<br>
> - const InputFile &File, const uint8_t *Loc) const {<br>
> + const uint8_t *Loc) const {<br>
> switch (Type) {<br>
> - default:<br>
> - return R_ABS;<br>
> case R_AARCH64_TLSDESC_ADR_PAGE21:<br>
> return R_TLSDESC_PAGE;<br>
> case R_AARCH64_TLSDESC_LD64_LO12:<br>
> @@ -104,6 +102,8 @@ RelExpr AArch64::getRelExpr(RelType Type<br>
> return R_GOT_PAGE_PC;<br>
> case R_AARCH64_NONE:<br>
> return R_NONE;<br>
> + default:<br>
> + return R_ABS;<br>
<br>
</span>Why change the location of the default label?</blockquote><div><br></div><div>Because in many places we write `default` at end, but that is an unrelated change, so I should've done in a separate patch.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>
> RelExpr ARM::getRelExpr(RelType Type, const SymbolBody &S,<br>
> - const InputFile &File, const uint8_t *Loc) const {<br>
> + const uint8_t *Loc) const {<br>
> switch (Type) {<br>
> - default:<br>
> - return R_ABS;<br>
> case R_ARM_THM_JUMP11:<br>
> return R_PC;<br>
> case R_ARM_CALL:<br>
> @@ -119,6 +117,8 @@ RelExpr ARM::getRelExpr(RelType Type, co<br>
> return R_NONE;<br>
> case R_ARM_TLS_LE32:<br>
> return R_TLS;<br>
> + default:<br>
> + return R_ABS;<br>
> }<br>
> }<br>
<br>
</span>Same here. Please commit these changes independently, it is much easier<br>
to read 2 independent patches than a combined one.<br>
<span><br>
<br>
> RelExpr AVR::getRelExpr(RelType Type, const SymbolBody &S,<br>
> - const InputFile &File, const uint8_t *Loc) const {<br>
> - switch (Type) {<br>
> - case R_AVR_CALL:<br>
> - return R_ABS;<br>
> - default:<br>
> - error(toString(&File) + ": unknown relocation type: " + toString(Type));<br>
> - return R_HINT;<br>
> - }<br>
> + const uint8_t *Loc) const {<br>
> + return R_ABS;<br>
> }<br>
<br>
</span>The default should be R_INVALID to maintain the old semantics, no?<br>
<span><br>
<br>
> @@ -353,8 +351,6 @@ template <class ELFT><br>
> int64_t MIPS<ELFT>::getImplicitAddend(<wbr>const uint8_t *Buf, RelType Type) const {<br>
> const endianness E = ELFT::TargetEndianness;<br>
> switch (Type) {<br>
> - default:<br>
> - return 0;<br>
> case R_MIPS_32:<br>
> case R_MIPS_GPREL32:<br>
> case R_MIPS_TLS_DTPREL32:<br>
> @@ -417,6 +413,8 @@ int64_t MIPS<ELFT>::getImplicitAddend(<wbr>co<br>
> return SignExtend64<25>(readShuffle<E<wbr>>(Buf) << 2);<br>
> case R_MICROMIPS_PC26_S1:<br>
> return SignExtend64<27>(readShuffle<E<wbr>>(Buf) << 1);<br>
> + default:<br>
> + return 0;<br>
> }<br>
> }<br>
<br>
</span>Another unrelated change.<br>
<span><br>
> @@ -453,20 +451,24 @@ calculateMipsRelChain(uint8_t *Loc, RelT<br>
> template <class ELFT><br>
> void MIPS<ELFT>::relocateOne(uint8_<wbr>t *Loc, RelType Type, uint64_t Val) const {<br>
> const endianness E = ELFT::TargetEndianness;<br>
> +<br>
<br>
</span>And another.<br>
<span><br>
> // Thread pointer and DRP offsets from the start of TLS data area.<br>
> // <a href="https://www.linux-mips.org/wiki/NPTL" rel="noreferrer" target="_blank">https://www.linux-mips.org/wik<wbr>i/NPTL</a><br>
> if (Type == R_MIPS_TLS_DTPREL_HI16 || Type == R_MIPS_TLS_DTPREL_LO16 ||<br>
> Type == R_MIPS_TLS_DTPREL32 || Type == R_MIPS_TLS_DTPREL64 ||<br>
> Type == R_MICROMIPS_TLS_DTPREL_HI16 ||<br>
> - Type == R_MICROMIPS_TLS_DTPREL_LO16)<br>
> + Type == R_MICROMIPS_TLS_DTPREL_LO16) {<br>
> Val -= 0x8000;<br>
> - else if (Type == R_MIPS_TLS_TPREL_HI16 || Type == R_MIPS_TLS_TPREL_LO16 ||<br>
> - Type == R_MIPS_TLS_TPREL32 || Type == R_MIPS_TLS_TPREL64 ||<br>
> - Type == R_MICROMIPS_TLS_TPREL_HI16 ||<br>
> - Type == R_MICROMIPS_TLS_TPREL_LO16)<br>
> + } else if (Type == R_MIPS_TLS_TPREL_HI16 || Type == R_MIPS_TLS_TPREL_LO16 ||<br>
> + Type == R_MIPS_TLS_TPREL32 || Type == R_MIPS_TLS_TPREL64 ||<br>
> + Type == R_MICROMIPS_TLS_TPREL_HI16 ||<br>
> + Type == R_MICROMIPS_TLS_TPREL_LO16) {<br>
> Val -= 0x7000;<br>
> + }<br>
<br>
</span>Why add {}? How is this related to the rest of the patch?<br>
<span><br>
> if (ELFT::Is64Bits || Config->MipsN32Abi)<br>
> std::tie(Type, Val) = calculateMipsRelChain(Loc, Type, Val);<br>
> +<br>
<br>
</span>Unrelated change.<br>
<div><div class="m_4060116358115427981h5"><br>
> switch (Type) {<br>
> case R_MIPS_32:<br>
> case R_MIPS_GPREL32:<br>
><br>
> Modified: lld/trunk/ELF/Arch/PPC.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Arch/PPC.cpp?rev=315552&r1=315551&r2=315552&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/lld/trunk/ELF/Arch/PPC.<wbr>cpp?rev=315552&r1=315551&r2=<wbr>315552&view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- lld/trunk/ELF/Arch/PPC.cpp (original)<br>
> +++ lld/trunk/ELF/Arch/PPC.cpp Wed Oct 11 20:14:06 2017<br>
> @@ -23,11 +23,22 @@ class PPC final : public TargetInfo {<br>
> public:<br>
> PPC() { GotBaseSymOff = 0x8000; }<br>
> void relocateOne(uint8_t *Loc, RelType Type, uint64_t Val) const override;<br>
> - RelExpr getRelExpr(RelType Type, const SymbolBody &S, const InputFile &File,<br>
> + RelExpr getRelExpr(RelType Type, const SymbolBody &S,<br>
> const uint8_t *Loc) const override;<br>
> };<br>
> } // namespace<br>
><br>
> +RelExpr PPC::getRelExpr(RelType Type, const SymbolBody &S,<br>
> + const uint8_t *Loc) const {<br>
> + switch (Type) {<br>
> + case R_PPC_REL24:<br>
> + case R_PPC_REL32:<br>
> + return R_PC;<br>
> + default:<br>
> + return R_ABS;<br>
> + }<br>
> +}<br>
<br>
</div></div>Why move the function?<br>
<span><br>
> RelExpr PPC64::getRelExpr(RelType Type, const SymbolBody &S,<br>
> - const InputFile &File, const uint8_t *Loc) const {<br>
> + const uint8_t *Loc) const {<br>
> switch (Type) {<br>
> - default:<br>
> - return R_ABS;<br>
> case R_PPC64_TOC16:<br>
> case R_PPC64_TOC16_DS:<br>
> case R_PPC64_TOC16_HA:<br>
> @@ -98,6 +96,8 @@ RelExpr PPC64::getRelExpr(RelType Type,<br>
> return R_PPC_TOC;<br>
> case R_PPC64_REL24:<br>
> return R_PPC_PLT_OPD;<br>
> + default:<br>
> + return R_ABS;<br>
> }<br>
> }<br>
<br>
</span>Why move the default?<br>
<span><br>
> +static bool hasBaseReg(uint8_t ModRM) { return (ModRM & 0xc7) != 0x5; }<br>
<br>
</span>Nice, but unrelated change. Should have been in another patch.<br>
<span><br>
> RelExpr X86::getRelExpr(RelType Type, const SymbolBody &S,<br>
> - const InputFile &File, const uint8_t *Loc) const {<br>
> + const uint8_t *Loc) const {<br>
> switch (Type) {<br>
> case R_386_8:<br>
> case R_386_16:<br>
> @@ -122,13 +124,7 @@ RelExpr X86::getRelExpr(RelType Type, co<br>
> // instruction. That means a ModRM byte is at Loc[-1]. By taking a look at<br>
> // the byte, we can determine whether the instruction is register-relative<br>
> // (i.e. it was generated for foo@GOT(%reg)) or absolute (i.e. foo@GOT).<br>
> - if ((Loc[-1] & 0xc7) != 0x5)<br>
> - return R_GOT_FROM_END;<br>
> - if (Config->Pic)<br>
> - error(toString(&File) + ": relocation " + toString(Type) + " against '" +<br>
> - S.getName() +<br>
> - "' without base register can not be used when PIC enabled");<br>
> - return R_GOT;<br>
> + return hasBaseReg(Loc[-1]) ? R_GOT_FROM_END : R_GOT;<br>
<br>
</span>This drops an error. Is the assumption that an R_GOT will produce the<br>
required error? At the very least you should call attention to it in the<br>
commit message.</blockquote><div><br></div><div>Sorry for not mentioning in the commit message. But now you should get "relocation cannot refer to absolute symbol" error.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>
> @@ -234,8 +229,6 @@ void X86::writePlt(uint8_t *Buf, uint64_<br>
><br>
> int64_t X86::getImplicitAddend(const uint8_t *Buf, RelType Type) const {<br>
> switch (Type) {<br>
> - default:<br>
> - return 0;<br>
> case R_386_8:<br>
> case R_386_PC8:<br>
> return SignExtend64<8>(*Buf);<br>
> @@ -252,6 +245,8 @@ int64_t X86::getImplicitAddend(const uin<br>
> case R_386_TLS_LDO_32:<br>
> case R_386_TLS_LE:<br>
> return SignExtend64<32>(read32le(Buf)<wbr>);<br>
> + default:<br>
> + return 0;<br>
> }<br>
> }<br>
<br>
</span>Another unrelated change.<br>
<span><br>
> @@ -286,9 +281,27 @@ void X86::relocateOne(uint8_t *Loc, RelT<br>
> checkInt<17>(Loc, Val, Type);<br>
> write16le(Loc, Val);<br>
> break;<br>
> - default:<br>
> + case R_386_32:<br>
> + case R_386_GLOB_DAT:<br>
> + case R_386_GOT32:<br>
> + case R_386_GOT32X:<br>
> + case R_386_GOTOFF:<br>
> + case R_386_GOTPC:<br>
> + case R_386_PC32:<br>
> + case R_386_PLT32:<br>
> + case R_386_RELATIVE:<br>
> + case R_386_TLS_GD:<br>
> + case R_386_TLS_GOTIE:<br>
> + case R_386_TLS_IE:<br>
> + case R_386_TLS_LDM:<br>
> + case R_386_TLS_LDO_32:<br>
> + case R_386_TLS_LE:<br>
> + case R_386_TLS_LE_32:<br>
<br>
</span>Nice, but unrelated change.<br>
<br>
Cheers,<br>
Rafael<br>
</blockquote></div><br></div></div>