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

John Baldwin
2019-01-28T20:41:00Z

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.

--
John Baldwin

Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â