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@xxxxxxxxxxxxxxxxxxxxxxx/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> Signed-off-by: Jeff King <peff@xxxxxxxx> --- On Thu, May 20 2021, Junio C Hamano wrote: > Jeff King <peff@xxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxx>), 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@xxxxxxxxxxxxxxxxxxxxxxx/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> + Signed-off-by: Jeff King <peff@xxxxxxxx> ## 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
