Pip Cet wrote:

> I'd prefer if the code were written as though FIXNATs weren't FIXNUMs at all.

I'm more of the opposite opinion. The original reason for XFIXNAT (under its old 
name) was performance, because long ago Emacs put the type tag in the most 
significant bits, and it was faster to mask it out than to smear the sign bit 
when you knew the fixnum was nonnegative. Nowadays that original reason is 
obsolete because the tags are in the least significant bits and XFIXNAT is just 
an alias for XFIXNUM, and if it were up to me we'd get rid of XFIXNAT entirely, 
and just use XFIXNUM. But old habits die hard....


>        ascent = image_spec_value (spec, QCascent, NULL);
>        if (FIXNUMP (ascent))
>          img->ascent = XFIXNAT (ascent);
> 
> in image.c is safe, because lookup_image is called only after valid_image_p.

Sorry, you've lost me. How does valid_image_p guarantee that 'ascent' (if a 
fixnum) is in the range 0..INT_MAX? Because if it's not in that range, the above 
code could trap.

> I'd prefer AREF (status, 8) here, for what readability the code gives us.

Sure, sounds good. See attached further patch.

> I'd prefer leaving the macros for now

I actually did it that way at first, but that failed miserably as the macros 
evaluated their arguments more than once when debugging , and lots of code 
expects them to behave like functions and evaluate their arguments exactly once. 
So functions it is.

> eassume (inline_function (x))
> doesn't work very well on current GCC, while eassume (MACRO (x)) is
> fine, so I'm sometimes building with DEFINE_KEY_OPS_AS_MACROS.

Hmm, I see I should have helped GCC more there. First, the two new eassumes 
should just be easserts as the assumptions are not helpful to the compiler. 
Second, I should restore the eassume that tells GCC that XFIXNAT returns a 
nonnegative integer.

> (The
> eassume/eassert situation is something else I've been planning to look
> at in more detail, though debugging it will be easier when
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91005 is fixed).

Sorry, I don't follow. That bug report seems to be about nested C functions; why 
is it relevant to Emacs, which doesn't use nested C functions?

>> * The ccl.c, fileio.c, image.c, and process.c fixes should use XFIXNUM,
>> not XFIXNAT, since the code is range-checking after extraction anyway
>> and the latter range-checking should do the right thing. Come to think
> 
> That's an argument that either XFIXNUM or XFIXNAT are fine, and in
> this case XFIXNAT seems, to me, much more readable.

No, as the range-checking is done after the XFIXNAT, which means the XFIXNAT 
could have an assertion failure.

>> of it, the ccl.c and image.c code is buggy regardless of the
>> XFIXNUM/XFIXNAT business since the Emacs integer might not fit into int
>> range, and that should be fixed too.
> 
> I don't see how either ccl.c or image.c are buggy for out-of-range
> integer arguments.

For example, ccl.c in master has this:

   if (FIXNUMP (AREF (status, i)))
     {
       i = XFIXNAT (AREF (status, 8));
       if (ccl.ic < i && i < ccl.size)
	ccl.ic = i;
     }

where i is a 32-bit int. If AREF (status, 8) is the 62-bit Lisp fixnum with 
value '2 * (EMACS_INT) INT_MAX + ccl.ic + 3', the assignment to i overflows, 
which can trap. Even if the assignment doesn't trap but merely wraps around, the 
next line won't do range checking correctly.

> When you want a position, you want a FIXNAT,
> not a negative number, so CHECK_FIXNAT_COERCE_MARKER everywhere would
> catch those cases where someone passes an obviously invalid argument.

But why stop there? 0 is an obviously invalid argument for buffers. :-) I think 
this is just an area where we disagree - I want a range check where you want a 
type check. It's safe either way and the range check is a tad faster (as the 
range check needs to be done anyway) and results in a crisper diagnostic when it 
fails.
> I'd prefer not touching the dosfns.c code, for the simple reason that
> if anyone still uses it, they might rely on the broken behavior.

We can't leave it alone, as XFIXNAT is now checking that its result is 
nonnegative when debugging is enabled. However, we could instead change the 
XFIXNATs to XFIXNUMs: this will generate exactly the same machine code on that 
platform when debugging is disabled, and will not trap when debugging is 
enabled. See attached patch.