[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#56393: Actually fix the long lines display bug
From: |
Gregory Heytings |
Subject: |
bug#56393: Actually fix the long lines display bug |
Date: |
Mon, 18 Jul 2022 12:58:59 +0000 |
Thanks. The crash is gone, of course,
That's good news.
I can send you a backtrace from yesterday:
[...]
As you see, the cause of problem was the same one:
[...]
Since you have now moved the iteration to begin at the "narrowed" BEGV,
this crash is also gone.
That's good news, too. Thanks for the other backtrace.
To tell the truth, I'm not sure this kind of fix is the correct
solution, because basically its success is a matter of luck (or lack
thereof). For example, recall the higher-level context of the first
segfault:
[...]
This is isearch.el trying to establish whether the position of the match
is visible in the window. To do that, it starts from the current
window-start position (which happens to be 1), and simulates display of
the buffer text portion up until point or until the end of window,
whichever comes first, to determine whether point is beyond the end of
the window. Your change makes the search start from some much later
position instead, which could very well produce incorrect results:
pos-visible-in-window-p could decide that point _is_ visible, when it
really isn't. (It doesn't happen in this particular case because the
newline is far away -- at the very end of the buffer -- so it isn't
visible in both cases. But that's sheer luck.)
Well, this happens only in buffers with long lines, and only when we are
inside a long line, so from my point of view it works as expected, and
moreover the risk is small. Would the following be better from your point
of view?
diff --git a/src/xdisp.c b/src/xdisp.c
index d69d7440bc..572ad2b854 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -8668,18 +8668,11 @@ get_visually_first_element (struct it *it)
bool string_p = STRINGP (it->string) || it->s;
ptrdiff_t eob = (string_p ? it->bidi_it.string.schars : ZV);
ptrdiff_t bob;
+ ptrdiff_t obegv = BEGV;
- SET_WITH_NARROWED_BEGV (it, bob, string_p ? 0 : BEGV);
-
- /* Reseat again when, as a consequence of the SET_WITH_NARROWED_BEGV
- above, the iterator is before bob. */
- if (!string_p && IT_CHARPOS (*it) < bob)
- {
- struct text_pos pos;
- pos.charpos = bob;
- pos.bytepos = CHAR_TO_BYTE (bob);
- reseat (it, pos, true);
- }
+ SET_WITH_NARROWED_BEGV (it, bob,
+ string_p ? 0 :
+ IT_BYTEPOS (*it) < BEGV ? obegv : BEGV);
if (STRINGP (it->string))
{
More generally, if you look at redisplay_window, you will see that about
2/3 of its code tries very hard to reuse the previous window-start
position, before it gives up and looks for a new starting position. So
in any situation where the previous window-start is far enough before
point, all that code will basically work with a buffer position that is
at risk of being before the "narrowed" BEGV. Thus, any code there which
tries stuff like start_display+move_it_to will risk hitting this kind of
problems -- either FETCH_BYTE will crash, or we risk producing the wrong
result because we force the code to jump to the "narrowed" BEGV before
doing anything, while its caller expects results relative to a different
position.
I understand. But note that temporarily narrowing the buffer happens only
at a few well-chosen places, which are situated rather low in the
abstraction layers, so the effect on other parts of the code is nil.
I think this is because the display engine assumes that BEGV stays put
during the entire redisplay_window lifetime, i.e. that all of the
subroutines it calls see the same value of BEGV. This is no longer so
on the branch, and I wonder whether and how we should handle this new
situation to keep the display code stable and reliable.
I don't know.
- bug#56393: Actually fix the long lines display bug, (continued)
- bug#56393: Actually fix the long lines display bug, Gregory Heytings, 2022/07/09
- bug#56393: Actually fix the long lines display bug, Gregory Heytings, 2022/07/09
- bug#56393: Actually fix the long lines display bug, Lars Ingebrigtsen, 2022/07/09
- bug#56393: Actually fix the long lines display bug, Gregory Heytings, 2022/07/09
- bug#56393: Actually fix the long lines display bug, Gregory Heytings, 2022/07/16
- bug#56393: Actually fix the long lines display bug, Eli Zaretskii, 2022/07/17
- bug#56393: Actually fix the long lines display bug, Gregory Heytings, 2022/07/17
- bug#56393: Actually fix the long lines display bug, Eli Zaretskii, 2022/07/17
- bug#56393: Actually fix the long lines display bug, Gregory Heytings, 2022/07/18
- bug#56393: Actually fix the long lines display bug, Eli Zaretskii, 2022/07/18
- bug#56393: Actually fix the long lines display bug,
Gregory Heytings <=
- bug#56393: Actually fix the long lines display bug, Eli Zaretskii, 2022/07/18
- bug#56393: Actually fix the long lines display bug, Gregory Heytings, 2022/07/18
- bug#56393: Actually fix the long lines display bug, Gregory Heytings, 2022/07/18
- bug#56393: Actually fix the long lines display bug, Eli Zaretskii, 2022/07/18
- bug#56393: Actually fix the long lines display bug, Gregory Heytings, 2022/07/18
- bug#56393: Actually fix the long lines display bug, Gregory Heytings, 2022/07/18
- bug#56393: Actually fix the long lines display bug, Gregory Heytings, 2022/07/18
- bug#56393: Actually fix the long lines display bug, Eli Zaretskii, 2022/07/18
- bug#56393: Actually fix the long lines display bug, Gregory Heytings, 2022/07/18
- bug#56393: Actually fix the long lines display bug, Eli Zaretskii, 2022/07/18