On Tue, Oct 2, 2012 at 10:29 PM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank" class="cremed">thakis@chromium.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">It looks like this only touches bitrig-related code, which was added<br>
by you originally. So lgtm.<br></blockquote><div><br></div><div>It doesn't look quite that good to me. ;] The reason I've not looked at this is in part because the patch is really hard to review:</div><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
>>>>The attached patch includes the following:<br>
>>>><br>
>>>>- reorder linking of the libraries<br>
>>>>- support linking with pthread_p when -pg is used.<br>
>>>>- understand -stdlib= correctly<br>
>>>>- support --sysroot<br>
>>>>- add tests for LD and -pg<br></div></div></blockquote><div><br></div><div>Can you break this up into 4 or 5 patches, making them more targeted? It's hard to tell what is and isn't tested here, or what changes have to do with each other.</div>
<div><br></div><div>Also, explain in more detail (maybe even in comments) for some of these -- 'reorder linking' isn't terribly clear what the end goal is or what was necessary to get there.</div><div><br></div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
>>>><br>
>>>>Please review.  Thank you!<br>
>>>><br>
>>>>David Hill<br>
>>><br>
>>>>diff --git a/lib/Driver/ToolChains.cpp b/lib/Driver/ToolChains.cpp<br>
>>>>index 7e19551..4ce8dd3 100644<br>
>>>>--- a/lib/Driver/ToolChains.cpp<br>
>>>>+++ b/lib/Driver/ToolChains.cpp<br>
>>>>@@ -1625,19 +1625,42 @@ void Bitrig::AddClangCXXStdlibIncludeArgs(const ArgList &DriverArgs,<br>
>>>>       DriverArgs.hasArg(options::OPT_nostdincxx))<br>
>>>>     return;<br>
>>>><br>
>>>>-  std::string Triple = getTriple().str();<br>
>>>>-  if (Triple.substr(0, 5) == "amd64")<br>
>>>>-    Triple.replace(0, 5, "x86_64");<br>
>>>>-<br>
>>>>-  addSystemInclude(DriverArgs, CC1Args, "/usr/include/c++/4.6.2");<br>
>>>>-  addSystemInclude(DriverArgs, CC1Args, "/usr/include/c++/4.6.2/backward");<br>
>>>>-  addSystemInclude(DriverArgs, CC1Args, "/usr/include/c++/4.6.2/" + Triple);<br>
>>>>+  CXXStdlibType Type = GetCXXStdlibType(DriverArgs);<br>
>>>>+  switch (Type) {<br>
>>>>+  case ToolChain::CST_Libcxx:<br>
>>>>+    addSystemInclude(DriverArgs, CC1Args,<br>
>>>>+                     getDriver().SysRoot + "/usr/include/c++/");<br>
>>>>+    break;<br>
>>>>+  case ToolChain::CST_Libstdcxx:<br>
>>>>+    std::string Triple = getTriple().str();<br>
>>>>+    if (Triple.substr(0, 5) == "amd64")<br>
>>>>+      Triple.replace(0, 5, "x86_64");<br>
>>>><br>
>>>>+    addSystemInclude(DriverArgs, CC1Args,<br>
>>>>+                     getDriver().SysRoot + "/usr/include/c++/stdc++");<br>
>>>>+    addSystemInclude(DriverArgs, CC1Args,<br>
>>>>+                     getDriver().SysRoot + "/usr/include/c++/stdc++/backward");<br>
>>>>+    addSystemInclude(DriverArgs, CC1Args,<br>
>>>>+                     getDriver().SysRoot + "/usr/include/c++/stdc++/" + Triple);<br>
>>>>+    break;<br>
>>>>+  }<br>
>>>> }<br>
>>>><br>
>>>> void Bitrig::AddCXXStdlibLibArgs(const ArgList &Args,<br>
>>>>                                  ArgStringList &CmdArgs) const {<br>
>>>>-  CmdArgs.push_back("-lstdc++");<br>
>>>>+  CXXStdlibType Type = GetCXXStdlibType(Args);<br>
>>>>+<br>
>>>>+  switch (Type) {<br>
>>>>+  case ToolChain::CST_Libcxx:<br>
>>>>+    CmdArgs.push_back("-lc++");<br>
>>>>+    CmdArgs.push_back("-lcxxrt");<br>
>>>>+    /* for now we borrow Unwind from supc++ */<br>
>>>>+    CmdArgs.push_back("-lgcc");<br>
>>>>+    break;<br>
>>>>+  case ToolChain::CST_Libstdcxx:<br>
>>>>+    CmdArgs.push_back("-lstdc++");<br>
>>>>+    break;<br>
>>>>+  }<br>
>>>> }<br>
>>>><br>
>>>> /// FreeBSD - FreeBSD tool chain which can call as(1) and ld(1) directly.<br>
>>>>diff --git a/lib/Driver/Tools.cpp b/lib/Driver/Tools.cpp<br>
>>>>index 0866c01..9aea69b 100644<br>
>>>>--- a/lib/Driver/Tools.cpp<br>
>>>>+++ b/lib/Driver/Tools.cpp<br>
>>>>@@ -5086,23 +5086,6 @@ void bitrig::Link::ConstructJob(Compilation &C, const JobAction &JA,<br>
>>>><br>
>>>>   if (!Args.hasArg(options::OPT_nostdlib) &&<br>
>>>>       !Args.hasArg(options::OPT_nodefaultlibs)) {<br>
>>>>-    if (D.CCCIsCXX) {<br>
>>>>-      getToolChain().AddCXXStdlibLibArgs(Args, CmdArgs);<br>
>>>>-      if (Args.hasArg(options::OPT_pg))<br>
>>>>-        CmdArgs.push_back("-lm_p");<br>
>>>>-      else<br>
>>>>-        CmdArgs.push_back("-lm");<br>
>>>>-    }<br>
>>>>-<br>
>>>>-    if (Args.hasArg(options::OPT_pthread))<br>
>>>>-      CmdArgs.push_back("-lpthread");<br>
>>>>-    if (!Args.hasArg(options::OPT_shared)) {<br>
>>>>-      if (Args.hasArg(options::OPT_pg))<br>
>>>>-        CmdArgs.push_back("-lc_p");<br>
>>>>-      else<br>
>>>>-        CmdArgs.push_back("-lc");<br>
>>>>-    }<br>
>>>>-<br>
>>>>     std::string myarch = "-lclang_rt.";<br>
>>>>     const llvm::Triple &T = getToolChain().getTriple();<br>
>>>>     llvm::Triple::ArchType Arch = T.getArch();<br>
>>>>@@ -5119,7 +5102,31 @@ void bitrig::Link::ConstructJob(Compilation &C, const JobAction &JA,<br>
>>>>           default:<br>
>>>>             assert(0 && "Unsupported architecture");<br>
>>>>      }<br>
>>>>+<br>
>>>>      CmdArgs.push_back(Args.MakeArgString(myarch));<br>
>>>>+<br>
>>>>+     if (D.CCCIsCXX) {<br>
>>>>+       getToolChain().AddCXXStdlibLibArgs(Args, CmdArgs);<br>
>>>>+       if (Args.hasArg(options::OPT_pg))<br>
>>>>+         CmdArgs.push_back("-lm_p");<br>
>>>>+       else<br>
>>>>+         CmdArgs.push_back("-lm");<br>
>>>>+     }<br>
>>>>+<br>
>>>>+     if (Args.hasArg(options::OPT_pthread)) {<br>
>>>>+       if (!Args.hasArg(options::OPT_shared) &&<br>
>>>>+           Args.hasArg(options::OPT_pg))<br>
>>>>+         CmdArgs.push_back("-lpthread_p");<br>
>>>>+      else<br>
>>>>+         CmdArgs.push_back("-lpthread");<br>
>>>>+     }<br>
>>>>+<br>
>>>>+     if (!Args.hasArg(options::OPT_shared)) {<br>
>>>>+       if (Args.hasArg(options::OPT_pg))<br>
>>>>+         CmdArgs.push_back("-lc_p");<br>
>>>>+       else<br>
>>>>+         CmdArgs.push_back("-lc");<br>
>>>>+     }<br>
>>>>   }<br>
>>>><br>
>>>>   if (!Args.hasArg(options::OPT_nostdlib) &&<br>
>>>>diff --git a/test/Driver/bitrig.c b/test/Driver/bitrig.c<br>
>>>>new file mode 100644<br>
>>>>index 0000000..412e79c<br>
>>>>--- /dev/null<br>
>>>>+++ b/test/Driver/bitrig.c<br>
>>>>@@ -0,0 +1,9 @@<br>
>>>>+// RUN: %clang -no-canonical-prefixes -ccc-clang-archs "" -target amd64-pc-bitrig %s -### 2>&1 \<br>
>>>>+// RUN:   | FileCheck --check-prefix=CHECK-LD %s<br>
>>>>+// CHECK-LD: clang{{.*}}" "-cc1" "-triple" "amd64-pc-bitrig"<br>
>>>>+// CHECK-LD: ld{{.*}}" "-e" "__start" "--eh-frame-hdr" "-Bdynamic" "-dynamic-linker" "{{.*}}ld.so" "-o" "a.out" "{{.*}}crt0.o" "{{.*}}crtbegin.o" "{{.*}}.o" "-lclang_rt.amd64" "-lc" "{{.*}}crtend.o"<br>

>>>>+<br>
>>>>+// RUN: %clang -no-canonical-prefixes -ccc-clang-archs "" -target amd64-pc-bitrig -pg -pthread %s -### 2>&1 \<br>
>>>>+// RUN:   | FileCheck --check-prefix=CHECK-PG %s<br>
>>>>+// CHECK-PG: clang{{.*}}" "-cc1" "-triple" "amd64-pc-bitrig"<br>
>>>>+// CHECK-PG: ld{{.*}}" "-e" "__start" "--eh-frame-hdr" "-Bdynamic" "-dynamic-linker" "{{.*}}ld.so" "-o" "a.out" "{{.*}}crt0.o" "{{.*}}crtbegin.o" "{{.*}}.o" "-lclang_rt.amd64" "-lpthread_p" "-lc_p" "{{.*}}crtend.o"<br>

>>><br>
>>>>_______________________________________________<br>
>>>>cfe-commits mailing list<br>
>>>><a href="mailto:cfe-commits@cs.uiuc.edu" class="cremed">cfe-commits@cs.uiuc.edu</a><br>
>>>><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
>>><br>
>>>_______________________________________________<br>
>>>cfe-commits mailing list<br>
>>><a href="mailto:cfe-commits@cs.uiuc.edu" class="cremed">cfe-commits@cs.uiuc.edu</a><br>
>>><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
>>_______________________________________________<br>
>>cfe-commits mailing list<br>
>><a href="mailto:cfe-commits@cs.uiuc.edu" class="cremed">cfe-commits@cs.uiuc.edu</a><br>
>><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu" class="cremed">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" class="cremed">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div>