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

[Antoine Tremblay <antoine dot tremblay at ericsson dot com>]

On 02/16/2015 06:36 PM, Pedro Alves wrote:
> 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.  */
Indeed, fixed.
> >         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?
Done.

> > +#include <unistd.h>
> > +#include <stdio.h>
> > +
> > +int
> > +main (void)
> > +{
> > +  int i = 0;
> Empty line after declarations.
Done
> > +  printf("call main");
> Space before parens.
Done, sorry
> > +  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).
Done
> > +
> > +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.
I added SIGILL but not SIGEMT since the problem is fixed in linux only...
> > +    "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"
Done
> > +
> > +# 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.
Indeed I had forgotten that one...

> As this part of the test can't be tested without stdio
> support, skip it with:
>
>    if ![target_info exists gdb,noinferiorio] {
Done
> +
> > +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
Done.

Patch v2 follows in the next mail...

Regards,

Antoine