patchFix -Wmaybe-uninitialized warnings under -O0

> When compiling Git under -O0, gcc (Arch Linux 9.3.0-1) 9.3.0 produces
> many -Wmaybe-uninitialized warnings. These are false positives since
> when Git is compiled under -O2, gcc is smart enough to see that the
> code paths that use these variables all initialise them beforehand.
> Nonetheless, these warnings block the compilation process when
> DEVELOPER=1 is enabled (which enables -Werror).
> 
> Fix these warnings by initializing these variables with dummy values (0,
> -1 or NULL as appropriate).

> On Wed, Apr 01, 2020 at 03:30:16AM -0400, Denton Liu wrote:
> 
> > When compiling Git under -O0, gcc (Arch Linux 9.3.0-1) 9.3.0 produces
> > many -Wmaybe-uninitialized warnings. These are false positives since
> > when Git is compiled under -O2, gcc is smart enough to see that the
> > code paths that use these variables all initialise them beforehand.
> > Nonetheless, these warnings block the compilation process when
> > DEVELOPER=1 is enabled (which enables -Werror).
> > 
> > Fix these warnings by initializing these variables with dummy values (0,
> > -1 or NULL as appropriate).
> 
> Hmph. I almost always compile with -O0 and have been using gcc 9.3.0
> since it was packaged for Debian a few weeks ago, but I don't see any of
> these warnings.
> 
> The current version in Debian unstable is 9.3.0-8, which picks up some
> extra patches from the upstream gcc-9 branch. But even if I download a
> snapshot of the original 9.3.0 release, it builds fine.
> 
> So why does your version behave differently? And if this is a temporary
> state for a buggy version of gcc (that may be fixed in the next point
> release), is it worth changing our source code to appease it?

> > So why does your version behave differently? And if this is a temporary
> > state for a buggy version of gcc (that may be fixed in the next point
> > release), is it worth changing our source code to appease it?
> 
> A correction to the earlier message... It seems like I wasn't reporting
> the correct settings. I was actually compiling with -Og, not -O0
> (whoops!).
> 
> I tested it with gcc-8 and it seems like it also reports the same
> problem. Also, -O1 reports warnings as well.

> On Wed, Apr 01, 2020 at 10:06:43AM -0400, Denton Liu wrote:
> 
> > > So why does your version behave differently? And if this is a temporary
> > > state for a buggy version of gcc (that may be fixed in the next point
> > > release), is it worth changing our source code to appease it?
> > 
> > A correction to the earlier message... It seems like I wasn't reporting
> > the correct settings. I was actually compiling with -Og, not -O0
> > (whoops!).
> > 
> > I tested it with gcc-8 and it seems like it also reports the same
> > problem. Also, -O1 reports warnings as well.
> 
> Ah, OK, I can reproduce easily with -Og (up through gcc-10). Most of
> them don't trigger with -O1; just the one in ref-filter.c.
> 
> That one's interesting. We have:
> 
>   int ret = 0;
>   ...
>   if (...)
>          ...
>   else
>          ret = for_each_fullref_in_pattern(...);
>   ...
>   return ret;
> 
> So we'd either have 0 or an assigned return. But the bug is actually in
> for_each_fullref_in_pattern(), which does this:
> 
>   int ret; /* uninitialized! */
> 
>   /* a bunch of early return conditionals */
>   if (...)
>     return ...;
> 
>   for_each_string_list_item(...) {
>     ret = for_each_fullref_in(...);

>   }
> 
>   return ret;
> 
> but that will return an uninitialized value when there are no patterns.
> I doubt we have such a case, but that may explain why -O0 does not
> complain (it assumes "in_pattern" will return a useful value) and -O2
> does not (it is able to figure out that it always does), but -O1 only
> inlines part of it.
> 
> Curiously, -Og _does_ find the correct function.
> 
> Your patch silences it, but is it doing the right thing? It sets "ret =
> 0", but we haven't actually iterated anything. Should it be an error
> instead?

