Regression in exec.c (print_section_info) ?

Regression in exec.c (print_section_info) ?

[Nick Roberts]

This works for me, e.g. with "info target" but perhaps there is a bigger
picture and this line was removed for another reason.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


*** exec.c	20 Apr 2008 10:20:39 +1200	1.73
--- exec.c	28 Apr 2008 13:33:24 +1200	
*************** print_section_info (struct target_ops *t
*** 546,551 ****
--- 546,552 ----
      {
        printf_filtered (_("\tEntry point: "));
        fputs_filtered (paddress (bfd_get_start_address (abfd)), gdb_stdout);
+       printf_filtered ("\n");
      }
    for (p = t->to_sections; p < t->to_sections_end; p++)
      {

Re: Regression in exec.c (print_section_info) ?

[Joel Brobecker]

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

> This works for me, e.g. with "info target" but perhaps there is a bigger
> picture and this line was removed for another reason.

I reviewed the history of the change, and I think it was an oversight.
The change that removed the newline was meant to get rid of some uses
of current_gdbarch, no more.

Could you test the attached? I think it's generally better for
internationalization to have one string rather than put together several
string blocks. I doubt it would make much difference in this case, but
might as well.

I would also like to see a new test if we don't already have one to
prevent this type of regression in the future. Please confirm that
you did run the patch against the testcase on at least one architecture.

One last thing: You also forgot to provide a ChangeLog entry. But *thank
you* for sending a unified diff - I just can't read context diffs!

-- 
Joel

[-- Attachment #2: exec.c.diff --]
[-- Type: text/plain, Size: 809 bytes --]

Index: exec.c
===================================================================
RCS file: /cvs/src/src/gdb/exec.c,v
retrieving revision 1.73
diff -u -p -r1.73 exec.c
--- exec.c	19 Apr 2008 02:07:19 -0000	1.73
+++ exec.c	29 Apr 2008 03:00:23 -0000
@@ -543,10 +543,8 @@ print_section_info (struct target_ops *t
   wrap_here ("        ");
   printf_filtered (_("file type %s.\n"), bfd_get_target (abfd));
   if (abfd == exec_bfd)
-    {
-      printf_filtered (_("\tEntry point: "));
-      fputs_filtered (paddress (bfd_get_start_address (abfd)), gdb_stdout);
-    }
+    printf_filtered (_("\tEntry point: %s\n"),
+                     paddress (bfd_get_start_address (abfd)))
   for (p = t->to_sections; p < t->to_sections_end; p++)
     {
       printf_filtered ("\t%s", hex_string_custom (p->addr, wid));

Re: Regression in exec.c (print_section_info) ?

[Nick Roberts]

 > > This works for me, e.g. with "info target" but perhaps there is a bigger
 > > picture and this line was removed for another reason.
 > 
 > I reviewed the history of the change, and I think it was an oversight.
 > The change that removed the newline was meant to get rid of some uses
 > of current_gdbarch, no more.
 > 
 > Could you test the attached? I think it's generally better for
 > internationalization to have one string rather than put together several
 > string blocks. I doubt it would make much difference in this case, but
 > might as well.
 >
 > I would also like to see a new test if we don't already have one to
 > prevent this type of regression in the future. 

It's easy to create a test for this specific case but I think it's very hard to
test the exact format of all CLI output in general, and probably not worth the
effort.  The next regression will likely occur elsewhere.

 >                                                 Please confirm that
 > you did run the patch against the testcase on at least one architecture.

I only have one architecture: my old PC.  I get the same testsuite results
with and without this change (After adding the semi-colon missing in your
patch.).

 > One last thing: You also forgot to provide a ChangeLog entry. 

I didn't really think of it as a patch, I was just trying to draw attention to
the regression.

 >                                                               But *thank
 > you* for sending a unified diff - I just can't read context diffs!

Actually it was a context diff - so you can read them!  They just look similar
when lines are added.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: Regression in exec.c (print_section_info) ?

[Joel Brobecker]

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

>  > I would also like to see a new test if we don't already have one to
>  > prevent this type of regression in the future. 
> 
> It's easy to create a test for this specific case but I think it's
> very hard to test the exact format of all CLI output in general, and
> probably not worth the effort.  The next regression will likely occur
> elsewhere.

I disagree.

We already do a lot of testing of CLI output, and even if we didn't,
we have to start somewhere. "The next regression will likely occur
elsewhere" might be true, but that doesn't rule out the possibility
of regressing again. Since it is easy to create a test, why not just
do it, even if you think it's not going to bring actual gain? I like
to think of it as insurance. I hope that it will prove useless, but
it's still there if I need it.

> I only have one architecture: my old PC.

That's perfect.

> I didn't really think of it as a patch, I was just trying to draw
> attention to the regression.

This approach is too casual for me. Everyone else writes a ChangeLog,
which takes at most 1 min of your time. Please follow this part of
the procedure, so that I can review the ChangeLog entry as well.

> Actually it was a context diff - so you can read them!  They just look
> similar when lines are added.

Argh. Consider sending unified diffs, I think others prefer them too.
When I see context diffs, I usually end up converting it to unified...

Anyway, I ended up checking the attached:

2008-05-01  Nick Roberts  <nickrob@snap.net.nz>

        * exec.c (print_section_info): Add missing '\n'.

2008-05-01  Joel Brobecker  <brobecker@adacore.com>

        * gdb.base/info-target.exp: New testcase.

Tested on x86-linux. The new testcase fails without the change to exec.c.

-- 
Joel

[-- Attachment #2: exec.c.diff --]
[-- Type: text/plain, Size: 839 bytes --]

Index: exec.c
===================================================================
RCS file: /cvs/src/src/gdb/exec.c,v
retrieving revision 1.73
retrieving revision 1.74
diff -u -p -r1.73 -r1.74
--- exec.c	19 Apr 2008 02:07:19 -0000	1.73
+++ exec.c	1 May 2008 17:46:32 -0000	1.74
@@ -543,10 +543,8 @@ print_section_info (struct target_ops *t
   wrap_here ("        ");
   printf_filtered (_("file type %s.\n"), bfd_get_target (abfd));
   if (abfd == exec_bfd)
-    {
-      printf_filtered (_("\tEntry point: "));
-      fputs_filtered (paddress (bfd_get_start_address (abfd)), gdb_stdout);
-    }
+    printf_filtered (_("\tEntry point: %s\n"),
+                     paddress (bfd_get_start_address (abfd)));
   for (p = t->to_sections; p < t->to_sections_end; p++)
     {
       printf_filtered ("\t%s", hex_string_custom (p->addr, wid));

[-- Attachment #3: info-target.exp --]
[-- Type: text/plain, Size: 1471 bytes --]

# Copyright 2008 Free Software Foundation, Inc.

# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation; either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program.  If not, see <http://www.gnu.org/licenses/>.

if $tracelevel then {
    strace $tracelevel
}

set prms_id 0
set bug_id 0

set testfile start
set srcfile ${testfile}.c
set binfile ${objdir}/${subdir}/${testfile}
if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
    return -1
}

gdb_exit
gdb_start
gdb_reinitialize_dir $srcdir/$subdir
gdb_load ${binfile}

set eol "\[\r\n\]+"

# Check the output of "info target".  Note that we are not interested
# in this case in checking the actual info, but rather to make sure that
# it is formatted properly.  For instance, make sure that no '\n' is
# missing at the end some lines.
gdb_test "info target" \
         "Symbols from \".*${testfile}.*\"\\..*${eol}Local exec file:${eol}.*Entry point: 0x\[0-9a-zA-Z\]+${eol}.*" \
         "info target"

Re: Regression in exec.c (print_section_info) ?

[Nick Roberts]

 >... 
 > > I didn't really think of it as a patch, I was just trying to draw
 > > attention to the regression.
 > 
 > This approach is too casual for me. Everyone else writes a ChangeLog,
 > which takes at most 1 min of your time. Please follow this part of
 > the procedure, so that I can review the ChangeLog entry as well.

It was only a one-liner and I was hoping that whoever made the regression would
correct it, perhaps as part of a bigger patch/picture.  With respect I think,
perhaps, you're shooting the messenger.

 >...
 > > Actually it was a context diff - so you can read them!  They just look
 > > similar when lines are added.

 > Argh. Consider sending unified diffs, I think others prefer them too.
 > When I see context diffs, I usually end up converting it to unified...

OK.  On emacs-devel, RMS asks for context diffs but I guess it's the
maintainer's prerogative to choose.

 > Anyway, I ended up checking the attached:

Thanks.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: Regression in exec.c (print_section_info) ?

[Joel Brobecker]

> OK.  On emacs-devel, RMS asks for context diffs but I guess it's the
> maintainer's prerogative to choose.

Well, it's not a big deal as far as I am concerned. I am still
going to review patches that use the context diff - it's just
going to take me longer, that's all.

-- 
Joel

Re: Regression in exec.c (print_section_info) ?

[Michael Snyder]

On Fri, 2008-05-02 at 13:05 +1200, Nick Roberts wrote:
> >... 
>  >...
>  > > Actually it was a context diff - so you can read them!  They just look
>  > > similar when lines are added.
> 
>  > Argh. Consider sending unified diffs, I think others prefer them too.
>  > When I see context diffs, I usually end up converting it to unified...
> 
> OK.  On emacs-devel, RMS asks for context diffs but I guess it's the
> maintainer's prerogative to choose.

Last time this discussion came up, someone introduced "diff -up", 
which combines the best-liked features of both.

Re: Regression in exec.c (print_section_info) ?

[Nick Roberts]

 > >  > Argh. Consider sending unified diffs, I think others prefer them too.
 > >  > When I see context diffs, I usually end up converting it to unified...
 > > 
 > > OK.  On emacs-devel, RMS asks for context diffs but I guess it's the
 > > maintainer's prerogative to choose.
 > 
 > Last time this discussion came up, someone introduced "diff -up", 
 > which combines the best-liked features of both.

Huh?  Doesn't the "u" in "diff -up" stand for unified - not exactly the
buddhist "middle way".  The option "p" shows the function name and is useful
with both unified and context diffs.

Anyway, I'm happy to post unified diffs since the maintainer needs to
understand them while I, hopefully, already do.  I only really look at them to
check I've not included garbage.

-- 
Nick                                           http://www.inet.net.nz/~nickrob