Re: [PATCH] err_ptr.h: introduce ERR_PTR_SAFE()

[David Laight]

On Sat, 16 May 2026 13:39:11 +0200
Amir Goldstein <amir73il@xxxxxxxxx> wrote:

> On Sat, May 16, 2026 at 10:42 AM David Laight
> <david.laight.linux@xxxxxxxxx> wrote:
> >
> > On Fri, 15 May 2026 21:26:04 +0200
> > Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> >  
> > > On Fri, May 15, 2026 at 8:30 PM David Laight
> > > <david.laight.linux@xxxxxxxxx> wrote:  
> > > >
> > > > On Thu, 14 May 2026 22:01:29 +0200
> > > > Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> > > >  
> > ...  
> > > >
> > > > The object code bloat would be noticeable if this were used everywhere.
> > > > But you could make it a bit simpler:
> > > >         if (__builtin_constant_p(__e))
> > > >                 BUILD_BUG_ON(__e && !IS_ERR_VALUE(__e));
> > > >         else if WARN_ON(__e && !IS_ERR_VALUE(__e))
> > > >                 __e = -MAX_ERRNO; // Or maybe -EINVAL to stop and other boundary errors
> > > >         (void *)__e;  
> > >
> > > Yeh that's nicer thanks.  
> >
> > Actually this might be better still (or just more succinct):
> >         void *__e = (void *)error;
> >         BUILD_BUG_ON(!statically_true(IS_ERR_OR_NULL(__e));  
> 
> This condition is wrong but also my compiler does not evaluate
> __builtin_constant_p(IS_ERR_OR_NULL(__e)) as true.
> 
> This works
> BUILD_BUG_ON(statically_true(!IS_ERR_VALUE(__e)));

Yes, it is easy to get those wrong - especially when typing quickly.

> 
> I think it is enough to statically assert on ERR_PTR(EINVAL)
> and no need to bother with ERR_PTR(0)

Then the tests don't match - which looks funny.
IS_ERR_VALUE(val) should be: val += 4095; jump_carry ...
and IS_ERR_OR_NULL(val): val--; val += 4096; jump_carry ...
but I can't remember whether gcc manages to do that.

> 
> >         if (WARN_ON(!IS_ERR_OR_NULL(__e))
> >                 __e = (void *)-EINVAL;  
> 
> Oh, anything but EINVAL please - the most overloaded error value
> My choice of meaningful error value would be EFAULT
> because without the safe helper we would be returning an address
> which is in most likelihood bad, so better be explicit about it.

I'm not sure about EFAULT; it is only really used for user copy failures.
IIRC at least one Unix (I've forgotten which) generates SIGSEGV when a
system call return of EFAULT.

There is also the 'problem' of PANIC_ON_WARN which is set by a lot
of distributions.
That (sort of) means than you might as well use BUG_ON() and get the
associated slightly smaller code size.
On x86-64 (and maybe a few others) both BUG_ON() and WARN_ON() just
execute UD2 (an undefined instruction) and the trap handler finds the
associated info and does the printk().
That makes the code smaller than pr_warn().
Someone needs to add a 'I_REALLY_MEAN_WARN_ON()' that never panics.
(And maybe with an option to not dump all the stack.)

-- David

> 
> >         __e;
> >
> > The WARN_ON() will be optimised away (valid) constants.
> >  
> 
> Yeh this looks nice I'll use this:
> 
> #define ERR_PTR_SAFE(error) ({                                  \
>         void *__e = (void *)(long)(error);                      \
>         BUILD_BUG_ON(statically_true(!IS_ERR_VALUE(__e)));      \
>         if (WARN_ON(!IS_ERR_OR_NULL(__e)))                      \
>                 __e = (void *)(long)-EFAULT;                    \
>         __e;                                                    \
> })
> 
> 
> Thanks!
> Amir.