[PATCH] D21891: [LLD][ARM] Thunk support framework for ARM and Mips
Peter Smith via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 7 11:33:30 PDT 2016
peter.smith added a subscriber: llvm-commits.
peter.smith added a comment.
Adding llvm-commits to subscribers. My apologies for forgetting to put this on earlier.
================
Comment at: ELF/InputSection.cpp:133
@@ +132,3 @@
+template <class ELFT>
+void InputSection<ELFT>::addThunk(const Thunk<ELFT> *thunk) {
+ Thunks.push_back(thunk);
----------------
ruiu wrote:
> Ditto.
Made corresponding change.
================
Comment at: ELF/InputSection.cpp:191
@@ -187,1 +190,3 @@
+ case R_THUNK_ABS:
return Body.getThunkVA<ELFT>();
+ case R_THUNK_PC:
----------------
ruiu wrote:
> I think you want to add `A`.
The A wasn't in the original Mips R_THUNK, I didn't change it just in case it was deliberate. Adding it doesn't cause the mips test to fail, presumably as the addend for jump instruction will be 0. Given that this has been generalised I've added the A.
================
Comment at: ELF/InputSection.h:213
@@ -213,1 +212,3 @@
+ // Now thunks is supported for MIPS and ARM target only.
+ void addThunk(const Thunk<ELFT> *thunk);
----------------
ruiu wrote:
> Parameters need to be in camel case as a rule, so `thunk` is not okay. In this context, I'd just name this `T`.
Ok, made change to T.
================
Comment at: ELF/Relocations.cpp:608
@@ -611,1 +607,3 @@
+ auto *Sec = cast<InputSection<ELFT>>(&C);
+ AddThunk<ELFT>(Type, Body, *Sec);
}
----------------
ruiu wrote:
> AddThunk -> addThunk
Ok, made change.
================
Comment at: ELF/Symbols.h:238
@@ +237,3 @@
+ // destination for callers of this Symbol.
+ std::unique_ptr<Thunk<ELFT>> ThunkData = nullptr;
+
----------------
ruiu wrote:
> I noticed that you can remove `= nullptr` as a std::unique_ptr is initialized to a nullptr by default.
Ok, made change.
================
Comment at: ELF/Symbols.h:314
@@ -309,1 +313,3 @@
+ // destination for callers of this Symbol.
+ std::unique_ptr<Thunk<ELFT>> ThunkData = nullptr;
bool needsCopy() const { return this->NeedsCopyOrPltAddr && !this->isFunc(); }
----------------
ruiu wrote:
> Ditto
Ok, made change.
================
Comment at: ELF/Symbols.h:238
@@ +237,3 @@
+ // destination for callers of this Symbol.
+ std::unique_ptr<Thunk<ELFT>> Alt;
+
----------------
ruiu wrote:
> Please use C++11 member initialization.
>
> std::unique_ptr<Thunk<ELFT>> Alt = nullptr;
>
> I don't think Alt is a good name because there are a lot of things associated to symbols that can be called "alt" such as PLT, GOT or copy relocations, etc. Because `Thunk` is a class name, we can't use the same name, so I don't have a good suggestion. Maybe ThunkData or ThunkObj?
Agreed, I used Alt as short for alternative. I've gone with ThunkData.
================
Comment at: ELF/Target.cpp:1552
@@ +1551,3 @@
+ return R_THUNK_PLT_PC;
+ else if ((S.getVA<ELF32LE>() & 1) == 0)
+ return R_THUNK_PC;
----------------
ruiu wrote:
> Remove `else` because it is after `return`.
Ok, made change.
================
Comment at: ELF/Target.cpp:1942-1946
@@ -1894,3 +1941,7 @@
// or target symbol is a PIC symbol.
- return (D->Section->getFile()->getObj().getHeader()->e_flags & EF_MIPS_PIC) ||
- (D->StOther & STO_MIPS_MIPS16) == STO_MIPS_PIC;
+ return ((D->Section->getFile()->getObj().getHeader()->e_flags &
+ EF_MIPS_PIC) ||
+ (D->StOther & STO_MIPS_MIPS16) == STO_MIPS_PIC)
+ ? R_THUNK_ABS
+ : Expr;
+}
----------------
ruiu wrote:
> Can you define variables for the boolean values?
>
> ELFFile<ELFT> &File = D->Section->getFile()->getObj();
> bool PicFile = File->e_flags & EF_MIPS_PIC;
> bool PicSym = (D->StOther & STO_MIPS_MIPS16) == STO_MIPS_PIC;
> return (PicFile || PicSym) ? R_THUNK_ABS : Expr;
Ok, made very similar change to above.
================
Comment at: ELF/Target.cpp:185-187
@@ -183,2 +184,5 @@
int32_t Index, unsigned RelOff) const override;
+ RelExpr adjustExprForThunk(RelExpr Expr, uint32_t RelocType,
+ const InputFile &File,
+ const SymbolBody &S) const override;
void relocateOne(uint8_t *Loc, uint32_t Type, uint64_t Val) const override;
----------------
ruiu wrote:
> Because we already have `getRelExpr`, I'd name this `getThunkExpr`. I believe existing adjust* functions have side effects (mutates parameters) which is different from this function.
Ok, I've made the change. I chose adjustThunkExpr to follow adjustRelaxExpr which also doesn't mutate its parameters.
================
Comment at: ELF/Thunks.cpp:49
@@ +48,3 @@
+// ARM Target Thunks
+template <class ELFT> static uint64_t ARMThunkDestVA(const SymbolBody &S) {
+ return S.isInPlt() ? S.getPltVA<ELFT>() : S.getVA<ELFT>();
----------------
ruiu wrote:
> ARMThunkDestVA -> armThunkDestVA (or getARMThunkDestVA) because function names should start with lowercase letters.
Ok, made change to getARMThunkDestVA
================
Comment at: ELF/Thunks.cpp:56
@@ +55,3 @@
+
+template <class ELFT> class ARMThumbV7ABSLongThunk final : public Thunk<ELFT> {
+public:
----------------
ruiu wrote:
> Please enclose the class definitions in this file with an anonymous namespace.
Ok, made change.
================
Comment at: ELF/Thunks.cpp:58
@@ +57,3 @@
+public:
+ virtual uint32_t size() const override { return 12; }
+ virtual void writeTo(uint8_t *Buf) const override {
----------------
ruiu wrote:
> ruiu wrote:
> > I'd put a blank line between them.
> We omit `virtual` if `override` is given.
Ok, removed all virtual keywords where override is also present.
================
Comment at: ELF/Thunks.cpp:58-59
@@ +57,4 @@
+public:
+ virtual uint32_t size() const override { return 12; }
+ virtual void writeTo(uint8_t *Buf) const override {
+ const uint8_t ATData[] = {
----------------
peter.smith wrote:
> ruiu wrote:
> > ruiu wrote:
> > > I'd put a blank line between them.
> > We omit `virtual` if `override` is given.
> Ok, removed all virtual keywords where override is also present.
Ok, made change.
================
Comment at: ELF/Thunks.cpp:69-70
@@ +68,4 @@
+ Target->relocateOne(Buf + 4, R_ARM_MOVT_ABS, S);
+ }
+ ARMThumbV7ABSLongThunk(const SymbolBody &Destination,
+ const InputSection<ELFT> &Owner)
----------------
ruiu wrote:
> Ditto
Have added a blank line between all similar cases.
================
Comment at: ELF/Thunks.cpp:162
@@ +161,3 @@
+template <class ELFT>
+static void ARMAddThunk(uint32_t RelocType, SymbolBody &S,
+ InputSection<ELFT> &IS) {
----------------
ruiu wrote:
> armAddThunk (or addThunkARM or anything that starts with a lowercase letter.)
Ok, have gone with addThunkARM.
================
Comment at: ELF/Thunks.cpp:201
@@ +200,3 @@
+ else
+ fatal("Symbol not DefinedRegular or Shared\n");
+}
----------------
ruiu wrote:
> Error messages should start with lowercase letters.
Ok, made change.
================
Comment at: ELF/Thunks.cpp:204
@@ +203,3 @@
+
+template <class ELFT> void MipsAddThunk(uint32_t RelocType, SymbolBody &S) {
+ if (S.hasThunk<ELFT>())
----------------
ruiu wrote:
> Lowercase.
Used addThunkMips and made static to match addThunkARM
================
Comment at: ELF/Thunks.cpp:223
@@ +222,3 @@
+ else
+ fatal("Add Thunk only supported for ARM and Mips\n");
+}
----------------
ruiu wrote:
> llvm_unreachable is better because it is unreacahable unless there is a bug in LLD's code. The general guideline is
>
> - if it is unreachable unless there is a programmer's error, llvm_unreachable (or assert)
> - if it is unreachable unless input files are corrupted, fatal
> - otherwise, error.
>
Ok, have used llvm_unreachable in this case.
================
Comment at: ELF/Thunks.cpp:226-238
@@ +225,15 @@
+
+template void ARMAddThunk<ELF32LE>(uint32_t RelocType, SymbolBody &S,
+ InputSection<ELF32LE> &IS);
+template void ARMAddThunk<ELF32BE>(uint32_t RelocType, SymbolBody &S,
+ InputSection<ELF32BE> &IS);
+template void ARMAddThunk<ELF64LE>(uint32_t RelocType, SymbolBody &S,
+ InputSection<ELF64LE> &IS);
+template void ARMAddThunk<ELF64BE>(uint32_t RelocType, SymbolBody &S,
+ InputSection<ELF64BE> &IS);
+
+template void MipsAddThunk<ELF32LE>(uint32_t RelocType, SymbolBody &S);
+template void MipsAddThunk<ELF32BE>(uint32_t RelocType, SymbolBody &S);
+template void MipsAddThunk<ELF64LE>(uint32_t RelocType, SymbolBody &S);
+template void MipsAddThunk<ELF64BE>(uint32_t RelocType, SymbolBody &S);
+
----------------
ruiu wrote:
> I don't think you need to instantiate these templates because no other compilation units use them.
Yes you are right, I've removed them.
================
Comment at: ELF/Thunks.cpp:240-247
@@ +239,10 @@
+
+template void AddThunk<ELF32LE>(uint32_t RelocType, SymbolBody &S,
+ InputSection<ELF32LE> &IS);
+template void AddThunk<ELF32BE>(uint32_t RelocType, SymbolBody &S,
+ InputSection<ELF32BE> &IS);
+template void AddThunk<ELF64LE>(uint32_t RelocType, SymbolBody &S,
+ InputSection<ELF64LE> &IS);
+template void AddThunk<ELF64BE>(uint32_t RelocType, SymbolBody &S,
+ InputSection<ELF64BE> &IS);
+
----------------
ruiu wrote:
> Nit: we omit parameter names in template instantiation. E.g.
>
> template void AddThunk<ELF64BE>(uint32_t, SymbolBody &,
> InputSection<ELF64BE> &);
Ok removed parameter names.
================
Comment at: ELF/Thunks.cpp:100
@@ +99,3 @@
+
+template <class ELFT> class ThumbARMV7ABSLongThunk final : public Thunk<ELFT> {
+public:
----------------
ruiu wrote:
> What's the naming convention in this file? This class is named `ThumbARM` but the other classes are named `ARMThumb`. Can you rename this `ARMThumbV7ABSLongThunk`?
There is a comment above:
// Specific ARM Thunk implementations. The naming convention is:
// Source State, TargetState, Target Requirement, ABS or PI, Range
I've added a "To" in between ARMThumb and ThumbARM to make this clearer: For example the classes are now:
ARMToThumbV7ABSLongThunk and ThumbToARMV7ABSLongThunk
================
Comment at: ELF/Thunks.cpp:121
@@ +120,3 @@
+
+template <class ELFT> class ThumbARMV7PILongThunk final : public Thunk<ELFT> {
+public:
----------------
ruiu wrote:
> Ditto
Should be addressed by the naming convention comment above.
================
Comment at: ELF/Thunks.cpp:228
@@ +227,3 @@
+template <class ELFT>
+void AddThunk(uint32_t RelocType, SymbolBody &S, InputSection<ELFT> &IS) {
+ if (Config->EMachine == EM_ARM)
----------------
ruiu wrote:
> AddThunk -> addThunk (all function names need to start with lowercase letters.)
Ok, made change.
I thought that in the original review comment AddThunk being globally visible had a different convention.
================
Comment at: ELF/Thunks.cpp:234
@@ +233,3 @@
+ else
+ llvm_unreachable("add Thunk only supported for ARM and Mips\n");
+}
----------------
ruiu wrote:
> Remove `\n`. A newline is automatically appended.
Ok, made change.
================
Comment at: ELF/Thunks.h:36
@@ +35,3 @@
+ virtual uint32_t size() const = 0;
+ virtual typename ELFT::uint getVA() const;
+ virtual void writeTo(uint8_t *Buf) const = 0;
----------------
ruiu wrote:
> You don't redefine this function in a derived class, so please remove `virtual`.
Ok, done.
================
Comment at: ELF/Thunks.h:14
@@ +13,3 @@
+#include "Relocations.h"
+#include "llvm/ADT/DenseMap.h"
+
----------------
ruiu wrote:
> Remove this #include because it's unused.
Ok, have made the change.
================
Comment at: test/ELF/arm-thumb-interwork-thunk.s:81-85
@@ +80,7 @@
+// CHECK-ABS-ARM-NEXT: arm_caller:
+// CHECK-ABS-ARM-NEXT: 1300: 3e ff ff fa blx #-776 <thumb_callee1>
+// CHECK-ABS-ARM-NEXT: 1304: 3d ff ff fa blx #-780 <thumb_callee1>
+// CHECK-ABS-ARM-NEXT: 1308: 06 00 00 ea b #24 <arm_caller+0x28>
+// CHECK-ABS-ARM-NEXT: 130c: 05 00 00 ea b #20 <arm_caller+0x28>
+// CHECK-ABS-ARM-NEXT: 1310: 07 00 00 ea b #28 <arm_caller+0x34>
+// CHECK-ABS-ARM-NEXT: 1314: 09 00 00 ea b #36 <arm_caller+0x40>
----------------
ruiu wrote:
> Remove trailing whitespaces.
Ok, done, apologies that those slipped through.
http://reviews.llvm.org/D21891
More information about the llvm-commits
mailing list