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
