OK, I've just fixed this patch and committed it in r165430. However, I had to do rather a lot of surgery on the tests. Please review, make sure I got it right, and take it into account for subsequent patches.
On Mon, Oct 8, 2012 at 7:16 AM, David Hill <[email protected]> wrote: > Ping. > > On Sun, Oct 07, 2012 at 08:44:53AM -0400, David Hill wrote: > >Ping. > > > >On Sat, Oct 06, 2012 at 08:02:55AM -0400, David Hill wrote: > >>Ping. > >> > >>On Fri, Oct 05, 2012 at 12:42:47PM -0400, David Hill wrote: > >>>Attached is a new patch.. comments inline. > >>> > >>>On Fri, Oct 05, 2012 at 12:20:12AM -0700, Chandler Carruth wrote: > >>>> On Wed, Oct 3, 2012 at 9:23 AM, David Hill <[email protected]> wrote: > >>>> > >>>> > OK, here is patch 1 (attached). > >>>> > > >>>> > Make Bitrig's clang understand -stdlib= correctly. > >>>> > With this patch, Bitrig can use a different c++ library without > pain and > >>>> > within the normal command line paramenters. > >>>> > > >>>> > >>>> Can you add a regression test for this behavior? > >>> > >>>Sure. > >>> > >>>> > >>>> Comments on the patch: > >>>> > >>>> + case ToolChain::CST_Libstdcxx: > >>>> + std::string Triple = getTriple().str(); > >>>> + if (Triple.substr(0, 5) == "amd64") > >>>> + Triple.replace(0, 5, "x86_64"); > >>>> > >>>> > >>>> Why is this in a std::string? The StringRef API is often more clear. > >>>> How about not doing this, and below: > >>>> > >>>> + addSystemInclude(DriverArgs, CC1Args, > >>>> + getDriver().SysRoot + "/usr/include/c++/stdc++/" > >>>> + Triple); > >>>> > >>>> 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. > >>> > >>>I don't understand the above at all. > >>> > >>>> > >>>> void Bitrig::AddCXXStdlibLibArgs(const ArgList &Args, > >>>> ArgStringList &CmdArgs) const { > >>>> - CmdArgs.push_back("-lstdc++"); > >>>> + CXXStdlibType Type = GetCXXStdlibType(Args); > >>>> > >>> > >>>Fixed. > >>> > >>>> Why the extra variable? > >>>> > >>>> + > >>>> + switch (Type) { > >>>> + case ToolChain::CST_Libcxx: > >>>> + CmdArgs.push_back("-lc++"); > >>>> + CmdArgs.push_back("-lcxxrt"); > >>>> + /* for now we borrow Unwind from supc++ */ > >>> > >>>Fixed. > >>> > >>>> > >>>> No C-style comments please, and please make comments form proper > >>>> English sentences. > >>>> > >>>> http://llvm.org/docs/CodingStandards.html#commenting > >>>> > >>>> + CmdArgs.push_back("-lgcc"); > >>>> + break; > >>>> + case ToolChain::CST_Libstdcxx: > >>>> + CmdArgs.push_back("-lstdc++"); > >>>> + break; > >>>> + } > >>>> } > >>>> > >>>> > >>>> > >>>> > > >>>> > Please review. > >>>> > > >>>> > Thanks, > >>>> > David Hill > >>>> > > >>>> > On Tue, Oct 02, 2012 at 11:42:35PM -0700, Chandler Carruth wrote: > >>>> > > On Tue, Oct 2, 2012 at 10:29 PM, Nico Weber <[email protected]> > wrote: > >>>> > > > >>>> > > > It looks like this only touches bitrig-related code, which was > added > >>>> > > > by you originally. So lgtm. > >>>> > > > > >>>> > > > >>>> > > 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: > >>>> > > > >>>> > > > >>>> > > > >>>>The attached patch includes the following: > >>>> > > > >>>> > >>>> > > > >>>>- reorder linking of the libraries > >>>> > > > >>>>- support linking with pthread_p when -pg is used. > >>>> > > > >>>>- understand -stdlib= correctly > >>>> > > > >>>>- support --sysroot > >>>> > > > >>>>- add tests for LD and -pg > >>>> > > > > >>>> > > > >>>> > > 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. > >>>> > > > >>>> > > 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. > >>>> > > > >>>> > > >>>> > >>>> > > > >>>>Please review. Thank you! > >>>> > > > >>>> > >>>> > > > >>>>David Hill > >>>> > > > >>> > >>>> > > > >>>>diff --git a/lib/Driver/ToolChains.cpp > b/lib/Driver/ToolChains.cpp > >>>> > > > >>>>index 7e19551..4ce8dd3 100644 > >>>> > > > >>>>--- a/lib/Driver/ToolChains.cpp > >>>> > > > >>>>+++ b/lib/Driver/ToolChains.cpp > >>>> > > > >>>>@@ -1625,19 +1625,42 @@ void > >>>> > > > Bitrig::AddClangCXXStdlibIncludeArgs(const ArgList &DriverArgs, > >>>> > > > >>>> DriverArgs.hasArg(options::OPT_nostdincxx)) > >>>> > > > >>>> return; > >>>> > > > >>>> > >>>> > > > >>>>- std::string Triple = getTriple().str(); > >>>> > > > >>>>- if (Triple.substr(0, 5) == "amd64") > >>>> > > > >>>>- Triple.replace(0, 5, "x86_64"); > >>>> > > > >>>>- > >>>> > > > >>>>- addSystemInclude(DriverArgs, CC1Args, > "/usr/include/c++/4.6.2"); > >>>> > > > >>>>- addSystemInclude(DriverArgs, CC1Args, > >>>> > > > "/usr/include/c++/4.6.2/backward"); > >>>> > > > >>>>- addSystemInclude(DriverArgs, CC1Args, > "/usr/include/c++/4.6.2/" > >>>> > + > >>>> > > > Triple); > >>>> > > > >>>>+ CXXStdlibType Type = GetCXXStdlibType(DriverArgs); > >>>> > > > >>>>+ switch (Type) { > >>>> > > > >>>>+ case ToolChain::CST_Libcxx: > >>>> > > > >>>>+ addSystemInclude(DriverArgs, CC1Args, > >>>> > > > >>>>+ getDriver().SysRoot + > "/usr/include/c++/"); > >>>> > > > >>>>+ break; > >>>> > > > >>>>+ case ToolChain::CST_Libstdcxx: > >>>> > > > >>>>+ std::string Triple = getTriple().str(); > >>>> > > > >>>>+ if (Triple.substr(0, 5) == "amd64") > >>>> > > > >>>>+ Triple.replace(0, 5, "x86_64"); > >>>> > > > >>>> > >>>> > > > >>>>+ addSystemInclude(DriverArgs, CC1Args, > >>>> > > > >>>>+ getDriver().SysRoot + > >>>> > "/usr/include/c++/stdc++"); > >>>> > > > >>>>+ addSystemInclude(DriverArgs, CC1Args, > >>>> > > > >>>>+ getDriver().SysRoot + > >>>> > > > "/usr/include/c++/stdc++/backward"); > >>>> > > > >>>>+ addSystemInclude(DriverArgs, CC1Args, > >>>> > > > >>>>+ getDriver().SysRoot + > >>>> > "/usr/include/c++/stdc++/" > >>>> > > > + Triple); > >>>> > > > >>>>+ break; > >>>> > > > >>>>+ } > >>>> > > > >>>> } > >>>> > > > >>>> > >>>> > > > >>>> void Bitrig::AddCXXStdlibLibArgs(const ArgList &Args, > >>>> > > > >>>> ArgStringList &CmdArgs) > const { > >>>> > > > >>>>- CmdArgs.push_back("-lstdc++"); > >>>> > > > >>>>+ CXXStdlibType Type = GetCXXStdlibType(Args); > >>>> > > > >>>>+ > >>>> > > > >>>>+ switch (Type) { > >>>> > > > >>>>+ case ToolChain::CST_Libcxx: > >>>> > > > >>>>+ CmdArgs.push_back("-lc++"); > >>>> > > > >>>>+ CmdArgs.push_back("-lcxxrt"); > >>>> > > > >>>>+ /* for now we borrow Unwind from supc++ */ > >>>> > > > >>>>+ CmdArgs.push_back("-lgcc"); > >>>> > > > >>>>+ break; > >>>> > > > >>>>+ case ToolChain::CST_Libstdcxx: > >>>> > > > >>>>+ CmdArgs.push_back("-lstdc++"); > >>>> > > > >>>>+ break; > >>>> > > > >>>>+ } > >>>> > > > >>>> } > >>>> > > > >>>> > >>>> > > > >>>> /// FreeBSD - FreeBSD tool chain which can call as(1) and > ld(1) > >>>> > > > directly. > >>>> > > > >>>>diff --git a/lib/Driver/Tools.cpp b/lib/Driver/Tools.cpp > >>>> > > > >>>>index 0866c01..9aea69b 100644 > >>>> > > > >>>>--- a/lib/Driver/Tools.cpp > >>>> > > > >>>>+++ b/lib/Driver/Tools.cpp > >>>> > > > >>>>@@ -5086,23 +5086,6 @@ void > bitrig::Link::ConstructJob(Compilation > >>>> > &C, > >>>> > > > const JobAction &JA, > >>>> > > > >>>> > >>>> > > > >>>> if (!Args.hasArg(options::OPT_nostdlib) && > >>>> > > > >>>> !Args.hasArg(options::OPT_nodefaultlibs)) { > >>>> > > > >>>>- if (D.CCCIsCXX) { > >>>> > > > >>>>- getToolChain().AddCXXStdlibLibArgs(Args, CmdArgs); > >>>> > > > >>>>- if (Args.hasArg(options::OPT_pg)) > >>>> > > > >>>>- CmdArgs.push_back("-lm_p"); > >>>> > > > >>>>- else > >>>> > > > >>>>- CmdArgs.push_back("-lm"); > >>>> > > > >>>>- } > >>>> > > > >>>>- > >>>> > > > >>>>- if (Args.hasArg(options::OPT_pthread)) > >>>> > > > >>>>- CmdArgs.push_back("-lpthread"); > >>>> > > > >>>>- if (!Args.hasArg(options::OPT_shared)) { > >>>> > > > >>>>- if (Args.hasArg(options::OPT_pg)) > >>>> > > > >>>>- CmdArgs.push_back("-lc_p"); > >>>> > > > >>>>- else > >>>> > > > >>>>- CmdArgs.push_back("-lc"); > >>>> > > > >>>>- } > >>>> > > > >>>>- > >>>> > > > >>>> std::string myarch = "-lclang_rt."; > >>>> > > > >>>> const llvm::Triple &T = getToolChain().getTriple(); > >>>> > > > >>>> llvm::Triple::ArchType Arch = T.getArch(); > >>>> > > > >>>>@@ -5119,7 +5102,31 @@ void > bitrig::Link::ConstructJob(Compilation > >>>> > &C, > >>>> > > > const JobAction &JA, > >>>> > > > >>>> default: > >>>> > > > >>>> assert(0 && "Unsupported architecture"); > >>>> > > > >>>> } > >>>> > > > >>>>+ > >>>> > > > >>>> CmdArgs.push_back(Args.MakeArgString(myarch)); > >>>> > > > >>>>+ > >>>> > > > >>>>+ if (D.CCCIsCXX) { > >>>> > > > >>>>+ getToolChain().AddCXXStdlibLibArgs(Args, CmdArgs); > >>>> > > > >>>>+ if (Args.hasArg(options::OPT_pg)) > >>>> > > > >>>>+ CmdArgs.push_back("-lm_p"); > >>>> > > > >>>>+ else > >>>> > > > >>>>+ CmdArgs.push_back("-lm"); > >>>> > > > >>>>+ } > >>>> > > > >>>>+ > >>>> > > > >>>>+ if (Args.hasArg(options::OPT_pthread)) { > >>>> > > > >>>>+ if (!Args.hasArg(options::OPT_shared) && > >>>> > > > >>>>+ Args.hasArg(options::OPT_pg)) > >>>> > > > >>>>+ CmdArgs.push_back("-lpthread_p"); > >>>> > > > >>>>+ else > >>>> > > > >>>>+ CmdArgs.push_back("-lpthread"); > >>>> > > > >>>>+ } > >>>> > > > >>>>+ > >>>> > > > >>>>+ if (!Args.hasArg(options::OPT_shared)) { > >>>> > > > >>>>+ if (Args.hasArg(options::OPT_pg)) > >>>> > > > >>>>+ CmdArgs.push_back("-lc_p"); > >>>> > > > >>>>+ else > >>>> > > > >>>>+ CmdArgs.push_back("-lc"); > >>>> > > > >>>>+ } > >>>> > > > >>>> } > >>>> > > > >>>> > >>>> > > > >>>> if (!Args.hasArg(options::OPT_nostdlib) && > >>>> > > > >>>>diff --git a/test/Driver/bitrig.c b/test/Driver/bitrig.c > >>>> > > > >>>>new file mode 100644 > >>>> > > > >>>>index 0000000..412e79c > >>>> > > > >>>>--- /dev/null > >>>> > > > >>>>+++ b/test/Driver/bitrig.c > >>>> > > > >>>>@@ -0,0 +1,9 @@ > >>>> > > > >>>>+// RUN: %clang -no-canonical-prefixes -ccc-clang-archs "" > -target > >>>> > > > amd64-pc-bitrig %s -### 2>&1 \ > >>>> > > > >>>>+// RUN: | FileCheck --check-prefix=CHECK-LD %s > >>>> > > > >>>>+// CHECK-LD: clang{{.*}}" "-cc1" "-triple" > "amd64-pc-bitrig" > >>>> > > > >>>>+// 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" > >>>> > > > >>>>+ > >>>> > > > >>>>+// RUN: %clang -no-canonical-prefixes -ccc-clang-archs "" > -target > >>>> > > > amd64-pc-bitrig -pg -pthread %s -### 2>&1 \ > >>>> > > > >>>>+// RUN: | FileCheck --check-prefix=CHECK-PG %s > >>>> > > > >>>>+// CHECK-PG: clang{{.*}}" "-cc1" "-triple" > "amd64-pc-bitrig" > >>>> > > > >>>>+// 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" > >>>> > > > >>> > >>>> > > > >>>>_______________________________________________ > >>>> > > > >>>>cfe-commits mailing list > >>>> > > > >>>>[email protected] > >>>> > > > >>>>http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >>>> > > > >>> > >>>> > > > >>>_______________________________________________ > >>>> > > > >>>cfe-commits mailing list > >>>> > > > >>>[email protected] > >>>> > > > >>>http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >>>> > > > >>_______________________________________________ > >>>> > > > >>cfe-commits mailing list > >>>> > > > >>[email protected] > >>>> > > > >>http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >>>> > > > > _______________________________________________ > >>>> > > > > cfe-commits mailing list > >>>> > > > > [email protected] > >>>> > > > > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >>>> > > > _______________________________________________ > >>>> > > > cfe-commits mailing list > >>>> > > > [email protected] > >>>> > > > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >>>> > > > > >>>> > > >> > >>>diff --git a/lib/Driver/ToolChains.cpp b/lib/Driver/ToolChains.cpp > >>>index 27375de..333a94a 100644 > >>>--- a/lib/Driver/ToolChains.cpp > >>>+++ b/lib/Driver/ToolChains.cpp > >>>@@ -1668,19 +1668,39 @@ void Bitrig::AddClangCXXStdlibIncludeArgs(const > ArgList &DriverArgs, > >>> DriverArgs.hasArg(options::OPT_nostdincxx)) > >>> return; > >>> > >>>- std::string Triple = getTriple().str(); > >>>- if (Triple.substr(0, 5) == "amd64") > >>>- Triple.replace(0, 5, "x86_64"); > >>>- > >>>- addSystemInclude(DriverArgs, CC1Args, "/usr/include/c++/4.6.2"); > >>>- addSystemInclude(DriverArgs, CC1Args, > "/usr/include/c++/4.6.2/backward"); > >>>- addSystemInclude(DriverArgs, CC1Args, "/usr/include/c++/4.6.2/" + > Triple); > >>>+ switch (GetCXXStdlibType(DriverArgs)) { > >>>+ case ToolChain::CST_Libcxx: > >>>+ addSystemInclude(DriverArgs, CC1Args, > >>>+ getDriver().SysRoot + "/usr/include/c++/"); > >>>+ break; > >>>+ case ToolChain::CST_Libstdcxx: > >>>+ std::string Triple = getTriple().str(); > >>>+ if (Triple.substr(0, 5) == "amd64") > >>>+ Triple.replace(0, 5, "x86_64"); > >>> > >>>+ addSystemInclude(DriverArgs, CC1Args, > >>>+ getDriver().SysRoot + "/usr/include/c++/stdc++"); > >>>+ addSystemInclude(DriverArgs, CC1Args, > >>>+ getDriver().SysRoot + > "/usr/include/c++/stdc++/backward"); > >>>+ addSystemInclude(DriverArgs, CC1Args, > >>>+ getDriver().SysRoot + "/usr/include/c++/stdc++/" > + Triple); > >>>+ break; > >>>+ } > >>> } > >>> > >>> void Bitrig::AddCXXStdlibLibArgs(const ArgList &Args, > >>> ArgStringList &CmdArgs) const { > >>>- CmdArgs.push_back("-lstdc++"); > >>>+ switch (GetCXXStdlibType(Args)) { > >>>+ case ToolChain::CST_Libcxx: > >>>+ CmdArgs.push_back("-lc++"); > >>>+ CmdArgs.push_back("-lcxxrt"); > >>>+ // Include supc++ to provide Unwind until provided by libcxx. > >>>+ CmdArgs.push_back("-lgcc"); > >>>+ break; > >>>+ case ToolChain::CST_Libstdcxx: > >>>+ CmdArgs.push_back("-lstdc++"); > >>>+ break; > >>>+ } > >>> } > >>> > >>> /// FreeBSD - FreeBSD tool chain which can call as(1) and ld(1) > directly. > >>>diff --git a/test/Driver/bitrig.c b/test/Driver/bitrig.c > >>>new file mode 100644 > >>>index 0000000..da18a73 > >>>--- /dev/null > >>>+++ b/test/Driver/bitrig.c > >>>@@ -0,0 +1,14 @@ > >>>+// RUN: %clang -no-canonical-prefixes -ccc-clang-archs "" -target > amd64-pc-bitrig %s -### 2>&1 \ > >>>+// RUN: | FileCheck --check-prefix=CHECK-LD %s > >>>+// CHECK-LD: clang{{.*}}" "-cc1" "-triple" "amd64-pc-bitrig" > >>>+// 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" > >>>+ > >>>+// RUN: %clang++ -no-canonical-prefixes -ccc-clang-archs "" -target > amd64-pc-bitrig %s -### 2>&1 \ > >>>+// RUN: | FileCheck --check-prefix=CHECK-LD %s > >>>+// CHECK-LD: clang{{.*}}" "-cc1" "-triple" "amd64-pc-bitrig" > >>>+// CHECK-LD: ld{{.*}}" "-e" "__start" "--eh-frame-hdr" "-Bdynamic" > "-dynamic-linker" "{{.*}}ld.so" "-o" "a.out" "{{.*}}crt0.o" > "{{.*}}crtbegin.o" "{{.*}}.o" "-lclang_rt.amd64" "-lstdc++" "-lm" "-lc" > "{{.*}}crtend.o" > >>>+ > >>>+// RUN: %clang++ -stdlib=c++ -no-canonical-prefixes -ccc-clang-archs > "" -target amd64-pc-bitrig %s -### 2>&1 \ > >>>+// RUN: | FileCheck --check-prefix=CHECK-LD %s > >>>+// CHECK-LD: clang{{.*}}" "-cc1" "-triple" "amd64-pc-bitrig" > >>>+// 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++" "-lcxxrt" "-lgcc" > "-lm" "-lc" "{{.*}}crtend.o" > >> > >>>_______________________________________________ > >>>cfe-commits mailing list > >>>[email protected] > >>>http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >> > >>_______________________________________________ > >>cfe-commits mailing list > >>[email protected] > >>http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >_______________________________________________ > >cfe-commits mailing list > >[email protected] > >http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
