bug#41520: 28.0.50; Crash in character.h due to assertion error

[Pip Cet]

On Mon, May 25, 2020 at 2:18 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Mon, 25 May 2020 07:28:23 +0000
> > Cc: 41520@debbugs.gnu.org
> > My suggestion is to drop the "eassume" on emacs-27, and fix FETCH_CHAR
> > to FETCH_BYTE on master.
>
> There's no eassume on emacs-27, this is new on master.

Yeah, I realized after hitting "Send" that I'd looked at the wrong tree :-)

> So I installed on emacs-27 branch a patch very similar to what you
> sent, except that it uses FETCH_BYTE in all cases where we compare to
> a newline: this is both more efficient and more correct.

I'm attaching a patch with a few more cases. I don't have a strong
preference for either FETCH_BYTE or FETCH_CHAR where both will do, but
we should be consistent.

> > (I'd like to reiterate my proposal to use a pos_t for bytepos/charpos
> > pairs, which would catch or silently fix (which happened in this case
> > on my pos_t branch) such bugs. The code on my branch reads:
> >
> >   else if (POS_CHAR_EQUAL (it->bidi_it.pos, bob)
> >        || (!string_p
> >            && (FETCH_CHAR (dec_pos (it->bidi_it.pos)) == '\n'
> >            || FETCH_CHAR (it->bidi_it.pos) == '\n')))
> >
> > which, while minimally slower, doesn't throw assertion errors.)
>
> That would require us to maintain both character and byte positions
> where we use these macros,

For now, I'd like to introduce them in Emacs proper only where we have
pairs of variables anyway.

> something that could be redundant
> overhead.

It would be redundant to use a pos_t where a ptrdiff_t suffices, yes.
I'm not proposing to do that at present, though I think there are some
places that do charpos/bytepos conversions unnecessarily because only
one of them is passed down the call chain. So, no, it wouldn't be
redundant overhead, and it might actually make some code paths faster.

> Moreover, I think we prefer having assertions in the debug
> builds rather then silent fixups (and in production eassume compiles
> into something that doesn't abort).

I see no way to have assertions, and I think free bug fixes are better
than undiscovered bugs. It's really hard to get things wrong with a
stronger type.

0001-Fix-more-single-byte-accesses.patch
Description: Text Data