[PATCH v3] trace2: refactor to avoid gcc warning under -O3

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/ (local)

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Jeff King <redacted>
---

On Thu, May 20 2021, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

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
[ref]), 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/ (local)
     
         Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
    +    Signed-off-by: Jeff King [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:"
    - #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