bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Pip Cet]

On Thu, Jun 27, 2019 at 8:28 AM Paul Eggert <eggert@cs.ucla.edu> wrote:
> 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....

I actually think that would be best, so we're only disagreeing about
what the second-best solution is :-)

> >        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.

valid_image_p only returns true if ->valid_p returns true, which only
happens when parse_image_spec returns true, which only happens if
:ascent is not present, Qcenter, or a fixnum in the 0..100 range.

> > 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?

I don't think it's really relevant to this discussion, though there
are similarities:

----

Just like having both XFIXNUM and XFIXNAT is confusing, so is having
eassume and eassert. My idea is to define eassume as follows:

#define eassume(cond) (__builtin_constant_p (!(cond) == !(cond)) ?
((cond) ? 0 : __builtin_unreachable ()) : 0)

That would catch most (not all) cases in which eassume() causes code
to be emitted.  I wanted to test this, and in particular to catch
false negatives like this eassume in lisp.h:

  eassume (0 <= i && i < bool_vector_size (a));

(GCC isn't smart enough to derive any information from the RHS of the
&&, so __builtin_constant_p (!(cond) == !(cond)) is false, but we only
actually use the LHS of the &&...)

So I resorted to this old trick to see whether code was being emitted:

int c;
asm(".pushsection .text.bad");
c = (cond);
asm(".popsection");

but that hack no longer works with -g3. Instead, we can define an
inner function with the relevant code, put it into a magic section,
then confirm that only the trivial function actually ended up in the
magic section.
----

> > 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.

Ah. Right. Thanks.

> > 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?

Because it's as far as we get only replacing characters in our code,
not typing in new ones.

> 0 is an obviously invalid argument for buffers. :-)

True, but then we'd have to add extra characters to distinguish the
buffer and string cases.

> 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.

Okay, let's agree to disagree on what the second-best solution is here.

> > 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.

Do DOS machines have enough memory to run Emacs with debugging
enabled? :-) You're absolutely right, of course.

Thanks for the changes! I think this code is a bit hard to read:

      margin = image_spec_value (spec, QCmargin, NULL);
      if (RANGED_FIXNUMP (0, margin, INT_MAX))
        img->vmargin = img->hmargin = XFIXNAT (margin);
      else if (CONSP (margin))
        {
          img->hmargin = XFIXNAT (XCAR (margin));
          img->vmargin = XFIXNAT (XCDR (margin));
        }
because range is checked for the :margin 3 case but not the :margin
'(3 4) case; I'm perfectly okay with taking a list and checking it
twice.