libx86emu
libx86emu copied to clipboard
wfeldt
Update decode.c
(1) Add a 32-bit stack in place of the current 16-bit stack so that the code is fully 32-bit.
(2) Allow room for the NULL terminating byte. (One of my compilers caught this).
I am running under Windows and Ubuntu. I also adapted the code to compile as C++.
Apparently it failed the 0008_seg_load_pm32.init regression test. I don't understand the libx86emu virtual memory system (especially related to the stack) well enough to proceed from here.
The compiler always treats this code as a "c" string: static char seg_name[7] = "ecsdfg?"; and appends a NULL terminating byte I have shown proof of this below:
#include
static char seg_name[7] = "ecsdfg?"; // original code static char seg_name1[] = "ecsdfg?"; static char seg_name2[] = {'e','c','s','d','f','g','?'};
// main() returns void to reduce clutter void main(int argc, char *argv[]) { //printf("sizeof(seg_name)=%d\n", sizeof(seg_name)); printf("sizeof(seg_name1)=%d\n", sizeof(seg_name1)); printf("sizeof(seg_name2)=%d\n", sizeof(seg_name2)); }
/*** compile Microsoft (R) C/C++ Optimizing Compiler Version 19.16.27026.1 for x86 Copyright (C) Microsoft Corporation. All rights reserved. test_seg_name.cpp test_seg_name.cpp(4): error C2117: 'seg_name': array bounds overflow test_seg_name.cpp(4): note: see declaration of 'seg_name'
run D:>test_seg_name sizeof(seg_name1)=8 sizeof(seg_name2)=7 ***/
------ Original Message ------ From: "Steffen Winterfeldt" [email protected] To: "wfeldt/libx86emu" [email protected] Cc: "plolcott" [email protected]; "Author" [email protected] Sent: 6/22/2020 10:26:26 AM Subject: Re: [wfeldt/libx86emu] Update decode.c (#30)
@wfeldt commented on this pull request.
In decode.c https://github.com/wfeldt/libx86emu/pull/30#discussion_r443641989:
@@ -1844,7 +1845,7 @@ void log_regs(x86emu_t *emu) void check_data_access(x86emu_t *emu, sel_t *seg, u32 ofs, u32 size) { char **p = &emu->log.ptr;
- static char seg_name[7] = "ecsdfg?";
- static char seg_name[8] = "ecsdfg?";
The original length is correct. It's not a string, just an array of chars. And the code clearly limits the access to index 6.
I see no warning in gcc 10, which is usually quite picky. Maybe your compiler has a way to tell it that here not a asciiz-string is meant.
If I add the 0 byte just to shut a warning down, I might get a warning about an unused byte by some other compiler later.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/wfeldt/libx86emu/pull/30#pullrequestreview-435034996, or unsubscribe https://github.com/notifications/unsubscribe-auth/AODSHISGCOGV3YTVIKHI2KTRX5Z2FANCNFSM4ODG744Q.
I don't understand the virtual memory system very well at all especially for stack usage.
When I ran the unmodified code the debug output seemed to indicate that it was using a 16-bit stack. When I tested this code to force it to overflow the 16-bit stack it failed indicating that it was a 16-bit stack. It did a fixed number of recursive calls to the same function.
With the change that I made trying to force a 16-bit overflow seemed to indicate that the stack was now in 32-bit mode. I thought that it would be a good idea to get this change reviewed because this was the only change that I made that could have side-effects.
The change to the demo program adding 32-bit mode was enormously helpful for my project. It would be really great if we could also get a 32-bit stack. I diligently strove to make absolute minimal changes to the code base to transform the code so it would work as C++ under Windows and Ubuntu.
------ Original Message ------ From: "Steffen Winterfeldt" [email protected] To: "wfeldt/libx86emu" [email protected] Cc: "plolcott" [email protected]; "Author" [email protected] Sent: 6/22/2020 10:17:39 AM Subject: Re: [wfeldt/libx86emu] Update decode.c (#30)
@wfeldt commented on this pull request.
In decode.c https://github.com/wfeldt/libx86emu/pull/30#discussion_r443635885:
@@ -91,6 +91,7 @@ API_SYM unsigned x86emu_run(x86emu_t *emu, unsigned flags)
if(ACC_D(emu->x86.R_CS_ACC)) { emu->x86.mode |= _MODE_DATA32 | _MODE_ADDR32 | _MODE_CODE32;
- emu->x86.mode |= _MODE_STACK32; // 2020-05-31 to be reviewed
No, that's not correct. The CS selector's D bit provides only the default for code and ('normal') data accesses - in absense of a segment prefix. For stack accesses always the SS segment info is used. IIRC I did some experiments on real hardware to get this implemented correctly back then.
I might be wrong, though. So, if you have a small code blob that works differently than the emulation, please post it here.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/wfeldt/libx86emu/pull/30#pullrequestreview-435026934, or unsubscribe https://github.com/notifications/unsubscribe-auth/AODSHIQ2KIRFXHIAR6FWZ5LRX5YZHANCNFSM4ODG744Q.
I don't understand the virtual memory system very well at all especially for stack usage.
Actually, I misread your original comment. I assumed you were still talking about x86emu-demo.c.
So, it would be correct to also set the stack to 32bit in 32 bit mode here:
https://github.com/wfeldt/libx86emu/blob/master/demo/x86emu-demo.c#L161-L162
Your proposal in this pr is not correct. The Intel processor manual says about the push instruction:
Decrements the stack pointer and then stores the source operand on the top of the stack. The address-size attribute of the stack segment determines the stack pointer size (16 bits or 32 bits), and the operand-size attribute of the current code segment determines the amount the stack pointer is decremented (2 bytes or 4 bytes). For example, if these address- and operand-size attributes are 32, the 32-bit ESP register (stack pointer) is decremented by 4 and, if they are 16, the 16-bit SP register is decremented by 2. (The B flag in the stack segment’s segment descriptor determines the stack’s address-size attribute, and the D flag in the current code segment’s segment descriptor, along with prefixes, determines the operand-size attribute and also the address-size attribute of the source operand.)
Attached is the code that I used to test the stack depth. It succeeds under my change and fails under your change. It has 1000 invocations with 128 byes of data per invocation.
#include <stdio.h> #include <stdint.h> #include <stdlib.h> #include <time.h>
int N = 0x11117777;
void Test_Recursive_Depth(int X, int X1, int X2, int X3, int X4, int X5, int X6, int X7, int X8, int X9, int XA, int XB, int XC, int XD, int XE, int XF, int Y0, int Y1, int Y2, int Y3, int Y4, int Y5, int Y6, int Y7, int Y8, int Y9, int YA, int YB, int YC, int YD, int YE, int YF) { // N is global while (N <= X) { N++; Test_Recursive_Depth(X,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15, 16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31); } }
int main() { N = 0; Test_Recursive_Depth(1000,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15, 16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31); __asm HLT return 1; }
This is not the question. I agree that the demo code should also enable 32-bit mode for the stack segment if --32 is used.
What I'm saying is that your patch - which derives the stack layout type from the CS segment attributes - is wrong.
Yes I understand. Mine is wrong because it breaks correct emulation. We can tell because it flunks the regression test. Now we also have another test to see if the 32-bit stack is working. The 32-bit stack test executes until completion of 50,000 instructions with my change that breaks emulation.
My next plan is to re-enable the regression tests on my code. My code is currently based on static linking.
------ Original Message ------ From: "Steffen Winterfeldt" [email protected] To: "wfeldt/libx86emu" [email protected] Cc: "plolcott" [email protected]; "Author" [email protected] Sent: 6/24/2020 4:39:28 AM Subject: Re: [wfeldt/libx86emu] Update decode.c (#30)
This is not the question. I agree that the demo code should also enable 32-bit mode for the stack segment if --32 is used.
What I'm saying is that your patch - which derives the stack layout type from the CS segment attributes - is wrong.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/wfeldt/libx86emu/pull/30#issuecomment-648713327, or unsubscribe https://github.com/notifications/unsubscribe-auth/AODSHITAEMOTBFLVSIWNBRLRYHCVBANCNFSM4ODG744Q.
I didn't realize that you gave me the whole answer in your last reply. I added this at the spot that you highlighted: emu->x86.R_SS_ACC |= (1 << 10); I tested it with my test code and obviously it worked.
In the mean time I very carefully re-applied all of the changes that so that the library now compiles under Windows and Linux as "c" and "c++" and passes all the regression tests. I made sure to make the absolute minimum changes required for the port. This makes a DIFF show all the changes very cleanly.
The port to Windows mostly required making the Linux lib stuff invisible to Windows. It would only take a little more work to make this a Windows DLL under Windows. The port to C++ mostly required explicit casts.
Thanks again for all your help.
I made the correct changes to the correct file this time.
I hardly know how github works at all so I may have updated the pull request incorrectly.
I did create another pull request but it didn't show up.
It looks like the system allowed me to directly merge to master. I didn't mean to do that.
I made the correct changes to the correct file this time.
Kind of; there's still the original commit; and the file you changed got added to the wrong directory.
I hardly know how github works at all so I may have updated the pull request incorrectly.
That's how it looks like.
I did create another pull request but it didn't show up.
Yup, I didn't see any.
It looks like the system allowed me to directly merge to master. I didn't mean to do that.
No, you did not merge into master here; possibly on your fork.
I can do a pr for the 32 bit one-line change, no problem. But if you have bigger changes in mind, like your C compiler adjustments, please do a clean new pull request.
That is great, thanks. I know that making fewer changes is better because it reduces the chance that any change will be a breaking change. (I have been a software engineer foe three decades.) I went through the whole process again several times and there are only a few changes to get it to build as "C" in visual studio without errors or warnings. Getting rid of the errors only required hiding the Linux attribute stuff from windows and using alternative port i/o functions. Getting rid of the warnings required adding some casts and turning off the warning related to strcat() safety. This will build as a visual studio project or from the command line batch file. I am proceeding to transforming this into building a Microsoft DLL. As soon as I have this much done I will create another pull request.