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

Antoine Tremblay antoine.tremblay@ericsson.com
Wed Feb 18 15:36:00 GMT 2015


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



More information about the Gdb-patches mailing list