Fix tsan warning: signal handler spoils errno

  When building gdb with -fsanitize=thread and running test-case
gdb.base/bg-exec-sigint-bp-cond.exp, I run into:
...
==================^M
WARNING: ThreadSanitizer: signal handler spoils errno (pid=25422)^M
    #0 handler_wrapper gdb/posix-hdep.c:66^M
    #1 decltype ({parm#2}({parm#3}...)) gdb::handle_eintr<>() \
         gdbsupport/eintr.h:67^M
    #2 gdb::waitpid(int, int*, int) gdbsupport/eintr.h:78^M
    #3 run_under_shell gdb/cli/cli-cmds.c:926^M
...

Likewise in:
- tui_sigwinch_handler with test-case gdb.python/tui-window.exp, and
- handle_sighup with test-case gdb.base/quit-live.exp.

Fix this by saving the original errno, and restoring it before returning [1].

Tested on x86_64-linux.

[1] https://www.gnu.org/software/libc/manual/html_node/POSIX-Safety-Concepts.html
---
 gdb/event-top.c   | 4 ++++
 gdb/posix-hdep.c  | 4 ++++
 gdb/tui/tui-win.c | 4 ++++
 3 files changed, 12 insertions(+)


base-commit: 01d8e0d24ad301f69b6f5ea39d073a728477507d
  

Tom de Vries <tdevries@suse.de> writes:

> When building gdb with -fsanitize=thread and running test-case
> gdb.base/bg-exec-sigint-bp-cond.exp, I run into:
> ...
> ==================^M
> WARNING: ThreadSanitizer: signal handler spoils errno (pid=25422)^M
>     #0 handler_wrapper gdb/posix-hdep.c:66^M
>     #1 decltype ({parm#2}({parm#3}...)) gdb::handle_eintr<>() \
>          gdbsupport/eintr.h:67^M
>     #2 gdb::waitpid(int, int*, int) gdbsupport/eintr.h:78^M
>     #3 run_under_shell gdb/cli/cli-cmds.c:926^M
> ...
>
> Likewise in:
> - tui_sigwinch_handler with test-case gdb.python/tui-window.exp, and
> - handle_sighup with test-case gdb.base/quit-live.exp.
>
> Fix this by saving the original errno, and restoring it before returning [1].
>
> Tested on x86_64-linux.
>
> [1] https://www.gnu.org/software/libc/manual/html_node/POSIX-Safety-Concepts.html
> ---
>  gdb/event-top.c   | 4 ++++
>  gdb/posix-hdep.c  | 4 ++++
>  gdb/tui/tui-win.c | 4 ++++
>  3 files changed, 12 insertions(+)
>
> diff --git a/gdb/event-top.c b/gdb/event-top.c
> index 45ad7b990fa..b3a1e4e82c4 100644
> --- a/gdb/event-top.c
> +++ b/gdb/event-top.c
> @@ -1480,8 +1480,12 @@ async_do_nothing (gdb_client_data arg)
>  static void
>  handle_sighup (int sig)
>  {
> +  int save_errno = errno;

Does:

  scoped_restore restore_errno = make_scoped_restore (&errno);

work here?

Thanks,
Andrew

> +
>    mark_async_signal_handler (sighup_token);
>    signal (sig, handle_sighup);
> +
> +  errno = save_errno;
>  }
>  
>  /* Called by the event loop to process a SIGHUP.  */
> diff --git a/gdb/posix-hdep.c b/gdb/posix-hdep.c
> index a0d5c585ebc..bde9a9a3e7b 100644
> --- a/gdb/posix-hdep.c
> +++ b/gdb/posix-hdep.c
> @@ -64,9 +64,13 @@ static c_c_handler_ftype *current_handler;
>  static void
>  handler_wrapper (int num)
>  {
> +  int save_errno = errno;
> +
>    signal (num, handler_wrapper);
>    if (current_handler != SIG_IGN)
>      current_handler (num);
> +
> +  errno = save_errno;
>  }
>  
>  /* See inferior.h.  */
> diff --git a/gdb/tui/tui-win.c b/gdb/tui/tui-win.c
> index e9a8e4651ea..9fe6288671c 100644
> --- a/gdb/tui/tui-win.c
> +++ b/gdb/tui/tui-win.c
> @@ -527,8 +527,12 @@ static struct async_signal_handler *tui_sigwinch_token;
>  static void
>  tui_sigwinch_handler (int signal)
>  {
> +  int save_errno = errno;
> +
>    mark_async_signal_handler (tui_sigwinch_token);
>    tui_set_win_resized_to (true);
> +
> +  errno = save_errno;
>  }
>  
>  /* Callback for asynchronously resizing TUI following a SIGWINCH signal.  */
>
> base-commit: 01d8e0d24ad301f69b6f5ea39d073a728477507d
> -- 
> 2.43.0
  

