[cfe-commits] [PATCH] Bitrig - multiple changes
Chandler Carruth
chandlerc at google.com
Fri Oct 5 00:20:12 PDT 2012
On Wed, Oct 3, 2012 at 9:23 AM, David Hill <dhill at mindcry.org> 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?
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.
void Bitrig::AddCXXStdlibLibArgs(const ArgList &Args,
ArgStringList &CmdArgs) const {
- CmdArgs.push_back("-lstdc++");
+ CXXStdlibType Type = GetCXXStdlibType(Args);
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++ */
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 <thakis at chromium.org> 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
> > > >>>>cfe-commits at cs.uiuc.edu
> > > >>>>http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> > > >>>
> > > >>>_______________________________________________
> > > >>>cfe-commits mailing list
> > > >>>cfe-commits at cs.uiuc.edu
> > > >>>http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> > > >>_______________________________________________
> > > >>cfe-commits mailing list
> > > >>cfe-commits at cs.uiuc.edu
> > > >>http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> > > > _______________________________________________
> > > > cfe-commits mailing list
> > > > cfe-commits at cs.uiuc.edu
> > > > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> > > _______________________________________________
> > > cfe-commits mailing list
> > > cfe-commits at cs.uiuc.edu
> > > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121005/7ab9f43d/attachment.html>
More information about the cfe-commits
mailing list