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