This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: [RFC] xfullpath and new regression test xfullpath.exp
- From: Eli Zaretskii <eliz at is dot elta dot co dot il>
- To: Joel Brobecker <brobecker at ACT-Europe dot FR>
- Cc: gdb-patches at sources dot redhat dot com
- Date: Mon, 1 Apr 2002 12:42:13 +0200 (IST)
- Subject: Re: [RFC] xfullpath and new regression test xfullpath.exp
On Mon, 1 Apr 2002, Joel Brobecker wrote:
> I implemented the new xfullpath function based on Andrew's comments, and
> made some adjustements to fix the problem reported in
>
> http://sources.redhat.com/ml/gdb-patches/2002-03/msg00345.html
>
> while being careful not to break Tom's setup. The patch is attached.
Thanks. However, this patch will not DTRT on DOS and Windows systems,
I'm afraid. Details below.
> * source.c (openp): Use xfullpath in place of gdb_realpath to
> avoid resolving the basename part of filenames when the
> associated file is a symbolic link. This fixes a potential
> inconsistency between the filenames known to GDB and the
> filenames it prints in the annotations.
This kind of detailed description should go into the source, IMHO. It's
okay to have it in the ChangeLog as well, but having it _only_ in the
ChangeLog is not a good idea: people who read the code don't expect to
find the associated comments in the logs.
+ /*
+ * xfullpath
+ *
+ * Return a copy of FILENAME, with its directory prefix canonicalized
+ * by gdb_realpath.
+ */
I believe this is not the comment style the GNU project uses.
Here are specific comments about the code part of your patch:
+ char *
+ xfullpath (const char *filename)
+ {
+ const char *base_name = lbasename (filename);
+ char *dir_name;
+ char *real_path;
+ char *result;
+
+ /* Extract the basename of filename, and return immediately
+ a copy of filename if it does not contain any directory prefix. */
+ if (base_name == filename)
+ return xstrdup (filename);
+
+ dir_name = alloca ((size_t) (base_name - filename + 1));
+ strncpy (dir_name, filename, base_name - filename);
+ dir_name[base_name - filename] = '\000';
This doesn't work correctly when the input name is something like
"d:foo". lbasename returns a pointer to `f', so you end up with the
directory being "d:", and the rest of the code will generate "d:/foo",
which is something very different in semantics. The Right Thing to do
in this case is to return either the unmodified "d:foo" or "d:./foo".
+ /* Canonicalize the directory prefix, and build the resulting
+ filename. Avoid returning a path which starts with 2 SLASH_CHARS
+ if the canonicalized path is the root path */
+ real_path = gdb_realpath (dir_name);
+ if (strcmp (real_path, SLASH_STRING) == 0)
+ result = concat (SLASH_STRING, base_name, NULL);
+ else
+ result = concat (real_path, SLASH_STRING, base_name, NULL);
This doesn't work if the input is "\\", which is also a root directory.
You should use the more portable IS_DIR_SEPARATOR macro (defined on
include/filenames.h) instead.
Finally, I don't understand why do you avoid duplicate slashes when
dir_name is "/", but not if it is "/foo/bar/". Why not handle the
other case as well? (This issue is a general one, not something specific
to MS-Windows, although the case of "d:/", the root directory on drive d:,
was what initially brought that to my attention.)