| Message ID | patch-1.1-782555daade-20210511T130231Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] trace2: refactor to avoid gcc warning under -O3 | expand |
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
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
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
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
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.
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; }
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(-)