Re: [PATCH v2] PR12331: Introduce stap options --sysroot and --sysenv

[Josh Stone <jistone@redhat.com>]

Looks pretty good.  I found a few things in review, noted below, but
otherwise I think we can go ahead and push this into the repo, and patch
any other issues as they arise.

We should have at least some minimal testing for these options.  It's
hard since the testsuite can't assume any location of a real sysroot,
but at least it should be sanity checked that something like this
doesn't break:  stap --sysroot=/ --sysenv=PATH=$PATH ...

Docs are needed too -- please update stap.1 and NEWS.

Thanks,
Josh


(review is heavily snipped, but hopefully it makes sense...)

On 02/09/2012 10:26 AM, Wade Farnsworth wrote:
>    // Hash compiler path, size, and mtime.  We're just going to assume
>    // we'll be using gcc. XXX: getting kbuild to spit out out would be
>    // better, especially since this is fooled by ccache.
> -  h.add_path("Compiler ", find_executable("gcc"));
> +  h.add_path("Compiler ", find_executable("gcc", "", dummy));

Not your fault, but that comment is getting more urgent. (or rather,
this way of hashing gcc is getting more irrelevent.)  Tracking the file
info of a ccache wrapper is not helpful, and this is also missing the
boat if CROSS_COMPILE is used.  May have to look at kbuild again, or
manually emulate kbuild's logic in choosing a compiler...

> @@ -86,6 +87,8 @@ systemtap_session::systemtap_session ():
>    kernel_release = string (buf.release);
>    release = kernel_release;
>    kernel_build_tree = "/lib/modules/" + kernel_release + "/build";
> +  sysroot = "";
> +  update_release_sysroot = false;

These two and sysenv should be addressed in the session copy-ctor too.

> +            case LONG_OPT_SYSROOT:
> +              if (client_options) {
> +                  cerr << _F("ERROR: %s invalid with %s", "--sysroot", "--client-options") << endl;
> +                  return 1;
> +              } else {
> +                  const char *spath = canonicalize_file_name (optarg);
> +                  if (spath == NULL) {
> +                      cerr << _F("ERROR: %s is an invalid directory for --sysroot", optarg) << endl;
> +                      return 1;
> +                  }
> +
> +                  sysroot = string(spath);
> +                  if (sysroot[sysroot.size() - 1] != '/')
> +                      sysroot.append("/");
> +
> +                  if (update_release_sysroot)
> +                      kernel_build_tree = sysroot + kernel_build_tree;

This method of "update_release_sysroot" feels convoluted to me.  For
one, if --sysroot occurs multiple times, it will get prepended every
time.  (If there are other reasons to prohibit multiple sysroot, then
please check that.)

How about instead, just delay sysroot prepending until after all arg
processing?  I think in passes_0_4() right at the start of pass 0 is
better, just before the comment that says:

 // Now that no further changes to s.kernel_build_tree can occur [...]

> +
> +                  debuginfo_path_insert_sysroot(sysroot);

This too should probably wait until options are all settled.

>  DwflPtr
> -setup_dwfl_user(const std::string &name)
> +setup_dwfl_user(const std::string &name, systemtap_session &s)
>  {
>    if (user_dwfl != NULL
>        && user_modset.size() == 1

Missing code to make use of the new "s" here?

> +        if (!sess.sysroot.empty() && ((pos = path.find(sess.sysroot)) != string::npos))
> +          path.replace(pos, sess.sysroot.length(), "/");

IMHO, this replacement is common enough to deserve a function.

>    // Canonicalize the path name.
>    char *cf = canonicalize_file_name (retpath.c_str());
>    if (cf)
>      {
> -      retpath = string(cf);
> +      string scf = string(cf);
> +      if (sysroot.empty())
> +        retpath = scf;
> +      else {
> +        int pos = scf.find(sysroot);
> +        if (pos == 0)
> +          retpath = scf;
> +        else {
> +          cerr << _F("ERROR: file %s not in sysroot %s", cf, sysroot.c_str()) << endl;
> +          exit(1);

We need to do better than an exit(1) here, because there will be
temporary files and possibly --remote connections that need to be
cleaned up.  Probably "throw runtime_error" will suffice instead.