Oh and about preserving the registers in my newKeyHandler - the "interrupt" keyword I use in the handler definition is supposed to "save all CPU registers and terminate the funcion with IRET" so I guess I dont need to explicitly PUSH/POP anything there.

I've reached into the manuals.
It clearly states the 'interrupt' function modifier generates prologue/epilogue to save and restore the registers, so you don't have to do that yourself.
Also it states (and I can confirm that with disassembly) that the DS register is loaded with the data segment of the program, except in 'huge' model, where DS is set to the data segment of the module where the function is defined.

So, yeah. You don't really need to take care of DS or other registers, Borland C++ 3.1 does that for you.

WhiteFalcon wrote on 2024-05-07, 06:36:

I understand, but what happens when you make sure DS is correct and then do LDS SI, (DS:)[variable]? Will it know where the variable is before it changes the DS or will it first change DS so it no longer has the correct segment of the variable, hence it loads some nonsesical adress from the wrong location?

Assume it loads all the bytes in one instant then sets the new values in DS and SI.

vstrakh wrote on 2024-05-07, 06:41:

So, yeah. You don't really need to take care of DS or other registers, Borland C++ 3.1 does that for you.

I can confirm that now too, I checked the generated asm. It pushes everything. Dont want to bloat it here too much, so please check this:
https://paste.ofcode.org/4enD6JkKZMxbYRt96ht7Zq

Maybe you can confirm if its getting the right location of screen_state from the asm?
Because I tried the modification you suggested and its still crashing exactly the same. I tried commenting out the Store part, just restore and it doesnt crash! It restores the screen with garbage apparently, but can be done repeatedly. Which means everything else should be fine and the problem really lies with the address of the virtual buffer.
I modified both of course, but for simplicity, just the current Store function:

1void StoreScreen(void)
2{
3	asm {
4		push ds
5		push es
6		pusha
7		pushf
8
9		mov ah, 0x03		// Store cursor position
10		xor bh, bh
11		int 0x10
12		mov [oldx], dl
13		mov [oldy], dh
14
15		mov ax, SEG screen_state
16		mov ds, ax
17		les di, screen_state		// Store video memory
18		mov cx, 80 * 25
19		mov ax, 0xB800
20		mov ds, ax
21		xor si, si
22		cld
23		rep movsw
24
25		popf
26		popa
27		pop es
28		pop ds
29	}
30}
31

jmarsh wrote on 2024-05-07, 06:43:

Assume it loads all the bytes in one instant then sets the new values in DS and SI.

Good to know, thanks.

WhiteFalcon wrote on 2024-05-07, 07:00:

Maybe you can confirm if its getting the right location of screen_state from the asm?

See the lines 40/41. It loads the DGROUP segment into BP then moves it into DS. It's exactly the same what gets loaded into AX in the line 84.
You didn't need to load DS explicitly later.

WhiteFalcon wrote on 2024-05-07, 07:00:

Because I tried the modification you suggested and its still crashing exactly the same.

It means the issue is elsewhere. The data segment is handled for you (as it turned out), and direction flag was fixed. There's something else.

vstrakh wrote on 2024-05-07, 07:10:

See the lines 40/41. It loads the DGROUP segment into BP then moves it into DS. It's exactly the same what gets loaded into AX in the line 84.
You didn't need to load DS explicitly later.

So it means BC++ generates safer code than expected, at least that. It was actually a bit to be expected because I just realized I am storing/restoring the cursor position there into global variables with no treatment of DS and that part worked from the start.

It means the issue is elsewhere. The data segment is handled for you (as it turned out), and direction flag was fixed. There's something else.

Dare I ask what? 😀 Is the asm code produced any clearer to spot the problem? It seems full of pushes and pops so security is tight..
Its a surprise to see screen_state defined as db 4 instead of a dd, but I assume thats normal and not a problem.

There is a point to clarify...
You've used malloc() to allocate memory. That memory comes from the heap when the app is loaded, and the heap is the entire low memory above your exe image.
But then you exit to make your app TSR, the heap is discarded. The save_state now points past the app executable image, and new apps are loaded there.
When you save the screen, you're destroying whatever is running, because the pointer points to the memory you do not own anymore.

Try declaring your saving area as a static array first, avoid using heap which will not exist after your app exists when installing TSR.

vstrakh wrote on 2024-05-07, 07:20:

There is a point to clarify... You've used malloc() to allocate memory. That memory comes from the heap when the app is loaded, […]
Show full quote

There is a point to clarify...
You've used malloc() to allocate memory. That memory comes from the heap when the app is loaded, and the heap is the entire low memory above your exe image.
But then you exit to make your app TSR, the heap is discarded. The save_state now points past the app executable image, and new apps are loaded there.
When you save the screen, you're destroying whatever is running, because the pointer points to the memory you do not own anymore.

Try declaring your saving area as a static array first, avoid using heap which will not exist after your app exists when installing TSR.

Well said, at first I had the malloc/free placed in mainTSR so that it got allocated every time the TSR was activated, maybe thats a better approach? I have had bad experience with combining static array and inline asm, it never worked as I expected and was another source of frustration. From what I have read, C lets you approach static and dynamic arrays (pointers) the same way, but asm does not.