On 12/9/24 18:11, Andrew Burgess wrote:
> Tom de Vries <tdevries@suse.de> writes:
> 
>> When building gdb with -fsanitize=thread and running test-case
>> gdb.base/bg-exec-sigint-bp-cond.exp, I run into:
>> ...
>> ==================^M
>> WARNING: ThreadSanitizer: signal handler spoils errno (pid=25422)^M
>>      #0 handler_wrapper gdb/posix-hdep.c:66^M
>>      #1 decltype ({parm#2}({parm#3}...)) gdb::handle_eintr<>() \
>>           gdbsupport/eintr.h:67^M
>>      #2 gdb::waitpid(int, int*, int) gdbsupport/eintr.h:78^M
>>      #3 run_under_shell gdb/cli/cli-cmds.c:926^M
>> ...
>>
>> Likewise in:
>> - tui_sigwinch_handler with test-case gdb.python/tui-window.exp, and
>> - handle_sighup with test-case gdb.base/quit-live.exp.
>>
>> Fix this by saving the original errno, and restoring it before returning [1].
>>
>> Tested on x86_64-linux.
>>
>> [1] https://www.gnu.org/software/libc/manual/html_node/POSIX-Safety-Concepts.html
>> ---
>>   gdb/event-top.c   | 4 ++++
>>   gdb/posix-hdep.c  | 4 ++++
>>   gdb/tui/tui-win.c | 4 ++++
>>   3 files changed, 12 insertions(+)
>>
>> diff --git a/gdb/event-top.c b/gdb/event-top.c
>> index 45ad7b990fa..b3a1e4e82c4 100644
>> --- a/gdb/event-top.c
>> +++ b/gdb/event-top.c
>> @@ -1480,8 +1480,12 @@ async_do_nothing (gdb_client_data arg)
>>   static void
>>   handle_sighup (int sig)
>>   {
>> +  int save_errno = errno;
> 
> Does:
> 
>    scoped_restore restore_errno = make_scoped_restore (&errno);
> 
> work here?
> 

Hi Andrew,

thanks for the review.

That seems to be the case, yes, I'll submit a v2.

Thanks,
- Tom

> Thanks,
> Andrew
> 
>> +
>>     mark_async_signal_handler (sighup_token);
>>     signal (sig, handle_sighup);
>> +
>> +  errno = save_errno;
>>   }
>>   
>>   /* Called by the event loop to process a SIGHUP.  */
>> diff --git a/gdb/posix-hdep.c b/gdb/posix-hdep.c
>> index a0d5c585ebc..bde9a9a3e7b 100644
>> --- a/gdb/posix-hdep.c
>> +++ b/gdb/posix-hdep.c
>> @@ -64,9 +64,13 @@ static c_c_handler_ftype *current_handler;
>>   static void
>>   handler_wrapper (int num)
>>   {
>> +  int save_errno = errno;
>> +
>>     signal (num, handler_wrapper);
>>     if (current_handler != SIG_IGN)
>>       current_handler (num);
>> +
>> +  errno = save_errno;
>>   }
>>   
>>   /* See inferior.h.  */
>> diff --git a/gdb/tui/tui-win.c b/gdb/tui/tui-win.c
>> index e9a8e4651ea..9fe6288671c 100644
>> --- a/gdb/tui/tui-win.c
>> +++ b/gdb/tui/tui-win.c
>> @@ -527,8 +527,12 @@ static struct async_signal_handler *tui_sigwinch_token;
>>   static void
>>   tui_sigwinch_handler (int signal)
>>   {
>> +  int save_errno = errno;
>> +
>>     mark_async_signal_handler (tui_sigwinch_token);
>>     tui_set_win_resized_to (true);
>> +
>> +  errno = save_errno;
>>   }
>>   
>>   /* Callback for asynchronously resizing TUI following a SIGWINCH signal.  */
>>
>> base-commit: 01d8e0d24ad301f69b6f5ea39d073a728477507d
>> -- 
>> 2.43.0
>
  

On Mon, Dec 9, 2024 at 6:50 PM Tom de Vries <tdevries@suse.de> wrote:

