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