On Wed, Oct 3, 2012 at 9:23 AM, David Hill <span dir="ltr"><<a href="mailto:dhill@mindcry.org" target="_blank" class="cremed">dhill@mindcry.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">OK, here is patch 1 (attached).<br>
<br>
Make Bitrig's clang understand -stdlib= correctly.<br>
With this patch, Bitrig can use a different c++ library without pain and<br>
within the normal command line paramenters.<br></blockquote><div><br></div><div>Can you add a regression test for this behavior?</div><div><br></div><div>Comments on the patch:</div><div><br></div><div><pre style="color:rgb(0,0,0);word-wrap:break-word;white-space:pre-wrap">
+ case ToolChain::CST_Libstdcxx:
+ std::string Triple = getTriple().str();
+ if (Triple.substr(0, 5) == "amd64")
+ Triple.replace(0, 5, "x86_64");
</pre><pre style="color:rgb(0,0,0);word-wrap:break-word;white-space:pre-wrap">Why is this in a std::string? The StringRef API is often more clear. How about not doing this, and below:</pre><pre style="word-wrap:break-word">
<pre style="color:rgb(0,0,0);white-space:pre-wrap;word-wrap:break-word">+ addSystemInclude(DriverArgs, CC1Args,
+ getDriver().SysRoot + "/usr/include/c++/stdc++/" + Triple);</pre><pre style="color:rgb(0,0,0);white-space:pre-wrap;word-wrap:break-word">Replace this with two addSystemInclude calls. One that does ".../stdc++/x86_64" + getTriple().drop_front(5) and another that does ".../stdc++/" + getTriple(). You can use .startswith("amd64") for the test.</pre>
<pre style="color:rgb(0,0,0);white-space:pre-wrap;word-wrap:break-word"> void Bitrig::AddCXXStdlibLibArgs(const ArgList &Args,
ArgStringList &CmdArgs) const {
- CmdArgs.push_back("-lstdc++");
+ CXXStdlibType Type = GetCXXStdlibType(Args);</pre><pre style="color:rgb(0,0,0);white-space:pre-wrap;word-wrap:break-word">Why the extra variable?</pre><pre style="color:rgb(0,0,0);white-space:pre-wrap;word-wrap:break-word">
+
+ switch (Type) {
+ case ToolChain::CST_Libcxx:
+ CmdArgs.push_back("-lc++");
+ CmdArgs.push_back("-lcxxrt");
+ /* for now we borrow Unwind from supc++ */</pre><pre style="color:rgb(0,0,0);white-space:pre-wrap;word-wrap:break-word">No C-style comments please, and please make comments form proper English sentences.</pre><pre style="word-wrap:break-word">
<font color="#000000"><span style="white-space:pre-wrap"><a href="http://llvm.org/docs/CodingStandards.html#commenting">http://llvm.org/docs/CodingStandards.html#commenting</a><br></span></font></pre><pre style="color:rgb(0,0,0);white-space:pre-wrap;word-wrap:break-word">
+ CmdArgs.push_back("-lgcc");
+ break;
+ case ToolChain::CST_Libstdcxx:
+ CmdArgs.push_back("-lstdc++");
+ break;
+ }
}</pre></pre></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Please review.<br>
<br>
Thanks,<br>
David Hill<br>
<div class=""><div class="h5"><br>
On Tue, Oct 02, 2012 at 11:42:35PM -0700, Chandler Carruth wrote:<br>
> On Tue, Oct 2, 2012 at 10:29 PM, Nico Weber <<a href="mailto:thakis@chromium.org" class="cremed">thakis@chromium.org</a>> wrote:<br>
><br>
> > It looks like this only touches bitrig-related code, which was added<br>
> > by you originally. So lgtm.<br>
> ><br>
><br>
> It doesn't look quite that good to me. ;] The reason I've not looked at<br>
> this is in part because the patch is really hard to review:<br>
><br>
><br>
> > >>>>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>
> ><br>
><br>
> Can you break this up into 4 or 5 patches, making them more targeted? It's<br>
> hard to tell what is and isn't tested here, or what changes have to do with<br>
> each other.<br>
><br>
> Also, explain in more detail (maybe even in comments) for some of these --<br>
> 'reorder linking' isn't terribly clear what the end goal is or what was<br>
> necessary to get there.<br>
><br>
> >>>><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<br>
> > 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,<br>
> > "/usr/include/c++/4.6.2/backward");<br>
> > >>>>- addSystemInclude(DriverArgs, CC1Args, "/usr/include/c++/4.6.2/" +<br>
> > 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 +<br>
> > "/usr/include/c++/stdc++/backward");<br>
> > >>>>+ addSystemInclude(DriverArgs, CC1Args,<br>
> > >>>>+ getDriver().SysRoot + "/usr/include/c++/stdc++/"<br>
> > + 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)<br>
> > 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,<br>
> > 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,<br>
> > 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<br>
> > 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"<br>
> > "-dynamic-linker" "{{.*}}ld.so" "-o" "a.out" "{{.*}}crt0.o"<br>
> > "{{.*}}crtbegin.o" "{{.*}}.o" "-lclang_rt.amd64" "-lc" "{{.*}}crtend.o"<br>
> > >>>>+<br>
> > >>>>+// RUN: %clang -no-canonical-prefixes -ccc-clang-archs "" -target<br>
> > 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"<br>
> > "-dynamic-linker" "{{.*}}ld.so" "-o" "a.out" "{{.*}}crt0.o"<br>
> > "{{.*}}crtbegin.o" "{{.*}}.o" "-lclang_rt.amd64" "-lpthread_p" "-lc_p"<br>
> > "{{.*}}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>
> ><br>
</div></div></blockquote></div><br></div>