[patch] Assert when 'break' with no arguments

[patch] Assert when 'break' with no arguments

[Aleksandar Ristovski]

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

Hello,

An issue exists where if 'break' command is issued while on a line that 
underwent inlining by the compiler gdb asserts with

Assertion `sal.pspace != NULL'

I narrowed down the issue to frame.c:find_frame_sal, combined with 
stack.c:set_last_displayed_sal and in the view of print_frame_info.

The fix proposed here would be to properly initialize 'sal' in 
find_frmae_sal. Additional check is performed in set_last_displayed_sal 
to make sure we do not set last_displayed_* vars and validate them if 
pspace is NULL as, clearly, the rest of the code expects it to be 
properly set.

I identified the same issue in 7.2, 7.3.1, 7.4 and HEAD. I have not 
checked earlier versions.

Test suite did not show regressions, and new test passes where it would 
fail without the patch.


ChangeLog:
2012-02-14  Aleksandar Ristovski  <aristovski@qnx.com>

        * frame.c (find_frame_sal): Initialise sal->pspace field from 
frame data.
        * stack.c (set_last_displayed_sal): Perform sanity check of the data
        passed in, in particular, validate that PSPACE is not NULL if 
requesting
        valid last_displayed_* data.


Test suite ChangeLOg:
2012-02-14  Aleksandar Ristovski  <aristovski@qnx.com>

     * gdb.base/break-inline.exp: New test.
     * gdb.base/break-inline.c: New test.


Attached are patch for the fix and tests.



Thank you,

Aleksandar Ristovski
QNX Software Systems

[-- Attachment #2: pspace-assert-201202141250.patch --]
[-- Type: text/x-patch, Size: 1281 bytes --]

Index: gdb/frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.304
diff -u -p -r1.304 frame.c
--- gdb/frame.c	4 Jan 2012 08:17:02 -0000	1.304
+++ gdb/frame.c	14 Feb 2012 16:35:12 -0000
@@ -2096,6 +2096,9 @@ find_frame_sal (struct frame_info *frame
 	   we can't do much better.  */
 	sal->pc = get_frame_pc (frame);
 
+      /* Set pspace with frame's pspace */
+      sal->pspace = get_frame_program_space (frame);
+
       return;
     }
 
Index: gdb/stack.c
===================================================================
RCS file: /cvs/src/src/gdb/stack.c,v
retrieving revision 1.247
diff -u -p -r1.247 stack.c
--- gdb/stack.c	7 Feb 2012 04:48:22 -0000	1.247
+++ gdb/stack.c	14 Feb 2012 16:35:12 -0000
@@ -904,11 +904,17 @@ set_last_displayed_sal (int valid, struc
 			CORE_ADDR addr, struct symtab *symtab,
 			int line)
 {
+  if (valid && pspace == NULL) {
+	warning(_("Trying to set NULL pspace."));
+  }
   last_displayed_sal_valid = valid;
   last_displayed_pspace = pspace;
   last_displayed_addr = addr;
   last_displayed_symtab = symtab;
   last_displayed_line = line;
+
+  if (valid && pspace == NULL)
+	last_displayed_sal_valid = 0;
 }
 
 /* Forget the last sal we displayed.  */

[-- Attachment #3: break-inline.exp --]
[-- Type: text/plain, Size: 995 bytes --]

#   Copyright 2012 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/>.

# This file was written by Rob Savoye. (rob@cygnus.com)



if { [prepare_for_testing break-inline.exp "break-inline" {} {debug nowarnings optimize=-O2}] } {
    return -1
}

gdb_test "start" "Temporary breakpoint.*"
gdb_test "next" "foo().*"


# Now test 'break' with no arguments.
gdb_test "break" ".*"


[-- Attachment #4: break-inline.c --]
[-- Type: text/x-csrc, Size: 908 bytes --]

/* This testcase is part of GDB, the GNU debugger.

   Copyright 2012 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/>.  */

#include <stdio.h>
static int g;
static inline void foo(void)
{
  g = 42;
  printf("%d\n", g);
}
int main(int argc, char *argv[])
{
  foo();
  return g;
}

Re: [patch] Assert when 'break' with no arguments

[Aleksandar Ristovski]

Please disregard line

"# This file was written by Rob Savoye. (rob@cygnus.com)"

from the test, I will remove it before commit.

Sorry about that.


On 12-02-14 01:02 PM, Aleksandar Ristovski wrote:
> Hello,
>
> An issue exists where if 'break' command is issued while on a line that
> underwent inlining by the compiler gdb asserts with
>
> Assertion `sal.pspace != NULL'
>
> I narrowed down the issue to frame.c:find_frame_sal, combined with
> stack.c:set_last_displayed_sal and in the view of print_frame_info.
>
> The fix proposed here would be to properly initialize 'sal' in
> find_frmae_sal. Additional check is performed in set_last_displayed_sal
> to make sure we do not set last_displayed_* vars and validate them if
> pspace is NULL as, clearly, the rest of the code expects it to be
> properly set.
>
> I identified the same issue in 7.2, 7.3.1, 7.4 and HEAD. I have not
> checked earlier versions.
>
> Test suite did not show regressions, and new test passes where it would
> fail without the patch.
>
>
> ChangeLog:
> 2012-02-14 Aleksandar Ristovski <aristovski@qnx.com>
>
> * frame.c (find_frame_sal): Initialise sal->pspace field from frame data.
> * stack.c (set_last_displayed_sal): Perform sanity check of the data
> passed in, in particular, validate that PSPACE is not NULL if requesting
> valid last_displayed_* data.
>
>
> Test suite ChangeLOg:
> 2012-02-14 Aleksandar Ristovski <aristovski@qnx.com>
>
> * gdb.base/break-inline.exp: New test.
> * gdb.base/break-inline.c: New test.
>
>
> Attached are patch for the fix and tests.
>
>
>
> Thank you,
>
> Aleksandar Ristovski
> QNX Software Systems

