[PATCH] D35169: Refactor DragonFly BSD toolchain driver.

2019-03-05 Thread Rimvydas via Phabricator via cfe-commits 
rimvydas added inline comments.



Comment at: lib/Driver/ToolChains/DragonFly.cpp:118
 }
-
CmdArgs.push_back(Args.MakeArgString(getToolChain().GetFilePath("crti.o")));
-if (Args.hasArg(options::OPT_shared) || Args.hasArg(options::OPT_pie))
-  CmdArgs.push_back(
-  Args.MakeArgString(getToolChain().GetFilePath("crtbeginS.o")));
+if (crt1)
+  CmdArgs.push_back(Args.MakeArgString(ToolChain.GetFilePath(crt1)));

mgorny wrote:
> Can't `crt1` be non-null only if `!Args.hasArg(options::OPT_shared` here? 
> i.e. is there a reason to do it like this instead of just pushing it inside 
> the above `if`?
The `crt1` can only be used if `!Args.hasArg(options::OPT_shared`, both 
declaration and `CmdArgs.push_back` could be moved inside. The only reason for 
this adjustment is to reduce stylistic differences against 
lib/Driver/ToolChains/FreeBSD.cpp driver layout.





Comment at: lib/Driver/ToolChains/DragonFly.cpp:123
+
+const char *crtbegin = nullptr;
+if (Args.hasArg(options::OPT_shared) || IsPIE)

mgorny wrote:
> This default will never be used.
Correct. Could be changed in both FreeBSD.cpp and DragonFly.cpp.



Comment at: lib/Driver/ToolChains/DragonFly.cpp:185
+if (Args.hasArg(options::OPT_shared) || IsPIE)
+  
CmdArgs.push_back(Args.MakeArgString(ToolChain.GetFilePath("crtendS.o")));
 else

mgorny wrote:
> Inconsistency here: above you used helper variable, here you duplicate the 
> whole line.
Same as two previous. Only to match lib/Driver/ToolChains/FreeBSD.cpp layout.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D35169/new/

https://reviews.llvm.org/D35169



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35169: Refactor DragonFly BSD toolchain driver.

2019-03-04 Thread Michał Górny via Phabricator via cfe-commits 
mgorny added inline comments.
Herald added subscribers: jdoerfert, krytarowski.



Comment at: lib/Driver/ToolChains/DragonFly.cpp:118
 }
-
CmdArgs.push_back(Args.MakeArgString(getToolChain().GetFilePath("crti.o")));
-if (Args.hasArg(options::OPT_shared) || Args.hasArg(options::OPT_pie))
-  CmdArgs.push_back(
-  Args.MakeArgString(getToolChain().GetFilePath("crtbeginS.o")));
+if (crt1)
+  CmdArgs.push_back(Args.MakeArgString(ToolChain.GetFilePath(crt1)));

Can't `crt1` be non-null only if `!Args.hasArg(options::OPT_shared` here? i.e. 
is there a reason to do it like this instead of just pushing it inside the 
above `if`?



Comment at: lib/Driver/ToolChains/DragonFly.cpp:123
+
+const char *crtbegin = nullptr;
+if (Args.hasArg(options::OPT_shared) || IsPIE)

This default will never be used.



Comment at: lib/Driver/ToolChains/DragonFly.cpp:185
+if (Args.hasArg(options::OPT_shared) || IsPIE)
+  
CmdArgs.push_back(Args.MakeArgString(ToolChain.GetFilePath("crtendS.o")));
 else

Inconsistency here: above you used helper variable, here you duplicate the 
whole line.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D35169/new/

https://reviews.llvm.org/D35169



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35169: Refactor DragonFly BSD toolchain driver.

2017-07-09 Thread Rimvydas via Phabricator via cfe-commits 
rimvydas created this revision.
Herald added a subscriber: emaste.

Make it more similar to FreeBSD one to reduce differences.
In preparations for later submissions.

