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
>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
