Re: [patch] symfile find_separate_debug_file

[Doug Evans <dje at google dot com>]

On Fri, Aug 14, 2009 at 6:15 AM, Aleksandar Ristovski<aristovski@qnx.com> wrote:
> Tom Tromey wrote:
>>>>>>>
>>>>>>> "Doug" == Doug Evans <dje@google.com> writes:
>>
>> Doug> fwiw, I wouldn't mind an elaboration of the comment, e.g. provide an
>> Doug> example when ?"unless there isn't" happens. ?Otherwise I have to go
>> Doug> looking and assume what I find is what the comment is referring to.
>>
>> Sounds reasonable to me.
>
> My initial idea was to remove that comment altogether since it describes an
> assumption that may have been good at the time, but doesn't stand any
> longer. But then went with "minimal change" approach.
>
> The code itself is clear and I don't think it needs explanation: if file
> does not contain any directory separators, use current directory. It happens
> when gdb opens a shared library from its current directory (e.g. libc.so.3).
>
> So how about I remove portion of that comment, and leave only generic part
> that makes no assumptions about presence of dir. separators? (new patch
> attached).
>
> --
> Aleksandar Ristovski
> QNX Software Systems
>
> * symfile.c (find_separate_debug_file): Fix the case when objfile has only
> basename as its name.
>
>

I'm not sure if things are that clear (to me anyway, apologies).

I totally understand wanting to make the code robust, regardless of
input.  But there's still some ambiguity in what is correct input.
For executables gdb goes to the trouble of recording the path as a
full path (even if specified without a path).  E.g. see
symfile_bfd_open.  And I found this in objfiles.h for struct objfile:

    /* The object file's name, tilde-expanded and absolute.
       Malloc'd; free it if you free this struct.  */

    char *name;

The comment in symfile.c said "Objfiles should always be absolute and
tilde expanded."  So maybe the bug is that somewhere during solib
processing its path didn't get fully expanded.  OTOH if the comment is
indeed no longer true (if only in the case of solibs), I'd still like
to see the comment in objfiles.h corrected and elaborated on.

For reference sake, here's your patch inline:

* symfile.c (find_separate_debug_file): Fix the case when objfile has
only basename as its name.

Index: gdb/symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.240
@@ -1377,15 +1371,21 @@ find_separate_debug_file (struct objfile
   dir = xstrdup (objfile->name);

   /* Strip off the final filename part, leaving the directory name,
-     followed by a slash.  Objfile names should always be absolute and
-     tilde-expanded, so there should always be a slash in there
-     somewhere.  */
-  for (i = strlen(dir) - 1; i >= 0; i--)
+     followed by a slash.  */
+  for (i = strlen (dir) - 1; i >= 0; i--)
     {
       if (IS_DIR_SEPARATOR (dir[i]))
        break;
     }
-  gdb_assert (i >= 0 && IS_DIR_SEPARATOR (dir[i]));
+  if (i < 0)
+    {
+      /* We are dealing with base name only.  Make it current dir.  */
+      if (strlen (dir) < 2)
+       dir = xrealloc (dir, 3);
+      dir[0] = '.';
+      dir[1] = '/';
+      i = 1;
+    }
   dir[i+1] = '\0';

   /* Set I to max (strlen (canon_name), strlen (dir)). */