> On 12/9/24 18:11, Andrew Burgess wrote:
> > Tom de Vries <tdevries@suse.de> writes:
> >
> >> When building gdb with -fsanitize=thread and running test-case
> >> gdb.base/bg-exec-sigint-bp-cond.exp, I run into:
> >> ...
> >> ==================^M
> >> WARNING: ThreadSanitizer: signal handler spoils errno (pid=25422)^M
> >>      #0 handler_wrapper gdb/posix-hdep.c:66^M
> >>      #1 decltype ({parm#2}({parm#3}...)) gdb::handle_eintr<>() \
> >>           gdbsupport/eintr.h:67^M
> >>      #2 gdb::waitpid(int, int*, int) gdbsupport/eintr.h:78^M
> >>      #3 run_under_shell gdb/cli/cli-cmds.c:926^M
> >> ...
> >>
>

Hi Tom,

when compiling without the  -fsanitize=thread, I can see the regression for
aarch64 in a
watchpoint-hw.exp test:

Temporary breakpoint 2 at 0x40066c: file
/root/build/gdb/testsuite/../../../binutils-gdb/
gdb/testsuite/gdb.base/watchpoint-hw.c, line 23.
Starting program:
/root/build/gdb/testsuite/outputs/gdb.base/watchpoint-hw/watchpoint-hw
warning: Unable to determine the number of hardware watchpoints available.
warning: Unable to determine the number of hardware breakpoints available.
warning: watchpoint 1 downgraded to software watchpoint
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
FAIL: gdb.base/watchpoint-hw.exp: start (timeout)
info watchpoints

Temporary breakpoint 2, main () at
/root/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.base/watchpoint-hw.c:23
23        return 0;
(gdb) info watchpoints
Num     Type           Disp Enb Address            What
1       watchpoint     keep y                      watchee
(gdb) FAIL: gdb.base/watchpoint-hw.exp: info watchpoints
testcase
/root/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.base/watchpoint-hw.exp
completed in 10 seconds


>
>
  

