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

From: Junio C Hamano <gitster@pobox.com>
Date: May 6, 2021, 01:26 UTC
Message-ID: <xmqqv97w1wl1.fsf@gitster.g>
In-Reply-To: <nycvar.QRO.7.76.6.2105051623230.50@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 23 quoted lines

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