Re: [patch] Assert when 'break' with no arguments

[Aleksandar Ristovski]

Please disregard line

"# This file was written by Rob Savoye. (rob@cygnus.com)"

from the test, I will remove it before commit.

Sorry about that.


On 12-02-14 01:02 PM, Aleksandar Ristovski wrote:
> Hello,
>
> An issue exists where if 'break' command is issued while on a line that
> underwent inlining by the compiler gdb asserts with
>
> Assertion `sal.pspace != NULL'
>
> I narrowed down the issue to frame.c:find_frame_sal, combined with
> stack.c:set_last_displayed_sal and in the view of print_frame_info.
>
> The fix proposed here would be to properly initialize 'sal' in
> find_frmae_sal. Additional check is performed in set_last_displayed_sal
> to make sure we do not set last_displayed_* vars and validate them if
> pspace is NULL as, clearly, the rest of the code expects it to be
> properly set.
>
> I identified the same issue in 7.2, 7.3.1, 7.4 and HEAD. I have not
> checked earlier versions.
>
> Test suite did not show regressions, and new test passes where it would
> fail without the patch.
>
>
> ChangeLog:
> 2012-02-14 Aleksandar Ristovski <aristovski@qnx.com>
>
> * frame.c (find_frame_sal): Initialise sal->pspace field from frame data.
> * stack.c (set_last_displayed_sal): Perform sanity check of the data
> passed in, in particular, validate that PSPACE is not NULL if requesting
> valid last_displayed_* data.
>
>
> Test suite ChangeLOg:
> 2012-02-14 Aleksandar Ristovski <aristovski@qnx.com>
>
> * gdb.base/break-inline.exp: New test.
> * gdb.base/break-inline.c: New test.
>
>
> Attached are patch for the fix and tests.
>
>
>
> Thank you,
>
> Aleksandar Ristovski
> QNX Software Systems

Re: [patch] Assert when 'break' with no arguments

[Joel Brobecker]

> ChangeLog:
> 2012-02-14  Aleksandar Ristovski  <aristovski@qnx.com>
> 
>        * frame.c (find_frame_sal): Initialise sal->pspace field from
> frame data.
>        * stack.c (set_last_displayed_sal): Perform sanity check of the data
>        passed in, in particular, validate that PSPACE is not NULL if
> requesting
>        valid last_displayed_* data.
> 
> 
> Test suite ChangeLOg:
> 2012-02-14  Aleksandar Ristovski  <aristovski@qnx.com>
> 
>     * gdb.base/break-inline.exp: New test.
>     * gdb.base/break-inline.c: New test.

I haven't looked at the validity of the patch (Pedro has a better
understanding of this area, for instance), but I still noticed some
trivial deviations from the GNU Coding Standards.

Your ChangeLog entry, for instance. Lines should be folded at 70 chars
(hard limit is 80 chars). Other violations highlighted below:

> +      /* Set pspace with frame's pspace */

End the sentence with a period (and two spaces before the '*/').

> +  if (valid && pspace == NULL) {
> +	warning(_("Trying to set NULL pspace."));
> +  }

Wrong formatting for first and second line.

> +  if (valid && pspace == NULL)
> +	last_displayed_sal_valid = 0;

Ditto for the second line (indentation is 2 spaces).

>    last_displayed_symtab = symtab;
>    last_displayed_line = line;
> +
>  }

Why are you adding an empty line here?

> #   Copyright 2012 Free
> #   Software Foundation, Inc.

Missing (C), and please join the two lines. If you copied some of
the testcase from another testcase, then you need to preserve the
original copyright years, I think.

> /* This testcase is part of GDB, the GNU debugger.
> 
>    Copyright 2012 Free Software
>    Foundation, Inc.

Same as above, please add the missing (C).

> #include <stdio.h>
> static int g;
> static inline void foo(void)
> {
>   g = 42;
>   printf("%d\n", g);
> }
> int main(int argc, char *argv[])
> {
>   foo();
>   return g;
> }

Is there a way to trigger the same problem without the dependency
on stdio.h? Many systems do not provide it (bareboard). I would
think that all you need is to define a function that has the same
profile as printf, no?

-- 
Joel

Re: [patch] Assert when 'break' with no arguments

[Aleksandar Ristovski]

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

On 12-02-14 01:53 PM, Joel Brobecker wrote:
>> ChangeLog:
>
> I haven't looked at the validity of the patch (Pedro has a better
> understanding of this area, for instance), but I still noticed some
> trivial deviations from the GNU Coding Standards.
>
> Your ChangeLog entry, for instance. Lines should be folded at 70 chars
> (hard limit is 80 chars). Other violations highlighted below:

Ok.

>
>> +      /* Set pspace with frame's pspace */
>
> End the sentence with a period (and two spaces before the '*/').

I removed the comment. It is stating what code unambiguously states already.

>
>> +  if (valid&&  pspace == NULL) {
>> +	warning(_("Trying to set NULL pspace."));
>> +  }
>
> Wrong formatting for first and second line.

Yes, thank you. Moved 'warning' and check down.

>
> Why are you adding an empty line here?
>
>> #   Copyright 2012 Free
>> #   Software Foundation, Inc.
>
> Missing (C), and please join the two lines. If you copied some of
> the testcase from another testcase, then you need to preserve the
> original copyright years, I think.

Added '(C)'. I copied only the copyright notice, the tests are new.

>> #include<stdio.h>

>
> Is there a way to trigger the same problem without the dependency
> on stdio.h? Many systems do not provide it (bareboard). I would
> think that all you need is to define a function that has the same
> profile as printf, no?
>

Yes, I removed printf and stdio, they are not important and the issue is 
still reproduced (the only important thing is that foo actually gets 
inlined which -O2 optimization level should do).

New patch, fixed tests attached.

Change logs:
2012-02-14  Aleksandar Ristovski  <aristovski@qnx.com>

     * frame.c (find_frame_sal): Initialise sal->pspace field from frame
     data.
     * stack.c (set_last_displayed_sal): Perform sanity check of the data
     passed in, in particular, validate that PSPACE is not NULL if 
requesting
     valid last_displayed_* data.


Testsuite:
2012-02-14  Aleksandar Ristovski  <aristovski@qnx.com>

     * gdb.base/break-inline.exp: New test.
     * gdb.base/break-inline.c: New test.






Thanks,

Aleksandar




[-- Attachment #2: pspace-assert-201202141406.patch --]
[-- Type: text/x-patch, Size: 1086 bytes --]

Index: gdb/frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.304
diff -u -p -r1.304 frame.c
--- gdb/frame.c	4 Jan 2012 08:17:02 -0000	1.304
+++ gdb/frame.c	14 Feb 2012 19:05:52 -0000
@@ -2096,6 +2096,8 @@ find_frame_sal (struct frame_info *frame
 	   we can't do much better.  */
 	sal->pc = get_frame_pc (frame);
 
+      sal->pspace = get_frame_program_space (frame);
+
       return;
     }
 
Index: gdb/stack.c
===================================================================
RCS file: /cvs/src/src/gdb/stack.c,v
retrieving revision 1.247
diff -u -p -r1.247 stack.c
--- gdb/stack.c	7 Feb 2012 04:48:22 -0000	1.247
+++ gdb/stack.c	14 Feb 2012 19:05:52 -0000
@@ -909,6 +909,11 @@ set_last_displayed_sal (int valid, struc
   last_displayed_addr = addr;
   last_displayed_symtab = symtab;
   last_displayed_line = line;
+  if (valid && pspace == NULL)
+    {
+      warning(_("Trying to set NULL pspace."));
+      clear_last_displayed_sal ();
+    }
 }
 
 /* Forget the last sal we displayed.  */

[-- Attachment #3: break-inline.exp --]
[-- Type: text/plain, Size: 927 bytes --]

#   Copyright (C) 2012 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 { [prepare_for_testing break-inline.exp "break-inline" {} {debug nowarnings optimize=-O2}] } {
    return -1
}

gdb_test "start" "Temporary breakpoint.*foo().*"

# Now test 'break' with no arguments.
gdb_test "break" "Breakpoint.*"


[-- Attachment #4: break-inline.c --]
[-- Type: text/x-csrc, Size: 872 bytes --]

/* This testcase is part of GDB, the GNU debugger.

   Copyright (C) 2012 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/>.  */

static int g;
static inline void foo(void)
{
  g = 42;
}
int main(int argc, char *argv[])
{
  foo();
  return g;
}

Re: [patch] Assert when 'break' with no arguments

[Alfred M. Szmidt]

   2012-02-14  Aleksandar Ristovski  <aristovski@qnx.com>

	* frame.c (find_frame_sal): Initialise sal->pspace field from frame
	data.

I'm guessing that American spelling is prefered, since that is what is
used in GDB.

	* stack.c (set_last_displayed_sal): Perform sanity check of the data
	passed in, in particular, validate that PSPACE is not NULL if requesting
	valid last_displayed_* data.

Please don't use regexps in the ChangeLog.  Also, descriptions on why
the change is done is better placed in the source file.  So here, you
could just write:

  Validate that PSPACE is not NULL.

Or something, 

   Testsuite:
   2012-02-14  Aleksandar Ristovski  <aristovski@qnx.com>

	* gdb.base/break-inline.exp: New test.
	* gdb.base/break-inline.c: New test.

This should say `New file.' since that is what you have added.

   [2:text/x-patch Hide Save:pspace-assert-201202141406.patch (1kB)]
   Index: gdb/frame.c
   ===================================================================
   RCS file: /cvs/src/src/gdb/frame.c,v
   retrieving revision 1.304
   diff -u -p -r1.304 frame.c
   --- gdb/frame.c	4 Jan 2012 08:17:02 -0000	1.304
   +++ gdb/frame.c	14 Feb 2012 19:05:52 -0000
   @@ -2096,6 +2096,8 @@ find_frame_sal (struct frame_info *frame
	      we can't do much better.  */
	   sal->pc = get_frame_pc (frame);

   +      sal->pspace = get_frame_program_space (frame);
   +
	  return;
	}

   Index: gdb/stack.c
   ===================================================================
   RCS file: /cvs/src/src/gdb/stack.c,v
   retrieving revision 1.247
   diff -u -p -r1.247 stack.c
   --- gdb/stack.c	7 Feb 2012 04:48:22 -0000	1.247
   +++ gdb/stack.c	14 Feb 2012 19:05:52 -0000
   @@ -909,6 +909,11 @@ set_last_displayed_sal (int valid, struc
      last_displayed_addr = addr;
      last_displayed_symtab = symtab;
      last_displayed_line = line;
   +  if (valid && pspace == NULL)
   +    {
   +      warning(_("Trying to set NULL pspace."));

Space after opening paren (though not for _()).

   +      clear_last_displayed_sal ();
   +    }
    }

    /* Forget the last sal we displayed.  */


   [3:text/plain Hide Save:break-inline.exp (927B)]
   #   Copyright (C) 2012 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 { [prepare_for_testing break-inline.exp "break-inline" {} {debug nowarnings optimize=-O2}] } {
       return -1
   }

   gdb_test "start" "Temporary breakpoint.*foo().*"

   # Now test 'break' with no arguments.
   gdb_test "break" "Breakpoint.*"



   [4:text/x-csrc Hide Save:break-inline.c (872B)]
   /* This testcase is part of GDB, the GNU debugger.

      Copyright (C) 2012 Free Software
      Foundation, Inc.

This isn't formated correctly.

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

   static int g;

Add a extra newline here.

   static inline void foo(void)

This should be,

static inline void
foo (void)

   {
     g = 42;
   }

Add a extra newline here.

   int main(int argc, char *argv[])

This should be,
int
main (int argc, char *argv[])

   {
     foo();

Space after opening paren.

     return g;
   }

Re: [patch] Assert when 'break' with no arguments

[Aleksandar Ristovski]

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

On 12-02-14 03:12 PM, Alfred M. Szmidt wrote:
>     2012-02-14  Aleksandar Ristovski<aristovski@qnx.com>
>
> 	* frame.c (find_frame_sal): Initialise sal->pspace field from frame
> 	data.
>
> I'm guessing that American spelling is prefered, since that is what is
> used in GDB.

Changed.

>
> 	* stack.c (set_last_displayed_sal): Perform sanity check of the data
> 	passed in, in particular, validate that PSPACE is not NULL if requesting
> 	valid last_displayed_* data.
>
> Please don't use regexps in the ChangeLog.  Also, descriptions on why
> the change is done is better placed in the source file.  So here, you
> could just write:
>
>    Validate that PSPACE is not NULL.

Ok.

>
> Or something,
>
>     Testsuite:
>     2012-02-14  Aleksandar Ristovski<aristovski@qnx.com>
>
> 	* gdb.base/break-inline.exp: New test.
> 	* gdb.base/break-inline.c: New test.
>
> This should say `New file.' since that is what you have added.

Ok.

>
> Space after opening paren (though not for _()).

Ok.

>        Copyright (C) 2012 Free Software
>        Foundation, Inc.
>
> This isn't formated correctly.

Ok.


Other changes performed as well.


2012-02-14  Aleksandar Ristovski  <aristovski@qnx.com>

     * frame.c (find_frame_sal): Initialize sal->pspace field from frame
     data.
     * stack.c (set_last_displayed_sal): Validate that PSPACE is not NULL.



2012-02-14  Aleksandar Ristovski  <aristovski@qnx.com>

     * gdb.base/break-inline.exp: New file.
     * gdb.base/break-inline.c: New file.



Patch + tests attached.


Thanks,

Aleksandar




[-- Attachment #2: pspace-assert-201202141536.patch --]
[-- Type: text/x-patch, Size: 1087 bytes --]

Index: gdb/frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.304
diff -u -p -r1.304 frame.c
--- gdb/frame.c	4 Jan 2012 08:17:02 -0000	1.304
+++ gdb/frame.c	14 Feb 2012 20:43:07 -0000
@@ -2096,6 +2096,8 @@ find_frame_sal (struct frame_info *frame
 	   we can't do much better.  */
 	sal->pc = get_frame_pc (frame);
 
