git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
May 11, 2021, 13:04 UTC
Message-ID
<patch-1.1-782555daade-20210511T130231Z-avarab@gmail.com>
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
Previous: Junio C HamanoNext: Jeff Hostetler
Message 21 of 31 in “Fix -Wmaybe-uninitialized warnings under -O0”
  1. Fix -Wmaybe-uninitialized warnings under -O0Denton Liu, Apr 1, 2020
  2. Jeff KingApr 1, 2020
  3. Denton LiuApr 1, 2020
  4. Jeff KingApr 3, 2020
  5. Jeff KingApr 3, 2020
  6. Denton LiuApr 4, 2020
  7. Jeff KingApr 4, 2020
  8. trace2: refactor to avoid gcc warning under -O3Ævar Arnfjörð Bjarmason, May 5, 2021
  9. Junio C HamanoMay 5, 2021
  10. Jeff KingMay 5, 2021
  11. Johannes SchindelinMay 5, 2021
  12. Junio C HamanoMay 6, 2021
  13. Johannes SchindelinMay 6, 2021
  14. Junio C HamanoMay 6, 2021
  15. Johannes SchindelinMay 11, 2021
  16. Jeff KingMay 11, 2021
  17. Junio C HamanoMay 11, 2021
  18. Jeff KingMay 11, 2021
  19. Junio C HamanoMay 11, 2021
  20. Junio C HamanoMay 11, 2021
  21. trace2: refactor to avoid gcc warning under -O3Ævar Arnfjörð Bjarmason, May 11, 2021
  22. Jeff HostetlerMay 11, 2021
  23. Jeff KingMay 11, 2021
  24. Jeff KingMay 11, 2021
  25. Junio C HamanoMay 11, 2021
  26. Junio C HamanoMay 20, 2021
  27. trace2: refactor to avoid gcc warning under -O3Ævar Arnfjörð Bjarmason, May 20, 2021
  28. Jeff KingMay 20, 2021
  29. Junio C HamanoMay 20, 2021
  30. Jeff KingMay 21, 2021
  31. Denton LiuApr 1, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.