Re: RFC: gdb_assert with 'catch signal ...' and fork

[Pedro Alves <palves@redhat.com>]

Hi Philippe,

Thanks for working on this.

On 05/03/2013 11:37 PM, Philippe Waroquiers wrote:
> gdb 7.6 'catch signal' interacts badly with fork, causing a gdb_assert
> such as
>   break-catch-sig.c:152: internal-error: signal_catchpoint_remove_location: Assertion `signal_catch_counts[iter] > 0' failed.
>   A problem internal to GDB has been detected,
>   further debugging may prove unreliable.
>   Quit this debugging session? (y or n)
>
> This can be reproduced with a (patched) gdb.base/catch-signal.c.
> The problem is that the signal_catch_counts is decremented
> by detach_breakpoints.
> The patch below modifies catch-signal.c, catch-signal.exp
> and modifies breakpoint.c so as to avoid the assert.
>
> 2 questions:
> 1. The fix is based on the assumption that detach_breakpoints
>    only has to detach breakpoints and watchpoints,
>    but not catchpoints (see breakpoints.h comments,
>    that do not fully match the implementation which
>    e.g. also detach the single step breakpoints).
>    Unclear to me if this is the good fix ?
>    (the fix causes no regtest failure)

Unfortunately not.  A catchpoint can be a breakpoint behind
the scenes.  See break-catch-throw.c:

  /* We need to reset 'type' in order for code in breakpoint.c to do
     the right thing.  */
  cp->base.type = bp_breakpoint;

(I don't know exactly why this is needed though.)

It's better to instead look at the location type, and skip
bp_loc_other locations.  See init_bp_location and:

enum bp_loc_type
{
  bp_loc_software_breakpoint,
  bp_loc_hardware_breakpoint,
  bp_loc_hardware_watchpoint,
  bp_loc_other			/* Miscellaneous...  */
};

I don't think any current catchpoint type needs to be detached per
inferior, so I think that'll be sufficient.

> 2. when running the modified catch-signal test with an
>    unpatched gdb, the test fails but gdb.log does not contain
>    the assert. Rather, the test fails with 'the program exited'.
>    When doing the same commands manually, it shows the gdb assert.
>    No idea why this difference of behaviour.

I tried it, and the test doesn't trigger a gdb_assert for me.  Instead,
the program exists, as when running it manually:

 Running ../../../src/gdb/testsuite/gdb.base/catch-signal.exp ...
 FAIL: gdb.base/catch-signal.exp: SIGHUP: got SIGHUP after fork (the program exited)
 FAIL: gdb.base/catch-signal.exp: 1: got SIGHUP after fork (the program exited)
 FAIL: gdb.base/catch-signal.exp: SIGHUP SIGUSR2: got SIGHUP after fork (the program exited)

You must have had the test do a "next" or "step" or some such
instead of a "continue" at some point, because that's one way
I can trigger the assert:

main () at ../../../src/gdb/testsuite/gdb.base/foll-fork.c:23
23        int  v = 5;
(gdb) n
25        pid = fork ();
(gdb) catch signal SIGINT
Catchpoint 2 (signal SIGINT)
(gdb) n
../../src/gdb/break-catch-sig.c:152: internal-error: signal_catchpoint_remove_location: Assertion `signal_catch_counts[iter] > 0' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n)

> Index: gdb/testsuite/gdb.base/catch-signal.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/testsuite/gdb.base/catch-signal.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 catch-signal.c
> --- gdb/testsuite/gdb.base/catch-signal.c	3 May 2013 19:16:56 -0000	1.3
> +++ gdb/testsuite/gdb.base/catch-signal.c	3 May 2013 22:39:48 -0000
> @@ -15,6 +15,7 @@
>     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 <stdlib.h>
>  #include <signal.h>
>  #include <unistd.h>
>  
> @@ -44,5 +45,17 @@ main ()
>    raise (SIGHUP);		/* fourth HUP */
>  
>    raise (SIGINT);		/* first INT */
> +
> +  signal (SIGCHLD, handle);
> +  switch (fork())		/* fork marker */
> +  {
> +    case -1:
> +      perror ("fork");
> +      exit (1);
> +    case 0:
> +      exit (0);
> +  }
> +  (void) wait(NULL);
> +  raise (SIGHUP);		/* fifth HUP */
>  }

"catch signal" works on all targets, but some targets,
like mingw32 don't have 'fork', so this will make the test
not compile there, losing test coverage.  The easy way out
is to split this to a new catch-signal-fork.c|exp test.

>  
> Index: gdb/testsuite/gdb.base/catch-signal.exp
> ===================================================================
> RCS file: /cvs/src/src/gdb/testsuite/gdb.base/catch-signal.exp,v
> retrieving revision 1.4
> diff -u -p -r1.4 catch-signal.exp
> --- gdb/testsuite/gdb.base/catch-signal.exp	3 May 2013 19:16:56 -0000	1.4
> +++ gdb/testsuite/gdb.base/catch-signal.exp	3 May 2013 22:39:48 -0000
> @@ -83,7 +83,16 @@ proc test_catch_signal {signame} {
>  	    y
>  	gdb_test "catch signal SIGINT" "Catchpoint .*"
>  	gdb_test "continue" "Catchpoint .* SIGINT.*"
> +
> +	delete_breakpoints
>          
> +	# Test interaction with fork. This used to cause a gdb_assert.

As above, this set of steps does not cause the gdb_assert, so best
remove or redo this comment.  Also, two spaces after period.

> +	gdb_breakpoint ${srcfile}:[gdb_get_line_number "fork marker"]
> +	gdb_continue_to_breakpoint "fork marker"
> +	gdb_test "handle SIGHUP nostop noprint pass" \
> +	    "SIGHUP.*No.*No.*Yes.*"
> +	gdb_test "catch signal SIGHUP" "Catchpoint .*"
> +	gdb_test "continue" "Catchpoint .* SIGHUP.*" "got SIGHUP after fork"
>      }
>  }

-- 
Pedro Alves