> >   for_each_string_list_item(...) {
> >     ret = for_each_fullref_in(...);
> 
> This loop is missing a bit of important context:
> 
> 	if (ret)
> 		break;
> 

> > Your patch silences it, but is it doing the right thing? It sets "ret =
> > 0", but we haven't actually iterated anything. Should it be an error
> > instead?
> 
> I understood the semantics of for_each_fullref_in_pattern() (in the
> non-early return case) to be "for each item, iterate and compute a
> value; if that value is non-zero return it. If not found, return zero".
> The missing context above is important since, without it, we lose the
> semantics.
> 
> If I'm understanding the above correctly then, studying this function in
> a vacuum, it would make sense to assign a default value of 0 since if
> the for operates on an empty list, the function should consider the item
> vacuously not found and we should return 0 as a result.

> 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.
>
> 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/
> ---
>  trace2/tr2_dst.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)

> diff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c
> index ae052a07fe2..c2aba71041b 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 *saved_errno)
>  {
>  	int fd;
>  	struct sockaddr_un sa;
>  
>  	fd = socket(AF_UNIX, sock_type, 0);
> -	if (fd == -1)
> -		return errno;
> +	if (fd == -1) {
> +		*saved_errno = 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;
> +		*saved_errno = errno;
>  		close(fd);
> -		return e;
> +		return -1;
>  	}
>  
>  	*out_fd = fd;
> @@ -227,7 +230,7 @@ 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;
>  
>  	/*
> @@ -271,15 +274,15 @@ 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))
>  			goto connected;
> -		if (e != EPROTOTYPE)
> +		if (saved_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,
> +					     &saved_errno))
>  			goto connected;
>  	}
>  
> @@ -287,7 +290,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(saved_errno));
>  
>  	tr2_dst_trace_disable(dst);
>  	return 0;

> The patch makes sense to me, modulo that the way the variable
> "saved_errno" introduced by this patch is used and the way a
> variable with that name is typically used in our codebase are at
> odds.  I.e. we typically call a variable "saved_errno" when it is
> used in this pattern:
> 
>     if (a_syscall_whose_error_condition_we_care_about()) {
> 	int saved_errno = errno;
> 	perform_some_cleanup_operation_that_might_clobber_errno();
> 	return error_errno(..., saved_errno);
> 	/*
> 	 * or
> 	 * errno = saved_errno;
> 	 * return -1;
> 	 * and let the caller handle 'errno'
> 	 */
>     }
> 
> But since I do not think of a better name for this new variable that
> is not exactly used like so, let's queue it as-is.

> 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.