While there, add more handling of flags (including OpenMP 
https://reviews.llvm.org/D35129).


https://reviews.llvm.org/D35169

Files:
  lib/Driver/ToolChains/DragonFly.cpp
  lib/Driver/ToolChains/DragonFly.h

Index: lib/Driver/ToolChains/DragonFly.h
===
--- lib/Driver/ToolChains/DragonFly.h
+++ lib/Driver/ToolChains/DragonFly.h
@@ -11,12 +11,13 @@
 #define LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_DRAGONFLY_H
 
 #include "Gnu.h"
-#include "clang/Driver/Tool.h"
+#include "clang/Driver/Driver.h"
 #include "clang/Driver/ToolChain.h"
 
 namespace clang {
 namespace driver {
 namespace tools {
+
 /// dragonfly -- Directly call GNU Binutils assembler and linker
 namespace dragonfly {
 class LLVM_LIBRARY_VISIBILITY Assembler : public GnuTool {
Index: lib/Driver/ToolChains/DragonFly.cpp
===
--- lib/Driver/ToolChains/DragonFly.cpp
+++ lib/Driver/ToolChains/DragonFly.cpp
@@ -10,7 +10,6 @@
 #include "DragonFly.h"
 #include "CommonArgs.h"
 #include "clang/Driver/Compilation.h"
-#include "clang/Driver/Driver.h"
 #include "clang/Driver/Options.h"
 #include "llvm/Option/ArgList.h"
 
@@ -54,21 +53,37 @@
  const InputInfoList ,
  const ArgList ,
  const char *LinkingOutput) const {
-  const Driver  = getToolChain().getDriver();
+  const toolchains::DragonFly  =
+  static_cast(getToolChain());
+  const Driver  = ToolChain.getDriver();
+  const llvm::Triple::ArchType Arch = ToolChain.getArch();
+  const bool IsPIE =
+  !Args.hasArg(options::OPT_shared) && Args.hasArg(options::OPT_pie);
   ArgStringList CmdArgs;
 
+  // Silence warning for "clang -g foo.o -o foo"
+  Args.ClaimAllArgs(options::OPT_g_Group);
+  // and "clang -emit-llvm foo.o -o foo"
+  Args.ClaimAllArgs(options::OPT_emit_llvm);
+  // and for "clang -w foo.o -o foo". Other warning options are already
+  // handled somewhere else.
+  Args.ClaimAllArgs(options::OPT_w);
+
   if (!D.SysRoot.empty())
 CmdArgs.push_back(Args.MakeArgString("--sysroot=" + D.SysRoot));
 
+  if (IsPIE)
+CmdArgs.push_back("-pie");
+
   CmdArgs.push_back("--eh-frame-hdr");
   if (Args.hasArg(options::OPT_static)) {
 CmdArgs.push_back("-Bstatic");
   } else {
 if (Args.hasArg(options::OPT_rdynamic))
   CmdArgs.push_back("-export-dynamic");
-if (Args.hasArg(options::OPT_shared))
+if (Args.hasArg(options::OPT_shared)) {
   CmdArgs.push_back("-Bshareable");
-else {
+} else {
   CmdArgs.push_back("-dynamic-linker");
   CmdArgs.push_back("/usr/libexec/ld-elf.so.2");
 }
@@ -78,7 +93,7 @@
 
   // When building 32-bit code on DragonFly/pc64, we have to explicitly
   // instruct ld in the base system to link 32-bit code.
-  if (getToolChain().getArch() == llvm::Triple::x86) {
+  if (Arch == llvm::Triple::x86) {
 CmdArgs.push_back("-m");
 CmdArgs.push_back("elf_i386");
   }
@@ -91,43 +106,52 @@
   }
 
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles)) {
+const char *crt1 = nullptr;
 if (!Args.hasArg(options::OPT_shared)) {
   if (Args.hasArg(options::OPT_pg))
-CmdArgs.push_back(
-Args.MakeArgString(getToolChain().GetFilePath("gcrt1.o")));
-  else {
-if (Args.hasArg(options::OPT_pie))
-  CmdArgs.push_back(
-  Args.MakeArgString(getToolChain().GetFilePath("Scrt1.o")));
-else
-  CmdArgs.push_back(
-  Args.MakeArgString(getToolChain().GetFilePath("crt1.o")));
-  }
+crt1 = "gcrt1.o";
+  else if (IsPIE)
+crt1 = "Scrt1.o";
+  else
+crt1 = "crt1.o";
 }
-CmdArgs.push_back(Args.MakeArgString(getToolChain().GetFilePath("crti.o")));
-if (Args.hasArg(options::OPT_shared) || Args.hasArg(options::OPT_pie))
-  CmdArgs.push_back(
-  Args.MakeArgString(getToolChain().GetFilePath("crtbeginS.o")));
+if (crt1)
+  CmdArgs.push_back(Args.MakeArgString(ToolChain.GetFilePath(crt1)));
+
+CmdArgs.push_back(Args.MakeArgString(ToolChain.GetFilePath("crti.o")));
+
+const char *crtbegin = nullptr;
+if (Args.hasArg(options::OPT_shared) || IsPIE)
+  crtbegin = "crtbeginS.o";
 else
-  CmdArgs.push_back(
-  Args.MakeArgString(getToolChain().GetFilePath("crtbegin.o")));
+  crtbegin = "crtbegin.o";
+
+CmdArgs.push_back(Args.MakeArgString(ToolChain.GetFilePath(crtbegin)));
   }
 
-  Args.AddAllArgs(CmdArgs,
-  {options::OPT_L, options::OPT_T_Group, options::OPT_e});
+  Args.AddAllArgs(CmdArgs, options::OPT_L);
+  ToolChain.AddFilePathLibArgs(Args, CmdArgs);
+  Args.AddAllArgs(CmdArgs, options::OPT_T_Group);
+  Args.AddAllArgs(CmdArgs, options::OPT_e);