[prev in list] [next in list] [prev in thread] [next in thread] 

List:       gdb-patches
Subject:    [PATCH 2/2] Trim trailing directory separators from new sysroots.
From:       jhb () FreeBSD ! org (John Baldwin)
Date:       2019-01-28 20:41:00
Message-ID: 79e03f4a-6b48-4d30-8427-a7594bc38bb6 () FreeBSD ! org
[Download RAW message or body]

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

????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????



[prev in list] [next in list] [prev in thread] [next in thread]