[patch]change dwarf2_start_subfile() to adapt inappropriate dir name

[patch]change dwarf2_start_subfile() to adapt inappropriate dir name

[JuYoung Kim]

Hello, My name is Kim, Juyoung at TmaxCore co. in south korea.
I've sent this email last week but haven't receive any reply or inform.
Today I found the email was encoded in korean and maybe you could not read it.
So, I send this mail again, and I hope you reply me as possible as you can, please.

It is about subfile for DWARF.
When you see the function dwarf2_start_subfile() in the source, there is a comment about source's file name.
Some compiler ( like RCVT3.1 ) include '/' at the end of dir name.

For example,
in case, DW_AT_name: /srcdir/list0.c
you expect
     files.files[0].name: list0.h
     files.files[0].dir: /srcdir
but, the DWARF made by RVCT3.1 is like this.
    files.files[0].name: list0.h
    files.files[0].dir: /srcdir/

then the function dwarf2_start_subfile(); make the fullname "/srcdir//list0.c"
, which result in the gdb cannot find the source's debugging information.

The function dwarf2_start_subfile() is at gdb/dwarf2read.c:8650.

The original source code is

.........
  if (!IS_ABSOLUTE_PATH (filename) && dirname != NULL) {
    fullname = concat (dirname, SLASH_STRING, filename, (char *)NULL);
  }
  else
    fullname = filename;
........

I changed it as below.

........
  if (!IS_ABSOLUTE_PATH (filename) && dirname != NULL) {
     if (dirname[strlen(dirname)-1] == '/' || dirname[strlen(dirname)-1] == '')
        fullname = concat (dirname, filename, (char *)NULL);
     else fullname = concat (dirname, SLASH_STRING, filename, (char *)NULL);
  }
  else
     fullname = filename;
..........

This is my first time to report the patch.
So please, inform me if my way of reporting is not proper.

Thanks.

Re: [patch]change dwarf2_start_subfile() to adapt inappropriate dir name

[Eli Zaretskii]

