[PATCH] Linux: Fix '__close_nocancel_nostatus' clobbering 'errno' [BZ #33035]

[Adhemerval Zanella Netto]

On 30/05/25 13:32, Maciej W. Rozycki wrote:
> On Fri, 30 May 2025, Florian Weimer wrote:
> 
>>> Implement '__close_nocancel_nostatus' then as a standalone function in 
>>> terms of INTERNAL_SYSCALL_CALL, which leaves 'errno' intact.  This is a 
>>> generic Linux version analogous to '__close_nocancel'.  Platforms may 
>>> choose to implement an inline variant where the syscall invocation code 
>>> sequence is short enough to be beneficial over a function call.
>>
>> I think you should just switch the implementation to
>> INTERNAL_SYSCALL_CALL, like we use today for __writev_nocancel_nostatus.
> 
>  You mean by keeping it inline?
> 
>  But unlike with '__writev_nocancel_nostatus', which is only called once 
> in resolv/herror.c and a couple of times in gmon/gmon.c, our code base is 
> sprinkled generously with calls to '__close_nocancel_nostatus', and for 
> example on 64-bit POWER the out-of-line compilation of the function 
> expands to 25 machine instructions (no extra stack frame is produced, so 
> no extra overhead).
> 
>  Are you sure it will be beneficial performance-wise to have such a large 
> chunk of code repeatedly pasted inline?

I would simplicity, the _nocancel and _nostatus are essentially wrappers
over INLINE_SYSCALL_CALL/INTERNAL_SYSCALL_CALLS that are mainly added
to abstract it for Hurd.

The inline also remove the need to export it from GLIBC_PRIVATE.

> 
>  For reference `__close_nocancel' is 43 instructions on 64-bit POWER, a 
> bit more, but less frequently called (43 times vs 58).  Why would it be 
> appropriate to be out of line then, where's the dividing line?
> 
>  As I say there are likely platforms that will indeed benefit from an 
> inline version, but given that it's all handwritten assembly I expect that 
> GCC won't be able to automatically figure out whether automatic inlining 
> is beneficial or not.  Consequently that would have to be sorted by hand 
> in individual platform's sysdeps/ subdirectories.  The same may stand for 
> `__close_nocancel' actually.
> 
>  Overall I think that there is nothing wrong with my implementation as it 
> stands and I'd leave it to people with specific interest in individual 
> platforms to figure out if they want an inline variant.  I'd rather stayed 
> consistent with `__close_nocancel' here.
> 
>   Maciej
>