From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
"Denton Liu" <liu.denton@gmail.com>,
"Jeff Hostetler" <jeffhost@microsoft.com>,
"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH v2] trace2: refactor to avoid gcc warning under -O3
Date: Tue, 11 May 2021 15:04:28 +0200 [thread overview]
Message-ID: <patch-1.1-782555daade-20210511T130231Z-avarab@gmail.com> (raw)
In-Reply-To: <patch-1.1-87d9bcf1095-20210505T083951Z-avarab@gmail.com>
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
next prev parent reply other threads:[~2021-05-11 13:04 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-01 7:30 [PATCH] Fix -Wmaybe-uninitialized warnings under -O0 Denton Liu
[not found] ` <CAPUEspgBkmxszgBee8C9hZnEwqztf-XKEj7LB_jWVFJaJCge0w@mail.gmail.com>
2020-04-01 9:05 ` Denton Liu
2020-04-01 9:52 ` Jeff King
2020-04-01 14:06 ` Denton Liu
2020-04-03 14:04 ` Jeff King
2020-04-03 14:38 ` Jeff King
2020-04-04 12:07 ` Denton Liu
2020-04-04 14:21 ` Jeff King
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
2021-05-06 1:26 ` Junio C Hamano
2021-05-06 20:29 ` Johannes Schindelin
2021-05-06 21:10 ` Junio C Hamano
2021-05-11 14:34 ` Johannes Schindelin
2021-05-11 18:00 ` Jeff King
2021-05-11 20:58 ` Junio C Hamano
2021-05-11 21:07 ` Jeff King
2021-05-11 21:33 ` Junio C Hamano
2021-05-11 7:03 ` Junio C Hamano
2021-05-11 13:04 ` Ævar Arnfjörð Bjarmason [this message]
2021-05-11 16:40 ` [PATCH v2] " Jeff Hostetler
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
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
2021-05-21 9:34 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=patch-1.1-782555daade-20210511T130231Z-avarab@gmail.com \
--to=avarab@gmail.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jeffhost@microsoft.com \
--cc=liu.denton@gmail.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.