[PATCH] D11845: Properly pass through the PIC mode to the integrated assembler when doing assembly-only, and unify the Driver's PIC argument parsing.

[Joerg Sonnenberger via cfe-commits]

joerg added a subscriber: joerg.
joerg added a comment.

I agree in principle. The "-static" thing is already in the initial code, but doesn't make sense to me.


================
Comment at: lib/Driver/Tools.cpp:2928
@@ +2927,3 @@
+/// Parses the various -fpic/-fPIC/-fpie/-fPIE arguments.  Then,
+/// smooshes them together with platform defaults, to decide with
+/// whether this compile should be using PIC mode or not. Returns the
----------------
Drop the last with?

================
Comment at: lib/Driver/Tools.cpp:2930
@@ +2929,3 @@
+/// whether this compile should be using PIC mode or not. Returns the
+/// answer in a tuple of (RelocationModel, PICLevel, IsPIE).
+static std::tuple<llvm::Reloc::Model, int, bool>
----------------
Not a native speaker, but "returns as tuple" sounds more natural?

Why is the PICLevel signed?

================
Comment at: lib/Driver/Tools.cpp:2998
@@ +2997,3 @@
+  // both PIC and PIE are disabled. Any PIE option implicitly enables PIC
+  // at the same level.
+  Arg *LastPICArg = Args.getLastArg(options::OPT_fPIC, options::OPT_fno_PIC,
----------------
No need to rant about GCC behavior that much. Just state that older GCC versions provided different (inconsistent behavior).

================
Comment at: lib/Driver/Tools.cpp:3022
@@ +3021,3 @@
+  // Introduce a Darwin-specific hack. If the default is PIC but the flags
+  // specified while enabling PIC enabled level 1 PIC, just force it back to
+  // level 2 PIC instead. This matches the behavior of Darwin GCC (based on my
----------------
Difficult to parse. Please rewrite the second sentence.

================
Comment at: lib/Driver/Tools.cpp:3032
@@ +3031,3 @@
+    PIC = PIE = false;
+  if (Args.hasArg(options::OPT_static))
+    PIC = PIE = false;
----------------
This check doesn't make sense to me. When using just -S, -static is completely ignored. So why should it be relevant here?

================
Comment at: lib/Driver/Tools.cpp:3087
@@ -2943,1 +3086,3 @@
                          const ArgList &Args, const char *LinkingOutput) const {
+  std::string TripleStr = getToolChain().ComputeEffectiveClangTriple(Args);
+  const llvm::Triple Triple(TripleStr);
----------------
This part looks like a clean up that can be applied separately first?


http://reviews.llvm.org/D11845