On Tue, Sep 8, 2020 at 8:12 AM Dave Martin <Dave.Martin@xxxxxxx> 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@xxxxxxxxxx> > > 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
