* [PATCH v2] [gdb] Handle EINTR in fgets
@ 2025-08-18 8:32 Tom de Vries
2025-08-18 10:23 ` Andrew Burgess
0 siblings, 1 reply; 3+ messages in thread
From: Tom de Vries @ 2025-08-18 8:32 UTC (permalink / raw)
To: gdb-patches
Usually, find_charset_names calls iconv -l to get the list of supported
charsets, allowing us to use a charset not in the default list:
...
$ /usr/bin/gdb -q -batch -ex "set charset CP858"
$
...
If the call to iconv -l fails somehow, gdb silently falls back to using the
default list, which gets us instead:
...
$ PATH= /usr/bin/gdb -q -batch -ex "set charset CP858"
Undefined item: "CP858".
$
...
PR gdb/33274 reports that gdb occasionally fails in the same way, because
fgets returns nullptr before the output of iconv -l is read entirely.
We asked the reporter to try out a patch handling errno == EINTR after
fgets returns nullptr, and that fixed the problem.
Fix this by:
- adding an inline function gdb::fgets that handles EINTR, and
- using it instead of fgets.
Tested on x86_64-linux.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33274
---
gdb/charset.c | 3 ++-
gdb/linux-nat.c | 3 ++-
gdb/nat/linux-btrace.c | 3 ++-
gdb/nat/linux-osdata.c | 13 +++++++------
gdb/nat/linux-procfs.c | 7 ++++---
gdbserver/linux-i386-ipa.cc | 3 ++-
gdbsupport/eintr.h | 15 +++++++++++++++
7 files changed, 34 insertions(+), 13 deletions(-)
diff --git a/gdb/charset.c b/gdb/charset.c
index 259362563b2..abf3fe800d2 100644
--- a/gdb/charset.c
+++ b/gdb/charset.c
@@ -25,6 +25,7 @@
#include "gdbsupport/environ.h"
#include "arch-utils.h"
#include <ctype.h>
+#include "gdbsupport/eintr.h"
#ifdef USE_WIN32API
#include <windows.h>
@@ -842,7 +843,7 @@ find_charset_names (void)
char *start, *r;
int len;
- r = fgets (buf, sizeof (buf), in);
+ r = gdb::fgets (buf, sizeof (buf), in);
if (!r)
break;
len = strlen (r);
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index f3179279d92..39f1fb59ae0 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -64,6 +64,7 @@
#include "gdbsupport/scope-exit.h"
#include "gdbsupport/gdb-sigmask.h"
#include "gdbsupport/common-debug.h"
+#include "gdbsupport/eintr.h"
#include <unordered_map>
/* This comment documents high-level logic of this file.
@@ -4301,7 +4302,7 @@ linux_proc_pending_signals (int pid, sigset_t *pending,
if (procfile == NULL)
error (_("Could not open %s"), fname);
- while (fgets (buffer, PATH_MAX, procfile.get ()) != NULL)
+ while (gdb::fgets (buffer, PATH_MAX, procfile.get ()) != NULL)
{
/* Normal queued signals are on the SigPnd line in the status
file. However, 2.6 kernels also have a "shared" pending
diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
index 9eadd46c9be..08c7b795a86 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -26,6 +26,7 @@
#include "gdbsupport/filestuff.h"
#include "gdbsupport/scoped_fd.h"
#include "gdbsupport/scoped_mmap.h"
+#include "gdbsupport/eintr.h"
#include <inttypes.h>
@@ -202,7 +203,7 @@ linux_determine_kernel_start (void)
uint64_t addr;
int match;
- line = fgets (buffer, sizeof (buffer), file.get ());
+ line = gdb::fgets (buffer, sizeof (buffer), file.get ());
if (line == NULL)
break;
diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c
index b52a8ed5f36..bfdc26f2795 100644
--- a/gdb/nat/linux-osdata.c
+++ b/gdb/nat/linux-osdata.c
@@ -35,6 +35,7 @@
#include <dirent.h>
#include <sys/stat.h>
#include "gdbsupport/filestuff.h"
+#include "gdbsupport/eintr.h"
#include <algorithm>
#include "linux-procfs.h"
@@ -562,7 +563,7 @@ linux_xfer_osdata_cpus ()
do
{
- if (fgets (buf, sizeof (buf), fp.get ()))
+ if (gdb::fgets (buf, sizeof (buf), fp.get ()))
{
char *key, *value;
int i = 0;
@@ -780,7 +781,7 @@ print_sockets (unsigned short family, int tcp, std::string &buffer)
do
{
- if (fgets (buf, sizeof (buf), fp.get ()))
+ if (gdb::fgets (buf, sizeof (buf), fp.get ()))
{
uid_t uid;
unsigned int local_port, remote_port, state;
@@ -961,7 +962,7 @@ linux_xfer_osdata_shm ()
do
{
- if (fgets (buf, sizeof (buf), fp.get ()))
+ if (gdb::fgets (buf, sizeof (buf), fp.get ()))
{
key_t key;
uid_t uid, cuid;
@@ -1057,7 +1058,7 @@ linux_xfer_osdata_sem ()
do
{
- if (fgets (buf, sizeof (buf), fp.get ()))
+ if (gdb::fgets (buf, sizeof (buf), fp.get ()))
{
key_t key;
uid_t uid, cuid;
@@ -1137,7 +1138,7 @@ linux_xfer_osdata_msg ()
do
{
- if (fgets (buf, sizeof (buf), fp.get ()))
+ if (gdb::fgets (buf, sizeof (buf), fp.get ()))
{
key_t key;
PID_T lspid, lrpid;
@@ -1231,7 +1232,7 @@ linux_xfer_osdata_modules ()
do
{
- if (fgets (buf, sizeof (buf), fp.get ()))
+ if (gdb::fgets (buf, sizeof (buf), fp.get ()))
{
char *name, *dependencies, *status, *tmp, *saveptr;
unsigned int size;
diff --git a/gdb/nat/linux-procfs.c b/gdb/nat/linux-procfs.c
index d4f9af32bb9..1927a21314e 100644
--- a/gdb/nat/linux-procfs.c
+++ b/gdb/nat/linux-procfs.c
@@ -19,6 +19,7 @@
#include "linux-procfs.h"
#include "gdbsupport/filestuff.h"
#include "gdbsupport/unordered_set.h"
+#include "gdbsupport/eintr.h"
#include <dirent.h>
#include <sys/stat.h>
#include <utility>
@@ -42,7 +43,7 @@ linux_proc_get_int (pid_t lwpid, const char *field, int warn)
return -1;
}
- while (fgets (buf, sizeof (buf), status_file.get ()))
+ while (gdb::fgets (buf, sizeof (buf), status_file.get ()))
if (strncmp (buf, field, field_len) == 0 && buf[field_len] == ':')
{
retval = strtol (&buf[field_len + 1], NULL, 10);
@@ -140,7 +141,7 @@ linux_proc_pid_get_state (pid_t pid, int warn, enum proc_state *state)
}
have_state = 0;
- while (fgets (buffer, sizeof (buffer), procfile.get ()) != NULL)
+ while (gdb::fgets (buffer, sizeof (buffer), procfile.get ()) != NULL)
if (startswith (buffer, "State:"))
{
have_state = 1;
@@ -317,7 +318,7 @@ linux_proc_tid_get_name (ptid_t ptid)
if (comm_file == NULL)
return NULL;
- comm_val = fgets (comm_buf, sizeof (comm_buf), comm_file.get ());
+ comm_val = gdb::fgets (comm_buf, sizeof (comm_buf), comm_file.get ());
if (comm_val != NULL)
{
diff --git a/gdbserver/linux-i386-ipa.cc b/gdbserver/linux-i386-ipa.cc
index 17af6eb3610..7d6b9600c34 100644
--- a/gdbserver/linux-i386-ipa.cc
+++ b/gdbserver/linux-i386-ipa.cc
@@ -21,6 +21,7 @@
#include <sys/mman.h>
#include "tracepoint.h"
#include "gdbsupport/x86-xstate.h"
+#include "gdbsupport/eintr.h"
#include "arch/i386-linux-tdesc.h"
#include "arch/x86-linux-tdesc-features.h"
@@ -138,7 +139,7 @@ initialize_fast_tracepoint_trampoline_buffer (void)
return;
}
- if (fgets (buf, IPA_BUFSIZ, f))
+ if (gdb::fgets (buf, IPA_BUFSIZ, f))
sscanf (buf, "%llu", &mmap_min_addr);
fclose (f);
diff --git a/gdbsupport/eintr.h b/gdbsupport/eintr.h
index 04f86c8c3df..800cab561b4 100644
--- a/gdbsupport/eintr.h
+++ b/gdbsupport/eintr.h
@@ -116,6 +116,21 @@ write (int fd, const void *buf, size_t count)
return gdb::handle_eintr (-1, ::write, fd, buf, count);
}
+inline char*
+fgets (char *str, int count, FILE* stream)
+{
+ char *ret;
+
+ do
+ {
+ errno = 0;
+ ret = ::fgets (str, count, stream);
+ }
+ while (ret == nullptr && ferror (stream) && errno == EINTR);
+
+ return ret;
+}
+
} /* namespace gdb */
#endif /* GDBSUPPORT_EINTR_H */
base-commit: 570f4c0c11910d845c35e94764ded54cb1111583
--
2.43.0
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] [gdb] Handle EINTR in fgets
2025-08-18 8:32 [PATCH v2] [gdb] Handle EINTR in fgets Tom de Vries
@ 2025-08-18 10:23 ` Andrew Burgess
2025-08-18 12:29 ` Tom de Vries
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Burgess @ 2025-08-18 10:23 UTC (permalink / raw)
To: Tom de Vries, gdb-patches
Tom de Vries <tdevries@suse.de> writes:
> Usually, find_charset_names calls iconv -l to get the list of supported
> charsets, allowing us to use a charset not in the default list:
> ...
> $ /usr/bin/gdb -q -batch -ex "set charset CP858"
> $
> ...
>
> If the call to iconv -l fails somehow, gdb silently falls back to using the
> default list, which gets us instead:
> ...
> $ PATH= /usr/bin/gdb -q -batch -ex "set charset CP858"
> Undefined item: "CP858".
> $
> ...
>
> PR gdb/33274 reports that gdb occasionally fails in the same way, because
> fgets returns nullptr before the output of iconv -l is read entirely.
>
> We asked the reporter to try out a patch handling errno == EINTR after
> fgets returns nullptr, and that fixed the problem.
>
> Fix this by:
> - adding an inline function gdb::fgets that handles EINTR, and
> - using it instead of fgets.
>
> Tested on x86_64-linux.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33274
> ---
> gdb/charset.c | 3 ++-
> gdb/linux-nat.c | 3 ++-
> gdb/nat/linux-btrace.c | 3 ++-
> gdb/nat/linux-osdata.c | 13 +++++++------
> gdb/nat/linux-procfs.c | 7 ++++---
> gdbserver/linux-i386-ipa.cc | 3 ++-
> gdbsupport/eintr.h | 15 +++++++++++++++
> 7 files changed, 34 insertions(+), 13 deletions(-)
>
> diff --git a/gdb/charset.c b/gdb/charset.c
> index 259362563b2..abf3fe800d2 100644
> --- a/gdb/charset.c
> +++ b/gdb/charset.c
> @@ -25,6 +25,7 @@
> #include "gdbsupport/environ.h"
> #include "arch-utils.h"
> #include <ctype.h>
> +#include "gdbsupport/eintr.h"
>
> #ifdef USE_WIN32API
> #include <windows.h>
> @@ -842,7 +843,7 @@ find_charset_names (void)
> char *start, *r;
> int len;
>
> - r = fgets (buf, sizeof (buf), in);
> + r = gdb::fgets (buf, sizeof (buf), in);
> if (!r)
> break;
> len = strlen (r);
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index f3179279d92..39f1fb59ae0 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -64,6 +64,7 @@
> #include "gdbsupport/scope-exit.h"
> #include "gdbsupport/gdb-sigmask.h"
> #include "gdbsupport/common-debug.h"
> +#include "gdbsupport/eintr.h"
> #include <unordered_map>
>
> /* This comment documents high-level logic of this file.
> @@ -4301,7 +4302,7 @@ linux_proc_pending_signals (int pid, sigset_t *pending,
> if (procfile == NULL)
> error (_("Could not open %s"), fname);
>
> - while (fgets (buffer, PATH_MAX, procfile.get ()) != NULL)
> + while (gdb::fgets (buffer, PATH_MAX, procfile.get ()) != NULL)
> {
> /* Normal queued signals are on the SigPnd line in the status
> file. However, 2.6 kernels also have a "shared" pending
> diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
> index 9eadd46c9be..08c7b795a86 100644
> --- a/gdb/nat/linux-btrace.c
> +++ b/gdb/nat/linux-btrace.c
> @@ -26,6 +26,7 @@
> #include "gdbsupport/filestuff.h"
> #include "gdbsupport/scoped_fd.h"
> #include "gdbsupport/scoped_mmap.h"
> +#include "gdbsupport/eintr.h"
>
> #include <inttypes.h>
>
> @@ -202,7 +203,7 @@ linux_determine_kernel_start (void)
> uint64_t addr;
> int match;
>
> - line = fgets (buffer, sizeof (buffer), file.get ());
> + line = gdb::fgets (buffer, sizeof (buffer), file.get ());
> if (line == NULL)
> break;
>
> diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c
> index b52a8ed5f36..bfdc26f2795 100644
> --- a/gdb/nat/linux-osdata.c
> +++ b/gdb/nat/linux-osdata.c
> @@ -35,6 +35,7 @@
> #include <dirent.h>
> #include <sys/stat.h>
> #include "gdbsupport/filestuff.h"
> +#include "gdbsupport/eintr.h"
> #include <algorithm>
> #include "linux-procfs.h"
>
> @@ -562,7 +563,7 @@ linux_xfer_osdata_cpus ()
>
> do
> {
> - if (fgets (buf, sizeof (buf), fp.get ()))
> + if (gdb::fgets (buf, sizeof (buf), fp.get ()))
> {
> char *key, *value;
> int i = 0;
> @@ -780,7 +781,7 @@ print_sockets (unsigned short family, int tcp, std::string &buffer)
>
> do
> {
> - if (fgets (buf, sizeof (buf), fp.get ()))
> + if (gdb::fgets (buf, sizeof (buf), fp.get ()))
> {
> uid_t uid;
> unsigned int local_port, remote_port, state;
> @@ -961,7 +962,7 @@ linux_xfer_osdata_shm ()
>
> do
> {
> - if (fgets (buf, sizeof (buf), fp.get ()))
> + if (gdb::fgets (buf, sizeof (buf), fp.get ()))
> {
> key_t key;
> uid_t uid, cuid;
> @@ -1057,7 +1058,7 @@ linux_xfer_osdata_sem ()
>
> do
> {
> - if (fgets (buf, sizeof (buf), fp.get ()))
> + if (gdb::fgets (buf, sizeof (buf), fp.get ()))
> {
> key_t key;
> uid_t uid, cuid;
> @@ -1137,7 +1138,7 @@ linux_xfer_osdata_msg ()
>
> do
> {
> - if (fgets (buf, sizeof (buf), fp.get ()))
> + if (gdb::fgets (buf, sizeof (buf), fp.get ()))
> {
> key_t key;
> PID_T lspid, lrpid;
> @@ -1231,7 +1232,7 @@ linux_xfer_osdata_modules ()
>
> do
> {
> - if (fgets (buf, sizeof (buf), fp.get ()))
> + if (gdb::fgets (buf, sizeof (buf), fp.get ()))
> {
> char *name, *dependencies, *status, *tmp, *saveptr;
> unsigned int size;
> diff --git a/gdb/nat/linux-procfs.c b/gdb/nat/linux-procfs.c
> index d4f9af32bb9..1927a21314e 100644
> --- a/gdb/nat/linux-procfs.c
> +++ b/gdb/nat/linux-procfs.c
> @@ -19,6 +19,7 @@
> #include "linux-procfs.h"
> #include "gdbsupport/filestuff.h"
> #include "gdbsupport/unordered_set.h"
> +#include "gdbsupport/eintr.h"
> #include <dirent.h>
> #include <sys/stat.h>
> #include <utility>
> @@ -42,7 +43,7 @@ linux_proc_get_int (pid_t lwpid, const char *field, int warn)
> return -1;
> }
>
> - while (fgets (buf, sizeof (buf), status_file.get ()))
> + while (gdb::fgets (buf, sizeof (buf), status_file.get ()))
> if (strncmp (buf, field, field_len) == 0 && buf[field_len] == ':')
> {
> retval = strtol (&buf[field_len + 1], NULL, 10);
> @@ -140,7 +141,7 @@ linux_proc_pid_get_state (pid_t pid, int warn, enum proc_state *state)
> }
>
> have_state = 0;
> - while (fgets (buffer, sizeof (buffer), procfile.get ()) != NULL)
> + while (gdb::fgets (buffer, sizeof (buffer), procfile.get ()) != NULL)
> if (startswith (buffer, "State:"))
> {
> have_state = 1;
> @@ -317,7 +318,7 @@ linux_proc_tid_get_name (ptid_t ptid)
> if (comm_file == NULL)
> return NULL;
>
> - comm_val = fgets (comm_buf, sizeof (comm_buf), comm_file.get ());
> + comm_val = gdb::fgets (comm_buf, sizeof (comm_buf), comm_file.get ());
>
> if (comm_val != NULL)
> {
> diff --git a/gdbserver/linux-i386-ipa.cc b/gdbserver/linux-i386-ipa.cc
> index 17af6eb3610..7d6b9600c34 100644
> --- a/gdbserver/linux-i386-ipa.cc
> +++ b/gdbserver/linux-i386-ipa.cc
> @@ -21,6 +21,7 @@
> #include <sys/mman.h>
> #include "tracepoint.h"
> #include "gdbsupport/x86-xstate.h"
> +#include "gdbsupport/eintr.h"
> #include "arch/i386-linux-tdesc.h"
> #include "arch/x86-linux-tdesc-features.h"
>
> @@ -138,7 +139,7 @@ initialize_fast_tracepoint_trampoline_buffer (void)
> return;
> }
>
> - if (fgets (buf, IPA_BUFSIZ, f))
> + if (gdb::fgets (buf, IPA_BUFSIZ, f))
> sscanf (buf, "%llu", &mmap_min_addr);
>
> fclose (f);
> diff --git a/gdbsupport/eintr.h b/gdbsupport/eintr.h
> index 04f86c8c3df..800cab561b4 100644
> --- a/gdbsupport/eintr.h
> +++ b/gdbsupport/eintr.h
> @@ -116,6 +116,21 @@ write (int fd, const void *buf, size_t count)
> return gdb::handle_eintr (-1, ::write, fd, buf, count);
> }
>
> +inline char*
> +fgets (char *str, int count, FILE* stream)
> +{
> + char *ret;
> +
> + do
> + {
> + errno = 0;
> + ret = ::fgets (str, count, stream);
> + }
> + while (ret == nullptr && ferror (stream) && errno == EINTR);
Having read through the v1 thread, I just want to make sure I'm clear on
the motivation for not using handle_eintr: your concern is that errno
might be set in the EOF case?
I'm not arguing with your reading of the spec, but that does seem
unlikely. So much so that I do think that's worth a comment explaining
the logic here. It's also worth the comment explaining that we've never
_seen_ such behaviour, just that we're worried such behaviour _might_ be
in-spec.
That comment could fill in the missing header comment for this function.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] [gdb] Handle EINTR in fgets
2025-08-18 10:23 ` Andrew Burgess
@ 2025-08-18 12:29 ` Tom de Vries
0 siblings, 0 replies; 3+ messages in thread
From: Tom de Vries @ 2025-08-18 12:29 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
On 8/18/25 12:23, Andrew Burgess wrote:
> Tom de Vries <tdevries@suse.de> writes:
>
>> Usually, find_charset_names calls iconv -l to get the list of supported
>> charsets, allowing us to use a charset not in the default list:
>> ...
>> $ /usr/bin/gdb -q -batch -ex "set charset CP858"
>> $
>> ...
>>
>> If the call to iconv -l fails somehow, gdb silently falls back to using the
>> default list, which gets us instead:
>> ...
>> $ PATH= /usr/bin/gdb -q -batch -ex "set charset CP858"
>> Undefined item: "CP858".
>> $
>> ...
>>
>> PR gdb/33274 reports that gdb occasionally fails in the same way, because
>> fgets returns nullptr before the output of iconv -l is read entirely.
>>
>> We asked the reporter to try out a patch handling errno == EINTR after
>> fgets returns nullptr, and that fixed the problem.
>>
>> Fix this by:
>> - adding an inline function gdb::fgets that handles EINTR, and
>> - using it instead of fgets.
>>
>> Tested on x86_64-linux.
>>
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33274
>> ---
>> gdb/charset.c | 3 ++-
>> gdb/linux-nat.c | 3 ++-
>> gdb/nat/linux-btrace.c | 3 ++-
>> gdb/nat/linux-osdata.c | 13 +++++++------
>> gdb/nat/linux-procfs.c | 7 ++++---
>> gdbserver/linux-i386-ipa.cc | 3 ++-
>> gdbsupport/eintr.h | 15 +++++++++++++++
>> 7 files changed, 34 insertions(+), 13 deletions(-)
>>
>> diff --git a/gdb/charset.c b/gdb/charset.c
>> index 259362563b2..abf3fe800d2 100644
>> --- a/gdb/charset.c
>> +++ b/gdb/charset.c
>> @@ -25,6 +25,7 @@
>> #include "gdbsupport/environ.h"
>> #include "arch-utils.h"
>> #include <ctype.h>
>> +#include "gdbsupport/eintr.h"
>>
>> #ifdef USE_WIN32API
>> #include <windows.h>
>> @@ -842,7 +843,7 @@ find_charset_names (void)
>> char *start, *r;
>> int len;
>>
>> - r = fgets (buf, sizeof (buf), in);
>> + r = gdb::fgets (buf, sizeof (buf), in);
>> if (!r)
>> break;
>> len = strlen (r);
>> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
>> index f3179279d92..39f1fb59ae0 100644
>> --- a/gdb/linux-nat.c
>> +++ b/gdb/linux-nat.c
>> @@ -64,6 +64,7 @@
>> #include "gdbsupport/scope-exit.h"
>> #include "gdbsupport/gdb-sigmask.h"
>> #include "gdbsupport/common-debug.h"
>> +#include "gdbsupport/eintr.h"
>> #include <unordered_map>
>>
>> /* This comment documents high-level logic of this file.
>> @@ -4301,7 +4302,7 @@ linux_proc_pending_signals (int pid, sigset_t *pending,
>> if (procfile == NULL)
>> error (_("Could not open %s"), fname);
>>
>> - while (fgets (buffer, PATH_MAX, procfile.get ()) != NULL)
>> + while (gdb::fgets (buffer, PATH_MAX, procfile.get ()) != NULL)
>> {
>> /* Normal queued signals are on the SigPnd line in the status
>> file. However, 2.6 kernels also have a "shared" pending
>> diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
>> index 9eadd46c9be..08c7b795a86 100644
>> --- a/gdb/nat/linux-btrace.c
>> +++ b/gdb/nat/linux-btrace.c
>> @@ -26,6 +26,7 @@
>> #include "gdbsupport/filestuff.h"
>> #include "gdbsupport/scoped_fd.h"
>> #include "gdbsupport/scoped_mmap.h"
>> +#include "gdbsupport/eintr.h"
>>
>> #include <inttypes.h>
>>
>> @@ -202,7 +203,7 @@ linux_determine_kernel_start (void)
>> uint64_t addr;
>> int match;
>>
>> - line = fgets (buffer, sizeof (buffer), file.get ());
>> + line = gdb::fgets (buffer, sizeof (buffer), file.get ());
>> if (line == NULL)
>> break;
>>
>> diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c
>> index b52a8ed5f36..bfdc26f2795 100644
>> --- a/gdb/nat/linux-osdata.c
>> +++ b/gdb/nat/linux-osdata.c
>> @@ -35,6 +35,7 @@
>> #include <dirent.h>
>> #include <sys/stat.h>
>> #include "gdbsupport/filestuff.h"
>> +#include "gdbsupport/eintr.h"
>> #include <algorithm>
>> #include "linux-procfs.h"
>>
>> @@ -562,7 +563,7 @@ linux_xfer_osdata_cpus ()
>>
>> do
>> {
>> - if (fgets (buf, sizeof (buf), fp.get ()))
>> + if (gdb::fgets (buf, sizeof (buf), fp.get ()))
>> {
>> char *key, *value;
>> int i = 0;
>> @@ -780,7 +781,7 @@ print_sockets (unsigned short family, int tcp, std::string &buffer)
>>
>> do
>> {
>> - if (fgets (buf, sizeof (buf), fp.get ()))
>> + if (gdb::fgets (buf, sizeof (buf), fp.get ()))
>> {
>> uid_t uid;
>> unsigned int local_port, remote_port, state;
>> @@ -961,7 +962,7 @@ linux_xfer_osdata_shm ()
>>
>> do
>> {
>> - if (fgets (buf, sizeof (buf), fp.get ()))
>> + if (gdb::fgets (buf, sizeof (buf), fp.get ()))
>> {
>> key_t key;
>> uid_t uid, cuid;
>> @@ -1057,7 +1058,7 @@ linux_xfer_osdata_sem ()
>>
>> do
>> {
>> - if (fgets (buf, sizeof (buf), fp.get ()))
>> + if (gdb::fgets (buf, sizeof (buf), fp.get ()))
>> {
>> key_t key;
>> uid_t uid, cuid;
>> @@ -1137,7 +1138,7 @@ linux_xfer_osdata_msg ()
>>
>> do
>> {
>> - if (fgets (buf, sizeof (buf), fp.get ()))
>> + if (gdb::fgets (buf, sizeof (buf), fp.get ()))
>> {
>> key_t key;
>> PID_T lspid, lrpid;
>> @@ -1231,7 +1232,7 @@ linux_xfer_osdata_modules ()
>>
>> do
>> {
>> - if (fgets (buf, sizeof (buf), fp.get ()))
>> + if (gdb::fgets (buf, sizeof (buf), fp.get ()))
>> {
>> char *name, *dependencies, *status, *tmp, *saveptr;
>> unsigned int size;
>> diff --git a/gdb/nat/linux-procfs.c b/gdb/nat/linux-procfs.c
>> index d4f9af32bb9..1927a21314e 100644
>> --- a/gdb/nat/linux-procfs.c
>> +++ b/gdb/nat/linux-procfs.c
>> @@ -19,6 +19,7 @@
>> #include "linux-procfs.h"
>> #include "gdbsupport/filestuff.h"
>> #include "gdbsupport/unordered_set.h"
>> +#include "gdbsupport/eintr.h"
>> #include <dirent.h>
>> #include <sys/stat.h>
>> #include <utility>
>> @@ -42,7 +43,7 @@ linux_proc_get_int (pid_t lwpid, const char *field, int warn)
>> return -1;
>> }
>>
>> - while (fgets (buf, sizeof (buf), status_file.get ()))
>> + while (gdb::fgets (buf, sizeof (buf), status_file.get ()))
>> if (strncmp (buf, field, field_len) == 0 && buf[field_len] == ':')
>> {
>> retval = strtol (&buf[field_len + 1], NULL, 10);
>> @@ -140,7 +141,7 @@ linux_proc_pid_get_state (pid_t pid, int warn, enum proc_state *state)
>> }
>>
>> have_state = 0;
>> - while (fgets (buffer, sizeof (buffer), procfile.get ()) != NULL)
>> + while (gdb::fgets (buffer, sizeof (buffer), procfile.get ()) != NULL)
>> if (startswith (buffer, "State:"))
>> {
>> have_state = 1;
>> @@ -317,7 +318,7 @@ linux_proc_tid_get_name (ptid_t ptid)
>> if (comm_file == NULL)
>> return NULL;
>>
>> - comm_val = fgets (comm_buf, sizeof (comm_buf), comm_file.get ());
>> + comm_val = gdb::fgets (comm_buf, sizeof (comm_buf), comm_file.get ());
>>
>> if (comm_val != NULL)
>> {
>> diff --git a/gdbserver/linux-i386-ipa.cc b/gdbserver/linux-i386-ipa.cc
>> index 17af6eb3610..7d6b9600c34 100644
>> --- a/gdbserver/linux-i386-ipa.cc
>> +++ b/gdbserver/linux-i386-ipa.cc
>> @@ -21,6 +21,7 @@
>> #include <sys/mman.h>
>> #include "tracepoint.h"
>> #include "gdbsupport/x86-xstate.h"
>> +#include "gdbsupport/eintr.h"
>> #include "arch/i386-linux-tdesc.h"
>> #include "arch/x86-linux-tdesc-features.h"
>>
>> @@ -138,7 +139,7 @@ initialize_fast_tracepoint_trampoline_buffer (void)
>> return;
>> }
>>
>> - if (fgets (buf, IPA_BUFSIZ, f))
>> + if (gdb::fgets (buf, IPA_BUFSIZ, f))
>> sscanf (buf, "%llu", &mmap_min_addr);
>>
>> fclose (f);
>> diff --git a/gdbsupport/eintr.h b/gdbsupport/eintr.h
>> index 04f86c8c3df..800cab561b4 100644
>> --- a/gdbsupport/eintr.h
>> +++ b/gdbsupport/eintr.h
>> @@ -116,6 +116,21 @@ write (int fd, const void *buf, size_t count)
>> return gdb::handle_eintr (-1, ::write, fd, buf, count);
>> }
>>
>> +inline char*
>> +fgets (char *str, int count, FILE* stream)
>> +{
>> + char *ret;
>> +
>> + do
>> + {
>> + errno = 0;
>> + ret = ::fgets (str, count, stream);
>> + }
>> + while (ret == nullptr && ferror (stream) && errno == EINTR);
>
> Having read through the v1 thread, I just want to make sure I'm clear on
> the motivation for not using handle_eintr: your concern is that errno
> might be set in the EOF case?
>
Hi Andrew,
thanks for the review.
Yes, that is my concern.
> I'm not arguing with your reading of the spec, but that does seem
> unlikely.
Agreed.
> So much so that I do think that's worth a comment explaining
> the logic here. It's also worth the comment explaining that we've never
> _seen_ such behaviour, just that we're worried such behaviour _might_ be
> in-spec.
>
Good point. I've added a comment in a v3 (
https://sourceware.org/pipermail/gdb-patches/2025-August/220017.html ).
> That comment could fill in the missing header comment for this function.
>
Since it's a comment about the implementation of the function, I've
added it in the function.
[ The missing header comment seems to be a feature of all the functions
in the file, probably because they're wrappers and the interface is
defined by what they're wrapping, and there's only one difference in
behaviour implemented by the wrapper, which is described in detail at
the top of the file. ]
Thanks,
- Tom
> Thanks,
> Andrew
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-08-18 12:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-08-18 8:32 [PATCH v2] [gdb] Handle EINTR in fgets Tom de Vries
2025-08-18 10:23 ` Andrew Burgess
2025-08-18 12:29 ` Tom de Vries
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox