[patch] symfile find_separate_debug_file

[patch] symfile find_separate_debug_file

[Aleksandar Ristovski]

[-- Attachment #1: Type: text/plain, Size: 794 bytes --]

Hello,

I run into this accidentally. When debugging a shared object 
with embedded symbol file name (.gnu_debuglink section), we 
made an assumption that objfile would always have full path. 
However, if shared object was opened using base name only, 
we will end up with an objfile with the same name.

By looking at allocate_objfile which generates the name for 
an objfile, it is clear that there could be any kind of path 
including full path, relative path or file name only.

The patch checks for such situation and instead of 
gdb_assert-ing, creates current directory prefix.


Tested by running the case manually.


Thanks,

-- 
Aleksandar Ristovski
QNX Software Systems


ChangeLog:

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


[-- Attachment #2: symfile-20090810.diff --]
[-- Type: text/x-patch, Size: 1126 bytes --]

Index: gdb/symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.240
diff -u -p -r1.240 symfile.c
--- gdb/symfile.c	3 Aug 2009 17:00:34 -0000	1.240
+++ gdb/symfile.c	10 Aug 2009 17:56:24 -0000
@@ -1379,13 +1373,21 @@ find_separate_debug_file (struct objfile
   /* 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--)
+     somewhere, unless there isn't.  See allocate_objfile.  */
+  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)). */

Re: [patch] symfile find_separate_debug_file

[Tom Tromey]

>>>>> "Aleksandar" == Aleksandar Ristovski <aristovski@qnx.com> writes:

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

Aleksandar> +     somewhere, unless there isn't.  See allocate_objfile.  */

Funny comment :)

This is ok.

It seems to me that we probably need some kind of OBJF_IS_FILE flag,
because we do have objfiles that don't come from files, but that still
have names.  Constructing file names derived from these names seems
bogus.

Tom

Re: [patch] symfile find_separate_debug_file

[Doug Evans]

On Thu, Aug 13, 2009 at 2:37 PM, Tom Tromey<tromey@redhat.com> wrote:
>>>>>> "Aleksandar" == Aleksandar Ristovski <aristovski@qnx.com> writes:
>
> Aleksandar> * symfile.c (find_separate_debug_file): Fix the case when
> Aleksandar> objfile has only basename as its name.
>
> Aleksandar> +     somewhere, unless there isn't.  See allocate_objfile.  */
>
> Funny comment :)

Ya, reminds me of one in c++:

3.5 Program and Linkage [basic.link] paragraph 6:
"extern means static except when it doesn't"

> This is ok.
>
> It seems to me that we probably need some kind of OBJF_IS_FILE flag,
> because we do have objfiles that don't come from files, but that still
> have names.  Constructing file names derived from these names seems
> bogus.

fwiw, I wouldn't mind an elaboration of the comment, e.g. provide an
example when  "unless there isn't" happens.  Otherwise I have to go
looking and assume what I find is what the comment is referring to.

Re: [patch] symfile find_separate_debug_file

[Tom Tromey]

>>>>> "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.

Tom

Re: [patch] symfile find_separate_debug_file

[Aleksandar Ristovski]

[-- Attachment #1: Type: text/plain, Size: 1076 bytes --]

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.


[-- Attachment #2: symfile-20090814.diff --]
[-- Type: text/x-patch, Size: 1005 bytes --]

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)). */

Re: [patch] symfile find_separate_debug_file

[Doug Evans]

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)). */