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

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	git@vger.kernel.org, "Jeff King" <peff@peff.net>,
	"Denton Liu" <liu.denton@gmail.com>,
	"Jeff Hostetler" <jeffhost@microsoft.com>
Subject: Re: [PATCH] trace2: refactor to avoid gcc warning under -O3
Date: Thu, 6 May 2021 22:29:57 +0200 (CEST)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.2105062229270.50@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <xmqqv97w1wl1.fsf@gitster.g>

[-- Attachment #1: Type: text/plain, Size: 1811 bytes --]

Hi Junio,

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

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

Right, the oneline I suggested was only for the original patch, with which
I disagreed.

Ciao,
Dscho