Re: [PATCH] Fix non executable stack handling when calling functions in the inferior.

[Pedro Alves <palves at redhat dot com>]

On 02/12/2015 08:49 PM, Antoine Tremblay wrote:

> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 169188a..a0c0e1c 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -3070,9 +3070,14 @@ linux_nat_filter_event (int lwpid, int status)
>  	}
>  
>        /* When using hardware single-step, we need to report every signal.
> -	 Otherwise, signals in pass_mask may be short-circuited.  */
> +	 Otherwise, signals in pass_mask may be short-circuited
> +	 unless these signals are SIGILL, SIGSEGV or SIGEMT.
> +	 See handle_inferior_event for more information.  */

I think it'd be clearer if this mentioned _why_ these signals
are important.  Like:

	 Otherwise, signals in pass_mask may be short-circuited,
	 except signals that might be caused by a breakpoint.  */


>        if (!lp->step
> -	  && WSTOPSIG (status) && sigismember (&pass_mask, WSTOPSIG (status)))
> +	  && WSTOPSIG (status) && sigismember (&pass_mask, WSTOPSIG (status))
> +	  && signo != GDB_SIGNAL_ILL
> +	  && signo != GDB_SIGNAL_SEGV
> +	  && signo != GDB_SIGNAL_EMT)

By the same logic, this should filter SIGTRAP too.  Also, no need
to handle SIGEMT on Linux.

It'd be nice to move this to a separate predicate function.  We
actually already have one, in gdbserver/linux-low.c's
wstatus_maybe_breakpoint, which would be ideal here.  Could you move
that to e.g., nat/linux-ptrace.c, and use it?

While at it, GDBserver should need this fix too, right?
Could you handle that as well?

> +#include <unistd.h>
> +#include <stdio.h>
> +
> +int
> +main (void)
> +{
> +  int i = 0;

Empty line after declarations.

> +  printf("call main");

Space before parens.

> +  i++; /* set dprintf here */
> +  return 0;



> +standard_testfile

The test should be skipped on targets that can't do infcalls
(look for gdb,cannot_call_functions).

> +
> +set dp_location [gdb_get_line_number "set dprintf here"]
> +
> +if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    fail "Can't run to main to make the tests"
> +    return -1
> +}
> +
> +gdb_test "handle SIGSEGV nostop noprint" \

This should also do the same to SIGILL and SIGEMT, to
cover all targets.

> +    "Signal\[ \t\]+Stop\[ \t\]+Print\[ \t\]+Pass to program\[ \t\]+Description\r\nSIGSEGV\[ \t\]+No\[ \t\]+No\[ \t\]+Yes\[ \t\].*"
> +
> +gdb_test "call printf(\"test\\n\")" "test.*"

Could you make this part of the test not rely on stdio?  Not all
targets support that.  Add a dummy function to the program
that is meant to be called from GDB, and then call that.  Something
like:

gdb_test "print return_one ()" " = 1"

This part should pass with gdbserver:

 make check RUNTESTFLAGS="--target_board=native-gdbserver mytestname.exp"

> +
> +# Clean up the breakpoint state.
> +delete_breakpoints
> +
> +# Also test with dprintf since the original bug was noticed using dprintf.
> +gdb_test "dprintf $dp_location,\"testdprintf\\n\"" "Dprintf .*"

I think you should explicitly set the dprintf-style to call.

As this part of the test can't be tested without stdio
support, skip it with:

  if ![target_info exists gdb,noinferiorio] {

+
> +gdb_test "continue" "testdprintf.*"
> 

Please set a breakpoint at the end of main, before issuing
that "continue" to avoid problems on targets where
gdb_continue_to_end would need to do something magic.

Thanks,
Pedro Alves