Re: [PATCH] Add --sysroot option

[Josh Stone <jistone at redhat dot com>]

On 01/09/2012 06:51 AM, Wade Farnsworth wrote:
> This adds a --sysroot option to facilitate different file locations for
> user-space processes and libraries at compile-time versus run-time when
> generating stap kernel modules for a remote system.

This enhancement is PR12331, so please prefix as such in your patch's
subject.  If you like, you may also claim assignment on the bug itself.
  http://sourceware.org/bugzilla/show_bug.cgi?id=12331

> 
> For example if we compile the probe:
> 
> process("/bin/foo").begin {}
> 
> while passing "--sysroot=/bar/baz" to stap, symbols will be taken from
> /bar/baz/bin/foo, but the resulting .ko will still refer to /bin/foo.
> This allows the probe to be used on a different machine than the one it
> is compiled on, so long as the compiling machine replicates the
> necessary files in the sysroot directory.  This is a fairly typical use case
> for embedded Linux.
> 
> Known limitations:
> 1. Probes must contain the absolute file path to the process or library.
>    If a relative path is used, it will be assumed to be located in the
>    top level of the sysroot.
> 2. Similarly probes on scripts containing "#!/usr/bin/env bash" or similar
>    will also assume that the interpreter is located in the sysroot
>    top level.
> The reason for these limitations is that we do not have access to the
> target machine's environment compile-time, so some assumptions must
> necessarily be made.

It is a bit sticky that we don't know the target environment PATH or
LD_LIBRARY_PATH.  However, I think even assuming the same as this host
is better than nothing, so one can at least benefit from the usual
/bin:/usr/bin:... sort of paths.  If need be, we can add another option
for this, something like --sysenv=PATH=/bin:/system/bin:etc.

So at least the basic part of this can be achieved by making
find_executable() smarter about sysroot.  In calls like this:

> diff --git a/tapsets.cxx b/tapsets.cxx
> index 177f565..50464dc 100644
> --- a/tapsets.cxx
> +++ b/tapsets.cxx
> @@ -584,13 +584,13 @@ base_query::base_query(dwflpp & dw, literal_map_t const & params):
>        has_statement = get_number_param(params, TOK_STATEMENT, statement_num_val);
>  
>        if (has_process)
> -        module_val = find_executable (module_val);
> +        module_val = find_executable (sess.sysroot + module_val);
>        if (has_library)
>          {
>            if (! contains_glob_chars (library_name))
>              {
>                path = module_val;
> -              module_val = find_executable (library_name, "LD_LIBRARY_PATH");
> +              module_val = find_executable (sess.sysroot + library_name, "LD_LIBRARY_PATH");
>              }
>            else
>              path = library_name;

Notice that find_executable() takes an environment variable to to use
for path searching (and defaults to "PATH").  I think the sysroot needs
to be taken as a separate parameter in these calls, so it can be
searched appropriately.

The current behavior of find_executable() is if the name already has a
'/', return as-is, else search the path.  The new behavior needs to also
prefix everything, so for names with '/' return sysroot/name, else
search sysroot/PATH1/name, sysroot/PATH2/name, etc. and return whatever
matches.

This can still be escaped by relative paths that traverse upwards, e.g.
"../../../usr/bin/foo".  User shoots self in foot, news at 11.  But this
may matter for the client-server mode if a server has chosen to build
from some sysroot.  Perhaps ".." paths should be disallowed when sysroot
is in use?  I'm not really sure how well we can really protect this, but
better to at least try.

Also, I think clients should not be allowed to pass sysroot at all,
given that they don't really know how the server is laid out in the
first place.  See how the client_options flag is used in restricting
other options, and please do the same for --sysroot.

Your patch focuses on process/library paths, but I think there are a few
other cases that will need sysroot treatment too.  For one, I would
expect that some of the dwarf processing will need munging, especially
for the case with split debuginfo from the main binary.  Another is for
locating the kernel binaries and build tree -- if -r is given as a path,
that's probably fine, but for plain versions we should probably look in
sysroot/lib/modules/RELEASE/...

Thanks for tackling this feature, and sorry for the delayed review.

Josh