* Re: [PATCH] trace2: refactor to avoid gcc warning under -O3
2021-05-05 8:40 ` [PATCH] trace2: refactor to avoid gcc warning under -O3 Ævar Arnfjörð Bjarmason
@ 2021-05-05 9:47 ` Junio C Hamano
2021-05-05 13:34 ` Jeff King
2021-05-05 14:38 ` Johannes Schindelin
` (2 subsequent siblings)
3 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2021-05-05 9:47 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, Denton Liu, Jeff Hostetler
Æ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;
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH] trace2: refactor to avoid gcc warning under -O3
2021-05-05 9:47 ` Junio C Hamano
@ 2021-05-05 13:34 ` Jeff King
0 siblings, 0 replies; 31+ messages in thread
From: Jeff King @ 2021-05-05 13:34 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ævar Arnfjörð Bjarmason, git, Denton Liu,
Jeff Hostetler
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] trace2: refactor to avoid gcc warning under -O3
2021-05-05 8:40 ` [PATCH] trace2: refactor to avoid gcc warning under -O3 Ævar Arnfjörð Bjarmason
2021-05-05 9:47 ` Junio C Hamano
@ 2021-05-05 14:38 ` Johannes Schindelin
2021-05-06 1:26 ` Junio C Hamano
2021-05-11 7:03 ` Junio C Hamano
2021-05-11 13:04 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
3 siblings, 1 reply; 31+ messages in thread
From: Johannes Schindelin @ 2021-05-05 14:38 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Jeff King, Denton Liu, Jeff Hostetler
[-- Attachment #1: Type: text/plain, Size: 4633 bytes --]
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
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH] trace2: refactor to avoid gcc warning under -O3
2021-05-05 14:38 ` Johannes Schindelin
@ 2021-05-06 1:26 ` Junio C Hamano
2021-05-06 20:29 ` Johannes Schindelin
0 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2021-05-06 1:26 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Ævar Arnfjörð Bjarmason, git, Jeff King,
Denton Liu, Jeff Hostetler
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.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] trace2: refactor to avoid gcc warning under -O3
2021-05-06 1:26 ` Junio C Hamano
@ 2021-05-06 20:29 ` Johannes Schindelin
2021-05-06 21:10 ` Junio C Hamano
0 siblings, 1 reply; 31+ messages in thread
From: Johannes Schindelin @ 2021-05-06 20:29 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ævar Arnfjörð Bjarmason, git, Jeff King,
Denton Liu, Jeff Hostetler
[-- Attachment #1: Type: text/plain, Size: 1811 bytes --]
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] trace2: refactor to avoid gcc warning under -O3
2021-05-06 20:29 ` Johannes Schindelin
@ 2021-05-06 21:10 ` Junio C Hamano
2021-05-11 14:34 ` Johannes Schindelin
0 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2021-05-06 21:10 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Ævar Arnfjörð Bjarmason, git, Jeff King,
Denton Liu, Jeff Hostetler
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?
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] trace2: refactor to avoid gcc warning under -O3
2021-05-06 21:10 ` Junio C Hamano
@ 2021-05-11 14:34 ` Johannes Schindelin
2021-05-11 18:00 ` Jeff King
0 siblings, 1 reply; 31+ messages in thread
From: Johannes Schindelin @ 2021-05-11 14:34 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ævar Arnfjörð Bjarmason, git, Jeff King,
Denton Liu, Jeff Hostetler
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] trace2: refactor to avoid gcc warning under -O3
2021-05-11 14:34 ` Johannes Schindelin
@ 2021-05-11 18:00 ` Jeff King
2021-05-11 20:58 ` Junio C Hamano
0 siblings, 1 reply; 31+ messages in thread
From: Jeff King @ 2021-05-11 18:00 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason, git,
Denton Liu, Jeff Hostetler
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] trace2: refactor to avoid gcc warning under -O3
2021-05-11 18:00 ` Jeff King
@ 2021-05-11 20:58 ` Junio C Hamano
2021-05-11 21:07 ` Jeff King
0 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2021-05-11 20:58 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin, Ævar Arnfjörð Bjarmason, git,
Denton Liu, Jeff Hostetler
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.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] trace2: refactor to avoid gcc warning under -O3
2021-05-11 20:58 ` Junio C Hamano
@ 2021-05-11 21:07 ` Jeff King
2021-05-11 21:33 ` Junio C Hamano
0 siblings, 1 reply; 31+ messages in thread
From: Jeff King @ 2021-05-11 21:07 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Ævar Arnfjörð Bjarmason, git,
Denton Liu, Jeff Hostetler
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] trace2: refactor to avoid gcc warning under -O3
2021-05-05 8:40 ` [PATCH] trace2: refactor to avoid gcc warning under -O3 Ævar Arnfjörð Bjarmason
2021-05-05 9:47 ` Junio C Hamano
2021-05-05 14:38 ` Johannes Schindelin
@ 2021-05-11 7:03 ` Junio C Hamano
2021-05-11 13:04 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
3 siblings, 0 replies; 31+ messages in thread
From: Junio C Hamano @ 2021-05-11 7:03 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, Denton Liu, Jeff Hostetler
Æ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.
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v2] trace2: refactor to avoid gcc warning under -O3
2021-05-05 8:40 ` [PATCH] trace2: refactor to avoid gcc warning under -O3 Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-05-11 7:03 ` Junio C Hamano
@ 2021-05-11 13:04 ` Ævar Arnfjörð Bjarmason
2021-05-11 16:40 ` Jeff Hostetler
2021-05-11 17:54 ` Jeff King
3 siblings, 2 replies; 31+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-11 13:04 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Denton Liu, Jeff Hostetler,
Johannes Schindelin, Ævar Arnfjörð Bjarmason
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.
See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61846 for a related
bug report against GCC.
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/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
On Tue, May 11 2021, Junio C Hamano wrote:
> What's the concensus if any on this topic?
Having read Johannes's comments I think it's still most readable to
just return -1 unconditionally. The resulting code isn't weird, I'd
argue that it's a better pattern to save away errno like this, but the
commit messages notes that we're working around a GCC bug.
> In any case, this needs to be signed off before it gets carved into
> our history.
Done, and also changed the variable name to minimize the size of the
diff. A shorter name allowed for less re-flowing of lines.
Range-diff against v1:
1: 87d9bcf1095 ! 1: 782555daade trace2: refactor to avoid gcc warning under -O3
@@ Commit message
error, and save away the value of errno in a variable the caller
passes in.
+ See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61846 for a related
+ bug report against GCC.
+
1.
trace2/tr2_dst.c: In function ‘tr2_dst_get_trace_fd.part.5’:
@@ Commit message
^~
2. https://lore.kernel.org/git/20200404142131.GA679473@coredump.intra.peff.net/
+ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
+
## trace2/tr2_dst.c ##
@@ trace2/tr2_dst.c: static int tr2_dst_try_path(struct tr2_dst *dst, const char *tgt_value)
#define PREFIX_AF_UNIX_STREAM "af_unix:stream:"
@@ trace2/tr2_dst.c: static int tr2_dst_try_path(struct tr2_dst *dst, const char *t
-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 *out_fd, int *e)
{
int fd;
struct sockaddr_un sa;
@@ trace2/tr2_dst.c: static int tr2_dst_try_path(struct tr2_dst *dst, const char *t
- if (fd == -1)
- return errno;
+ if (fd == -1) {
-+ *saved_errno = errno;
++ *e = errno;
+ return -1;
+ }
@@ trace2/tr2_dst.c: static int tr2_dst_try_path(struct tr2_dst *dst, const char *t
if (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) == -1) {
- int e = errno;
-+ *saved_errno = errno;
++ *e = errno;
close(fd);
- return e;
+ return -1;
}
*out_fd = fd;
-@@ trace2/tr2_dst.c: 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;
-
- /*
@@ trace2/tr2_dst.c: 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))
++ if (!tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd, &e))
goto connected;
-- if (e != EPROTOTYPE)
-+ if (saved_errno != EPROTOTYPE)
+ if (e != 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))
++ if (!tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd, &e))
goto connected;
}
-@@ trace2/tr2_dst.c: 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;
trace2/tr2_dst.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c
index ae052a07fe2..a44fe6b73e0 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 *e)
{
int fd;
struct sockaddr_un sa;
fd = socket(AF_UNIX, sock_type, 0);
- if (fd == -1)
- return errno;
+ if (fd == -1) {
+ *e = 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;
+ *e = errno;
close(fd);
- return e;
+ return -1;
}
*out_fd = fd;
@@ -271,15 +274,13 @@ 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, &e))
goto connected;
if (e != 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, &e))
goto connected;
}
--
2.31.1.909.g789bb6d90e
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH v2] trace2: refactor to avoid gcc warning under -O3
2021-05-11 13:04 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
@ 2021-05-11 16:40 ` Jeff Hostetler
2021-05-11 17:54 ` Jeff King
1 sibling, 0 replies; 31+ messages in thread
From: Jeff Hostetler @ 2021-05-11 16:40 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Junio C Hamano, Jeff King, Denton Liu, Jeff Hostetler,
Johannes Schindelin
On 5/11/21 9:04 AM, Æ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.
>
...
I suppose if we really need to paper around a compiler bug,
then this fix LGTM.
Thanks,
Jeff
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] trace2: refactor to avoid gcc warning under -O3
2021-05-11 13:04 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
2021-05-11 16:40 ` Jeff Hostetler
@ 2021-05-11 17:54 ` Jeff King
2021-05-11 18:08 ` Jeff King
1 sibling, 1 reply; 31+ messages in thread
From: Jeff King @ 2021-05-11 17:54 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Denton Liu, Jeff Hostetler,
Johannes Schindelin
On Tue, May 11, 2021 at 03:04:28PM +0200, Æ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.
Thanks, I think this describes the problem nicely.
> On Tue, May 11 2021, Junio C Hamano wrote:
>
> > What's the concensus if any on this topic?
>
> Having read Johannes's comments I think it's still most readable to
> just return -1 unconditionally. The resulting code isn't weird, I'd
> argue that it's a better pattern to save away errno like this, but the
> commit messages notes that we're working around a GCC bug.
Agreed. Returning "-1" is the usual style in our code base. And while I
think the original code is correct, I did have to go double-check the C
standard to confirm that it's so.
I slightly disagree with the notion that gcc's behavior is a bug. It
seems more like a lack of feature (it does not have any way to annotate
this special property of errno). But that is neither here nor there for
your patch, and really a matter of opinion. :)
> > In any case, this needs to be signed off before it gets carved into
> > our history.
>
> Done, and also changed the variable name to minimize the size of the
> diff. A shorter name allowed for less re-flowing of lines.
It's quite short. I'm OK with it for a static-local function with few
callers like this, though.
-Peff
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] trace2: refactor to avoid gcc warning under -O3
2021-05-11 17:54 ` Jeff King
@ 2021-05-11 18:08 ` Jeff King
2021-05-11 21:09 ` Junio C Hamano
2021-05-20 0:20 ` Junio C Hamano
0 siblings, 2 replies; 31+ messages in thread
From: Jeff King @ 2021-05-11 18:08 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Denton Liu, Jeff Hostetler,
Johannes Schindelin
On Tue, May 11, 2021 at 01:54:19PM -0400, Jeff King wrote:
> > > In any case, this needs to be signed off before it gets carved into
> > > our history.
> >
> > Done, and also changed the variable name to minimize the size of the
> > diff. A shorter name allowed for less re-flowing of lines.
>
> It's quite short. I'm OK with it for a static-local function with few
> callers like this, though.
Actually, I just noticed that you did not introduce "e" in the caller.
So it is not even a new name, and you are just following convention.
I also wondered briefly why we needed the out-parameter at all, and not
just letting the caller look at errno. The answer is that we need to
preserve it across the close() call. The more usual thing in our code
base _would_ be to use saved_errno, but not have it as an out-parameter.
I.e.:
diff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c
index ae052a07fe..bda283e7f4 100644
--- a/trace2/tr2_dst.c
+++ b/trace2/tr2_dst.c
@@ -204,15 +204,16 @@ static int tr2_dst_try_uds_connect(const char *path, int sock_type, int *out_fd)
fd = socket(AF_UNIX, sock_type, 0);
if (fd == -1)
- return 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;
+ int saved_errno = errno;
close(fd);
- return e;
+ errno = saved_errno;
+ return -1;
}
*out_fd = fd;
@@ -227,7 +228,6 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,
{
unsigned int uds_try = 0;
int fd;
- int e;
const char *path = NULL;
/*
@@ -271,23 +271,21 @@ 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))
goto connected;
- if (e != EPROTOTYPE)
+ if (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))
goto connected;
}
error:
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(errno));
tr2_dst_trace_disable(dst);
return 0;
I do prefer that approach, since I think it's more idiomatic for our
code base, but for the sake of wrapping up this simple fix which has
been discussed much more than I think it deserves, I am OK with either.
:)
(I also found it interesting that the "error" goto in the caller only
has one source. I think the code would be easier to reason about if it
were inlined, but I'm happy to stop here for now).
-Peff
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH v2] trace2: refactor to avoid gcc warning under -O3
2021-05-11 18:08 ` Jeff King
@ 2021-05-11 21:09 ` Junio C Hamano
2021-05-20 0:20 ` Junio C Hamano
1 sibling, 0 replies; 31+ messages in thread
From: Junio C Hamano @ 2021-05-11 21:09 UTC (permalink / raw)
To: Jeff King
Cc: Ævar Arnfjörð Bjarmason, git, Denton Liu,
Jeff Hostetler, Johannes Schindelin
Jeff King <peff@peff.net> writes:
> I also wondered briefly why we needed the out-parameter at all, and not
> just letting the caller look at errno. The answer is that we need to
> preserve it across the close() call. The more usual thing in our code
> base _would_ be to use saved_errno, but not have it as an out-parameter.
>
> I.e.:
>
> diff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c
> index ae052a07fe..bda283e7f4 100644
> --- a/trace2/tr2_dst.c
> +++ b/trace2/tr2_dst.c
> @@ -204,15 +204,16 @@ static int tr2_dst_try_uds_connect(const char *path, int sock_type, int *out_fd)
>
> fd = socket(AF_UNIX, sock_type, 0);
> if (fd == -1)
> - return 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;
> + int saved_errno = errno;
> close(fd);
> - return e;
> + errno = saved_errno;
> + return -1;
> }
>
> *out_fd = fd;
> @@ -227,7 +228,6 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,
> {
> unsigned int uds_try = 0;
> int fd;
> - int e;
> const char *path = NULL;
>
> /*
> @@ -271,23 +271,21 @@ 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))
> goto connected;
> - if (e != EPROTOTYPE)
> + if (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))
> goto connected;
> }
>
> error:
> 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(errno));
>
> tr2_dst_trace_disable(dst);
> return 0;
>
>
> I do prefer that approach, since I think it's more idiomatic for our
> code base, but for the sake of wrapping up this simple fix which has
> been discussed much more than I think it deserves, I am OK with either.
> :)
Yeah, the above looks nicer to me too.
>
> (I also found it interesting that the "error" goto in the caller only
> has one source. I think the code would be easier to reason about if it
> were inlined, but I'm happy to stop here for now).
>
> -Peff
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH v2] trace2: refactor to avoid gcc warning under -O3
2021-05-11 18:08 ` Jeff King
2021-05-11 21:09 ` Junio C Hamano
@ 2021-05-20 0:20 ` Junio C Hamano
2021-05-20 11:05 ` [PATCH v3] " Ævar Arnfjörð Bjarmason
1 sibling, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2021-05-20 0:20 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, Jeff King
Cc: git, Denton Liu, Jeff Hostetler, Johannes Schindelin
Jeff King <peff@peff.net> writes:
> I also wondered briefly why we needed the out-parameter at all, and not
> just letting the caller look at errno. The answer is that we need to
> preserve it across the close() call. The more usual thing in our code
> base _would_ be to use saved_errno, but not have it as an out-parameter.
>
> I.e.:
>
> diff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c
> index ae052a07fe..bda283e7f4 100644
> --- a/trace2/tr2_dst.c
> +++ b/trace2/tr2_dst.c
> @@ -204,15 +204,16 @@ static int tr2_dst_try_uds_connect(const char *path, int sock_type, int *out_fd)
>
> fd = socket(AF_UNIX, sock_type, 0);
> if (fd == -1)
> - return 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;
> + int saved_errno = errno;
> close(fd);
> - return e;
> + errno = saved_errno;
> + return -1;
> }
>
> ...
>
> I do prefer that approach, since I think it's more idiomatic for our
> code base, but for the sake of wrapping up this simple fix which has
> been discussed much more than I think it deserves, I am OK with either.
> :)
I think this alternative is more readable as well.
I'll mark the topic to be "Expecting a reroll" in the what's cooking
report.
Thanks.
^ permalink raw reply [flat|nested] 31+ messages in thread* [PATCH v3] trace2: refactor to avoid gcc warning under -O3
2021-05-20 0:20 ` Junio C Hamano
@ 2021-05-20 11:05 ` Ævar Arnfjörð Bjarmason
2021-05-20 13:13 ` Jeff King
0 siblings, 1 reply; 31+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-20 11:05 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Denton Liu, Jeff Hostetler,
Johannes Schindelin, Ævar Arnfjörð Bjarmason
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 by using the well-established "saved_errno" pattern,
along with returning -1 ourselves instead of "errno". The caller can
thus rely on our "errno" on failure.
See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61846 for a related
bug report against GCC.
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/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
On Thu, May 20 2021, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
>> I also wondered briefly why we needed the out-parameter at all, and not
>> just letting the caller look at errno. The answer is that we need to
>> preserve it across the close() call. The more usual thing in our code
>> base would be to use saved_errno, but not have it as an out-parameter.
>> [...]
> I think this alternative is more readable as well.
>
> I'll mark the topic to be "Expecting a reroll" in the what's cooking
> report.
>
> Thanks.
Here's that re-roll, thanks both.
The patch is entirely Jeff's at this point (from
<YJrIMbr6VkYGQMfs@coredump.intra.peff.net>), with my amended commit
message. So I added his SOB per his recent-ish ML "every patch of mine
can be assumed to have my SOB" message.
Range-diff against v2:
1: 782555daad ! 1: 2e41e3e4cb trace2: refactor to avoid gcc warning under -O3
@@ Commit message
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.
+ assume that errno must be non-zero after a failed syscall.
+
+ Let's work around by using the well-established "saved_errno" pattern,
+ along with returning -1 ourselves instead of "errno". The caller can
+ thus rely on our "errno" on failure.
See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61846 for a related
bug report against GCC.
@@ Commit message
2. https://lore.kernel.org/git/20200404142131.GA679473@coredump.intra.peff.net/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
+ Signed-off-by: Jeff King <peff@peff.net>
## trace2/tr2_dst.c ##
-@@ trace2/tr2_dst.c: 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 *e)
- {
- int fd;
- struct sockaddr_un sa;
+@@ trace2/tr2_dst.c: static int tr2_dst_try_uds_connect(const char *path, int sock_type, int *out_fd)
fd = socket(AF_UNIX, sock_type, 0);
-- if (fd == -1)
+ if (fd == -1)
- return errno;
-+ if (fd == -1) {
-+ *e = 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;
-+ *e = errno;
++ int saved_errno = errno;
close(fd);
- return e;
++ errno = saved_errno;
+ return -1;
}
*out_fd = fd;
+@@ trace2/tr2_dst.c: static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,
+ {
+ unsigned int uds_try = 0;
+ int fd;
+- int e;
+ const char *path = NULL;
+
+ /*
@@ trace2/tr2_dst.c: 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, &e))
++ if (!tr2_dst_try_uds_connect(path, SOCK_STREAM, &fd))
goto connected;
- if (e != EPROTOTYPE)
+- if (e != EPROTOTYPE)
++ if (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, &e))
++ if (!tr2_dst_try_uds_connect(path, SOCK_DGRAM, &fd))
goto connected;
}
+@@ trace2/tr2_dst.c: 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(errno));
+
+ tr2_dst_trace_disable(dst);
+ return 0;
trace2/tr2_dst.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c
index ae052a07fe..bda283e7f4 100644
--- a/trace2/tr2_dst.c
+++ b/trace2/tr2_dst.c
@@ -204,15 +204,16 @@ static int tr2_dst_try_uds_connect(const char *path, int sock_type, int *out_fd)
fd = socket(AF_UNIX, sock_type, 0);
if (fd == -1)
- return 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;
+ int saved_errno = errno;
close(fd);
- return e;
+ errno = saved_errno;
+ return -1;
}
*out_fd = fd;
@@ -227,7 +228,6 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,
{
unsigned int uds_try = 0;
int fd;
- int e;
const char *path = NULL;
/*
@@ -271,15 +271,13 @@ 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))
goto connected;
- if (e != EPROTOTYPE)
+ if (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))
goto connected;
}
@@ -287,7 +285,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(errno));
tr2_dst_trace_disable(dst);
return 0;
--
2.32.0.rc0.406.g73369325f8d
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH v3] trace2: refactor to avoid gcc warning under -O3
2021-05-20 11:05 ` [PATCH v3] " Ævar Arnfjörð Bjarmason
@ 2021-05-20 13:13 ` Jeff King
2021-05-20 22:08 ` Junio C Hamano
0 siblings, 1 reply; 31+ messages in thread
From: Jeff King @ 2021-05-20 13:13 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Denton Liu, Jeff Hostetler,
Johannes Schindelin
On Thu, May 20, 2021 at 01:05:45PM +0200, Ævar Arnfjörð Bjarmason wrote:
> >> I also wondered briefly why we needed the out-parameter at all, and not
> >> just letting the caller look at errno. The answer is that we need to
> >> preserve it across the close() call. The more usual thing in our code
> >> base would be to use saved_errno, but not have it as an out-parameter.
> >> [...]
> > I think this alternative is more readable as well.
> >
> > I'll mark the topic to be "Expecting a reroll" in the what's cooking
> > report.
> >
> > Thanks.
>
> Here's that re-roll, thanks both.
Thanks, this looks OK to me. There is one minor nit (introduced by me!)
below, but I'm not sure if we should care or not.
> The patch is entirely Jeff's at this point (from
> <YJrIMbr6VkYGQMfs@coredump.intra.peff.net>), with my amended commit
> message. So I added his SOB per his recent-ish ML "every patch of mine
> can be assumed to have my SOB" message.
So I have definitely said that, and I stand by it. But as a matter of
project policy, I think we probably shouldn't consider that "enough".
The point of the DSO is to make an affirmative statement about a
particular patch. So probably my blanket statement should be taken more
as "I will almost definitely add my signoff if you ask me". :)
And of course in the case of this particular patch, it is very much:
Signed-off-by: Jeff King <peff@peff.net>
> @@ -271,15 +271,13 @@ 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))
> goto connected;
> - if (e != EPROTOTYPE)
> + if (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))
> goto connected;
> }
>
> @@ -287,7 +285,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(errno));
We expect the value of errno to persist across tr2_dst_want_warning()
and tr2_sysenv_display_name() here. The former may call getenv() and
atoi(). I think that's probably fine, but if we wanted to be really
paranoid, we'd have to preserve errno manually here, too.
-Peff
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH v3] trace2: refactor to avoid gcc warning under -O3
2021-05-20 13:13 ` Jeff King
@ 2021-05-20 22:08 ` Junio C Hamano
2021-05-21 9:34 ` Jeff King
0 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2021-05-20 22:08 UTC (permalink / raw)
To: Jeff King
Cc: Ævar Arnfjörð Bjarmason, git, Denton Liu,
Jeff Hostetler, Johannes Schindelin
Jeff King <peff@peff.net> writes:
>> @@ -287,7 +285,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(errno));
>
> We expect the value of errno to persist across tr2_dst_want_warning()
> and tr2_sysenv_display_name() here. The former may call getenv() and
> atoi(). I think that's probably fine, but if we wanted to be really
> paranoid, we'd have to preserve errno manually here, too.
Being "really paranoid" consistently within the file would mean a
change like the attached, I would think, on top of what was posted.
Or tr2_dst_want_warning() and tr2_sysenv_display_name() can be
taught to preserve errno like tr2_dst_dry_uds_connect() was taught
to do so by the patch under discussion, which may reduce the amount
of apparent change, but constantly moving the contents of errno
around just in case we later might want to use its value feels
dirty.
I dunno.
trace2/tr2_dst.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git c/trace2/tr2_dst.c w/trace2/tr2_dst.c
index 0031476350..f740a0a076 100644
--- c/trace2/tr2_dst.c
+++ w/trace2/tr2_dst.c
@@ -62,11 +62,13 @@ static int tr2_dst_try_auto_path(struct tr2_dst *dst, const char *tgt_prefix)
}
if (fd == -1) {
+ int saved_errno = errno;
+
if (tr2_dst_want_warning())
warning("trace2: could not open '%.*s' for '%s' tracing: %s",
(int) base_path_len, path.buf,
tr2_sysenv_display_name(dst->sysenv_var),
- strerror(errno));
+ strerror(saved_errno));
tr2_dst_trace_disable(dst);
strbuf_release(&path);
@@ -86,6 +88,8 @@ static int tr2_dst_try_path(struct tr2_dst *dst, const char *tgt_value)
{
int fd = open(tgt_value, O_WRONLY | O_APPEND | O_CREAT, 0666);
if (fd == -1) {
+ int saved_errno = errno;
+
if (tr2_dst_want_warning())
warning("trace2: could not open '%s' for '%s' tracing: %s",
tgt_value,
@@ -140,6 +144,7 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,
unsigned int uds_try = 0;
int fd;
const char *path = NULL;
+ int saved_errno;
/*
* Allow "af_unix:[<type>:]<absolute_path>"
@@ -193,10 +198,11 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,
}
error:
+ saved_errno = errno;
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(errno));
+ strerror(saved_errno));
tr2_dst_trace_disable(dst);
return 0;
@@ -276,6 +282,7 @@ int tr2_dst_trace_want(struct tr2_dst *dst)
void tr2_dst_write_line(struct tr2_dst *dst, struct strbuf *buf_line)
{
int fd = tr2_dst_get_trace_fd(dst);
+ int saved_errno;
strbuf_complete_line(buf_line); /* ensure final NL on buffer */
@@ -297,9 +304,10 @@ void tr2_dst_write_line(struct tr2_dst *dst, struct strbuf *buf_line)
if (write(fd, buf_line->buf, buf_line->len) >= 0)
return;
+ saved_errno = errno;
if (tr2_dst_want_warning())
warning("unable to write trace to '%s': %s",
tr2_sysenv_display_name(dst->sysenv_var),
- strerror(errno));
+ strerror(saved_errno));
tr2_dst_trace_disable(dst);
}
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH v3] trace2: refactor to avoid gcc warning under -O3
2021-05-20 22:08 ` Junio C Hamano
@ 2021-05-21 9:34 ` Jeff King
0 siblings, 0 replies; 31+ messages in thread
From: Jeff King @ 2021-05-21 9:34 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ævar Arnfjörð Bjarmason, git, Denton Liu,
Jeff Hostetler, Johannes Schindelin
On Fri, May 21, 2021 at 07:08:11AM +0900, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> >> @@ -287,7 +285,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(errno));
> >
> > We expect the value of errno to persist across tr2_dst_want_warning()
> > and tr2_sysenv_display_name() here. The former may call getenv() and
> > atoi(). I think that's probably fine, but if we wanted to be really
> > paranoid, we'd have to preserve errno manually here, too.
>
> Being "really paranoid" consistently within the file would mean a
> change like the attached, I would think, on top of what was posted.
>
> Or tr2_dst_want_warning() and tr2_sysenv_display_name() can be
> taught to preserve errno like tr2_dst_dry_uds_connect() was taught
> to do so by the patch under discussion, which may reduce the amount
> of apparent change, but constantly moving the contents of errno
> around just in case we later might want to use its value feels
> dirty.
>
> I dunno.
>
> trace2/tr2_dst.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
Ah, yeah. I didn't look to see if there were existing cases of the same
thing.
I could go either way on this kind of saved_errno thing in general (the
tr2 functions called in between are really quite unlikely to set errno
(I am not even sure if getenv() and atoi() can, so this really might
just be future-proofing in case those tr2 functions get more
complicated).
But seeing that there are other cases of the same, I definitely think it
is not something that should be in Ævar's patch. It is a cleanup we
could do on top if we cared to.
-Peff
^ permalink raw reply [flat|nested] 31+ messages in thread