[2/2] Trim trailing directory separators from new sysroots.

The separate debugfile logic assumes that the sysroot does not include
a trailing separator (all of the sysroot logic in
find_separate_debug_file requires the next character after the sysroot
in a object file path to be a directory separator).  However, normal
filename completion will result in setting a sysroot with a trailing
directory separator.  If the directory portion of a new sysroot ends
with a directory separator and is not specifying the root directory,
trim the trailing separator.  This permits the sysroot logic to work
the same if a sysroot of "/myroot/" is specified instead of "/myroot".

Note that the sysroot displayed by 'show sysroot' will reflect the
trimmed sysroot.

gdb/ChangeLog:

	* solib.c (gdb_sysroot_changed): Trim trailing directory separator
	from new sysroot.
---
 gdb/ChangeLog | 5 +++++
 gdb/solib.c   | 8 ++++++++
 2 files changed, 13 insertions(+)

On 2019-01-22 8:03 p.m., John Baldwin wrote:
> The separate debugfile logic assumes that the sysroot does not include
> a trailing separator (all of the sysroot logic in
> find_separate_debug_file requires the next character after the sysroot
> in a object file path to be a directory separator).  However, normal
> filename completion will result in setting a sysroot with a trailing
> directory separator.  If the directory portion of a new sysroot ends
> with a directory separator and is not specifying the root directory,
> trim the trailing separator.  This permits the sysroot logic to work
> the same if a sysroot of "/myroot/" is specified instead of "/myroot".
> 
> Note that the sysroot displayed by 'show sysroot' will reflect the
> trimmed sysroot.

I am not sure about this, it seems fragile to do this one off thing.  The
point where the path is stripped is quite far from the point where it has
an impact, so it's really not obvious why we do it (though this could be
fixed by saying why we do it in the comment).

I think it would be a step in a better direction to have an "is_child_path"
function that returns whether a path is a child of another one.  It would
be responsible for handling the case of the parent potentially having a
trailing directory separator.  The implementation of such function might
get hairy, but the  advantage is that it should be pretty easy to unit
test.

Finally, I think it would make this code more obvious:

      if (canon_dir != NULL
	  && filename_ncmp (canon_dir, gdb_sysroot,
			    strlen (gdb_sysroot)) == 0
	  && IS_DIR_SEPARATOR (canon_dir[strlen (gdb_sysroot)]))

vs

      if (canon_dir != NULL && is_child_patn (gdb_sysroot, canon_dir))

Simon

On 1/26/19 11:10 AM, Simon Marchi wrote:
> On 2019-01-22 8:03 p.m., John Baldwin wrote:
>> The separate debugfile logic assumes that the sysroot does not include
>> a trailing separator (all of the sysroot logic in
>> find_separate_debug_file requires the next character after the sysroot
>> in a object file path to be a directory separator).  However, normal
>> filename completion will result in setting a sysroot with a trailing
>> directory separator.  If the directory portion of a new sysroot ends
>> with a directory separator and is not specifying the root directory,
>> trim the trailing separator.  This permits the sysroot logic to work
>> the same if a sysroot of "/myroot/" is specified instead of "/myroot".
>>
>> Note that the sysroot displayed by 'show sysroot' will reflect the
>> trimmed sysroot.
> 
> I am not sure about this, it seems fragile to do this one off thing.  The
> point where the path is stripped is quite far from the point where it has
> an impact, so it's really not obvious why we do it (though this could be
> fixed by saying why we do it in the comment).
> 
> I think it would be a step in a better direction to have an "is_child_path"
> function that returns whether a path is a child of another one.  It would
> be responsible for handling the case of the parent potentially having a
> trailing directory separator.  The implementation of such function might
> get hairy, but the  advantage is that it should be pretty easy to unit
> test.
> 
> Finally, I think it would make this code more obvious:
> 
>       if (canon_dir != NULL
> 	  && filename_ncmp (canon_dir, gdb_sysroot,
> 			    strlen (gdb_sysroot)) == 0
> 	  && IS_DIR_SEPARATOR (canon_dir[strlen (gdb_sysroot)]))
> 
> vs
> 
>       if (canon_dir != NULL && is_child_patn (gdb_sysroot, canon_dir))

I have taken this approach, but it ended up being a bit more than just a
boolean check as I needed to reliably get the base path of the child
component as the existing 'canon_dir + strlen (gdb_sysroot)' assumed no
trailing separator in gdb_sysroot.  I'll post the updated version along
with another fix I encountered in some more testing today as a v2 in a
sec.