[PATCH] Linking of shared libraries for MIPS little-endian 32-bit target

Rui Ueyama ruiu at google.com
Wed Nov 13 13:03:31 PST 2013



================
Comment at: lib/ReaderWriter/ELF/Mips/MipsELFTypes.h:17
@@ +16,3 @@
+
+typedef llvm::object::ELFType<llvm::support::little, 2, false> MipsELFType;
+
----------------
Is this the only architecture you're going to support? Should we name Mips32ELFType?

================
Comment at: lib/ReaderWriter/ELF/Mips/MipsGOT.cpp:69
@@ +68,3 @@
+  const DefinedAtom *da = dyn_cast<DefinedAtom>(a);
+  auto isLocal = da && da->scope() == Atom::scopeTranslationUnit;
+
----------------
s/auto/bool/

================
Comment at: lib/ReaderWriter/ELF/Mips/MipsGOT.cpp:79
@@ +78,3 @@
+  }
+#ifndef NDEBUG
+  ga->_name = "__got_";
----------------
I'd write like this instead of #ifndef

  DEBUG_WITH_TYPE("MipsGOT", {
      ga->name = ...
      ...
      llvm::dbgs() << ...
   });

Note that it's probably better to use MipsGOT or something instead of GOT.

================
Comment at: lib/ReaderWriter/ELF/Mips/MipsGOT.cpp:100
@@ +99,3 @@
+
+#ifndef NDEBUG
+  DEBUG_WITH_TYPE("GOT", llvm::dbgs() << "[ GOT ] Adding GOT header\n");
----------------
No need to use #ifndef. DEBUG_WITH_TYPE will be expanded to do{}while(0) if NDEBUG is not defined.

================
Comment at: lib/ReaderWriter/ELF/Mips/MipsRelocationHandler.cpp:25
@@ +24,3 @@
+      result |                                                               \
+      (uint32_t) * reinterpret_cast<llvm::support::ulittle32_t *>(location);
+
----------------
Why macro? Can it be an inline function?

================
Comment at: lib/ReaderWriter/ELF/Mips/MipsRelocationHandler.cpp:33
@@ +32,3 @@
+/// local/external: T-word32 S + A
+void reloc32(uint8_t *location, uint64_t P, uint64_t S, int64_t A) {
+  uint32_t result = (uint32_t)(S + A);
----------------
In LLD local variables usually start with a lowercase letter.

================
Comment at: lib/ReaderWriter/ELF/Mips/MipsRelocationHandler.cpp:188
@@ +187,3 @@
+    s.flush();
+    llvm_unreachable(str.c_str());
+  }
----------------
nit: s.str() would flush the stream and then return the result string, so you can remove the line above.

================
Comment at: lib/ReaderWriter/ELF/Mips/MipsSectionChunks.h:27
@@ +26,3 @@
+    this->_flags |= SHF_MIPS_GPREL;
+    this->_align2 = 16;
+  }
----------------
2^16 is too large. Typo of 4?

================
Comment at: lib/ReaderWriter/ELF/Mips/MipsTarget.h:10
@@ +9,2 @@
+
+#include "MipsLinkingContext.h"
----------------
Do you really need this file?

================
Comment at: lib/ReaderWriter/ELF/Mips/MipsTargetHandler.h:27
@@ +26,3 @@
+public:
+  MipsTargetLayout(const MipsLinkingContext &hti)
+      : TargetLayout<MipsELFType>(hti),
----------------
"ti" standed for "target info". Because LinkingContext used to be named TargetInfo, some local variables still contain "ti". It no longer makes sense. I'd think "ctx" is preferred.


http://llvm-reviews.chandlerc.com/D2156



More information about the llvm-commits mailing list