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

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Mon, 25 May 2020 07:28:23 +0000
> Cc: 41520@debbugs.gnu.org
> 
> On Mon, May 25, 2020 at 7:06 AM Stefan Kangas <stefan@marxist.se> wrote:
> > When editing an org-mode document, I saw a crash due to this assertion
> > error:
> 
> It's a bug in this code in xdisp.c:
> 
>   else if (it->bidi_it.charpos == bob
>        || (!string_p
>            && (FETCH_CHAR (it->bidi_it.bytepos - 1) == '\n'
>            || FETCH_CHAR (it->bidi_it.bytepos) == '\n')))

Ouch!

> The first FETCH_CHAR should be a FETCH_BYTE to avoid the assertion error.
> 
> There's at least one other place that has the same error, so I'll grep
> some more before sending a patch.

Thanks.

> 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.  That is why
these problems were never exposed before: the old versions of macros
didn't signal any errors in these cases, they just produced some wrong
values, which can never be equal to a newline.

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'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, something that could be redundant
overhead.  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).