Hi Chandler, if you care enough about bitrig to reject mostly harmless patches, it kinda feels like you should also care enough to not ignore the follow-up patch for over two days. This really discourages sending small, focused patches.
Nico On Fri, Oct 5, 2012 at 7:18 AM, David Hill <[email protected]> wrote: > OK? > > On Wed, Oct 03, 2012 at 12:23:29PM -0400, David Hill 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. >> >>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 81f38878..2c841ff 100644 >>--- a/lib/Driver/ToolChains.cpp >>+++ b/lib/Driver/ToolChains.cpp >>@@ -1665,19 +1665,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; >>+ } >> } > >>_______________________________________________ >>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
