[cfe-commits] r90577 - in /cfe/trunk: include/clang/Driver/Options.td lib/Driver/Tools.cpp tools/driver/driver.cpp

[Daniel Dunbar]

Hi Rafael,

On Fri, Dec 4, 2009 at 11:31 AM, Rafael Espindola
<rafael.espindola at gmail.com> wrote:
> Author: rafael
> Date: Fri Dec  4 13:31:58 2009
> New Revision: 90577
>
> URL: http://llvm.org/viewvc/llvm-project?rev=90577&view=rev
> Log:
> Add gcc's -no-canonical-prefixes option to clang.

I'm not really happy about this patch (adding argument parsing logic
in tools/driver in particular). Is it also really true that this is
all this option does for gcc?

I assume this is fixing something in a google symlink madness
environment, but I think there is a better solution. The main thing we
are using this for is to find our "resources" (lib files and includes)
-- what if we searched the following locations in order:
 a. relative to argv[0]
 b. relative to argv[0], following top-level links (NOT realpath)
 c. relative to GetMainExecutable()

I suspect this would solve your problem, and would also find the
library directory much faster (than dladdr + realpath and friends) in
the common case.

I have a few other nits re the patch, inline, for posterity.

> Modified:
>    cfe/trunk/include/clang/Driver/Options.td
>    cfe/trunk/lib/Driver/Tools.cpp
>    cfe/trunk/tools/driver/driver.cpp
>
> Modified: cfe/trunk/include/clang/Driver/Options.td
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=90577&r1=90576&r2=90577&view=diff
>
> ==============================================================================
> --- cfe/trunk/include/clang/Driver/Options.td (original)
> +++ cfe/trunk/include/clang/Driver/Options.td Fri Dec  4 13:31:58 2009
> @@ -375,6 +375,8 @@
>  def mwarn_nonportable_cfstrings : Flag<"-mwarn-nonportable-cfstrings">, Group<m_Group>;
>  def m_Separate : Separate<"-m">, Group<m_Group>;
>  def m_Joined : Joined<"-m">, Group<m_Group>;
> +def no_canonical_prefixes : Flag<"-no-canonical-prefixes">, Flags<[DriverOption]>,
> +  HelpText<"Do not resolve symbolic links, turn relative paths into absolute ones, or do anything else to identify the executable">;

This should probably be under help hidden, or perhaps just in the man
page, its certainly not something to make the "short" help list.

>  def no_cpp_precomp : Flag<"-no-cpp-precomp">;
>  def no_integrated_cpp : Flag<"-no-integrated-cpp">, Flags<[DriverOption]>;
>  def no__dead__strip__inits__and__terms : Flag<"-no_dead_strip_inits_and_terms">;
>
> Modified: cfe/trunk/lib/Driver/Tools.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=90577&r1=90576&r2=90577&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Driver/Tools.cpp (original)
> +++ cfe/trunk/lib/Driver/Tools.cpp Fri Dec  4 13:31:58 2009
> @@ -1104,6 +1104,9 @@
>   // care to warn the user about.
>   Args.ClaimAllArgs(options::OPT_clang_ignored_f_Group);
>   Args.ClaimAllArgs(options::OPT_clang_ignored_m_Group);
> +
> +  // -no-canonical-prefixes is used very early in main.
> +  Args.ClaimAllArgs(options::OPT_no_canonical_prefixes);

This isn't the right place to claim, the option was handled in the
main() so it should be claimed in the driver. But the only reason this
is needed at all is because the argument parsing was done by hand.

>  }
>
>  void gcc::Common::ConstructJob(Compilation &C, const JobAction &JA,
>
> Modified: cfe/trunk/tools/driver/driver.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/driver/driver.cpp?rev=90577&r1=90576&r2=90577&view=diff
>
> ==============================================================================
> --- cfe/trunk/tools/driver/driver.cpp (original)
> +++ cfe/trunk/tools/driver/driver.cpp Fri Dec  4 13:31:58 2009
> @@ -60,7 +60,10 @@
>   OS << '\n';
>  }
>
> -llvm::sys::Path GetExecutablePath(const char *Argv0) {
> +llvm::sys::Path GetExecutablePath(const char *Argv0, bool CanonicalPrefixes) {
> +  if (!CanonicalPrefixes)
> +    return llvm::sys::Path(Argv0);
> +
>   // This just needs to be some symbol in the binary; C++ doesn't
>   // allow taking the address of ::main however.
>   void *P = (void*) (intptr_t) GetExecutablePath;
> @@ -190,7 +193,16 @@
>     return cc1_main(argv+2, argv+argc, argv[0],
>                     (void*) (intptr_t) GetExecutablePath);
>
> -  llvm::sys::Path Path = GetExecutablePath(argv[0]);
> +  bool CanonicalPrefixes = true;
> +  for (int i = 1; i < argc; ++i) {
> +    if (llvm::StringRef(argv[i]) == "-no-canonical-prefixes") {
> +      CanonicalPrefixes = false;
> +      break;
> +    }
> +  }
> +
> +  llvm::sys::Path Path = GetExecutablePath(argv[0], CanonicalPrefixes);
> +
>   DriverDiagnosticPrinter DiagClient(Path.getBasename(), llvm::errs());
>
>   Diagnostic Diags(&DiagClient);
> @@ -264,4 +276,3 @@
>
>   return Res;
>  }
> -
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>