On 12/9/24 18:46, Tom de Vries wrote:
> On 12/9/24 18:11, Andrew Burgess wrote:
>> Tom de Vries <tdevries@suse.de> writes:
>>
>>> When building gdb with -fsanitize=thread and running test-case
>>> gdb.base/bg-exec-sigint-bp-cond.exp, I run into:
>>> ...
>>> ==================^M
>>> WARNING: ThreadSanitizer: signal handler spoils errno (pid=25422)^M
>>>      #0 handler_wrapper gdb/posix-hdep.c:66^M
>>>      #1 decltype ({parm#2}({parm#3}...)) gdb::handle_eintr<>() \
>>>           gdbsupport/eintr.h:67^M
>>>      #2 gdb::waitpid(int, int*, int) gdbsupport/eintr.h:78^M
>>>      #3 run_under_shell gdb/cli/cli-cmds.c:926^M
>>> ...
>>>
>>> Likewise in:
>>> - tui_sigwinch_handler with test-case gdb.python/tui-window.exp, and
>>> - handle_sighup with test-case gdb.base/quit-live.exp.
>>>
>>> Fix this by saving the original errno, and restoring it before 
>>> returning [1].
>>>
>>> Tested on x86_64-linux.
>>>
>>> [1] https://www.gnu.org/software/libc/manual/html_node/POSIX-Safety- 
>>> Concepts.html
>>> ---
>>>   gdb/event-top.c   | 4 ++++
>>>   gdb/posix-hdep.c  | 4 ++++
>>>   gdb/tui/tui-win.c | 4 ++++
>>>   3 files changed, 12 insertions(+)
>>>
>>> diff --git a/gdb/event-top.c b/gdb/event-top.c
>>> index 45ad7b990fa..b3a1e4e82c4 100644
>>> --- a/gdb/event-top.c
>>> +++ b/gdb/event-top.c
>>> @@ -1480,8 +1480,12 @@ async_do_nothing (gdb_client_data arg)
>>>   static void
>>>   handle_sighup (int sig)
>>>   {
>>> +  int save_errno = errno;
>>
>> Does:
>>
>>    scoped_restore restore_errno = make_scoped_restore (&errno);
>>
>> work here?
>>
> 
> Hi Andrew,
> 
> thanks for the review.
> 
> That seems to be the case, yes, I'll submit a v2.
> 

Submitted here ( 
https://sourceware.org/pipermail/gdb-patches/2024-December/213937.html ).

Thanks,
- Tom

> Thanks,
> - Tom
> 
>> Thanks,
>> Andrew
>>
>>> +
>>>     mark_async_signal_handler (sighup_token);
>>>     signal (sig, handle_sighup);
>>> +
>>> +  errno = save_errno;
>>>   }
>>>   /* Called by the event loop to process a SIGHUP.  */
>>> diff --git a/gdb/posix-hdep.c b/gdb/posix-hdep.c
>>> index a0d5c585ebc..bde9a9a3e7b 100644
>>> --- a/gdb/posix-hdep.c
>>> +++ b/gdb/posix-hdep.c
>>> @@ -64,9 +64,13 @@ static c_c_handler_ftype *current_handler;
>>>   static void
>>>   handler_wrapper (int num)
>>>   {
>>> +  int save_errno = errno;
>>> +
>>>     signal (num, handler_wrapper);
>>>     if (current_handler != SIG_IGN)
>>>       current_handler (num);
>>> +
>>> +  errno = save_errno;
>>>   }
>>>   /* See inferior.h.  */
>>> diff --git a/gdb/tui/tui-win.c b/gdb/tui/tui-win.c
>>> index e9a8e4651ea..9fe6288671c 100644
>>> --- a/gdb/tui/tui-win.c
>>> +++ b/gdb/tui/tui-win.c
>>> @@ -527,8 +527,12 @@ static struct async_signal_handler 
>>> *tui_sigwinch_token;
>>>   static void
>>>   tui_sigwinch_handler (int signal)
>>>   {
>>> +  int save_errno = errno;
>>> +
>>>     mark_async_signal_handler (tui_sigwinch_token);
>>>     tui_set_win_resized_to (true);
>>> +
>>> +  errno = save_errno;
>>>   }
>>>   /* Callback for asynchronously resizing TUI following a SIGWINCH 
>>> signal.  */
>>>
>>> base-commit: 01d8e0d24ad301f69b6f5ea39d073a728477507d
>>> -- 
>>> 2.43.0
>>
>
  

On 12/9/24 21:25, Alexandra Petlanova Hajkova wrote:
> 
> 
> On Mon, Dec 9, 2024 at 6:50 PM Tom de Vries <tdevries@suse.de 
> <mailto:tdevries@suse.de>> wrote:
> 
>     On 12/9/24 18:11, Andrew Burgess wrote:
>      > Tom de Vries <tdevries@suse.de <mailto:tdevries@suse.de>> writes:
>      >
>      >> When building gdb with -fsanitize=thread and running test-case
>      >> gdb.base/bg-exec-sigint-bp-cond.exp, I run into:
>      >> ...
>      >> ==================^M
>      >> WARNING: ThreadSanitizer: signal handler spoils errno (pid=25422)^M
>      >>      #0 handler_wrapper gdb/posix-hdep.c:66^M
>      >>      #1 decltype ({parm#2}({parm#3}...)) gdb::handle_eintr<>() \
>      >>           gdbsupport/eintr.h:67^M
>      >>      #2 gdb::waitpid(int, int*, int) gdbsupport/eintr.h:78^M
>      >>      #3 run_under_shell gdb/cli/cli-cmds.c:926^M
>      >> ...
>      >>
> 
> 
> Hi Tom,
> 
> when compiling without the  -fsanitize=thread, I can see the regression 
> for aarch64 in a
> watchpoint-hw.exp test:
> 
> Temporary breakpoint 2 at 0x40066c: file /root/build/gdb/ 
> testsuite/../../../binutils-gdb/
> gdb/testsuite/gdb.base/watchpoint-hw.c, line 23.
> Starting program: /root/build/gdb/testsuite/outputs/gdb.base/watchpoint- 
> hw/watchpoint-hw
> warning: Unable to determine the number of hardware watchpoints available.
> warning: Unable to determine the number of hardware breakpoints available.
> warning: watchpoint 1 downgraded to software watchpoint
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib64/libthread_db.so.1".
> FAIL: gdb.base/watchpoint-hw.exp: start (timeout)
> info watchpoints
> 

Hi Alexandra,

thanks for mentioning this.

Sofar I have not been able to reproduce it. I've tried it on two 
different aarch64 machines.

I've looked at the place where the warning is produced, and it would be 
interesting to know whether it's the ptrace call that is failing or not, 
and if so, what the associated errno is.

Could you try that experiment?

Thanks,
- Tom

> Temporary breakpoint 2, main () at /root/build/gdb/testsuite/../../../ 
> binutils-gdb/gdb/testsuite/gdb.base/watchpoint-hw.c:23
> 23        return 0;
> (gdb) info watchpoints
> Num     Type           Disp Enb Address            What
> 1       watchpoint     keep y                      watchee
> (gdb) FAIL: gdb.base/watchpoint-hw.exp: info watchpoints
> testcase /root/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/ 
> gdb.base/watchpoint-hw.exp completed in 10 seconds
> 
>