[PATCH v10 2/7] arch: move SA_* definitions to generic headers

[Peter Collingbourne]

On Tue, Sep 8, 2020 at 8:12 AM Dave Martin <Dave.Martin at arm.com> wrote:
>
> On Fri, Aug 21, 2020 at 10:10:12PM -0700, Peter Collingbourne wrote:
> > Most architectures with the exception of alpha, mips, parisc and
> > sparc use the same values for these flags. Move their definitions into
> > asm-generic/signal-defs.h and allow the architectures with non-standard
> > values to override them. Also, document the non-standard flag values
> > in order to make it easier to add new generic flags in the future.
> >
> > Signed-off-by: Peter Collingbourne <pcc at google.com>
>
> While this looks reasonable, I've just realised that you strip the "U"
> from some arches' definitions here.
>
> So, on powerpc and x86, this changes the type of flags other than
> SA_RESETHAND from unsigned int to int.
>
> While I can't see this breaking any sensible use of these flags, there's
> a chance that there is software relying on this distinction by
> accident.

While it's true that it's technically possible that making these
signed could change semantics, I'm having trouble seeing a realistic
way in which software could be relying on this. Can you see one? I can
think of cases like if the code does something like left shifts one of
the flag bits into the sign bit (technically undefined behavior) and
then right shifts it back (in C this would need to all be done in a
single expression without storing to a variable; in C++ I suppose you
could use auto to preserve the signedness in a variable's type). For
example:

int x = (SA_NODEFER << 1) >> 1;

would give a different value to x if we made SA_NODEFER signed. But I
wouldn't really expect software to be doing this sort of thing even
accidentally, or much more than or'ing the flags together and
assigning them to a variable, or passing them as a parameter, or some
other operation which would fix the type.

I believe that the kernel's uapi guarantee applies at the binary
level, not at the source level. If that were not the case, I think we
would not be allowed to add any new declaration to an existing .h file
for fear of conflicting with a user program's identically spelled
declaration. And that seems more likely to me than software that would
do this sort of thing.

> I wonder whether it's worth doing something like
>
>         #ifdef ARCH_WANT_STRICTLY_UNSIGNED_SA_FLAGS
>         #define __SA_FLAG_VAL(x) x ## U
>         #else
>         #define __SA_FLAG_VAL(x) x
>         #endif
>
>         #ifndef SA_NOCLDSTOP
>         #define SA_NOCLDSTOP __SA_FLAG_VAL(0x00000001)
>         #endif
>
>         /* ... */

If we do this I would mildly prefer to keep the existing #defines in
the arch-specific headers as if the arch had different flag values, as
this would leave the arch-specific legacy cruft in the arch-specific
headers where it belongs.

> Mind you, the historical situation also has issues, e.g. because
> sa_flags in struct sigaction is an int, assigning
>
>         struct sigaction sa;
>
>         sa.sa_flags = SA_RESETHAND;
>
> implies an overflow and so isn't portably safe (at least in theory).  I
> guess we are getting away with it today.  Preserving the situation by
> keeping the "U"s where appropriate would at least avoid making the
> situation worse.

I believe that the result of this assignment (involving an unsigned to
signed conversion) is implementation defined and not undefined (which
would be problematic). And in all the implementations that matter, as
well as the C++ standard starting with C++20, this is a no-op cast
assuming two's complement. I'm not sure what this has to do with
making the constants signed because, as you pointed out, SA_RESETHAND
would remain unsigned despite the absence of 'U' because its value
does not fit in an int.

Peter