+      sal->pspace = get_frame_program_space (frame);
+
       return;
     }
 
Index: gdb/stack.c
===================================================================
RCS file: /cvs/src/src/gdb/stack.c,v
retrieving revision 1.247
diff -u -p -r1.247 stack.c
--- gdb/stack.c	7 Feb 2012 04:48:22 -0000	1.247
+++ gdb/stack.c	14 Feb 2012 20:43:08 -0000
@@ -909,6 +909,11 @@ set_last_displayed_sal (int valid, struc
   last_displayed_addr = addr;
   last_displayed_symtab = symtab;
   last_displayed_line = line;
+  if (valid && pspace == NULL)
+    {
+      warning (_("Trying to set NULL pspace."));
+      clear_last_displayed_sal ();
+    }
 }
 
 /* Forget the last sal we displayed.  */

[-- Attachment #3: break-inline.exp --]
[-- Type: text/plain, Size: 927 bytes --]

#   Copyright (C) 2012 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 { [prepare_for_testing break-inline.exp "break-inline" {} {debug nowarnings optimize=-O2}] } {
    return -1
}

gdb_test "start" "Temporary breakpoint.*foo().*"

# Now test 'break' with no arguments.
gdb_test "break" "Breakpoint.*"


[-- Attachment #4: break-inline.c --]
[-- Type: text/x-csrc, Size: 874 bytes --]

/* This testcase is part of GDB, the GNU debugger.

   Copyright (C) 2012 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/>.  */

static int g;

static inline void
foo (void)
{
  g = 42;
}

int
main (int argc, char *argv[])
{
  foo ();
  return g;
}

Re: [patch] Assert when 'break' with no arguments

[Tom Tromey]

>>>>> "Aleksandar" == Aleksandar Ristovski <aristovski@qnx.com> writes:

Aleksandar> 2012-02-14  Aleksandar Ristovski  <aristovski@qnx.com>
Aleksandar>     * frame.c (find_frame_sal): Initialize sal->pspace field from frame
Aleksandar>     data.
Aleksandar>     * stack.c (set_last_displayed_sal): Validate that PSPACE is not NULL.

Aleksandar> 2012-02-14  Aleksandar Ristovski  <aristovski@qnx.com>

Aleksandar>     * gdb.base/break-inline.exp: New file.
Aleksandar>     * gdb.base/break-inline.c: New file.


This is ok.  Thanks.

Tom

Re: [patch] Assert when 'break' with no arguments

[Aleksandar Ristovski]

On 12-02-15 11:09 AM, Tom Tromey wrote:
>>>>>> "Aleksandar" == Aleksandar Ristovski<aristovski@qnx.com>  writes:
>
> Aleksandar>  2012-02-14  Aleksandar Ristovski<aristovski@qnx.com>
> Aleksandar>      * frame.c (find_frame_sal): Initialize sal->pspace field from frame
> Aleksandar>      data.
> Aleksandar>      * stack.c (set_last_displayed_sal): Validate that PSPACE is not NULL.
>
> Aleksandar>  2012-02-14  Aleksandar Ristovski<aristovski@qnx.com>
>
> Aleksandar>      * gdb.base/break-inline.exp: New file.
> Aleksandar>      * gdb.base/break-inline.c: New file.
>
>
> This is ok.  Thanks.
>
> Tom
>

Committed, thank you.

---
Aleksandar

[commit] testsuite: Fix break-inline.exp with gdbserver

[Jan Kratochvil]

On Wed, 15 Feb 2012 20:29:26 +0100, Aleksandar Ristovski wrote:
> On 12-02-15 11:09 AM, Tom Tromey wrote:
> > Aleksandar>  2012-02-14  Aleksandar Ristovski<aristovski@qnx.com>
> > 
> > Aleksandar>      * gdb.base/break-inline.exp: New file.
> > Aleksandar>      * gdb.base/break-inline.c: New file.
[...]
> Committed, thank you.

with
	http://sourceware.org/gdb/wiki/TestingGDB#Testing_gdbserver_in_a_native_configuration
+ERROR: gdbserver does not support start without extended-remote

Checked in.

The former internal error is then reproducible even with gdbserver.

(Plus I fixed the parantheses which had no effect before, forgot it in
ChangeLog.)


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2012-02/msg00109.html

--- src/gdb/testsuite/ChangeLog	2012/02/17 19:24:26	1.3078
+++ src/gdb/testsuite/ChangeLog	2012/02/19 13:05:27	1.3079
@@ -1,3 +1,9 @@
+2012-02-19  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	Fix for gdbserver non-extended mode.
+	* gdb.base/break-inline.exp (start): Replace "start" by gdb_breakpoint
+	and gdb_run_cmd.
+
 2012-02-17  Tom Tromey  <tromey@redhat.com>
 
 	* gdb.python/py-events.py (exit_handler): Add test for 'dir'.
--- src/gdb/testsuite/gdb.base/break-inline.exp	2012/02/15 19:27:59	1.1
+++ src/gdb/testsuite/gdb.base/break-inline.exp	2012/02/19 13:05:28	1.2
@@ -18,7 +18,9 @@
     return -1
 }
 
