| Message ID | patch-1.1-87d9bcf1095-20210505T083951Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | trace2: refactor to avoid gcc warning under -O3 | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Refactor tr2_dst_try_uds_connect() to avoid a gcc warning[1] that > appears under -O3 (but not -O2). This makes the build pass under > DEVELOPER=1 without needing a DEVOPTS=no-error. > > This can be reproduced with GCC Debian 8.3.0-6, but not e.g. with > clang 7.0.1-8+deb10u2. We've had this warning since > ee4512ed481 (trace2: create new combined trace facility, 2019-02-22). > > As noted in [2] this warning happens because the compiler doesn't > assume that errno must be non-zero after a failed syscall. Let's work > around it as suggested in that analysis. We now return -1 ourselves on > error, and save away the value of errno in a variable the caller > passes in. > > 1. > > trace2/tr2_dst.c: In function ‘tr2_dst_get_trace_fd.part.5’: > trace2/tr2_dst.c:296:10: warning: ‘fd’ may be used uninitialized in this function [-Wmaybe-uninitialized] > dst->fd = fd; > ~~~~~~~~^~~~ > trace2/tr2_dst.c:229:6: note: ‘fd’ was declared here > int fd; > ^~ > 2. https://lore.kernel.org/git/20200404142131.GA679473@coredump.intra.peff.net/ > --- > trace2/tr2_dst.c | 27 +++++++++++++++------------ > 1 file changed, 15 insertions(+), 12 deletions(-) The patch makes sense to me, modulo that the way the variable "saved_errno" introduced by this patch is used and the way a variable with that name is typically used in our codebase are at odds. I.e. we typically call a variable "saved_errno" when it is used in this pattern: if (a_syscall_whose_error_condition_we_care_about()) { int saved_errno = errno; perform_some_cleanup_operation_that_might_clobber_errno(); return error_errno(..., saved_errno); /* * or * errno = saved_errno; * return -1; * and let the caller handle 'errno' */ } But since I do not think of a better name for this new variable that is not exactly used like so, let's queue it as-is. Thanks. > diff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c > index ae052a07fe2..c2aba71041b 100644 > --- a/trace2/tr2_dst.c > +++ b/trace2/tr2_dst.c > @@ -197,22 +197,25 @@ static int tr2_dst_try_path(struct tr2_dst *dst, const char *tgt_value) > #define PREFIX_AF_UNIX_STREAM "af_unix:stream:" > #define PREFIX_AF_UNIX_DGRAM "af_unix:dgram:" > > -static int tr2_dst_try_uds_connect(const char *path, int sock_type, int *out_fd) > +static int tr2_dst_try_uds_connect(const char *path, int sock_type, > + int *out_fd, int *saved_errno) > { > int fd; > struct sockaddr_un sa; > > fd = socket(AF_UNIX, sock_type, 0); > - if (fd == -1) > - return errno; > + if (fd == -1) { > + *saved_errno = errno; > + return -1; > + } > > sa.sun_family = AF_UNIX; > strlcpy(sa.sun_path, path, sizeof(sa.sun_path)); > > if (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) == -1) { > - int e = errno; > + *saved_errno = errno; > close(fd); > - return e; > + return -1; > } > > *out_fd = fd; > @@ -227,7 +230,7 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst, > { > unsigned int uds_try = 0; > int fd; > - int e; > + int saved_errno; > const char *path = NULL; > > /* > @@ -271,15 +274,15 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst, > } > > if (uds_try & TR2_DST_UDS_TRY_STREAM) { > - e = tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd); > - if (!e) > + if (!tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd, > + &saved_errno)) > goto connected; > - if (e != EPROTOTYPE) > + if (saved_errno != EPROTOTYPE) > goto error; > } > if (uds_try & TR2_DST_UDS_TRY_DGRAM) { > - e = tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd); > - if (!e) > + if (!tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd, > + &saved_errno)) > goto connected; > } > > @@ -287,7 +290,7 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst, > if (tr2_dst_want_warning()) > warning("trace2: could not connect to socket '%s' for '%s' tracing: %s", > path, tr2_sysenv_display_name(dst->sysenv_var), > - strerror(e)); > + strerror(saved_errno)); > > tr2_dst_trace_disable(dst); > return 0;
On Wed, May 05, 2021 at 06:47:29PM +0900, Junio C Hamano wrote: > The patch makes sense to me, modulo that the way the variable > "saved_errno" introduced by this patch is used and the way a > variable with that name is typically used in our codebase are at > odds. I.e. we typically call a variable "saved_errno" when it is > used in this pattern: > > if (a_syscall_whose_error_condition_we_care_about()) { > int saved_errno = errno; > perform_some_cleanup_operation_that_might_clobber_errno(); > return error_errno(..., saved_errno); > /* > * or > * errno = saved_errno; > * return -1; > * and let the caller handle 'errno' > */ > } > > But since I do not think of a better name for this new variable that > is not exactly used like so, let's queue it as-is. I'd probably have just called it "err", but I think it is fine either way. :) The patch also looks good to me. I used to compile with -O3 occasionally to fix warnings, but given the date on this commit, it seems I have not done so in quite a while. (It reproduces on gcc 10 for me, which is not surprising). -Peff
Hi Ævar, On Wed, 5 May 2021, Ævar Arnfjörð Bjarmason wrote: > Refactor tr2_dst_try_uds_connect() to avoid a gcc warning[1] that > appears under -O3 (but not -O2). This makes the build pass under > DEVELOPER=1 without needing a DEVOPTS=no-error. > > This can be reproduced with GCC Debian 8.3.0-6, but not e.g. with > clang 7.0.1-8+deb10u2. We've had this warning since > ee4512ed481 (trace2: create new combined trace facility, 2019-02-22). > > As noted in [2] this warning happens because the compiler doesn't > assume that errno must be non-zero after a failed syscall. Let's work > around it as suggested in that analysis. We now return -1 ourselves on > error, and save away the value of errno in a variable the caller > passes in. It would probably be a lot nicer if you lead with this insight. I could imagine, for example, that a oneline like this would be much more helpful to any reader: trace2: do not assume errno != 0 after a failed syscall The first two paragraphs are less interesting than the third paragraph, too, therefore I would recommend About the patch... > > 1. > > trace2/tr2_dst.c: In function ‘tr2_dst_get_trace_fd.part.5’: > trace2/tr2_dst.c:296:10: warning: ‘fd’ may be used uninitialized in this function [-Wmaybe-uninitialized] > dst->fd = fd; > ~~~~~~~~^~~~ > trace2/tr2_dst.c:229:6: note: ‘fd’ was declared here > int fd; > ^~ > 2. https://lore.kernel.org/git/20200404142131.GA679473@coredump.intra.peff.net/ > --- > trace2/tr2_dst.c | 27 +++++++++++++++------------ > 1 file changed, 15 insertions(+), 12 deletions(-) > > diff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c > index ae052a07fe2..c2aba71041b 100644 > --- a/trace2/tr2_dst.c > +++ b/trace2/tr2_dst.c > @@ -197,22 +197,25 @@ static int tr2_dst_try_path(struct tr2_dst *dst, const char *tgt_value) > #define PREFIX_AF_UNIX_STREAM "af_unix:stream:" > #define PREFIX_AF_UNIX_DGRAM "af_unix:dgram:" > > -static int tr2_dst_try_uds_connect(const char *path, int sock_type, int *out_fd) > +static int tr2_dst_try_uds_connect(const char *path, int sock_type, > + int *out_fd, int *saved_errno) > { > int fd; > struct sockaddr_un sa; > > fd = socket(AF_UNIX, sock_type, 0); > - if (fd == -1) > - return errno; > + if (fd == -1) { > + *saved_errno = errno; > + return -1; > + } I don't think this is necessary. My manual page for socket(2) says this: RETURN VALUE If the connection or binding succeeds, zero is returned. On error, -1 is returned, and errno is set appropriately. > sa.sun_family = AF_UNIX; > strlcpy(sa.sun_path, path, sizeof(sa.sun_path)); > > if (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) == -1) { > - int e = errno; > + *saved_errno = errno; > close(fd); > - return e; > + return -1; Likewise, my manual page for connect(2) says the same as for socket(2): upon return value -1, errno is set appropriately (i.e. non-zero). Therefore, I would say this patch is actually only papering over an overzealous (and incorrect) compiler warning. If you _must_ shut up GCC, a better idea would be a much less intrusive, easier to read /* GCC thinks socket()/connect() might fail to set errno */ return errno ? errno : EIO; Ciao, Dscho > } > > *out_fd = fd; > @@ -227,7 +230,7 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst, > { > unsigned int uds_try = 0; > int fd; > - int e; > + int saved_errno; > const char *path = NULL; > > /* > @@ -271,15 +274,15 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst, > } > > if (uds_try & TR2_DST_UDS_TRY_STREAM) { > - e = tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd); > - if (!e) > + if (!tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd, > + &saved_errno)) > goto connected; > - if (e != EPROTOTYPE) > + if (saved_errno != EPROTOTYPE) > goto error; > } > if (uds_try & TR2_DST_UDS_TRY_DGRAM) { > - e = tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd); > - if (!e) > + if (!tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd, > + &saved_errno)) > goto connected; > } > > @@ -287,7 +290,7 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst, > if (tr2_dst_want_warning()) > warning("trace2: could not connect to socket '%s' for '%s' tracing: %s", > path, tr2_sysenv_display_name(dst->sysenv_var), > - strerror(e)); > + strerror(saved_errno)); > > tr2_dst_trace_disable(dst); > return 0; > -- > 2.31.1.745.g2af7c6593ce > >
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Hi Ævar, > > On Wed, 5 May 2021, Ævar Arnfjörð Bjarmason wrote: > >> Refactor tr2_dst_try_uds_connect() to avoid a gcc warning[1] that >> appears under -O3 (but not -O2). This makes the build pass under >> DEVELOPER=1 without needing a DEVOPTS=no-error. >> >> This can be reproduced with GCC Debian 8.3.0-6, but not e.g. with >> clang 7.0.1-8+deb10u2. We've had this warning since >> ee4512ed481 (trace2: create new combined trace facility, 2019-02-22). >> >> As noted in [2] this warning happens because the compiler doesn't >> assume that errno must be non-zero after a failed syscall. Let's work >> around it as suggested in that analysis. We now return -1 ourselves on >> error, and save away the value of errno in a variable the caller >> passes in. > > It would probably be a lot nicer if you lead with this insight. I could > imagine, for example, that a oneline like this would be much more helpful > to any reader: > > trace2: do not assume errno != 0 after a failed syscall But that is misleading. My understanding is that this patch is about working around compilers that do not know that a failed syscall means errno would be set to non-zero. Am I mistaken? Otherwise I'd strongly prefer to see a word that hints that this is an otherwise unneeded workaround for comiplers. Your suggested title instead hints that it is wrong to assume that errno will be set to non-zero after a syscall. I do not think that is the message we want to send to our readers.
Hi Junio, On Thu, 6 May 2021, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > Hi Ævar, > > > > On Wed, 5 May 2021, Ævar Arnfjörð Bjarmason wrote: > > > >> Refactor tr2_dst_try_uds_connect() to avoid a gcc warning[1] that > >> appears under -O3 (but not -O2). This makes the build pass under > >> DEVELOPER=1 without needing a DEVOPTS=no-error. > >> > >> This can be reproduced with GCC Debian 8.3.0-6, but not e.g. with > >> clang 7.0.1-8+deb10u2. We've had this warning since > >> ee4512ed481 (trace2: create new combined trace facility, 2019-02-22). > >> > >> As noted in [2] this warning happens because the compiler doesn't > >> assume that errno must be non-zero after a failed syscall. Let's work > >> around it as suggested in that analysis. We now return -1 ourselves on > >> error, and save away the value of errno in a variable the caller > >> passes in. > > > > It would probably be a lot nicer if you lead with this insight. I could > > imagine, for example, that a oneline like this would be much more helpful > > to any reader: > > > > trace2: do not assume errno != 0 after a failed syscall > > But that is misleading. > > My understanding is that this patch is about working around > compilers that do not know that a failed syscall means errno would > be set to non-zero. Am I mistaken? > > Otherwise I'd strongly prefer to see a word that hints that this is > an otherwise unneeded workaround for comiplers. Your suggested > title instead hints that it is wrong to assume that errno will be > set to non-zero after a syscall. I do not think that is the message > we want to send to our readers. Right, the oneline I suggested was only for the original patch, with which I disagreed. Ciao, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> Otherwise I'd strongly prefer to see a word that hints that this is >> an otherwise unneeded workaround for comiplers. Your suggested >> title instead hints that it is wrong to assume that errno will be >> set to non-zero after a syscall. I do not think that is the message >> we want to send to our readers. > > Right, the oneline I suggested was only for the original patch, with which > I disagreed. I actually do not know how your rewrite could be better, though. /* GCC thinks socket()/connect() might fail to set errno */ return errno ? errno : EIO; If a compiler thinks errno may *not* be set, can 'errno' be reliably used to decide if it can be returned as-is or a fallback value EIO should be used, without triggering the same (incorrect) working in the first place?
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Refactor tr2_dst_try_uds_connect() to avoid a gcc warning[1] that > appears under -O3 (but not -O2). This makes the build pass under > DEVELOPER=1 without needing a DEVOPTS=no-error. > > This can be reproduced with GCC Debian 8.3.0-6, but not e.g. with > clang 7.0.1-8+deb10u2. We've had this warning since > ee4512ed481 (trace2: create new combined trace facility, 2019-02-22). > > As noted in [2] this warning happens because the compiler doesn't > assume that errno must be non-zero after a failed syscall. Let's work > around it as suggested in that analysis. We now return -1 ourselves on > error, and save away the value of errno in a variable the caller > passes in. > > 1. > > trace2/tr2_dst.c: In function ‘tr2_dst_get_trace_fd.part.5’: > trace2/tr2_dst.c:296:10: warning: ‘fd’ may be used uninitialized in this function [-Wmaybe-uninitialized] > dst->fd = fd; > ~~~~~~~~^~~~ > trace2/tr2_dst.c:229:6: note: ‘fd’ was declared here > int fd; > ^~ > 2. https://lore.kernel.org/git/20200404142131.GA679473@coredump.intra.peff.net/ > --- > trace2/tr2_dst.c | 27 +++++++++++++++------------ > 1 file changed, 15 insertions(+), 12 deletions(-) What's the concensus if any on this topic? In any case, this needs to be signed off before it gets carved into our history. Thanks.
Hi Junio, On Fri, 7 May 2021, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > >> Otherwise I'd strongly prefer to see a word that hints that this is > >> an otherwise unneeded workaround for comiplers. Your suggested > >> title instead hints that it is wrong to assume that errno will be > >> set to non-zero after a syscall. I do not think that is the message > >> we want to send to our readers. > > > > Right, the oneline I suggested was only for the original patch, with which > > I disagreed. > > I actually do not know how your rewrite could be better, though. > > /* GCC thinks socket()/connect() might fail to set errno */ > return errno ? errno : EIO; > > If a compiler thinks errno may *not* be set, can 'errno' be reliably > used to decide if it can be returned as-is or a fallback value EIO > should be used, without triggering the same (incorrect) working in > the first place? Oh, I guess I mistook the problem for something else, then. If the problem is that `errno` is not set in case of failure, the resolution is easy (and even recommended in the manual page of `errno`): simply set it to 0 before the syscall (or in the function that relies on `errno == 0` means success). There is really no need to introduce a `saved_errno` variable (which to me would suggest that we need to save the current value of `errno` and reinstate it later, as we do sometimes e.g. when we call `close()` after noticing an error whose `errno` we need to preserve for the caller). Ciao, Dscho
On Tue, May 11, 2021 at 04:34:49PM +0200, Johannes Schindelin wrote: > On Fri, 7 May 2021, Junio C Hamano wrote: > > > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > > >> Otherwise I'd strongly prefer to see a word that hints that this is > > >> an otherwise unneeded workaround for comiplers. Your suggested > > >> title instead hints that it is wrong to assume that errno will be > > >> set to non-zero after a syscall. I do not think that is the message > > >> we want to send to our readers. > > > > > > Right, the oneline I suggested was only for the original patch, with which > > > I disagreed. > > > > I actually do not know how your rewrite could be better, though. > > > > /* GCC thinks socket()/connect() might fail to set errno */ > > return errno ? errno : EIO; > > > > If a compiler thinks errno may *not* be set, can 'errno' be reliably > > used to decide if it can be returned as-is or a fallback value EIO > > should be used, without triggering the same (incorrect) working in > > the first place? > > Oh, I guess I mistook the problem for something else, then. > > If the problem is that `errno` is not set in case of failure, the > resolution is easy (and even recommended in the manual page of `errno`): > simply set it to 0 before the syscall (or in the function that relies on > `errno == 0` means success). I don't think that is the problem. According to the standard, errno is always set to a non-zero value after a syscall failure. The problem is only that the compiler does not incorporate that special knowledge of the variable, so it generates a warning even though we can reason that the situation it describes is impossible. -Peff
Jeff King <peff@peff.net> writes: > On Tue, May 11, 2021 at 04:34:49PM +0200, Johannes Schindelin wrote: > >> On Fri, 7 May 2021, Junio C Hamano wrote: >> >> > /* GCC thinks socket()/connect() might fail to set errno */ >> > return errno ? errno : EIO; >> > >> > If a compiler thinks errno may *not* be set, can 'errno' be reliably >> > used to decide if it can be returned as-is or a fallback value EIO >> > should be used, without triggering the same (incorrect) working in >> > the first place? >> >> Oh, I guess I mistook the problem for something else, then. >> >> If the problem is that `errno` is not set in case of failure, the >> resolution is easy (and even recommended in the manual page of `errno`): >> simply set it to 0 before the syscall (or in the function that relies on >> `errno == 0` means success). > > I don't think that is the problem. According to the standard, errno is > always set to a non-zero value after a syscall failure. > > The problem is only that the compiler does not incorporate that special > knowledge of the variable, so it generates a warning even though we can > reason that the situation it describes is impossible. Yes, that is what I tried to say (i.e. if the compiler does not know errno has a defined value at certain places in our code and complain, then "return errno ? errno : EIO" would get the same warning because bases its outcome on the value of errno the compiler thinks is possibly undefined at that point), but apparently I failed to convey that clearly enough. Thanks.
On Wed, May 12, 2021 at 05:58:47AM +0900, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > On Tue, May 11, 2021 at 04:34:49PM +0200, Johannes Schindelin wrote: > > > >> On Fri, 7 May 2021, Junio C Hamano wrote: > >> > >> > /* GCC thinks socket()/connect() might fail to set errno */ > >> > return errno ? errno : EIO; > >> > > >> > If a compiler thinks errno may *not* be set, can 'errno' be reliably > >> > used to decide if it can be returned as-is or a fallback value EIO > >> > should be used, without triggering the same (incorrect) working in > >> > the first place? > >> > >> Oh, I guess I mistook the problem for something else, then. > >> > >> If the problem is that `errno` is not set in case of failure, the > >> resolution is easy (and even recommended in the manual page of `errno`): > >> simply set it to 0 before the syscall (or in the function that relies on > >> `errno == 0` means success). > > > > I don't think that is the problem. According to the standard, errno is > > always set to a non-zero value after a syscall failure. > > > > The problem is only that the compiler does not incorporate that special > > knowledge of the variable, so it generates a warning even though we can > > reason that the situation it describes is impossible. > > Yes, that is what I tried to say (i.e. if the compiler does not know > errno has a defined value at certain places in our code and > complain, then "return errno ? errno : EIO" would get the same > warning because bases its outcome on the value of errno the compiler > thinks is possibly undefined at that point), but apparently I failed > to convey that clearly enough. One thing in what you said puzzles me, though. The problem is not that the compiler thinks errno is not set at all (i.e., undefined). It simply does not know whether it is non-zero or not after the call. So switching to "return errno ? errno : EIO" does indeed work here, because the compiler now knows that the result of that return will always be non-zero. It must assume that "errno" is always defined to _something_, because it's a global (so there's no undefined behavior here). It was only it's zero/non-zero implication that let to code-paths were "fd" could be used uninitialized. -Peff
Jeff King <peff@peff.net> writes: > So switching to "return errno ? errno : EIO" does indeed work here, > because the compiler now knows that the result of that return will > always be non-zero. OK.
diff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c index ae052a07fe2..c2aba71041b 100644 --- a/trace2/tr2_dst.c +++ b/trace2/tr2_dst.c @@ -197,22 +197,25 @@ static int tr2_dst_try_path(struct tr2_dst *dst, const char *tgt_value) #define PREFIX_AF_UNIX_STREAM "af_unix:stream:" #define PREFIX_AF_UNIX_DGRAM "af_unix:dgram:" -static int tr2_dst_try_uds_connect(const char *path, int sock_type, int *out_fd) +static int tr2_dst_try_uds_connect(const char *path, int sock_type, + int *out_fd, int *saved_errno) { int fd; struct sockaddr_un sa; fd = socket(AF_UNIX, sock_type, 0); - if (fd == -1) - return errno; + if (fd == -1) { + *saved_errno = errno; + return -1; + } sa.sun_family = AF_UNIX; strlcpy(sa.sun_path, path, sizeof(sa.sun_path)); if (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) == -1) { - int e = errno; + *saved_errno = errno; close(fd); - return e; + return -1; } *out_fd = fd; @@ -227,7 +230,7 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst, { unsigned int uds_try = 0; int fd; - int e; + int saved_errno; const char *path = NULL; /* @@ -271,15 +274,15 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst, } if (uds_try & TR2_DST_UDS_TRY_STREAM) { - e = tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd); - if (!e) + if (!tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd, + &saved_errno)) goto connected; - if (e != EPROTOTYPE) + if (saved_errno != EPROTOTYPE) goto error; } if (uds_try & TR2_DST_UDS_TRY_DGRAM) { - e = tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd); - if (!e) + if (!tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd, + &saved_errno)) goto connected; } @@ -287,7 +290,7 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst, if (tr2_dst_want_warning()) warning("trace2: could not connect to socket '%s' for '%s' tracing: %s", path, tr2_sysenv_display_name(dst->sysenv_var), - strerror(e)); + strerror(saved_errno)); tr2_dst_trace_disable(dst); return 0;
Refactor tr2_dst_try_uds_connect() to avoid a gcc warning[1] that appears under -O3 (but not -O2). This makes the build pass under DEVELOPER=1 without needing a DEVOPTS=no-error. This can be reproduced with GCC Debian 8.3.0-6, but not e.g. with clang 7.0.1-8+deb10u2. We've had this warning since ee4512ed481 (trace2: create new combined trace facility, 2019-02-22). As noted in [2] this warning happens because the compiler doesn't assume that errno must be non-zero after a failed syscall. Let's work around it as suggested in that analysis. We now return -1 ourselves on error, and save away the value of errno in a variable the caller passes in. 1. trace2/tr2_dst.c: In function ‘tr2_dst_get_trace_fd.part.5’: trace2/tr2_dst.c:296:10: warning: ‘fd’ may be used uninitialized in this function [-Wmaybe-uninitialized] dst->fd = fd; ~~~~~~~~^~~~ trace2/tr2_dst.c:229:6: note: ‘fd’ was declared here int fd; ^~ 2. https://lore.kernel.org/git/20200404142131.GA679473@coredump.intra.peff.net/ --- trace2/tr2_dst.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-)