vstrakh wrote on 2024-05-07, 07:20:

Try declaring your saving area as a static array first, avoid using heap which will not exist after your app exists when installing TSR.

Yay, you nailed it!! When I changed screen_state to a static array, of course the asm complained with the LES/LDS commands. So I created a pointer to this static array and now it works.
Thank you, thank you! 😀

For the record, I changed it back to dynamic, but allocate/free it every time the TSR is called and it works too now. Amazing.

WhiteFalcon wrote on 2024-05-07, 07:37:

at first I had the malloc/free placed in mainTSR so that it got allocated every time the TSR was activated, maybe thats a better approach?

You can't use malloc() after you exit the app. The memory is already not yours, it belongs to the app running after you exit - either transient part of command.com, or some app - all of the memory belongs to that new app, there are no free parts to cut into.
You must allocate entire needed buffer in the data segment, which will survive the exit, and take few additional KBs of conventional memory, but it will be exclusively yours.
There is a more complex alternative with XMS - allocate as much extended memory as you want, and save/restore screen there. The handle of XMS block will live after the app exits, so is the memory block.

WhiteFalcon wrote on 2024-05-07, 07:37:

I have had bad experience with combining static array and inline asm, it never worked as I expected and was another source of frustration. From what I have read, C lets you approach static and dynamic arrays (pointers) the same way, but asm does not.

For simplicity declare the array with different name, and save the pointer to that array into save_state variable, so your asm doesn't change.

WhiteFalcon wrote on 2024-05-07, 07:43:

For the record, I changed it back to dynamic, but allocate/free it every time the TSR is called and it works too now. Amazing.

You cannot allocate/free the heap memory after you exit - there is no heap to allocate from.

vstrakh wrote on 2024-05-07, 07:48:

You must allocate entire needed buffer in the data segment, which will survive the exit, and take few additional KBs of conventional memory, but it will be exclusively yours.

Thats strange, it seems to work now in both EDIT and BC++, with dynamic allocation. But I will change it back to static to be safe, I did use a different buffer so can keep all the code the same.

There is a more complex alternative with XMS - allocate as much extended memory as you want, and save/restore screen there. The handle of XMS block will live after the app exits, so is the memory block.

Exactly what I was thinking about this morning, but it would definitely bring more trouble to solve and even if not, it would make the program larger. As its written in BC++ and not pure asm, its already bigger than it should be and I will need it to access a file and it (maybe partially) into memory, so it will need even more memory. Which means I will have to allocate another buffer for the text file to be read in later.

I mean I hear you, but I have a NULL test in there to return when the memory is not allocated and it is still working, even when called from BC++. Maybe the malloc function is just misfiring and not reporting the problem because its not meant to be used in a TSR?

WhiteFalcon wrote on 2024-05-07, 07:54:

Thats strange, it seems to work now in both EDIT and BC++, with dynamic allocation.

"Seems to work" is a bad argument with dos 😀 It works until conditions change radically enough to break your assumptions.

WhiteFalcon wrote on 2024-05-07, 07:54:

There is a more complex alternative with XMS - allocate as much extended memory as you want, and save/restore screen there. The handle of XMS block will live after the app exits, so is the memory block.

Exactly what I was thinking about this morning, but it would definitely bring more trouble to solve and even if not, it would make the program larger.

4KB of static buffer vs 200 bytes of code to allocate and use XMS - that's a worth trade.

vstrakh wrote on 2024-05-07, 08:00:

"Seems to work" is a bad argument with dos 😀 It works until conditions change radically enough to break your assumptions.

True, I will change it back to static.

4KB of static buffer vs 200 bytes of code to allocate and use XMS - that's a worth trade.

Good point. I will try that if the buffers start to be too big. I believe I will face enough adversity when trying to read a file from TSR, seeing as even grabbing a text screen was a lot of trouble 😀

So I changed it to static and it works fine in DOSBox, but it freezes instantly in real DOS 6.22 on my 486 with a weird buzzing sound, probably from the speaker 🙁 I know DOSBox is meant to work great for games and possibly not other software, but its still quite a disappointment.

WhiteFalcon wrote on 2024-05-07, 08:32:

So I changed it to static

Post the code 😀

vstrakh wrote on 2024-05-07, 08:38:

Post the code 😀

I know I am being a bother, but why is my happiness alway SO short lived? 😀 The code works great in DOSBox so why does it wreak havoc on the real thing?
It does load and show the text message there with the buzzing sound and a hard lockup. So it could be just anything, even the key interrupt, will probably have to try commenting it all out and testing line by line..

https://paste.ofcode.org/i2dLjiqXzAfYCtjX7fHmav

So far I don't like the use of CLI/STI within the interrupt handler.
When interrupt handler is entered the interrupts are disabled already, so no need in CLI.
When you exit the interrupt you don't need STI, because flags will be restored by IRET.
If you enable interrupts before entering the original handler - this means the IRQ was not acknowledged/cleared yet with the PIC (the original handler does that), so you have the same interrupt triggered again and again because the PIC still asserts the same interrupt request.

Also, according to Borland manuals, the interrupt handler does not take any arguments, yet you've declared is as variadic function (...).