-gdb_test "start" "Temporary breakpoint.*foo().*"
+gdb_breakpoint "main" "temporary"
+gdb_run_cmd
+gdb_test "" "Temporary breakpoint.*foo\\(\\).*"
 
 # Now test 'break' with no arguments.
 gdb_test "break" "Breakpoint.*"

Re: [patch] Assert when 'break' with no arguments

[Pedro Alves]

On 02/14/2012 08:44 PM, Aleksandar Ristovski wrote:
> --- gdb/stack.c	7 Feb 2012 04:48:22 -0000	1.247
> +++ gdb/stack.c	14 Feb 2012 20:43:08 -0000
> @@ -909,6 +909,11 @@ set_last_displayed_sal (int valid, struc
>    last_displayed_addr = addr;
>    last_displayed_symtab = symtab;
>    last_displayed_line = line;
> +  if (valid && pspace == NULL)
> +    {
> +      warning (_("Trying to set NULL pspace."));

Is there a case when this isn't a gdb bug?  IOW, any reason this isn't a gdb_assert?
This seems like a rather cryptic warning to show to a user.

> +      clear_last_displayed_sal ();
> +    }
>  }
>  


-- 
Pedro Alves

Re: [patch] Assert when 'break' with no arguments

[Aleksandar Ristovski]

