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
