Few windows issues post v668 #607
Comments
|
There's something wrong with the autowrap/ignaw thing:
These issues don't happen with v668. I suspect the issue is at the commits related to clear-to-EOL, but sorry, I'm just unable to follow that code... (I'm whispering, maybe reconsider reverting to the original one-liner which I suggested, which is way way simpler than what you committed). They might also be related to the new console modes (both with/without VT) and the auto_wrap/ignaw setting on windows, in conjunction with the clear-EOL changes. |
|
Interesting. This happens to me frequently (the hang). But yeah, I'll definitely try to reproduce it reliably.
I think not. It's a complex makefile. I don't think we want the default makefile more complex than it already is. You can check it out here avih@ba53695 And it has the advantage that old revisions don't change, so at most it would stop working in the future (if unmaintained), but it will be able to build v322 - v668 forever. (from v537 onwards it's pretty much the current Makefile.wng, except that the current makefile didn't exist at v537, so it's hard to built with Makefile.wng at v537, which where this makefile helps). |
|
It appears that the long line display problem was caused by dee132a. That was the change that you suggested because you thought auto_wrap was being set incorrectly in Windows. With this change, long lines are displayed correctly: |
|
As far as I recall, wIth xterm we have autowrap=1 and ignaw=1, and the windows terminal behaves exactly the same as xterm in this regard, and IIRC this complies with my analysis back then. These values must be more correct as far the behavior goes. autowrap=0 means there's no wrap at all. not "delayed" and not anything - it just stays on the same line until And vt mode definitely wraps - delayed - like xterm, which must mean that both should be 1. Am I missing something? |
|
With autowrap=1 and ignaw=1, when pdone() completes a wrapped line, it does not add a newline. It expects that the next char printed will auto-wrap. This is how it works on xterm. I don't know exactly how the Windows terminal works, but based on this bug, it seems to be doing something different than xterm. |
|
BTW I was able to reproduce the hang and repaint loop once, by scrolling through main.c by holding down the down arrow key. I tried it about 10 more times and haven't been able to reproduce it again. |
Yeah, I can reproduce that it doesn't wrap at all without \n, and that adding ENABLE_WRAP_AT_EOL_OUTPUT seems to make it behave like xterm. I'm testing by adding this function, and calling it if vt succeeds as static void test_wrap(HANDLE con) {
CONSOLE_SCREEN_BUFFER_INFO scr;
GetConsoleScreenBufferInfo(con, &scr);
int width = 1 + scr.srWindow.Right - scr.srWindow.Left;
for (int i = 0; i < 3; ++i) {
WriteConsole(con, "\n\n\n", 3, 0, 0);
for (int j = 0; j < width - 1 + i; ++j) {
char c = '0' + j % 10;
WriteConsole(con, &c, 1, 0, 0);
}
Sleep(1000);
}
sleep(3000);
exit(42);
}Additionally adding also DISABLE_NEWLINE_AUTO_RETURN breaks the behavior, as it appears to do what it says on the tin - that That being said, I need to re-read my analysis and conclusions from back then (and find this discussion first). IIRC there were some gotchas, incorrect docs, and other things, and I want to check whether that commit was hasty (the one which only sets ENABLE_PROCESSED_OUTPUT | ENABLE_VIRTUAL_TERMINAL_PROCESSING), or whether the conclusions seem incorrect currently. If you remember where we we've had this discussion, please post a link. |
|
The conclusion seems to be here #497 (comment) Reading now. |
|
Well, my conclusions back then:
This is really not what I'm seeing now, and I said it quite conclussively back then. Specifically, when starting with ENABLE_PROCESSED_OUTPUT | ENABLE_VIRTUAL_TERMINAL_PROCESSING (which are documented as should be used together), then:
I'm very puzzled by this. I can't yet resolve the conflict. Maybe I didn't test with a new console buffer? (I did test these independently of "less", I want to say as close as possible to the "less" use case, but I need to find these test programs to know for a fact). It's also possible that a newer windows terminal which I'm using now behaves differently. I need to also test it with the windows console (which I haven't so far), although it might have changed too in Windows updates... Regardless of VT mode, non-VT also has issues with long lines, both with and without First, disable vt: change I'm pretty sure the |
That was in the windows terminal. In the windows console (cmd.exe window) ENABLE_WRAP_AT_EOL_OUTPUT does indeed get ignored when VT is enabled as I concluded back then, and it does vt-wrap (ignaw=1) even without this bit set. So the initial rendering with VT enabled and long lines is correct at the windows console with current master. Nothing is missing and the prompt is at the bottom. However, scrolling (one line at a time) few lines down and then few lines up, exposes the same issue as without VT - when scrolling up only the remainder of long lines show (enter from the top), and the begining of long lines dissappear. The same binary at the windows terminal is still broken like before starting at the initial rendering. DISABLE_NEWLINE_AUTO_RETURN still has an effect also with VT at the windows console, in contrast to my initial conclusion. I can only conclude that the windows terminal changed since I concluded it, and I don't know what to say about the windows console and DISABLE_NEWLINE_AUTO_RETURN, other than hand waving that maybe it changed too during windows update. Either way, for VT mode, this works now and before in both the console and the terminal, and has vt100-wrap (ignaw=1): ENABLE_PROCESSED_OUTPUT |
ENABLE_VIRTUAL_TERMINAL_PROCESSING |
ENABLE_WRAP_AT_EOL_OUTPUTNon-VT is still broken with long lines in the windows console too, both with and without -X. |
I think it happens when reaching the bottom of the content, so maybe it would be easier to reproduce with content only some lines taller than the window height. I still didn't try to find a reliable reproducer. |
|
I've reproduced the hang 4 more times by scrolling main.c. It did not happen at the end of the content. However, in all four cases it happened at the exact same point in the file, with line 311 displayed at the top of the screen and line 338 at the bottom. That can't be a coincidence, but I don't yet know whether it's related to the file content or if it happens at the same point due to some timing issue. |
|
This is not definitive because the hang is very intermittent, but so far I have only reproduced it with the down arrow key. Holding down 'j' has not reproduced it. That might suggest it has something to do with the code that handles extended keys in Windows. But every case so far has always occurred at the same point in the file. This is true regardless of whether I start scrolling from the beginning of the file or press 'f' first and then start scrolling. I'm not sure how to interpret this set of symptoms. |
|
Maybe bisect? there are ~ 43 commits since v668, so that's 5-6 bisection steps? It would be best if we could automate the testing, even if not reproducible, I'd imagine that holding the down arrow over 100 consecutive invocation of "less" should reproduce it reliably-ish. Maybe try to add some code inside less which simulates this injection somehow? then running less with the same file in a loop would automatically "hold the down arrow" and should hopefully either complete the 100 iterations, or hang during one of them? EDIT: or invoke it in a loop and just keep holding the down arrow yourself, or put something on your down arrow key to keep it held for you? (and it needs the option to quit when reaching EOF). |
|
I think I found another symptom of this issue, but with a different thing on screen: I'm using this line in busybox-w32 sh: while sleep 0.5; do head -n 50 < ../testlines.txt | LESS= ./less -E; doneWhere the window is 33 lines tall and 64 cols wide, and most of the lines in testlines.txt are wider than the terminal. I'm holding down the arrow key, and within one or two iterations of "less" I get a whole screen full of: which similarly cannot be interrupted and can only be killed from the outside (obviously also after stopping holding the down arrow). EDIT: and my auto-repeat is the fastest which can be set on Windows, which I believe is about 30 repeats/sec. EDIT2: hmm.. but it doesn't happen if I save that output as a 50-lines file and use it directly in less in the same loop without that pipe... |
I've bisected this issue to 9ce4c71 ("Implement 23ff6a4 for Windows. Allow any key, not just ctrl-C and ctrl-X, to interrupt a file read.") It reproduces very reliably in bad revisions: it happens in exactly the second iteration of the loop. Maybe it's the cause of both symptoms? |
9ce4c71 changed iread on Windows so that if WIN32kbhit detects a keypress, it ungets the key and returns EOI. The problem is, the next call to iread will then detect that same queued key, unget it, and immediately return, so we are stuck in a loop. Change iread so that it looks only for a non-queued key; that is a newly pressed key from the terminal's input. Related to #607.
|
Let me know if 3894f47 fixes it for you. |
It fixes the loop use case, and seems highly relevant to the other symptom as well, so for now I'm assuming it also got fixed. I'll let you know if it happens again. Thanks. |
Not an issue in master. It resulted from a WIP patch of mine to hack alt-screen instead of new console buffer. I guess I didn't track the handles variables correctly, and it probably applied the mouse-deinit to the wrong console buffer. It reproduces reliably with my alt-screen hack, and goes away immediately if I disable it. So one less thing to worry about. EDIT: WriteConsole(con_out, "\033[?1049h", 8, 0, 0); // ALTSCR ONSo the normal init adds a new console buffer, but then the VT init switches to the alt-screen inside that new buffer, with the expectation that it both won't trash the scrollback because alt-screen (this does work as expected), and it will also make everything go away when returning to the main console buffer at the normal term deinit (this also normally works). That's when I realized that VT init is called with -X, but the normal init isn't, so this hack is broken with -X, first, because it enters alt-screen to begin with - which we probably don't want to do with -X, and then also because it doesn't exit from alt-screen (and also there was no new console buffer because no normal init/deinit, so it actually switches the main buffer to alt-screen, and it remains like that on exit). Hence the 3rd issue in my original post that -X should probably still set console mode even without VT. I don't know whether it's supposed to exit alt-screen when "less" exits (without less explicitly leaving alt-screen), and why the mouse mode remains in effect. Maybe because it stays in alt-screen and that wheel-to-arrows WT auto-thing only applies in alt-screen (which can make sense). But I'll need to keep this issue in mind when writing a proper alt-screen patch (which wouldn't be much bigger, just more correct), and ensure that indeed the mouse mode is restored as it should. So it wasn't for nothing afterall. |
These are verified fixed by this:
Then I reverted the background fix - ecf29c5 with these results:
The above got fixed by the revert. Do keep in mind that with -X it doesn't apply the no-wrap-at-all console mode, and therewore uses whatever mode the console happens to be at. That being said, I think that reverted commit should not have any effect on the behavior in such case, but it did change it for the worse (and fixed by me reverting it just now for the sake of testing).
The above, in all 4 combos of VT on/off and -X on/off, still have this issue in exactly the same form (as far as I can tell), so I'm guessing there's also something else in play, because, again, the autowrap mode commit has no effect when using -X while VT is disabled, because win32_init_term is never called in this case, and reverting the background-fix commit didn't fix it, but it did work in v668. So I bisected it (patching the VT console mode fix where required), which identified the offending commit as da2a9ec ("New strategy for handling highlighting"). Can't say I immediately see a connection between highlight strategy and broken long lines, but hopefully you do. |
|
Here's what I suggest: you handle the issues resulting from the background-fix (which is now only required for djgpp - not win XP/7/8) and the new highlight strategy, because they're really outside of my comfort zone, and I'll do the rest (VT console mode, correct init with and without -X, and everything else which I said I wanted to do). Sounds reasonable? Because I think otherwise we'd step on eachothers toes... Then I'd like to use the updated code as my main "less" for a while, in case some new issues show up (I noticed all the issues in this thread barely minutes after I built it, and there could be more...). |
|
Well this is puzzling. It seems that GetConsoleScreenBufferInfo() is returning an incorrect terminal size if I resize the terminal while running less. For example, I have a cmd window that is 75x30. |
|
After applying the ENABLE_WRAP_AT_EOL_OUTPUT patch, I am not seeing any display issues with long lines, as long as I make sure that |
|
Hm, the problem of GetConsoleScreenBufferInfo doesn't happen if I run less with -X, so I guess it has something to do with the VT stuff. Can the VT be a different size than the real terminal? |
I think it's the same in W10. Resizing the window while less is running and then exiting, and But it's not unique to less. Here's a minimal test program which has in the same behavior: #include <windows.h>
int main() {
HANDLE con = CreateConsoleScreenBuffer(
GENERIC_WRITE | GENERIC_READ,
FILE_SHARE_WRITE | FILE_SHARE_READ,
(LPSECURITY_ATTRIBUTES) NULL,
CONSOLE_TEXTMODE_BUFFER,
(LPVOID) NULL);
SetConsoleActiveScreenBuffer(con);
Sleep(5000);
return 42;
}But if I remove CreateConsoleScreenBuffer and SetConsoleActiveScreenBuffer and changing the size while it sleeps, the mode con thing does show the correct size after this program exists. I can only conclude that the size change is not registered for the main buffer while a non-main console buffer is active. I think this has always been flaky, and if an application seem to not recognize the window size, then usually maximizing and restoring the window fixes it for the currently-running app.
As above says, not the VT. The new console buffer. (-X when VT is supported only disables the new console buffer, but the mode is still set for VT the same way regardless of -X). |
It happens also when the mode thing shows the correct size. Issue 1 (apparently broken by the new highlight strategy thing), happens in all 4 combos of VT-enabled and -X:
Test case:
Expected result: it shows the same view of the previous lines. Actual result: only the remainder of each line enters from the top and shows on screen. The begining of the lines enetering from the top is missing. Issue 2 (fixed by reverting the background-fix commit):
This one seems to happen to me currently only in the windows terminal:
You can also test it by compiling back-to-back commits ecf29c5 (the background fix) - don't forget to disable VT, which does have this issue, vs the commit just before it - 82add99 - don't forget to disable VT - which doesn't have this issue. As noted, VT disabled and -X doesn't change the console mode, so we depend on whatever the initial mode happens to be. This is the 3rd Item in the first message which I wanted to fix - ensure that the console mode is set also with -X when VT is disabled. I didn't try to fix it yet, and I don't know whether it would fix the EOL markers, but I think it might. Regardless, I think it's undeniable that the background-fix commit also affects this, but unless you know better, I think it shouldn't (but it may, if what it thinks it knows about autowrap and ignaw values doesn't match reality). |
|
The first bug (incorrect display when scrolling long lines backwards) should be fixed by 5919111. This was not Windows-specific; it happened on Linux too. |
This is the exactly what happens here. New cmd.exe window apparently normally has only ENABLE_PROCESSED_OUTPUT and ENABLE_WRAP_AT_EOL_OUTPUT bits set, which correspond to autowrap=1 and ignaw=0 in "less", which are the indeed default on windows, and -X with VT disabled doesn't change the mode, so it stays like that, and what less thinks the state is happens to match reality and it works correctly (and does show the EOL markers). But a new cmd.exe tab in the windows terminal apparently also has the ENABLE_VIRTUAL_TERMINAL_PROCESSING bit set (confirmed, at least with WT 1.22), which should have ignaw=1 but it doesn't, and it also doesn't set the console mode with -X and TV disabled, so we still end up with autowrap=1 and ignaw=0, but now this doesn't match reality, and whatever changes the background-fix did apparently change the behavior slightly in this case. But the issue is hopefully not in the background fix, but rather that less assumes an incorrect state of the terminal. I'm guessing that in v668 there's no issue (at least in this case), because before the background-fix it happened to work also in this case, but it was still equally susceptible to such issues, but we were lucky and this doesn't happen in this case. I did a quick test that after less reads the console mode on init, if it has the VT bit set then it sets ignaw=1 and this fixes the issue. But this is due to luck, because there could be other bits set or unset which still make autowrap and ignaw not match reality. The real solution here is what I noted at the first post - that we should set the console mode with -X when VT is disabled, and not leave it to whatever random mode we were launched in. |
Confirmed with this test case. Thanks. Yes, I assumed it would happen on Linux too, but I didn't have the bandwidth to test it there too, and I didn't want to add more noise. There's currently only one Issue I'm aware of (missing EOL marker with -XS and VT disabled), which should be fixed once we set the console mode also with -X when VT is disabled. Shall I take it from here, including handling the missing console mode with the EOL issue, and adding the wrap-at-eol thing in VT mode and the other things I mentioned? |
That was a question. I don't want both of us to work on the same code. |
|
Yes, please go ahead with fixing those issues. |
Ok, I think that makes sense. The autowrap=1 and ignaw=0 case is when less does a clear_eol after printing a line that reaches the right margin (at the end of pdone). It assumes that the terminal has wrapped the cursor to the next line, so clear_eol will clear any erroneous color on the next line. But if the terminal has actually left the cursor at the right margin, then the clear_eol will just clear the last char in the line, so it will erase the > marker. |
Yeah. Your description sounds very reasonable to me. So that's another things we can now explain in even more details than my hand waving :) Anyway, I think it should be pretty straight forward from here. Hopefully I'll get it ready in the next few days. Thanks for the quick fixes. |
|
Can the win32 term init/deinit functions be called more than once during a single run? I don't think deinit clears things up properly (in case we need to run init again), but I can't fully follow the code, so I don't know whether it should do proper cleanup, or only read console mode once on startup, etc. Looks like get_term can be called in psignals, but I don't know whether it's called on windows... EDIT, but that get_term in psignals does get compiled on windows, e.g. if change it to I'd appreciate some insight on this. Thanks. |
|
For reference (and comments), here's how I think it should work:
Thoughts? |
|
Yes, a deinit/init pair is called in several places: in psignals (when handling the terminal stop signal SIGTSTP; I don't know if there is a SIGTSTP on Windows), in pipe_data (called when executing the I think the rest of your points look fine to me. |
|
Thanks. |
|
Hi @avih, any update on this? Are you still working on it? |
|
I put is aside for a bit, but yes. I want to get back. Unlike the SGR thing, there's bo real blocker here other than sitting down and making it happen with some attention. |
In case you plan to do a release, there are some issues on Windows which were not there in v668, and which are probably worth fixing first (I intend to do that, or at least identify the causes):
-X.-Xshould probably be interpreted on windows as "don't use alternate screen or new console buffer", which means the console needs only basic init (e.g. set vt mode), but in non-vt mode it probably should still set the console mode, but doesn't - becausewin32_init_termis not called at all.Other things I want to do:
-D<something>?There might be more things, but I've been using v668 till now, and now I have some time for "less".
The text was updated successfully, but these errors were encountered: