On Tue, May 11, 2021 at 03:04:28PM +0200, Æ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. Thanks, I think this describes the problem nicely. > 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. Agreed. Returning "-1" is the usual style in our code base. And while I think the original code is correct, I did have to go double-check the C standard to confirm that it's so. I slightly disagree with the notion that gcc's behavior is a bug. It seems more like a lack of feature (it does not have any way to annotate this special property of errno). But that is neither here nor there for your patch, and really a matter of opinion. :) > > 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. -Peff
