Hi, could you please review the two patches adding the configure flag --with-default-sysroot?
I addressed all the comments from the previous review by Rafael. Thanks, Sebastian -- Qualcomm Innovation Center, Inc is a member of Code Aurora Forum ---------- Forwarded message ---------- From: <[email protected]> Date: Fri, Apr 6, 2012 at 3:22 PM Subject: Re: [cfe-commits] Use GCC_INSTALL_PREFIX to initialize SysRoot To: Rafael EspÃndola <[email protected]> Cc: [email protected] Hi Rafael, See attached the patches with the changes that you suggested. Tested on amd64-linux. Ok to commit? Thanks, Sebastian -- Qualcomm Innovation Center, Inc is a member of Code Aurora Forum >>> + TranslatedArgs(_TranslatedArgs), Redirects(0), >>> SysRoot(D.SysRoot) >>> >>> Is this value of SysRoot ever used? It should not, right? It is >>> probably better to initialize SysRoot with an invalid value instead of >>> D.SysRoot. >> >> I'm not sure I understand your comment here. >> >> We can also implement getSysRoot by returning the sysroot of the driver. > > I am suggesting initializing with something like > > ... SysRoot(".....") > > to make sure we find any code using the SysRoot before the following code > runs. > >>> if (SysRoot != "") { >>> + llvm::SmallString<128> Prefix(SysRoot); >>> + if (Prefix.back() == '/') { >>> + llvm::sys::path::remove_filename(Prefix); // remove the / >>> + SysRoot = Prefix.str(); >>> + } >>> + } >>> >>> This points a StringRef to a value that is destroyed at the closing >>> brace. >> >> Right, I have not understood your remark last time. >> As the original code was not removing the trailing /, >> I removed that code. > > I think you have another use after free bug in > > + CmdArgs.push_back(sysroot.str().c_str()); > > since this creates a std::string and takes a pointer to the inner c > string. You probably have to use C.getArgs().MakeArgString. > >> Sebastian >> -- >> Qualcomm Innovation Center, Inc is a member of Code Aurora Forum > > Cheers, > Rafael > > p.s.: you dropped cfe-commits in the reply. > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>From a28cd8ed98978a8ca4ce7baa8b1debfcdec5e3ba Mon Sep 17 00:00:00 2001 From: Sebastian Pop <[email protected]> Date: Thu, 9 Feb 2012 13:02:01 -0600 Subject: [PATCH] add configure flag --with-default-sysroot --- autoconf/configure.ac | 7 +++++++ configure | 19 +++++++++++++++++-- include/llvm/Config/config.h.cmake | 3 --- include/llvm/Config/config.h.in | 3 +++ 4 files changed, 27 insertions(+), 5 deletions(-) diff --git a/autoconf/configure.ac b/autoconf/configure.ac index 884a21b..baf440e 100644 --- a/autoconf/configure.ac +++ b/autoconf/configure.ac @@ -838,6 +838,13 @@ AC_ARG_WITH(gcc-toolchain, AC_DEFINE_UNQUOTED(GCC_INSTALL_PREFIX,"$withval", [Directory where gcc is installed.]) +AC_ARG_WITH(sysroot, + AS_HELP_STRING([--with-default-sysroot], + [Add --sysroot=<path> to all compiler invocations.]),, + withval="") +AC_DEFINE_UNQUOTED(DEFAULT_SYSROOT,"$withval", + [Default <path> to all compiler invocations for --sysroot=<path>.]) + dnl Allow linking of LLVM with GPLv3 binutils code. AC_ARG_WITH(binutils-include, AS_HELP_STRING([--with-binutils-include], diff --git a/configure b/configure index 8ff4e62..be2cb72 100755 --- a/configure +++ b/configure @@ -1442,6 +1442,7 @@ Optional Packages: --with-c-include-dirs Colon separated list of directories clang will search for headers --with-gcc-toolchain Directory where gcc is installed. + --with-default-sysroot Add --sysroot=<path> to all compiler invocations. --with-binutils-include Specify path to binutils/include/ containing plugin-api.h file for gold plugin. --with-bug-report-url Specify the URL where bug reports should be @@ -3802,7 +3803,7 @@ else llvm_cv_target_os_type="Darwin" ;; *-*-minix*) llvm_cv_target_os_type="Minix" ;; - *-*-freebsd*| *-*-kfreebsd-gnu) + *-*-freebsd* | *-*-kfreebsd-gnu) llvm_cv_target_os_type="FreeBSD" ;; *-*-openbsd*) llvm_cv_target_os_type="OpenBSD" ;; @@ -5583,6 +5584,20 @@ _ACEOF +# Check whether --with-sysroot was given. +if test "${with_sysroot+set}" = set; then + withval=$with_sysroot; +else + withval="" +fi + + +cat >>confdefs.h <<_ACEOF +#define DEFAULT_SYSROOT "$withval" +_ACEOF + + + # Check whether --with-binutils-include was given. if test "${with_binutils_include+set}" = set; then withval=$with_binutils_include; @@ -10386,7 +10401,7 @@ else lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 lt_status=$lt_dlunknown cat > conftest.$ac_ext <<EOF -#line 10387 "configure" +#line 10404 "configure" #include "confdefs.h" #if HAVE_DLFCN_H diff --git a/include/llvm/Config/config.h.cmake b/include/llvm/Config/config.h.cmake index 69e3580..95c4d6c 100644 --- a/include/llvm/Config/config.h.cmake +++ b/include/llvm/Config/config.h.cmake @@ -11,9 +11,6 @@ /* Relative directory for resource files */ #define CLANG_RESOURCE_DIR "${CLANG_RESOURCE_DIR}" -/* Directory wherelibstdc++ is installed. */ -#define GCC_INSTALL_PREFIX "${GCC_INSTALL_PREFIX}" - /* Directories clang will search for headers */ #define C_INCLUDE_DIRS "${C_INCLUDE_DIRS}" diff --git a/include/llvm/Config/config.h.in b/include/llvm/Config/config.h.in index ccff7da..677bf2e 100644 --- a/include/llvm/Config/config.h.in +++ b/include/llvm/Config/config.h.in @@ -12,6 +12,9 @@ /* Directories clang will search for headers */ #undef C_INCLUDE_DIRS +/* Default <path> to all compiler invocations for --sysroot=<path>. */ +#undef DEFAULT_SYSROOT + /* Define if position independent code is enabled */ #undef ENABLE_PIC -- 1.7.5.4
>From 0c41bf9b2f62e47aed1036d598615391a3601e85 Mon Sep 17 00:00:00 2001 From: Sebastian Pop <[email protected]> Date: Thu, 9 Feb 2012 13:06:08 -0600 Subject: [PATCH] use DEFAULT_SYSROOT --- CMakeLists.txt | 4 ++++ include/clang/Config/config.h.cmake | 9 ++++++--- include/clang/Config/config.h.in | 9 ++++++--- include/clang/Driver/Compilation.h | 3 +++ lib/Driver/Compilation.cpp | 4 ++++ lib/Driver/Driver.cpp | 8 +++----- lib/Driver/Tools.cpp | 10 ++++++---- 7 files changed, 32 insertions(+), 15 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 1197610..c828482 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -66,6 +66,10 @@ set(CLANG_RESOURCE_DIR "" CACHE STRING set(C_INCLUDE_DIRS "" CACHE STRING "Colon separated list of directories clang will search for headers.") +set(GCC_INSTALL_PREFIX "" CACHE PATH "Directory where gcc is installed." ) +set(DEFAULT_SYSROOT "" CACHE PATH + "Default <path> to all compiler invocations for --sysroot=<path>." ) + set(CLANG_VENDOR "" CACHE STRING "Vendor-specific text for showing with version information.") diff --git a/include/clang/Config/config.h.cmake b/include/clang/Config/config.h.cmake index bd5dc31..c18c4cc 100644 --- a/include/clang/Config/config.h.cmake +++ b/include/clang/Config/config.h.cmake @@ -4,8 +4,11 @@ /* Relative directory for resource files */ #define CLANG_RESOURCE_DIR "${CLANG_RESOURCE_DIR}" -/* Directory where gcc is installed. */ -#define GCC_INSTALL_PREFIX "${GCC_INSTALL_PREFIX}" - /* Directories clang will search for headers */ #define C_INCLUDE_DIRS "${C_INCLUDE_DIRS}" + +/* Default <path> to all compiler invocations for --sysroot=<path>. */ +#define DEFAULT_SYSROOT "${DEFAULT_SYSROOT}" + +/* Directory where gcc is installed. */ +#define GCC_INSTALL_PREFIX "${GCC_INSTALL_PREFIX}" diff --git a/include/clang/Config/config.h.in b/include/clang/Config/config.h.in index 3f5d503..24ed6bd 100644 --- a/include/clang/Config/config.h.in +++ b/include/clang/Config/config.h.in @@ -9,10 +9,13 @@ /* Relative directory for resource files */ #undef CLANG_RESOURCE_DIR -/* Directory where gcc is installed. */ -#undef GCC_INSTALL_PREFIX - /* Directories clang will search for headers */ #undef C_INCLUDE_DIRS +/* Default <path> to all compiler invocations for --sysroot=<path>. */ +#undef DEFAULT_SYSROOT + +/* Directory where gcc is installed. */ +#undef GCC_INSTALL_PREFIX + #endif diff --git a/include/clang/Driver/Compilation.h b/include/clang/Driver/Compilation.h index fd88c3a..6f1a221 100644 --- a/include/clang/Driver/Compilation.h +++ b/include/clang/Driver/Compilation.h @@ -92,6 +92,9 @@ public: return FailureResultFiles; } + /// Returns the sysroot path. + StringRef getSysRoot() const; + /// getArgsForToolChain - Return the derived argument list for the /// tool chain \arg TC (or the default tool chain, if TC is not /// specified). diff --git a/lib/Driver/Compilation.cpp b/lib/Driver/Compilation.cpp index 42c8449..5553fc9 100644 --- a/lib/Driver/Compilation.cpp +++ b/lib/Driver/Compilation.cpp @@ -230,3 +230,7 @@ void Compilation::initCompilationForDiagnostics(void) { Redirects[1] = new const llvm::sys::Path(); Redirects[2] = new const llvm::sys::Path(); } + +StringRef Compilation::getSysRoot(void) const { + return getDriver().SysRoot; +} diff --git a/lib/Driver/Driver.cpp b/lib/Driver/Driver.cpp index 35e6398..57cb153 100644 --- a/lib/Driver/Driver.cpp +++ b/lib/Driver/Driver.cpp @@ -49,8 +49,8 @@ Driver::Driver(StringRef ClangExecutable, bool IsProduction, DiagnosticsEngine &Diags) : Opts(createDriverOptTable()), Diags(Diags), - ClangExecutable(ClangExecutable), UseStdLib(true), - DefaultTargetTriple(DefaultTargetTriple), + ClangExecutable(ClangExecutable), SysRoot(DEFAULT_SYSROOT), + UseStdLib(true), DefaultTargetTriple(DefaultTargetTriple), DefaultImageName(DefaultImageName), DriverTitle("clang \"gcc-compatible\" driver"), CCPrintOptionsFilename(0), CCPrintHeadersFilename(0), @@ -660,9 +660,7 @@ bool Driver::HandleImmediateArgs(const Compilation &C) { llvm::outs() << "\n"; llvm::outs() << "libraries: =" << ResourceDir; - std::string sysroot; - if (Arg *A = C.getArgs().getLastArg(options::OPT__sysroot_EQ)) - sysroot = A->getValue(C.getArgs()); + StringRef sysroot = C.getSysRoot(); for (ToolChain::path_list::const_iterator it = TC.getFilePaths().begin(), ie = TC.getFilePaths().end(); it != ie; ++it) { diff --git a/lib/Driver/Tools.cpp b/lib/Driver/Tools.cpp index 4c4ab7b..fc3a988 100644 --- a/lib/Driver/Tools.cpp +++ b/lib/Driver/Tools.cpp @@ -377,10 +377,11 @@ void Clang::AddPreprocessingOptions(Compilation &C, // If we have a --sysroot, and don't have an explicit -isysroot flag, add an // -isysroot to the CC1 invocation. - if (Arg *A = Args.getLastArg(options::OPT__sysroot_EQ)) { + StringRef sysroot = C.getSysRoot(); + if (sysroot != "") { if (!Args.hasArg(options::OPT_isysroot)) { CmdArgs.push_back("-isysroot"); - CmdArgs.push_back(A->getValue(Args)); + CmdArgs.push_back(C.getArgs().MakeArgString(sysroot)); } } @@ -3971,9 +3972,10 @@ void darwin::Link::AddLinkArgs(Compilation &C, // Give --sysroot= preference, over the Apple specific behavior to also use // --isysroot as the syslibroot. - if (const Arg *A = Args.getLastArg(options::OPT__sysroot_EQ)) { + StringRef sysroot = C.getSysRoot(); + if (sysroot != "") { CmdArgs.push_back("-syslibroot"); - CmdArgs.push_back(A->getValue(Args)); + CmdArgs.push_back(C.getArgs().MakeArgString(sysroot)); } else if (const Arg *A = Args.getLastArg(options::OPT_isysroot)) { CmdArgs.push_back("-syslibroot"); CmdArgs.push_back(A->getValue(Args)); -- 1.7.5.4
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