>
> 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/
> ---
>  trace2/tr2_dst.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c
> index ae052a07fe2..c2aba71041b 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 *saved_errno)
>  {
>  	int fd;
>  	struct sockaddr_un sa;
>
>  	fd = socket(AF_UNIX, sock_type, 0);
> -	if (fd == -1)
> -		return errno;
> +	if (fd == -1) {
> +		*saved_errno = 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;
> +		*saved_errno = errno;
>  		close(fd);
> -		return e;
> +		return -1;

>  	}
>
>  	*out_fd = fd;
> @@ -227,7 +230,7 @@ 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;
>
>  	/*
> @@ -271,15 +274,15 @@ 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))
>  			goto connected;
> -		if (e != EPROTOTYPE)
> +		if (saved_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,
> +					     &saved_errno))
>  			goto connected;
>  	}
>
> @@ -287,7 +290,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(saved_errno));
>
>  	tr2_dst_trace_disable(dst);
>  	return 0;
> --
> 2.31.1.745.g2af7c6593ce
>
>

> Hi Ævar,
>
> On Wed, 5 May 2021, Ævar Arnfjörð Bjarmason wrote:
>
>> 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.
>
> It would probably be a lot nicer if you lead with this insight. I could
> imagine, for example, that a oneline like this would be much more helpful
> to any reader:
>
> 	trace2: do not assume errno != 0 after a failed syscall

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > Hi Ævar,
> >
> > On Wed, 5 May 2021, Ævar Arnfjörð Bjarmason wrote:
> >
> >> 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.
> >
> > It would probably be a lot nicer if you lead with this insight. I could
> > imagine, for example, that a oneline like this would be much more helpful
> > to any reader:
> >
> > 	trace2: do not assume errno != 0 after a failed syscall
>
> But that is misleading.
>
> My understanding is that this patch is about working around
> compilers that do not know that a failed syscall means errno would
> be set to non-zero.  Am I mistaken?
>
> Otherwise I'd strongly prefer to see a word that hints that this is
> an otherwise unneeded workaround for comiplers.  Your suggested
> title instead hints that it is wrong to assume that errno will be
> set to non-zero after a syscall.  I do not think that is the message
> we want to send to our readers.

>> Otherwise I'd strongly prefer to see a word that hints that this is
>> an otherwise unneeded workaround for comiplers.  Your suggested
>> title instead hints that it is wrong to assume that errno will be
>> set to non-zero after a syscall.  I do not think that is the message
>> we want to send to our readers.
>
> Right, the oneline I suggested was only for the original patch, with which
> I disagreed.

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> Otherwise I'd strongly prefer to see a word that hints that this is
> >> an otherwise unneeded workaround for comiplers.  Your suggested
> >> title instead hints that it is wrong to assume that errno will be
> >> set to non-zero after a syscall.  I do not think that is the message
> >> we want to send to our readers.
> >
> > Right, the oneline I suggested was only for the original patch, with which
> > I disagreed.
>
> I actually do not know how your rewrite could be better, though.
>
> 		/* GCC thinks socket()/connect() might fail to set errno */
> 		return errno ? errno : EIO;
>
> If a compiler thinks errno may *not* be set, can 'errno' be reliably
> used to decide if it can be returned as-is or a fallback value EIO
> should be used, without triggering the same (incorrect) working in
> the first place?

> On Fri, 7 May 2021, Junio C Hamano wrote:
> 
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> > >> Otherwise I'd strongly prefer to see a word that hints that this is
> > >> an otherwise unneeded workaround for comiplers.  Your suggested
> > >> title instead hints that it is wrong to assume that errno will be
> > >> set to non-zero after a syscall.  I do not think that is the message
> > >> we want to send to our readers.
> > >
> > > Right, the oneline I suggested was only for the original patch, with which
> > > I disagreed.
> >
> > I actually do not know how your rewrite could be better, though.
> >
> > 		/* GCC thinks socket()/connect() might fail to set errno */
> > 		return errno ? errno : EIO;
> >
> > If a compiler thinks errno may *not* be set, can 'errno' be reliably
> > used to decide if it can be returned as-is or a fallback value EIO
> > should be used, without triggering the same (incorrect) working in
> > the first place?
> 
> Oh, I guess I mistook the problem for something else, then.
> 
> If the problem is that `errno` is not set in case of failure, the
> resolution is easy (and even recommended in the manual page of `errno`):
> simply set it to 0 before the syscall (or in the function that relies on
> `errno == 0` means success).

> On Tue, May 11, 2021 at 04:34:49PM +0200, Johannes Schindelin wrote:
>
>> On Fri, 7 May 2021, Junio C Hamano wrote:
>> 
>> > 		/* GCC thinks socket()/connect() might fail to set errno */
>> > 		return errno ? errno : EIO;
>> >
>> > If a compiler thinks errno may *not* be set, can 'errno' be reliably
>> > used to decide if it can be returned as-is or a fallback value EIO
>> > should be used, without triggering the same (incorrect) working in
>> > the first place?
>> 
>> Oh, I guess I mistook the problem for something else, then.
>> 
>> If the problem is that `errno` is not set in case of failure, the
>> resolution is easy (and even recommended in the manual page of `errno`):
>> simply set it to 0 before the syscall (or in the function that relies on
>> `errno == 0` means success).
>
> I don't think that is the problem. According to the standard, errno is
> always set to a non-zero value after a syscall failure.
>
> The problem is only that the compiler does not incorporate that special
> knowledge of the variable, so it generates a warning even though we can
> reason that the situation it describes is impossible.

> Jeff King <peff@peff.net> writes:
> 
> > On Tue, May 11, 2021 at 04:34:49PM +0200, Johannes Schindelin wrote:
> >
> >> On Fri, 7 May 2021, Junio C Hamano wrote:
> >> 
> >> > 		/* GCC thinks socket()/connect() might fail to set errno */
> >> > 		return errno ? errno : EIO;
> >> >
> >> > If a compiler thinks errno may *not* be set, can 'errno' be reliably
> >> > used to decide if it can be returned as-is or a fallback value EIO
> >> > should be used, without triggering the same (incorrect) working in
> >> > the first place?
> >> 
> >> Oh, I guess I mistook the problem for something else, then.
> >> 
> >> If the problem is that `errno` is not set in case of failure, the
> >> resolution is easy (and even recommended in the manual page of `errno`):
> >> simply set it to 0 before the syscall (or in the function that relies on
> >> `errno == 0` means success).
> >
> > I don't think that is the problem. According to the standard, errno is
> > always set to a non-zero value after a syscall failure.
> >
> > The problem is only that the compiler does not incorporate that special
> > knowledge of the variable, so it generates a warning even though we can
> > reason that the situation it describes is impossible.
> 
> Yes, that is what I tried to say (i.e. if the compiler does not know
> errno has a defined value at certain places in our code and
> complain, then "return errno ? errno : EIO" would get the same
> warning because bases its outcome on the value of errno the compiler
> thinks is possibly undefined at that point), but apparently I failed
> to convey that clearly enough.

> 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.
>
> 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/
> ---
>  trace2/tr2_dst.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)

