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

[James Y Knight via cfe-commits]

jyknight added inline comments.

================
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
----------------
joerg wrote:
> Drop the last with?
Done.

================
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>
----------------
joerg wrote:
> Not a native speaker, but "returns as tuple" sounds more natural?
> 
> Why is the PICLevel signed?
It's signed because I just used 'int' as a default; doesn't seem like it makes a difference, but made it unsigned.

================
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,
----------------
joerg wrote:
> No need to rant about GCC behavior that much. Just state that older GCC versions provided different (inconsistent behavior).
I don't think it's even worth saying that much -- the original comment was from 2012, and isn't really relevant information to anyone reading this. Removed.

================
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
----------------
joerg wrote:
> Difficult to parse. Please rewrite the second sentence.
Done.

================
Comment at: lib/Driver/Tools.cpp:3032
@@ +3031,3 @@
+    PIC = PIE = false;
+  if (Args.hasArg(options::OPT_static))
+    PIC = PIE = false;
----------------
joerg wrote:
> This check doesn't make sense to me. When using just -S, -static is completely ignored. So why should it be relevant here?
Not sure what you mean? This code isn't only used with -S -- it's used for all clang compile and assemble actions.

================
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);
----------------
joerg wrote:
> This part looks like a clean up that can be applied separately first?
Committed separately as r245154.


http://reviews.llvm.org/D11845