[PATCH] D17934: [ELF] Implement infrastructure for thunk code creation

Simon Atanasyan via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 9 20:56:11 PST 2016


atanasyan added inline comments.

================
Comment at: ELF/OutputSections.cpp:806
@@ +805,3 @@
+                                   SymbolBody &S) {
+  ThunkChunk<ELFT> *&TC = ThunkChunks[&C];
+  if (!TC)
----------------
ruiu wrote:
> Using a hash table feels a bit tricky. Can you add a pointer to SymbolBody to point to a thunk?
I need a link between input section and corresponding set of thunks to put the thunks in the right place during a "write" phase. I can add pointer to `ThunkChunk` to the `InputSectionBase` class. That will allow to remove this `DenseMap`. Is it acceptable to you?

================
Comment at: ELF/Target.cpp:1687
@@ -1676,2 +1686,3 @@
 
 template <class ELFT>
+void MipsTargetInfo<ELFT>::writeThunk(uint8_t *Buf, uint64_t S) const {
----------------
ruiu wrote:
> Needs a comment.
Will do

================
Comment at: ELF/Target.cpp:1721
@@ +1720,3 @@
+bool MipsTargetInfo<ELFT>::needsThunk(uint32_t Type, const InputFile &File,
+                            const SymbolBody &S) const {
+  // Any MIPS PIC code function is invoked with its address in register $t9.
----------------
ruiu wrote:
> Please run clang-format-diff.
OK

================
Comment at: ELF/Target.cpp:1733
@@ +1732,3 @@
+  // If current file has PIC code, LA25 stub is not required.
+  if (F->getObj().getHeader()->e_flags & llvm::ELF::EF_MIPS_PIC)
+    return false;
----------------
ruiu wrote:
> We have `using namespace llvm::ELF`.
OK

================
Comment at: ELF/Target.cpp:1735
@@ +1734,3 @@
+    return false;
+  auto *ElfSym = dyn_cast<DefinedRegular<ELFT>>(&S);
+  if (!ElfSym || !ElfSym->Section)
----------------
ruiu wrote:
> We usually name an object of type Elf_Sym ElfSym. In this case, the return type of RHS is still our own symbol type, so I'd name differently, say just Sym, D, Defined or something like that.
OK

================
Comment at: ELF/Target.cpp:1739-1745
@@ +1738,9 @@
+  // LA25 is required if target file has PIC code or ...
+  if (ElfSym->Section->getFile()->getObj().getHeader()->e_flags &
+      llvm::ELF::EF_MIPS_PIC)
+    return true;
+  // ... target symbol is a PIC symbol.
+  if ((ElfSym->Sym.st_other & llvm::ELF::STO_MIPS_MIPS16) == STO_MIPS_PIC)
+    return true;
+  return false;
+}
----------------
ruiu wrote:
> This can be
> 
>   return (ElfSym->Section->getFile()->getObj().getHeader()->e_flags & EF_MIPS_PIC) ||
>          (ElfSym->Sym.st_other & STO_MIPS_MIPS16) == STO_MIPS_PIC;
> 
OK

================
Comment at: ELF/Writer.cpp:346-352
@@ -345,1 +345,9 @@
 
+    // Some targets might require creation of thunk for relocations.
+    // For example MIPS needs LA25 thunk to local call PIC code from non-PIC.
+    if (Body && !Body->hasThunk() && Target->needsThunk(Type, File, *Body)) {
+      const InputSectionBase<ELFT> *Section =
+          cast<DefinedRegular<ELFT>>(Body)->Section;
+      Section->OutSec->addThunk(*Section, *Body);
+    }
+
----------------
ruiu wrote:
> I don't like to add new code to this function because this function is overloaded. Particularly, when this code will be executed is not very obvious because if a condition above is true and it reaches a `continue`, this code will be skipped. Will this code be skipped for all correct cases?
> 
> Can you create a new loop over relocations and move this code there?
OK. I will try to do so.

================
Comment at: ELF/Writer.cpp:1050-1051
@@ -1041,1 +1049,4 @@
 
+  for (OutputSectionBase<ELFT> *Sec : getSections())
+    Sec->reassignOffsets();
+
----------------
ruiu wrote:
> This seems a bit odd.
> 
> If we always reassign offsets, then we don't want to reassign offsets at all. Instead, we want to assign offsets right here only once.
You are right. The `assignOffsets` is better name.

================
Comment at: ELF/Writer.cpp:1068-1072
@@ -1056,2 +1067,7 @@
         CopyRelSymbols.push_back(SC);
+    if (auto *SS = dyn_cast<DefinedSynthetic<ELFT>>(Body))
+      // Setup values of synthetic symbols that need
+      // to point to end of a section.
+      if (SS->Value == DefinedSynthetic<ELFT>::End)
+        SS->Value = SS->Section.getSize();
 
----------------
ruiu wrote:
> I don't understand what this is supposed to do?
This and the next change are related. Now we add synthetic symbols pointing to beginning and end of output sections before relocation scanning phase. In this patch output section size might change during the relocation scanning. So synthetic symbols point to the end of sections need to be updated. There is a couple of possible solutions:
  - Add synthetic symbols after the relocation scanning. Not sure that this does not affect something in relocation scanning phase.
  - Use special value `DefinedSynthetic<ELFT>::End` to mark symbol that need to point to the end of sections. Update such symbols with the correct value later.
In this patch I use the second solution. Do you think it is safe to add synthetic symbols after the relocation scanning?


Repository:
  rL LLVM

http://reviews.llvm.org/D17934





More information about the llvm-commits mailing list