> 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.

> 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.

> > > 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.
> 
> It's quite short. I'm OK with it for a static-local function with few
> callers like this, though.

> 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.e.:
>
> 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,23 +271,21 @@ 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;
>  	}
>  
>  error:
>  	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;
>
>
> I do prefer that approach, since I think it's more idiomatic for our
> code base, but for the sake of wrapping up this simple fix which has
> been discussed much more than I think it deserves, I am OK with either.
> :)

>
> (I also found it interesting that the "error" goto in the caller only
> has one source. I think the code would be easier to reason about if it
> were inlined, but I'm happy to stop here for now).
>
> -Peff

> 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.e.:
>
> 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;
>  	}
>  
> ...
>
> I do prefer that approach, since I think it's more idiomatic for our
> code base, but for the sake of wrapping up this simple fix which has
> been discussed much more than I think it deserves, I am OK with either.
> :)

> Jeff King <peff@peff.net> 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.

> >> 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.

> @@ -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));

>> @@ -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));
>
> We expect the value of errno to persist across tr2_dst_want_warning()
> and tr2_sysenv_display_name() here. The former may call getenv() and
> atoi(). I think that's probably fine, but if we wanted to be really
> paranoid, we'd have to preserve errno manually here, too.

> Jeff King <peff@peff.net> writes:
> 
> >> @@ -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));
> >
> > We expect the value of errno to persist across tr2_dst_want_warning()
> > and tr2_sysenv_display_name() here. The former may call getenv() and
> > atoi(). I think that's probably fine, but if we wanted to be really
> > paranoid, we'd have to preserve errno manually here, too.
> 
> Being "really paranoid" consistently within the file would mean a
> change like the attached, I would think, on top of what was posted.
> 
> Or tr2_dst_want_warning() and tr2_sysenv_display_name() can be
> taught to preserve errno like tr2_dst_dry_uds_connect() was taught
> to do so by the patch under discussion, which may reduce the amount
> of apparent change, but constantly moving the contents of errno
> around just in case we later might want to use its value feels
> dirty.
> 
> I dunno.
> 
>  trace2/tr2_dst.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)

> On Wed, Apr 1, 2020 at 12:31 AM Denton Liu <liu.denton@gmail.com> wrote:
> 
> > Nonetheless, these warnings block the compilation process when
> > DEVELOPER=1 is enabled (which enables -Werror).
> >
> 
> you can also alternatively use DEVOPTS=no-error