Re: backtrace changes current source location

Re: backtrace changes current source location

[Felix Lee]

patch for
    http://sources.redhat.com/ml/gdb/2004-10/msg00414.html

gdb/ChangeLog
2004-10-26  Felix Lee  <felix+log1@specifixinc.com>

        * stack.c (backtrace_command_1): Backtrace shouldn't
        change current source location.

gdb/testsuite/ChangeLog
2004-10-26  Felix Lee  <felix+log1@specifixinc.com>

        * gdb.base/break.exp: Add test for line number after
        backtrace.

Index: gdb/stack.c
===================================================================
RCS file: /cvs/src/src/gdb/stack.c,v
retrieving revision 1.112
diff -u -p -r1.112 stack.c
--- gdb/stack.c	3 Aug 2004 00:57:26 -0000	1.112
+++ gdb/stack.c	26 Oct 2004 14:59:06 -0000
@@ -1170,8 +1170,13 @@ backtrace_command_1 (char *count_exp, in
        fi && count--;
        i++, fi = get_prev_frame (fi))
     {
+      struct symtab_and_line savesal;
       QUIT;
 
+      /* print_frame_info changes the current sal, which is not
+         useful behavior when user asks for a backtrace. */
+      savesal = get_current_source_symtab_and_line ();
+
       /* Don't use print_stack_frame; if an error() occurs it probably
          means further attempts to backtrace would fail (on the other
          hand, perhaps the code does or could be fixed to make sure
@@ -1179,6 +1184,8 @@ backtrace_command_1 (char *count_exp, in
       print_frame_info (fi, 1, LOCATION, 1);
       if (show_locals)
 	print_frame_local_vars (fi, 1, gdb_stdout);
+
+      set_current_source_symtab_and_line (&savesal);
     }
 
   /* If we've stopped before the end, mention that.  */
Index: gdb/testsuite/gdb.base/break.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/break.exp,v
retrieving revision 1.19
diff -u -p -r1.19 break.exp
--- gdb/testsuite/gdb.base/break.exp	9 Dec 2003 18:19:20 -0000	1.19
+++ gdb/testsuite/gdb.base/break.exp	26 Oct 2004 14:59:06 -0000
@@ -238,6 +238,21 @@ for {set i 6} {$i >= 1} {incr i -1} {
 			"run until file:function($i) breakpoint"
 }
 
+# make sure backtrace doesn't change current source location.
+
+gdb_test "set listsize 1" \
+    ".*" \
+    "set listsize 1"
+gdb_test "list $bp_location7" \
+    ".*" \
+    "list before backtrace"
+gdb_test "backtrace" \
+    "\#0  factorial .*" \
+    "backtrace at breakpoint"
+gdb_test "list -1" \
+    "$bp_location7\[\t ].*" \
+    "list after backtrace"
+
 #
 # Run until the breakpoint set at a quoted function
 #

Re: backtrace changes current source location

[Andrew Cagney]

Felix Lee wrote:
> patch for
>     http://sources.redhat.com/ml/gdb/2004-10/msg00414.html
> 
> gdb/ChangeLog
> 2004-10-26  Felix Lee  <felix+log1@specifixinc.com>
> 
>         * stack.c (backtrace_command_1): Backtrace shouldn't
>         change current source location.

Felix, can you find out where current_sal is being trashed?  GDB's 
trying to get away from all this global state - the code at that level 
shouldn't need to meddle with current_sal.

(Don't forget to consider the error case - if an error is thrown a 
restore would be lost)

> gdb/testsuite/ChangeLog
> 2004-10-26  Felix Lee  <felix+log1@specifixinc.com>
> 
>         * gdb.base/break.exp: Add test for line number after
>         backtrace.

Thanks for remembering this.  However, as a separate test, it should be 
in a separate file.

Andrew

> Index: gdb/stack.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/stack.c,v
> retrieving revision 1.112
> diff -u -p -r1.112 stack.c
> --- gdb/stack.c	3 Aug 2004 00:57:26 -0000	1.112
> +++ gdb/stack.c	26 Oct 2004 14:59:06 -0000
> @@ -1170,8 +1170,13 @@ backtrace_command_1 (char *count_exp, in
>         fi && count--;
>         i++, fi = get_prev_frame (fi))
>      {
> +      struct symtab_and_line savesal;
>        QUIT;
>  
> +      /* print_frame_info changes the current sal, which is not
> +         useful behavior when user asks for a backtrace. */
> +      savesal = get_current_source_symtab_and_line ();
> +
>        /* Don't use print_stack_frame; if an error() occurs it probably
>           means further attempts to backtrace would fail (on the other
>           hand, perhaps the code does or could be fixed to make sure
> @@ -1179,6 +1184,8 @@ backtrace_command_1 (char *count_exp, in
>        print_frame_info (fi, 1, LOCATION, 1);
>        if (show_locals)
>  	print_frame_local_vars (fi, 1, gdb_stdout);
> +
> +      set_current_source_symtab_and_line (&savesal);
>      }
>  
>    /* If we've stopped before the end, mention that.  */
> Index: gdb/testsuite/gdb.base/break.exp
> ===================================================================
> RCS file: /cvs/src/src/gdb/testsuite/gdb.base/break.exp,v
> retrieving revision 1.19
> diff -u -p -r1.19 break.exp
> --- gdb/testsuite/gdb.base/break.exp	9 Dec 2003 18:19:20 -0000	1.19
> +++ gdb/testsuite/gdb.base/break.exp	26 Oct 2004 14:59:06 -0000
> @@ -238,6 +238,21 @@ for {set i 6} {$i >= 1} {incr i -1} {
>  			"run until file:function($i) breakpoint"
>  }
>  
> +# make sure backtrace doesn't change current source location.
> +
> +gdb_test "set listsize 1" \
> +    ".*" \
> +    "set listsize 1"
> +gdb_test "list $bp_location7" \
> +    ".*" \
> +    "list before backtrace"
> +gdb_test "backtrace" \
> +    "\#0  factorial .*" \
> +    "backtrace at breakpoint"
> +gdb_test "list -1" \
> +    "$bp_location7\[\t ].*" \
> +    "list after backtrace"
> +
>  #
>  # Run until the breakpoint set at a quoted function
>  #
> 

Re: backtrace changes current source location

[Daniel Jacobowitz]

On Wed, Oct 27, 2004 at 01:34:09PM -0400, Andrew Cagney wrote:
> Felix Lee wrote:
> >patch for
> >    http://sources.redhat.com/ml/gdb/2004-10/msg00414.html
> >
> >gdb/ChangeLog
> >2004-10-26  Felix Lee  <felix+log1@specifixinc.com>
> >
> >        * stack.c (backtrace_command_1): Backtrace shouldn't
> >        change current source location.
> 
> Felix, can you find out where current_sal is being trashed?  GDB's 
> trying to get away from all this global state - the code at that level 
> shouldn't need to meddle with current_sal.

If you follow the link in his message above, Keith introduced it in
2002 - print_frame_info_base.

-- 
Daniel Jacobowitz

Re: backtrace changes current source location

[Felix Lee]

Andrew Cagney <cagney@gnu.org>:
> (Don't forget to consider the error case - if an error is thrown a 
> restore would be lost)

is it worth setting up an unwind handler for that?  I couldn't
think of a case where an error would be usual, and for unusual
errors, all bets are off.

> Thanks for remembering this.  However, as a separate test, it should be 
> in a separate file.

how about putting the test in list.exp?  or is the idea to move
toward one test per file?
--

Re: backtrace changes current source location

[Andrew Cagney]

Hmm, things have changed.

Felix Lee wrote:
> Andrew Cagney <cagney@gnu.org>:
> 
>>(Don't forget to consider the error case - if an error is thrown a 
>>restore would be lost)
> 
> 
> is it worth setting up an unwind handler for that?  I couldn't
> think of a case where an error would be usual, and for unusual
> errors, all bets are off.

As a debugger, we're no longer going to gamble with the user interface - 
even when there's an error the behavior should be well defined.

Can you find out why selected sal is being corrupted, code shouldn't be 
modifying it.

>>Thanks for remembering this.  However, as a separate test, it should be 
>>in a separate file.
> 
> 
> how about putting the test in list.exp?  or is the idea to move
> toward one test per file?

The latter, if foo.exp passes then feature foo works :-)  If you're at a 
loss for a name, just create a bug report and then call the test 
gdb<prnumber>.{c,.exp} (we're no longer sharing C files between test 
cases either :-).

Andrew

Re: backtrace changes current source location

[Daniel Jacobowitz]

On Fri, Oct 29, 2004 at 11:20:53AM -0400, Andrew Cagney wrote:
> Hmm, things have changed.
> 
> Felix Lee wrote:
> >Andrew Cagney <cagney@gnu.org>:
> >
> >>(Don't forget to consider the error case - if an error is thrown a 
> >>restore would be lost)
> >
> >
> >is it worth setting up an unwind handler for that?  I couldn't
> >think of a case where an error would be usual, and for unusual
> >errors, all bets are off.
> 
> As a debugger, we're no longer going to gamble with the user interface - 
> even when there's an error the behavior should be well defined.
> 
> Can you find out why selected sal is being corrupted, code shouldn't be 
> modifying it.

I wrote:
  On Wed, Oct 27, 2004 at 01:34:09PM -0400, Andrew Cagney wrote:
  > Felix Lee wrote:
  > >patch for
  > >    http://sources.redhat.com/ml/gdb/2004-10/msg00414.html
  > >
  > >gdb/ChangeLog
  > >2004-10-26  Felix Lee  <felix+log1@specifixinc.com>
  > >
  > >        * stack.c (backtrace_command_1): Backtrace shouldn't
  > >        change current source location.
  >
  > Felix, can you find out where current_sal is being trashed?  GDB's
  > trying to get away from all this global state - the code at that level
  > shouldn't need to meddle with current_sal.

  If you follow the link in his message above, Keith introduced it in
  2002 - print_frame_info_base.

Does that answer your question?

-- 
Daniel Jacobowitz

Re: backtrace changes current source location

[Felix Lee]

Andrew Cagney <cagney@gnu.org>:
> Can you find out why selected sal is being corrupted, code shouldn't be 
> modifying it.

this is happening in stack.c:print_frame_info().  any time
information about a frame is printed, sal is set to that frame's
location.  the motivation for that change was an MI issue where
sal didn't get set when it should be.

it seems reasonable to me to always set sal when a frame gets
printed, but this is inconvenient in the case of backtraces.
backtraces seem like an exception, rather than a flaw in the
general strategy, which is why I chose to save/restore sal in
backtrace.

my feeling is that sal is a user convenience for reducing typing,
and programs talking to gdb should not depend on it being
predictable, since it's easy for a program to specify precisely
what source location they're interested in.  but I don't feel
strongly about that.  I'll submit a new patch with the changes
you suggest.
--

Re: backtrace changes current source location

[Andrew Cagney]

Thanks!

Felix Lee wrote:
> Andrew Cagney <cagney@gnu.org>:
> 
>>Can you find out why selected sal is being corrupted, code shouldn't be 
>>modifying it.
> 
> 
> this is happening in stack.c:print_frame_info().  any time
> information about a frame is printed, sal is set to that frame's
> location.  the motivation for that change was an MI issue where
> sal didn't get set when it should be.
> 
> it seems reasonable to me to always set sal when a frame gets
> printed, but this is inconvenient in the case of backtraces.
> backtraces seem like an exception, rather than a flaw in the
> general strategy, which is why I chose to save/restore sal in
> backtrace.

Should:

(gdb) info frame 5

change things to frame 5's sal?

> my feeling is that sal is a user convenience for reducing typing,
> and programs talking to gdb should not depend on it being
> predictable, since it's easy for a program to specify precisely
> what source location they're interested in.  but I don't feel
> strongly about that.

Right.

MI and/or CLI should locally determine current-sal, and pass it down to 
print_frame_info.

> I'll submit a new patch with the changes
> you suggest.

I'm actually not yet suggesting anything yet :-).   I'm just trying to 
understand why two ends of GDB end up fighting over what the value of 
current-sal should be :-(

Lets get the testcase in.

Andrew

Re: backtrace changes current source location

[Felix Lee]

Andrew Cagney <cagney@gnu.org>:
> Should:
> (gdb) info frame 5
> change things to frame 5's sal?

it doesn't do that now.  it's only when a line like
    #4  0x08048392 in main () at z1.c:8
gets printed, which happens at breakpoints, or when the user
selects a frame with 'frame', and so forth.

> MI and/or CLI should locally determine current-sal, and pass it down to 
> print_frame_info.

ok, so an alternate implementation is to add a function
set_current_sal_to_frame(), and most places that call
print_frame_info() will also call set_current_sal_to_frame().
does that sound acceptable?

that will be a slightly larger patch.
--

Re: backtrace changes current source location

[Felix Lee]

ok, here's a new patch and testcase.  built and tested on
i686-pc-linux-gnu.  the testcase adds 7 PASS and 1 FAIL, which
becomes 8 PASS with the patch.

gdb/ChangeLog
2004-10-31  Felix Lee  <felix+log1@specifixinc.com>

        * stack.c (set_current_sal_from_frame): New function.
        (print_args_stub): Use it.
        (print_frame_info): Don't set sal here.

gdb/testsuite/ChangeLog
2004-10-31  Felix Lee  <felix+log1@specifixinc.com>

        * gdb.base/cursal.exp: New file.
        * gdb.base/cursal.c: New file.

Index: gdb/stack.c
===================================================================
RCS file: /cvs/src/src/gdb/stack.c,v
retrieving revision 1.115
diff -p -u -r1.115 stack.c
--- gdb/stack.c	30 Oct 2004 21:16:10 -0000	1.115
+++ gdb/stack.c	31 Oct 2004 10:42:55 -0000
@@ -100,6 +100,8 @@ static void print_frame (struct frame_in
 			 int print_args, 
 			 struct symtab_and_line sal);
 
+static void set_current_sal_from_frame (struct frame_info *, int);
+
 static void backtrace_command (char *, int);
 
 static void frame_info (char *, int);
@@ -128,9 +130,12 @@ struct print_stack_frame_args
 static int
 print_stack_frame_stub (void *args)
 {
-  struct print_stack_frame_args *p = (struct print_stack_frame_args *) args;
+  struct print_stack_frame_args *p = args;
+  int center = (p->print_what == SRC_LINE
+                || p->print_what == SRC_AND_LOC);
 
   print_frame_info (p->fi, p->print_level, p->print_what, p->print_args);
+  set_current_sal_from_frame (p->fi, center);
   return 0;
 }
 
@@ -401,6 +406,24 @@ print_args_stub (void *args)
   return 0;
 }
 
+/* Set the current source and line to the location of the given
+   frame, if possible.  When CENTER is true, adjust so the
+   relevant line is in the center of the next 'list'. */
+
+static void
+set_current_sal_from_frame (struct frame_info *fi, int center)
+{
+  struct symtab_and_line sal;
+
+  find_frame_sal (fi, &sal);
+  if (sal.symtab)
+    {
+      if (center)
+        sal.line = max (sal.line - get_lines_to_list () / 2, 1);
+      set_current_source_symtab_and_line (&sal);
+    }
+}
+
 /* Print information about a frame for frame "fi" at level "level".
    Used in "where" output, also used to emit breakpoint or step
    messages.  
@@ -477,12 +500,8 @@ print_frame_info (struct frame_info *fi,
 
   source_print = (print_what == SRC_LINE || print_what == SRC_AND_LOC);
 
-  if (sal.symtab)
-    set_current_source_symtab_and_line (&sal);
-
   if (source_print && sal.symtab)
     {
-      struct symtab_and_line cursal;
       int done = 0;
       int mid_statement = ((print_what == SRC_LINE)
 			   && (get_frame_pc (fi) != sal.pc));
@@ -513,11 +532,6 @@ print_frame_info (struct frame_info *fi,
 	      print_source_lines (sal.symtab, sal.line, sal.line + 1, 0);
 	    }
 	}
-      /* Make sure we have at least a default source file */
-      set_default_source_symtab_and_line ();
-      cursal = get_current_source_symtab_and_line ();
-      cursal.line = max (sal.line - get_lines_to_list () / 2, 1);
-      set_current_source_symtab_and_line (&cursal);
     }
 
   if (print_what != LOCATION)
