[lld] r315552 - Remove one parameter from Target::getRelExpr.
Rui Ueyama via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 24 21:11:47 PDT 2017
On Mon, Oct 23, 2017 at 10:01 AM, Rafael Avila de Espindola <
rafael.espindola at gmail.com> wrote:
> 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?
Because in many places we write `default` at end, but that is an unrelated
change, so I should've done in a separate patch.
> 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.
Sorry for not mentioning in the commit message. But now you should get
"relocation cannot refer to absolute symbol" error.
> @@ -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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171024/9838900d/attachment.html>
More information about the llvm-commits
mailing list