[driver] : Fix relocatable toolchain path-replacement in driver

Hi,

This change fixes a quirk happening for relocated toolchains.  Driver
remembers original-build directory
in std_prefix variable for being able later to modify path.  Sadly
this std_prefix variable gets modified
later on, and so update_path can't work any longer as desired.  This
patch fixes that by introducing an
constant variable holding the initial path without being later on modified.

ChangeLog

2013-09-11  Kai Tietz  <ktietz@redhat.com>

    * prefix.c (org_prefix): New static variable.
    (update_path): Use org_prefix instead of std_prefix.

Tested for i686-w64-mingw32, for x86_64-w64-mingw32, and for
x86_64-unknown-linux-gnu.  Ok for apply?

Regards,
Kai

On Wed, 11 Sep 2013, Kai Tietz wrote:

> This change fixes a quirk happening for relocated toolchains.  Driver
> remembers original-build directory

The original *build* directory should never be known to the driver; only 
the *configured* prefix.

This area is complicated and subtle; you need to be very careful and 
precise in the analysis included in any patch submission related to it.  
Get things absolutely clear in your head and then write a complete, 
careful, precise self-contained explanation of everything relevant in GCC, 
as if for an audience not at all familiar with the code in question.

> in std_prefix variable for being able later to modify path.  Sadly
> this std_prefix variable gets modified
> later on, and so update_path can't work any longer as desired.  This

You don't say what "as desired" is.  prefix.c contains a long description 
of *what* the path updates are, but no explanation of *why* or any overall 
design; it appears to be something Windows-specific.

update_path should, as I understand it, always be called with PATH being a 
relocated path (one that has had the configured prefix converted to the 
prefix where the toolchain is in fact installed, using 
make_relative_prefix, or that doesn't need any relocation, e.g. a path 
passed with a -B option).  In incpath.c, for example, you see how a path 
is computed using make_relative_prefix before update_path is called.  
Thus, it is correct for std_prefix to be the relocated prefix rather than 
the unrelocated one.

If there is a bug, I'd say it's in whatever code is calling update_path 
without having first done the make_relative_prefix relocation on the path 
being passed to update_path.  Thus, it is that caller that you need to 
fix.

2013/9/11 Joseph S. Myers <joseph@codesourcery.com>:
> On Wed, 11 Sep 2013, Kai Tietz wrote:
>
>> This change fixes a quirk happening for relocated toolchains.  Driver
>> remembers original-build directory
>
> The original *build* directory should never be known to the driver; only
> the *configured* prefix.
>
> This area is complicated and subtle; you need to be very careful and
> precise in the analysis included in any patch submission related to it.
> Get things absolutely clear in your head and then write a complete,
> careful, precise self-contained explanation of everything relevant in GCC,
> as if for an audience not at all familiar with the code in question.

That I agree on, and it took me some time to find this.

>> in std_prefix variable for being able later to modify path.  Sadly
>> this std_prefix variable gets modified
>> later on, and so update_path can't work any longer as desired.  This
>
> You don't say what "as desired" is.  prefix.c contains a long description
> of *what* the path updates are, but no explanation of *why* or any overall
> design; it appears to be something Windows-specific.

Well it might be Windows specific.  It shows its bad side on Windows
due old build-prefix remains on translation and leads to the try to
access an invalid path.  That isn't necessarily a problem as long as
the drive-letter exists.  Otherwise user gets system-failures by this.
 I am pretty sure that it isn't related to make_relative_path, as
otherwise we would see same behavior on other targets, too.

> update_path should, as I understand it, always be called with PATH being a
> relocated path (one that has had the configured prefix converted to the
> prefix where the toolchain is in fact installed, using
> make_relative_prefix, or that doesn't need any relocation, e.g. a path
> passed with a -B option).  In incpath.c, for example, you see how a path
> is computed using make_relative_prefix before update_path is called.
> Thus, it is correct for std_prefix to be the relocated prefix rather than
> the unrelocated one.

I understand this different.  See here translate_name function (used
by update_path).  If path is empty it falls back to PREFIX (not
std_prefix).  So I understand that this routine tries to cut off
compiled in prefix from paths and replaces it by key's path.  But
well, I might got this wrong.
The bad point about all this on native Windows targets is the use of
msys and its stupid auto-magic PATH-translation (means it converts
simply everything what it sees to begin with a slash ...).  By this it
might be that it is indeed a Windows specific thing here.
Nevertheless one I think we need to address ...


> If there is a bug, I'd say it's in whatever code is calling update_path
> without having first done the make_relative_prefix relocation on the path
> being passed to update_path.  Thus, it is that caller that you need to
> fix.
>
> --
> Joseph S. Myers
> joseph@codesourcery.com

Regards,
Kai

On Wed, 11 Sep 2013, Kai Tietz wrote:

> > update_path should, as I understand it, always be called with PATH being a
> > relocated path (one that has had the configured prefix converted to the
> > prefix where the toolchain is in fact installed, using
> > make_relative_prefix, or that doesn't need any relocation, e.g. a path
> > passed with a -B option).  In incpath.c, for example, you see how a path
> > is computed using make_relative_prefix before update_path is called.
> > Thus, it is correct for std_prefix to be the relocated prefix rather than
> > the unrelocated one.
> 
> I understand this different.  See here translate_name function (used
> by update_path).  If path is empty it falls back to PREFIX (not
> std_prefix).  So I understand that this routine tries to cut off
> compiled in prefix from paths and replaces it by key's path.  But
> well, I might got this wrong.

It doesn't really make sense to me for there to be a mixture of direct 
replacement of the configure-time prefix based on keys, and first 
replacing the configure-time prefix with the install location and then 
replacing that based on keys.

I think the most appropriate design is as I said: make_relative_prefix, 
and nothing else, deals with replacing the configure-time prefix with the 
install location, and then prefix.c deals with any subsequent replacements 
of the install location.  On that basis, translate_name should be using 
std_prefix not PREFIX, and any callers of these prefix.c interfaces that 
pass paths still using the configure-time prefix should be fixed so that 
the make_relative_prefix substitution occurs first.

You've found evidence of an inconsistency.  A fix needs to be based on a 
clear design - a clear understanding of what sort of paths should be used 
where and what interfaces should translate them to other kinds of paths - 
rather than just locally changing the paths used in one particular place 
without a basis for arguing that the change fits a coherent design and so 
won't cause problems elsewhere.