[PATCH] Implementation of R_ARM_TARGET1

Rui Ueyama ruiu at google.com
Mon Mar 30 14:39:39 PDT 2015


REPOSITORY
  rL LLVM

================
Comment at: lib/Driver/GnuLdDriver.cpp:430
@@ +429,3 @@
+    if (arg->getOption().matches(OPT_grp_targetopts))
+      ctx->applyTargetSpecificArgument(*arg, diag);
+
----------------
There's no ambiguity on command line option meaning (--arm-target1-rel and --arm-target1-abs clearly for ARM only). Also in this context we already know what the target architecture is. So I don't think we need to abstract the thing away from the driver.

You can just check value of "triple" to see if it's ARM, and if it's ARM, call setTargetRel1(true or false) directly here. If it's not ARM, print out a warning message.

================
Comment at: lib/ReaderWriter/ELF/ARM/ARMLinkingContext.cpp:40
@@ +39,3 @@
+  if (arg.getOption().getName() == "arm-target1-rel")
+  {
+    setTarget1Rel(true);
----------------
This parenthesis position style doesn't follow the LLVM coding style.

================
Comment at: lib/ReaderWriter/ELF/ARM/ARMLinkingContext.h:56
@@ -47,1 +55,3 @@
+private:
+  bool _target1Rel;
 };
----------------
Let's use C++11 initializer.

  bool _target1Rel = false;

================
Comment at: lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.h:26
@@ +25,3 @@
+                             ARMLinkingContext &context)
+      : _armLayout(layout),
+        _armContext(context) {}
----------------
Remove newline after comma.

Maybe you want to pass a boolean value instead of a context object itself? That makes it clear that you only need the boolean flag.

http://reviews.llvm.org/D8707

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list