> Date: Mon, 15 Nov 2010 01:21:14 +0000 (GMT)
> From: JuYoung Kim <j0.kim@samsung.com>
> Cc: 김주영 <j0.kim@samsung.com>
> 
>   if (!IS_ABSOLUTE_PATH (filename) && dirname != NULL) {
>      if (dirname[strlen(dirname)-1] == '/' || dirname[strlen(dirname)-1] == '')

Please use IS_DIR_SEPARATOR instead of testing for '/' literally.  And
the second part of the if clause cannot happen at all, so it should be
removed.

Also, the GNU coding standards say to put the braces like this:

   if (something)
    {
      do_something;
      do_something_else;
    }

>      else fullname = concat (dirname, SLASH_STRING, filename, (char *)NULL);

Make "else" have its own line, like this:

   else
     fullname = concat (dirname, SLASH_STRING, filename, NULL);

Re: Re: [patch]change dwarf2_start_subfile() to adapt inappropriate dir name

[JuYoung Kim]

Thanks for your kind comment.

I changed the source code as below :

........
  if (!IS_ABSOLUTE_PATH (filename) && dirname != NULL) 
    {
      if (IS_DIR_SEPARATOR(dirname[strlen (dirname) - 1]))
        fullname = concat (dirname, filename, (char *)NULL);
      else 
        fullname = concat (dirname, SLASH_STRING, filename, (char *)NULL);
    }
  else
    fullname = filename;
..........

I hope this code is proper to the standard, and no more to be fixed.
Please, tell me what is the next step that I should do for applying to the real source code.

Thanks.

------- Original Message -------
Sender : Eli Zaretskii<eliz@gnu.org>
Date : 2010-11-15 13:08 (GMT+09:00)
Title : Re: [patch]change dwarf2_start_subfile() to adapt inappropriate dir name
 
> Date: Mon, 15 Nov 2010 01:21:14 +0000 (GMT)
> From: JuYoung Kim 
> Cc: 김주영 
> 
>   if (!IS_ABSOLUTE_PATH (filename) && dirname != NULL) {
>      if (dirname[strlen(dirname)-1] == '/' || dirname[strlen(dirname)-1] == '')

Please use IS_DIR_SEPARATOR instead of testing for '/' literally.  And
the second part of the if clause cannot happen at all, so it should be
removed.

Also, the GNU coding standards say to put the braces like this:

   if (something)
    {
      do_something;
      do_something_else;
    }

>      else fullname = concat (dirname, SLASH_STRING, filename, (char *)NULL);

Make "else" have its own line, like this:

   else
     fullname = concat (dirname, SLASH_STRING, filename, NULL);

Re: Re: [patch]change dwarf2_start_subfile() to adapt inappropriate dir name

[Joel Brobecker]

[Would you like to be called "Kim", or "JuYoung"?]

> I hope this code is proper to the standard, and no more to be fixed.
> Please, tell me what is the next step that I should do for applying to
> the real source code.

For proper submission of patches to the GDB project, please take a look
at the file named gdb/CONTRIBUTE.  For the patch, most of us prefer
unified diffs (use "diff -u" as opposed to "diff -c").

Last question: Is your mailer capable of keeping the email threading
information?

-- 
Joel

Re: Re: [patch]change dwarf2_start_subfile() to adapt inappropriate dir name

[Nathan Froyd]

On Mon, Nov 15, 2010 at 08:58:34AM -0800, Joel Brobecker wrote:
> [Would you like to be called "Kim", or "JuYoung"?]
> 
> > I hope this code is proper to the standard, and no more to be fixed.
> > Please, tell me what is the next step that I should do for applying to
> > the real source code.
> 
> For proper submission of patches to the GDB project, please take a look
> at the file named gdb/CONTRIBUTE.  For the patch, most of us prefer
> unified diffs (use "diff -u" as opposed to "diff -c").

FWIW, we encountered this problem as well.  This is the patch we used to
fix it.  It's very similar to the proposed patch, but slightly more
complete.

Tested with powerpc-linux-gnu.  No regressions.

-Nathan

	* dwarf2read.c (dwarf_maybe_concatenate): New function.
	(dwarf_decode_lines): Use it.
	(dwarf2_start_subfile): Likewise.

Index: gdb/dwarf2read.c
===================================================================
--- gdb/dwarf2read.c	(revision 283114)
+++ gdb/dwarf2read.c	(revision 283115)
@@ -8355,6 +8355,32 @@ check_cu_functions (CORE_ADDR address, s
   return fn->lowpc;
 }
 
+/* Concatenate DIRNAME with FILENAME if FILENAME is not already
+   absolute.  DIRNAME may be NULL; if so, FILENAME is returned.  If the
+   return value is not equal to FILENAME, the caller is responsible for
+   free'ing it.  */
+
+static char *
+dwarf_maybe_concatenate (char *dirname, char *filename)
+{
+  if (!IS_ABSOLUTE_PATH (filename) && dirname != NULL)
+    {
+      int dir_len = strlen (dirname);
+      char *concat_name;
+
+      concat_name = concat (dirname, SLASH_STRING, filename, (char *) NULL);
+
+      if (dirname[dir_len-1] == SLASH_STRING[0])
+	memmove (concat_name+dir_len-1, concat_name+dir_len,
+		 /* SLASH_STRING and trailing NULL */
+		 strlen (filename) + 1 + 1);
+
+      return concat_name;
+    }
+  else
+    return filename;
+}
+
 /* Decode the Line Number Program (LNP) for the given line_header
    structure and CU.  The actual information extracted and the type
    of structures created from the LNP depends on the value of PST.
@@ -8633,25 +8659,22 @@ dwarf_decode_lines (struct line_header *
           {
             const struct file_entry fe = lh->file_names [file_index];
             char *include_name = fe.name;
+	    char *orig;
             char *dir_name = NULL;
             char *pst_filename = pst->filename;
 
             if (fe.dir_index)
               dir_name = lh->include_dirs[fe.dir_index - 1];
 
-            if (!IS_ABSOLUTE_PATH (include_name) && dir_name != NULL)
-              {
-                include_name = concat (dir_name, SLASH_STRING,
-				       include_name, (char *)NULL);
-                make_cleanup (xfree, include_name);
-              }
-
-            if (!IS_ABSOLUTE_PATH (pst_filename) && pst->dirname != NULL)
-              {
-                pst_filename = concat (pst->dirname, SLASH_STRING,
-				       pst_filename, (char *)NULL);
-                make_cleanup (xfree, pst_filename);
-              }
+	    orig = include_name;
+	    include_name = dwarf_maybe_concatenate (dir_name, include_name);
+	    if (include_name != orig)
+	      make_cleanup (xfree, include_name);
+
+	    orig = pst_filename;
+	    pst_filename = dwarf_maybe_concatenate (pst->dirname, pst_filename);
+	    if (pst_filename != orig)
+	      make_cleanup (xfree, pst_filename);
 
             if (gdb_filename_cmp (include_name, pst_filename) != 0)
               dwarf2_create_include_psymtab (include_name, pst, objfile);
@@ -8724,13 +8747,9 @@ dwarf2_start_subfile (char *filename, ch
      we concatenate it to the filename when it makes sense.
      Note that the Dwarf3 standard says (speaking of filenames in line
      information): ``The directory index is ignored for file names
-     that represent full path names''.  Thus ignoring dirname in the
-     `else' branch below isn't an issue.  */
+     that represent full path names''.  */
 
-  if (!IS_ABSOLUTE_PATH (filename) && dirname != NULL)
-    fullname = concat (dirname, SLASH_STRING, filename, (char *)NULL);
-  else
-    fullname = filename;
+  fullname = dwarf_maybe_concatenate (dirname, filename);
 
   start_subfile (fullname, comp_dir);
 

Re: [patch]change dwarf2_start_subfile() to adapt inappropriate dir name

[Eli Zaretskii]

> Date: Mon, 15 Nov 2010 10:05:14 -0800
> From: Nathan Froyd <froydnj@codesourcery.com>
> Cc: JuYoung Kim <j0.kim@samsung.com>, Eli Zaretskii <eliz@gnu.org>,
> 	"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> 
> +      if (dirname[dir_len-1] == SLASH_STRING[0])

Sorry, but comparing with SLASH_STRING[0] is as bad as comparing with
a literal '/' or '\\'.  The issue here is that DOS/Windows filesystems
can use both / and \, and even mix them freely in the same file name.
IS_DIR_SEPARATOR takes this into account, while comparing against a
single character will miss the other alternative.

SLASH_STRING is for _constructing_ file names (with `concat' or
equivalent code), not for taking file names apart or examining them.

Re: [patch]change dwarf2_start_subfile() to adapt inappropriate dir name

[Tom Tromey]

>>>>> "Nathan" == Nathan Froyd <froydnj@codesourcery.com> writes:

Nathan> FWIW, we encountered this problem as well.  This is the patch we used to
Nathan> fix it.  It's very similar to the proposed patch, but slightly more
Nathan> complete.

Thanks.

Nathan> +      if (dirname[dir_len-1] == SLASH_STRING[0])

Noting Eli's note...

Nathan> +	memmove (concat_name+dir_len-1, concat_name+dir_len,

Spaces around the operators.

Nathan> +		 /* SLASH_STRING and trailing NULL */
Nathan> +		 strlen (filename) + 1 + 1);

I think it is mildly better to use strlen (SLASH_STRING) rather than 1.
Or just call concat two different ways instead of memmove.

This is ok with these nits fixed.

Tom