* Re: Re: [patch]change dwarf2_start_subfile() to adapt inappropriate dir name
@ 2010-11-15 6:25 JuYoung Kim
2010-11-15 16:58 ` Joel Brobecker
0 siblings, 1 reply; 5+ messages in thread
From: JuYoung Kim @ 2010-11-15 6:25 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: gdb-patches
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);
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Re: [patch]change dwarf2_start_subfile() to adapt inappropriate dir name
2010-11-15 6:25 Re: [patch]change dwarf2_start_subfile() to adapt inappropriate dir name JuYoung Kim
@ 2010-11-15 16:58 ` Joel Brobecker
2010-11-15 18:05 ` Nathan Froyd
0 siblings, 1 reply; 5+ messages in thread
From: Joel Brobecker @ 2010-11-15 16:58 UTC (permalink / raw)
To: JuYoung Kim; +Cc: Eli Zaretskii, gdb-patches
[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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Re: [patch]change dwarf2_start_subfile() to adapt inappropriate dir name
2010-11-15 16:58 ` Joel Brobecker
@ 2010-11-15 18:05 ` Nathan Froyd
2010-11-15 18:34 ` Eli Zaretskii
2010-11-23 20:47 ` Tom Tromey
0 siblings, 2 replies; 5+ messages in thread
From: Nathan Froyd @ 2010-11-15 18:05 UTC (permalink / raw)
To: Joel Brobecker; +Cc: JuYoung Kim, Eli Zaretskii, gdb-patches
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);
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch]change dwarf2_start_subfile() to adapt inappropriate dir name
2010-11-15 18:05 ` Nathan Froyd
@ 2010-11-15 18:34 ` Eli Zaretskii
2010-11-23 20:47 ` Tom Tromey
1 sibling, 0 replies; 5+ messages in thread
From: Eli Zaretskii @ 2010-11-15 18:34 UTC (permalink / raw)
To: Nathan Froyd; +Cc: brobecker, j0.kim, gdb-patches
> 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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch]change dwarf2_start_subfile() to adapt inappropriate dir name
2010-11-15 18:05 ` Nathan Froyd
2010-11-15 18:34 ` Eli Zaretskii
@ 2010-11-23 20:47 ` Tom Tromey
1 sibling, 0 replies; 5+ messages in thread
From: Tom Tromey @ 2010-11-23 20:47 UTC (permalink / raw)
To: Nathan Froyd; +Cc: Joel Brobecker, JuYoung Kim, Eli Zaretskii, gdb-patches
>>>>> "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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-11-23 20:47 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-11-15 6:25 Re: [patch]change dwarf2_start_subfile() to adapt inappropriate dir name JuYoung Kim
2010-11-15 16:58 ` Joel Brobecker
2010-11-15 18:05 ` Nathan Froyd
2010-11-15 18:34 ` Eli Zaretskii
2010-11-23 20:47 ` Tom Tromey
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).