On 12-02-24 10:41 AM, Pedro Alves wrote:
> On 02/14/2012 08:44 PM, Aleksandar Ristovski wrote:
>> --- gdb/stack.c	7 Feb 2012 04:48:22 -0000	1.247
>> +++ gdb/stack.c	14 Feb 2012 20:43:08 -0000
>> @@ -909,6 +909,11 @@ set_last_displayed_sal (int valid, struc
>>     last_displayed_addr = addr;
>>     last_displayed_symtab = symtab;
>>     last_displayed_line = line;
>> +  if (valid&&  pspace == NULL)
>> +    {
>> +      warning (_("Trying to set NULL pspace."));
>
> Is there a case when this isn't a gdb bug?  IOW, any reason this isn't a gdb_assert?
> This seems like a rather cryptic warning to show to a user.

I believe it would be a bug.

However, gdb_assert is rather destructive. The warning is to bring it to 
our attention, but continue so users can actually do debugging if this 
happens.

My thinking was: if sal is invalidated the rest of the code should 
continue functioning thus providing more value to the user (albeit maybe 
somewhat confused user) than killing the session.

Anyway that was my rationale.


---
Aleksandar

Re: [patch] Assert when 'break' with no arguments

[Pedro Alves]

On 02/24/2012 03:54 PM, Aleksandar Ristovski wrote:
> On 12-02-24 10:41 AM, Pedro Alves wrote:
>> On 02/14/2012 08:44 PM, Aleksandar Ristovski wrote:
>>> --- gdb/stack.c    7 Feb 2012 04:48:22 -0000    1.247
>>> +++ gdb/stack.c    14 Feb 2012 20:43:08 -0000
>>> @@ -909,6 +909,11 @@ set_last_displayed_sal (int valid, struc
>>>     last_displayed_addr = addr;
>>>     last_displayed_symtab = symtab;
>>>     last_displayed_line = line;
>>> +  if (valid&&  pspace == NULL)
>>> +    {
>>> +      warning (_("Trying to set NULL pspace."));
>>
>> Is there a case when this isn't a gdb bug?  IOW, any reason this isn't a gdb_assert?
>> This seems like a rather cryptic warning to show to a user.
> 
> I believe it would be a bug.
> 
> However, gdb_assert is rather destructive. The warning is to bring it to our attention, but continue so users can actually do debugging if this happens.
> 
> My thinking was: if sal is invalidated the rest of the code should continue functioning thus providing more value to the user (albeit maybe somewhat confused user) than killing the session.
> 
> Anyway that was my rationale.

Trouble is that cryptic warnings usually end up unnoticed
and unreported (users will just shrug at gdb's lameness).

If we clear the last displayed sal before issuing an internal
error, then we're more sure to catch the bug, and users can still say
no to "Quit this debugging session? (y or n)" and continue
debugging.  WDYT?

diff --git a/gdb/stack.c b/gdb/stack.c
index 070d658..22b16a5 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -911,8 +911,9 @@ set_last_displayed_sal (int valid, struct program_space *pspace,
   last_displayed_line = line;
   if (valid && pspace == NULL)
     {
-      warning (_("Trying to set NULL pspace."));
       clear_last_displayed_sal ();
+      internal_error (__FILE__, __LINE__,
+		      _("Trying to set NULL pspace."));
     }
 }


-- 
Pedro Alves

Re: [patch] Assert when 'break' with no arguments

[Aleksandar Ristovski]

On 12-02-24 11:05 AM, Pedro Alves wrote:
>
> Trouble is that cryptic warnings usually end up unnoticed
> and unreported (users will just shrug at gdb's lameness).
>
> If we clear the last displayed sal before issuing an internal
> error, then we're more sure to catch the bug, and users can still say
> no to "Quit this debugging session? (y or n)" and continue
> debugging.  WDYT?
>
> diff --git a/gdb/stack.c b/gdb/stack.c
> index 070d658..22b16a5 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -911,8 +911,9 @@ set_last_displayed_sal (int valid, struct program_space *pspace,
>     last_displayed_line = line;
>     if (valid&&  pspace == NULL)
>       {
> -      warning (_("Trying to set NULL pspace."));
>         clear_last_displayed_sal ();
> +      internal_error (__FILE__, __LINE__,
> +		      _("Trying to set NULL pspace."));
>       }
>   }
>
>

I understand your motive; strictly speaking, this is more correct, so 
fine by me.

The error is not supposed to happen anyway :-)

Thanks,

Aleksandar

Re: [patch] Assert when 'break' with no arguments

[Pedro Alves]

On 02/24/2012 04:16 PM, Aleksandar Ristovski wrote:
> I understand your motive; strictly speaking, this is more correct, so fine by me.
> 
> The error is not supposed to happen anyway :-)

Yeah :-)  I've ran the testsuite, just in case, and applied it, with:

2012-02-24  Pedro Alves  <palves@redhat.com>

	* stack.c (set_last_displayed_sal): Issue internal_error instead
	of warning, and issue it after clearing the last displayed sal.

Thanks,
-- 
Pedro Alves