[PATCH] D34618: [LLD] Add basic 64-bit SPARC support

Mark Kettenis via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jun 26 08:46:01 PDT 2017


kettenis marked 5 inline comments as done.
kettenis added inline comments.


================
Comment at: lld/ELF/Arch/SPARCV9.cpp:44
+
+  PageSize = 8192;
+  DefaultMaxPageSize = 0x100000;
----------------
ruiu wrote:
> Is SPARC always 8k page?
Yes, for SPARV9 the minumum page size is 8k. Older (32-bit only) SPARV7 processor supported smaller page sizes. That hardware is now more than 20 years old and pretty much dead.  OpenBSD doesn't support 32-bit SPARC anymore, so I didn't write any code for it.  But if somebody else is interested in doing that, bits of the SPARCV9 code could be re-used.


================
Comment at: lld/ELF/Arch/SPARCV9.cpp:59
+  case R_SPARC_PC22:
+    if (S.getName() == "_GLOBAL_OFFSET_TABLE_")
+      return R_GOTONLY_PC;
----------------
ruiu wrote:
> In lld we try to avoid string comparison as it is slow. If you need to do this, you can add a new member `GlobalOffsetTable` to `ElfSym` defined in Symbols.h and compare a given symbol with that.
Thanks.


================
Comment at: lld/ELF/Arch/SPARCV9.cpp:61
+      return R_GOTONLY_PC;
+  // fallthrough
+  case R_SPARC_DISP32:
----------------
ruiu wrote:
> Use LLVM_FALLTHROUGH.
Will do.


================
Comment at: lld/ELF/Arch/SPARCV9.cpp:86
+    checkUInt<32>(Loc, Val, Type);
+    write32be(Loc, Val);
+    break;
----------------
ruiu wrote:
> Is SPARCV9 big-endian only?
Instruction access is always big-endian. In theory it is possible to run the processor in a mode where the default data access is little-endian, which could affect some of the relocations listed here. In practice nobody ever supported this. The GNU toolchain only ever supported little-endian 32-bit SPARC using the a.out format.


================
Comment at: lld/ELF/SyntheticSections.cpp:1093
     add({DT_PLTRELSZ, In<ELFT>::RelaPlt->getParent()->Size});
-    add({Config->EMachine == EM_MIPS ? DT_MIPS_PLTGOT : DT_PLTGOT,
-         InX::GotPlt});
+    switch (Config->EMachine) {
+    case EM_MIPS:
----------------
ruiu wrote:
> nit: since there are only three choices, I'd do it with if ~ else if ~ else ~.
I think the switch here is better. Future support for 32-bit SPARC would straightforward that way.


================
Comment at: lld/ELF/SyntheticSections.cpp:1638-1639
+      HeaderSize(S) {
+  if (Config->EMachine == EM_SPARCV9)
+    this->Flags |= SHF_WRITE;
+}
----------------
ruiu wrote:
> This is interesting. Can you describe why the .plt segment has to be writable on Sparc? I'd like to add a comment here about the background.
I'll add a comment.


https://reviews.llvm.org/D34618





More information about the llvm-commits mailing list