[PATCH v2] trace2: refactor to avoid gcc warning under -O3
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-05-11 13:04:36
Subsystem:
the rest · Maintainer:
Linus Torvalds
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/ (local) Signed-off-by: Ævar Arnfjörð Bjarmason <redacted> --- 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/ (local)
+ Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
+
## 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