Index: gdb/testsuite/gdb.base/cursal.c
===================================================================
RCS file: gdb/testsuite/gdb.base/cursal.c
diff -N gdb/testsuite/gdb.base/cursal.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.base/cursal.c	31 Oct 2004 10:42:55 -0000
@@ -0,0 +1,19 @@
+void
+func2 ()
+{
+  int v2 = 2;
+}
+
+void
+func1 ()
+{
+  func2 ();
+}
+
+
+int
+main ()
+{
+  int v0 = 0;
+  func1 ();
+}
Index: gdb/testsuite/gdb.base/cursal.exp
===================================================================
RCS file: gdb/testsuite/gdb.base/cursal.exp
diff -N gdb/testsuite/gdb.base/cursal.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.base/cursal.exp	31 Oct 2004 10:42:55 -0000
@@ -0,0 +1,78 @@
+# Copyright 2004 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 2 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, write to the Free Software
+# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.  
+
+if $tracelevel {
+    strace $tracelevel
+}
+
+set prms_id 0
+set bug_id 0
+
+set testfile "cursal"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if {[gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != ""} {
+    gdb_suppress_entire_file "Testcase compile failed, so all tests in this file will automatically fail."
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+gdb_test "set listsize 1" \
+    ".*"
+
+# initial sal should be first statement in main
+gdb_test "list" \
+    "v0 = 0;" \
+    "list before run"
+
+if {! [runto_main]} {
+    gdb_suppress_tests
+}
+
+gdb_test "list" \
+    "v0 = 0;" \
+    "list in main"
+
+if {! [runto "func2"]} {
+    gdb_suppress_tests
+}
+
+gdb_test "list" \
+    "v2 = 2;" \
+    "list in func2"
+
+# make sure backtrace doesn't change current source location.
+gdb_test "backtrace" \
+    ".*"
+gdb_test "list -1" \
+    "v2 = 2;" \
+    "list after backtrace"
+
+# check the window
+gdb_test "set listsize 3" \
+    ".*"
+if {! [runto_main]} {
+    gdb_suppress_tests
+}
+gdb_test "list" \
+    "func1 \\(\\);" \
+    "list size 3"
+
+return 0

Re: backtrace changes current source location

[Andrew Cagney]

Felix Lee wrote:
> ok, here's a new patch and testcase.  built and tested on
> i686-pc-linux-gnu.  the testcase adds 7 PASS and 1 FAIL, which
> becomes 8 PASS with the patch.
> 
> gdb/ChangeLog
> 2004-10-31  Felix Lee  <felix+log1@specifixinc.com>
> 
>         * stack.c (set_current_sal_from_frame): New function.
>         (print_args_stub): Use it.
>         (print_frame_info): Don't set sal here.

Hey, cute.  Yes, thanks!

Andrew

> gdb/testsuite/ChangeLog
> 2004-10-31  Felix Lee  <felix+log1@specifixinc.com>
> 
>         * gdb.base/cursal.exp: New file.
>         * gdb.base/cursal.c: New file.
> 
> Index: gdb/stack.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/stack.c,v
> retrieving revision 1.115
> diff -p -u -r1.115 stack.c
> --- gdb/stack.c	30 Oct 2004 21:16:10 -0000	1.115
> +++ gdb/stack.c	31 Oct 2004 10:42:55 -0000
> @@ -100,6 +100,8 @@ static void print_frame (struct frame_in
>  			 int print_args, 
>  			 struct symtab_and_line sal);
>  
> +static void set_current_sal_from_frame (struct frame_info *, int);
> +
>  static void backtrace_command (char *, int);
>  
>  static void frame_info (char *, int);
> @@ -128,9 +130,12 @@ struct print_stack_frame_args
>  static int
>  print_stack_frame_stub (void *args)
>  {
> -  struct print_stack_frame_args *p = (struct print_stack_frame_args *) args;
> +  struct print_stack_frame_args *p = args;
> +  int center = (p->print_what == SRC_LINE
> +                || p->print_what == SRC_AND_LOC);
>  
>    print_frame_info (p->fi, p->print_level, p->print_what, p->print_args);
> +  set_current_sal_from_frame (p->fi, center);
>    return 0;
>  }
>  
> @@ -401,6 +406,24 @@ print_args_stub (void *args)
>    return 0;
>  }
>  
> +/* Set the current source and line to the location of the given
> +   frame, if possible.  When CENTER is true, adjust so the
> +   relevant line is in the center of the next 'list'. */
> +
> +static void
> +set_current_sal_from_frame (struct frame_info *fi, int center)
> +{
> +  struct symtab_and_line sal;
> +
> +  find_frame_sal (fi, &sal);
> +  if (sal.symtab)
> +    {
> +      if (center)
> +        sal.line = max (sal.line - get_lines_to_list () / 2, 1);
> +      set_current_source_symtab_and_line (&sal);
> +    }
> +}
> +
>  /* Print information about a frame for frame "fi" at level "level".
>     Used in "where" output, also used to emit breakpoint or step
>     messages.  
> @@ -477,12 +500,8 @@ print_frame_info (struct frame_info *fi,
>  
>    source_print = (print_what == SRC_LINE || print_what == SRC_AND_LOC);
>  
> -  if (sal.symtab)
> -    set_current_source_symtab_and_line (&sal);
> -
>    if (source_print && sal.symtab)
>      {
> -      struct symtab_and_line cursal;
>        int done = 0;
>        int mid_statement = ((print_what == SRC_LINE)
>  			   && (get_frame_pc (fi) != sal.pc));
> @@ -513,11 +532,6 @@ print_frame_info (struct frame_info *fi,
>  	      print_source_lines (sal.symtab, sal.line, sal.line + 1, 0);
>  	    }
>  	}
> -      /* Make sure we have at least a default source file */
> -      set_default_source_symtab_and_line ();
> -      cursal = get_current_source_symtab_and_line ();
> -      cursal.line = max (sal.line - get_lines_to_list () / 2, 1);
> -      set_current_source_symtab_and_line (&cursal);
>      }
>  
>    if (print_what != LOCATION)
> Index: gdb/testsuite/gdb.base/cursal.c
> ===================================================================
> RCS file: gdb/testsuite/gdb.base/cursal.c
> diff -N gdb/testsuite/gdb.base/cursal.c
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ gdb/testsuite/gdb.base/cursal.c	31 Oct 2004 10:42:55 -0000
> @@ -0,0 +1,19 @@
> +void
> +func2 ()
> +{
> +  int v2 = 2;
> +}
> +
> +void
> +func1 ()
> +{
> +  func2 ();
> +}
> +
> +
> +int
> +main ()
> +{
> +  int v0 = 0;
> +  func1 ();
> +}
> Index: gdb/testsuite/gdb.base/cursal.exp
> ===================================================================
> RCS file: gdb/testsuite/gdb.base/cursal.exp
> diff -N gdb/testsuite/gdb.base/cursal.exp
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ gdb/testsuite/gdb.base/cursal.exp	31 Oct 2004 10:42:55 -0000
> @@ -0,0 +1,78 @@
> +# Copyright 2004 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 2 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, write to the Free Software
> +# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.  
> +
> +if $tracelevel {
> +    strace $tracelevel
> +}
> +
> +set prms_id 0
> +set bug_id 0
> +
> +set testfile "cursal"
> +set srcfile ${testfile}.c
> +set binfile ${objdir}/${subdir}/${testfile}
> +
> +if {[gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != ""} {
> +    gdb_suppress_entire_file "Testcase compile failed, so all tests in this file will automatically fail."
> +}
> +
> +gdb_exit
> +gdb_start
> +gdb_reinitialize_dir $srcdir/$subdir
> +gdb_load ${binfile}
> +
> +gdb_test "set listsize 1" \
> +    ".*"
> +
> +# initial sal should be first statement in main
> +gdb_test "list" \
> +    "v0 = 0;" \
> +    "list before run"
> +
> +if {! [runto_main]} {
> +    gdb_suppress_tests
> +}
> +
> +gdb_test "list" \
> +    "v0 = 0;" \
> +    "list in main"
> +
> +if {! [runto "func2"]} {
> +    gdb_suppress_tests
> +}
> +
> +gdb_test "list" \
> +    "v2 = 2;" \
> +    "list in func2"
> +
> +# make sure backtrace doesn't change current source location.
> +gdb_test "backtrace" \
> +    ".*"
> +gdb_test "list -1" \
> +    "v2 = 2;" \
> +    "list after backtrace"
> +
> +# check the window
> +gdb_test "set listsize 3" \
> +    ".*"
> +if {! [runto_main]} {
> +    gdb_suppress_tests
> +}
> +gdb_test "list" \
> +    "func1 \\(\\);" \
> +    "list size 3"
> +
> +return 0
> 

Re: backtrace changes current source location

[Daniel Jacobowitz]

On Sun, Oct 31, 2004 at 08:38:03PM -0800, Felix Lee wrote:
> Index: gdb/testsuite/gdb.base/cursal.c
> ===================================================================
> RCS file: gdb/testsuite/gdb.base/cursal.c
> diff -N gdb/testsuite/gdb.base/cursal.c
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ gdb/testsuite/gdb.base/cursal.c	31 Oct 2004 10:42:55 -0000
> @@ -0,0 +1,19 @@

Current policy is that this needs to have a copyright notice, please.

> +if {[gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != ""} {
> +    gdb_suppress_entire_file "Testcase compile failed, so all tests in this file will automatically fail."
> +}

> +if {! [runto_main]} {
> +    gdb_suppress_tests
> +}
> +

Also, don't use the *suppress* procedures - just return -1, and an
"untested" for the first one.

-- 
Daniel Jacobowitz

Re: backtrace changes current source location

[Felix Lee]

ok, checking this in, with the changes mentioned below.

Daniel Jacobowitz <drow@false.org>:
> On Sun, Oct 31, 2004 at 08:38:03PM -0800, Felix Lee wrote:
> > Index: gdb/testsuite/gdb.base/cursal.c
> > ===================================================================
> > RCS file: gdb/testsuite/gdb.base/cursal.c
> > diff -N gdb/testsuite/gdb.base/cursal.c
> > --- /dev/null	1 Jan 1970 00:00:00 -0000
> > +++ gdb/testsuite/gdb.base/cursal.c	31 Oct 2004 10:42:55 -0000
> > @@ -0,0 +1,19 @@
> 
> Current policy is that this needs to have a copyright notice, please.
> 
> > +if {[gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != ""} {
> > +    gdb_suppress_entire_file "Testcase compile failed, so all tests in this file will automatically fail."
> > +}
> 
> > +if {! [runto_main]} {
> > +    gdb_suppress_tests
> > +}
> > +
> 
> Also, don't use the *suppress* procedures - just return -1, and an
> "untested" for the first one.
> 
> -- 
> Daniel Jacobowitz

Procedure for large drop?

[Paul Hilfinger]

HP would like to contribute their modified version of (an older) GDB
to the FSF.  The assignment is already in place (whereby designated
individuals in HP can assign contributions).  Their code is not
currently suitable for direct inclusion in the GDB sources, but
AdaCore will be carving patches out of it (for the ia64, mostly).  I
presume that the FSF should have a copy of the raw source in some form
as a record of exactly what has been assigned.  Where should this go
(what's the procedure for getting a largish file stored somewhere)?
Thanks.

Paul Hilfinger
AdaCore, Inc.

Re: Procedure for large drop?

[Daniel Jacobowitz]

On Mon, Aug 15, 2005 at 06:02:49AM -0400, Paul Hilfinger wrote:
> 
> HP would like to contribute their modified version of (an older) GDB
> to the FSF.  The assignment is already in place (whereby designated
> individuals in HP can assign contributions).  Their code is not
> currently suitable for direct inclusion in the GDB sources, but
> AdaCore will be carving patches out of it (for the ia64, mostly).  I
> presume that the FSF should have a copy of the raw source in some form
> as a record of exactly what has been assigned.  Where should this go
> (what's the procedure for getting a largish file stored somewhere)?
> Thanks.

I'm of mixed minds as to whether this is necessary, but we do have a
convenient place to put the file: ftp.sourceware.org/pub/gdb/contrib/.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: Procedure for large drop?

[Paul Hilfinger]

> I'm of mixed minds as to whether this is necessary, but we do have a
> convenient place to put the file: ftp.sourceware.org/pub/gdb/contrib/.

Thanks. I'm not sure about this either, but 

1. The FSF is finicky about having documented assurances of code
   ownership (and rightly so).

2. HP's existing assignment agreement applies only to contributions 
   explicitly made by specific employees.

3. AdaCore does not own any of the code from which some of its planned
   submissions will derive; HP does.

So I conclude that some sort of symbolic act is needed so that AdaCore
will clearly be submitting mods to sources clearly assigned to the FSF.

Again, thanks for your help.

Paul Hilfinger

Re: Procedure for large drop?

[Jason Molenda]

On Aug 15, 2005, at 2:19 PM, Paul Hilfinger wrote:

> So I conclude that some sort of symbolic act is needed so that AdaCore
> will clearly be submitting mods to sources clearly assigned to the  
> FSF.

The last time I did this with Apple's sources, Andrew wanted the  
same.  I sent some e-mail to the gdb list proclaiming that everything  
contained in the tarball was hereby assigned to the FSF, by the  
authority vested in me.  (what authority did I have?  None, but it  
was all for appearances as we had a blanket assignment for our gdb  
work on file already.)

J

Re: Procedure for large drop?

[Paul Hilfinger]

> I'm of mixed minds as to whether this is necessary, but we do have a
> convenient place to put the file: ftp.sourceware.org/pub/gdb/contrib/.

And one more question, then: how does one get a file dropped here? (While
we've had several dropped, they were never directly by us, it seems).
Thanks.

Paul Hilfinger

Re: Procedure for large drop?

[Daniel Jacobowitz]

On Sat, Aug 20, 2005 at 05:07:32AM -0400, Paul Hilfinger wrote:
> 
> > I'm of mixed minds as to whether this is necessary, but we do have a
> > convenient place to put the file: ftp.sourceware.org/pub/gdb/contrib/.
> 
> And one more question, then: how does one get a file dropped here? (While
> we've had several dropped, they were never directly by us, it seems).
> Thanks.

Just ask anyone who has shell access to sourceware, and is in the "gdb"
group.  I know that includes at least Andrew Cagney and myself.  The
overseers group could presumably do it too.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

[patch] Pushing Inferior Function Arguments onto Stack on PowerPC64 machines

[janani]

> Is Andrew's comment about the ABI correct, or incorrect?  Do you know
>> for sure?
>> ! 		 		       /* WARNING: cagney/2003-09-21: As best I can
>> ! 		 		          tell, the ABI specifies that the value should
>> ! 		 		          be left aligned.  Unfortunately, GCC doesn't
>> ! 		 		          do this - it instead right aligns even sized
>> ! 		 		          values and puts odd sized values on the
>> ! 		 		          stack.  Work around that by putting both a
>> ! 		 		          left and right aligned value into the
>> ! 		 		          register (hopefully no one notices :-^).
>> ! 		 		          Arrrgh!  */

> -- 
> Daniel Jacobowitz
> CodeSourcery


I am new to this, so my interpretation might not be completely  
accurate, but the way I read the note below ( a snippet from the GNU  
GCC Manual about passing function arguments in registers) is that  
since PPC64 is big endian, even though the default is to pad downward  
(i.e. right align), if the size if greater than the size of an int,  
you need to pad upward (left align).
.......

FUNCTION_ARG_PADDING (mode, type)
     If defined, a C expression which determines whether, and in which  
direction, to pad out an argument with extra space. The value should  
be of type enum direction: either upward to pad above the argument,  
downward to pad below, or none to inhibit padding.

     The amount of padding is always just enough to reach the next  
multiple of FUNCTION_ARG_BOUNDARY; this macro does not control it.

     This macro has a default definition which is right for most  
systems. For little-endian machines, the default is to pad upward. For  
big-endian machines, the default is to pad downward for an argument of  
constant size shorter than an int, and upward otherwise.
.....

Janani Janakiraman

Re: [patch] Pushing Inferior Function Arguments onto Stack on PowerPC64 machines

[Daniel Jacobowitz]

On Wed, Oct 04, 2006 at 03:00:34PM -0400, janani@linux.ibm.com wrote:
> I am new to this, so my interpretation might not be completely  
> accurate, but the way I read the note below ( a snippet from the GNU  
> GCC Manual about passing function arguments in registers) is that  
> since PPC64 is big endian, even though the default is to pad downward  
> (i.e. right align), if the size if greater than the size of an int,  
> you need to pad upward (left align).

You're trying to answer the wrong question :-)

It's not "what does GCC do", but "what does the platform ABI say we
should do".  Is GCC conforming to the ABI?  Is the ABI wrong, or out of
date, or was Andrew's reading of it wrong, or...

There could be a real problem here, so it's important that we
understand what is _supposed_ to happen before we make a change.
If GCC is violating the ABI, then either GCC or the ABI may need to be
updated.  If GDB is misinterpreting the ABI, then just GDB needs to be
changed.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Pushing Inferior Function Arguments onto Stack on PowerPC64 machines

[Daniel Jacobowitz]

On Wed, Oct 04, 2006 at 03:10:56PM -0400, Daniel Jacobowitz wrote:
> On Wed, Oct 04, 2006 at 03:00:34PM -0400, janani@linux.ibm.com wrote:
> > I am new to this, so my interpretation might not be completely  
> > accurate, but the way I read the note below ( a snippet from the GNU  
> > GCC Manual about passing function arguments in registers) is that  
> > since PPC64 is big endian, even though the default is to pad downward  
> > (i.e. right align), if the size if greater than the size of an int,  
> > you need to pad upward (left align).
> 
> You're trying to answer the wrong question :-)

By the way, your From address (janani@linux.ibm.com) is not valid;
could you fix that?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Pushing Inferior Function Arguments onto Stack on PowerPC64 machines

[Mark Kettenis]

> Date: Wed, 4 Oct 2006 15:10:56 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Wed, Oct 04, 2006 at 03:00:34PM -0400, janani@linux.ibm.com wrote:
> > I am new to this, so my interpretation might not be completely  
> > accurate, but the way I read the note below ( a snippet from the GNU  
> > GCC Manual about passing function arguments in registers) is that  
> > since PPC64 is big endian, even though the default is to pad downward  
> > (i.e. right align), if the size if greater than the size of an int,  
> > you need to pad upward (left align).

Blindly trusting spftware documentation is a bit... erh, naive ;-).

> You're trying to answer the wrong question :-)
> 
> It's not "what does GCC do", but "what does the platform ABI say we
> should do".  Is GCC conforming to the ABI?  Is the ABI wrong, or out of
> date, or was Andrew's reading of it wrong, or...
> 
> There could be a real problem here, so it's important that we
> understand what is _supposed_ to happen before we make a change.
> If GCC is violating the ABI, then either GCC or the ABI may need to be
> updated.  If GDB is misinterpreting the ABI, then just GDB needs to be
> changed.

Indeed.  GCC has been getting things wrong in this area before.  And
on top of that, some versions of GCC get it right while other versions
get it wrong.  If there are indeed versions of GCC in widespread use
that do not follow the ABI, we should probably try to add a workaround
in GCC.  But if there are also versions of GCC that do follow the ABI,
adding such a workaround might be impossible.

Mark

Re: [patch] Pushing Inferior Function Arguments onto Stack on PowerPC64 machines

[Daniel Jacobowitz]

On Wed, Oct 04, 2006 at 02:44:56PM -0500, Janani Janakiraman wrote:
> 
> Sorry about the email address goofup. Was trying to use a new account so
> that HTML tags don't get in the way.
> Have to figure that out later.
> 
> From reading the PPC64 Platform ABI  at
> http://www.freestandards.org/spec/ELF/ppc64/PPC-elf64abi-1.9.html
> ( see section 3.1.7)  it appears that on Big Endian machines,
> values are  left aligned. But looks like GCC has different rules and right
> aligns the values which are put on the
> registers. This matches  Andrew Cagney  comment in the code, that says that
> ABI specifies that the values
> should be left aligned. But like I said in my earlier note, GCC appears to
> want the values to be
> right aligned.  Would appreciate another set of eyes looking at it as I
> don't have too much experience
> in this area.

I'm going to CC a couple of people who are hopefully much more familiar
with PowerPC64 ABI issues than I am.  Alan, David, could one of you
take a quick look at this?

The problematic code is in gdb/ppc-sysv-tdep.c, in the function
ppc64_sysv_abi_push_dummy_call.  It has to do with the passing of
aggregates with non-word-sized tails.

> Daniel Jacobowitz <drow@false.org> wrote on 10/04/2006 02:10:56 PM:
> 
> > On Wed, Oct 04, 2006 at 03:00:34PM -0400, janani@linux.ibm.com wrote:
> > > I am new to this, so my interpretation might not be completely
> > > accurate, but the way I read the note below ( a snippet from the GNU
> > > GCC Manual about passing function arguments in registers) is that
> > > since PPC64 is big endian, even though the default is to pad downward
> > > (i.e. right align), if the size if greater than the size of an int,
> > > you need to pad upward (left align).
> >
> > You're trying to answer the wrong question :-)
> >
> > It's not "what does GCC do", but "what does the platform ABI say we
> > should do".  Is GCC conforming to the ABI?  Is the ABI wrong, or out of
> > date, or was Andrew's reading of it wrong, or...
> >
> > There could be a real problem here, so it's important that we
> > understand what is _supposed_ to happen before we make a change.
> > If GCC is violating the ABI, then either GCC or the ABI may need to be
> > updated.  If GDB is misinterpreting the ABI, then just GDB needs to be
> > changed.
> >
> > --
> > Daniel Jacobowitz
> > CodeSourcery
> 
> > !                            /* WARNING: cagney/2003-09-21: As best I can
> > !                               tell, the ABI specifies that the value
> should
> > !                               be left aligned.  Unfortunately, GCC
> doesn't
> > !                               do this - it instead right aligns even
> sized
> > !                               values and puts odd sized values on the
> > !                               stack.  Work around that by putting both
> a
> > !                               left and right aligned value into the
> > !                               register (hopefully no one notices :-^).
> > !                               Arrrgh!  */
> > !                            /* Left aligned (8 byte values such as
> pointers
> > !                               fill the buffer).  */
> > !                            memcpy (regval, val + byte, len);
> > !                            /* Right aligned (but only if even).  */
> > !                            if (len == 1 || len == 2 || len == 4)
> > !                                  memcpy (regval + tdep->wordsize - len,
> 
> Janani Janakiraman
-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Pushing Inferior Function Arguments onto Stack on PowerPC64 machines

[Andreas Schwab]

Mark Kettenis <mark.kettenis@xs4all.nl> writes:

> Indeed.  GCC has been getting things wrong in this area before.  And
> on top of that, some versions of GCC get it right while other versions
> get it wrong.  If there are indeed versions of GCC in widespread use
> that do not follow the ABI, we should probably try to add a workaround
> in GCC.  But if there are also versions of GCC that do follow the ABI,
> adding such a workaround might be impossible.

AFAICS, GCC on AIX follows the ABI here, but ppc64-linux deviates from it.
This is controlled by AGGREGATES_PAD_UPWARD_ALWAYS in
rs6000.c:function_arg_padding in the gcc source.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [patch] Pushing Inferior Function Arguments onto Stack on PowerPC64 machines

[David Edelsohn]

>>>>> Daniel Jacobowitz writes:

>> From reading the PPC64 Platform ABI  at
>> http://www.freestandards.org/spec/ELF/ppc64/PPC-elf64abi-1.9.html
>> ( see section 3.1.7)  it appears that on Big Endian machines,
>> values are  left aligned. But looks like GCC has different rules and right
>> aligns the values which are put on the
>> registers. This matches  Andrew Cagney  comment in the code, that says that
>> ABI specifies that the values
>> should be left aligned. But like I said in my earlier note, GCC appears to
>> want the values to be
>> right aligned.  Would appreciate another set of eyes looking at it as I
>> don't have too much experience
>> in this area.

	The PPC64 Linux ABI changed.  AIX always pads upwards.  PPC64
Linux pads aggregates smaller than a doubleword downward.

"An aggregate or union smaller than one doubleword in size is padded so
that it appears in the least significant bits of the doubleword.  All
others are padded, if necessary, at their tail."

David

Re: [patch] Pushing Inferior Function Arguments onto Stack on PowerPC64 machines

[Daniel Jacobowitz]

On Wed, Oct 04, 2006 at 04:43:04PM -0400, David Edelsohn wrote:
> >>>>> Daniel Jacobowitz writes:
> 
> >> From reading the PPC64 Platform ABI  at
> >> http://www.freestandards.org/spec/ELF/ppc64/PPC-elf64abi-1.9.html
> >> ( see section 3.1.7)  it appears that on Big Endian machines,
> >> values are  left aligned. But looks like GCC has different rules and right
> >> aligns the values which are put on the
> >> registers. This matches  Andrew Cagney  comment in the code, that says that
> >> ABI specifies that the values
> >> should be left aligned. But like I said in my earlier note, GCC appears to
> >> want the values to be
> >> right aligned.  Would appreciate another set of eyes looking at it as I
> >> don't have too much experience
> >> in this area.
> 
> 	The PPC64 Linux ABI changed.  AIX always pads upwards.  PPC64
> Linux pads aggregates smaller than a doubleword downward.
> 
> "An aggregate or union smaller than one doubleword in size is padded so
> that it appears in the least significant bits of the doubleword.  All
> others are padded, if necessary, at their tail."

Thank you (and thanks to Andreas for answering, too).

In that case, the Linux and AIX configurations ought to be calling
different functions here.  Janani, if you want to try to fix this,
I would recommend:

  - Rename the existing function.  Add an argument to it, is_linux.
  - Create a wrapper function with the old name, in the same file,
    for AIX to use.
  - Create a wrapper function in ppc-linux-tdep.c which calls it with
    is_linux == 1.
  - Call set_gdbarch_push_dummy_call in the PPC64 section of
    ppc_linux_init_abi.

-- 
Daniel Jacobowitz
CodeSourcery