[PATCH 0/1] MR6780: Avoid the for-loop be optimized out
This stops GCC from optimizing out a `for` loop inside the `dump_cv_sst_src_module()` function, when tools/winedump is compiled with optimization settings -O1 or higher. Because the `baseSrcLn` field of the `struct OMFSourceFile` was declared as an array of one element **and it is an interior member** of the structure: ```c typedef struct OMFSourceFile { unsigned short cSeg; unsigned short reserved; unsigned int baseSrcLn[1]; unsigned short cFName; char Name; } OMFSourceFile; ``` The compiler/optimizer assumes that the `baseSrcLn` array may have only zero or one element. And generates the corresponding code. The following shows code that GCC 14.2.1 generates before and after this patch. Before patch: ``` 524f: e8 0c ef ff ff call 4160 <printf(a)plt> # this printf(3)'s "File table: ..." # r15 stores sourceFile->cSeg; assigned above. 5254: 66 41 83 3c 24 00 cmpw $0x0,(%r12) # r12 is sourceFile 525a: 74 22 je 527e <dump_codeview+0x70e> 525c: 4b 8d 44 bc 04 lea 0x4(%r12,%r15,4),%rax #rax is seg_info_dw 5261: 45 8b 44 24 04 mov 0x4(%r12),%r8d # sourceFile->baseSrcLn[0] 5266: be 01 00 00 00 mov $0x1,%esi 526b: 48 8d 3d fe 02 04 00 lea 0x402fe(%rip),%rdi # 45570 5272: 8b 48 04 mov 0x4(%rax),%ecx 5275: 8b 10 mov (%rax),%edx 5277: 31 c0 xor %eax,%eax 5279: e8 e2 ee ff ff call 4160 <printf(a)plt> 527e: 41 0f b6 06 movzbl (%r14),%eax # eax = sourceModule + ofs ``` The code checks if the `sourceFile->cSeg` is zero or not, and if not it `printf(3)`s information about the first element in the `baseSrcLn` and first offset pair. There is no `for` loop there, only its first iteration. After patch: ``` 525c: e8 ff ee ff ff call 4160 <printf(a)plt> # this printf(3)'s "File table: ..." # r12 is seg_info_dw and r15 stores integer 1; both assigned above. # r15 is `i' -- the loop counter. 5261: 66 41 83 3e 00 cmpw $0x0,(%r14) # r14 is sourceFile 5266: 74 37 je 529f <dump_codeview+0x72f> 5268: 0f 1f 84 00 00 00 00 nopl 0x0(%rax,%rax,1) 526f: 00 5270: 43 8b 54 fc f8 mov -0x8(%r12,%r15,8),%edx 5275: 43 8b 4c fc fc mov -0x4(%r12,%r15,8),%ecx 527a: 44 89 fe mov %r15d,%esi 527d: 31 c0 xor %eax,%eax 527f: 47 8b 04 be mov (%r14,%r15,4),%r8d # sourceFile->baseSrcLn[i] 5283: 48 8d 3d e6 02 04 00 lea 0x402e6(%rip),%rdi # 45570 528a: 49 83 c7 01 add $0x1,%r15 528e: e8 cd ee ff ff call 4160 <printf(a)plt> 5293: 41 0f b7 06 movzwl (%r14),%eax 5297: 41 8d 57 ff lea -0x1(%r15),%edx 529b: 39 c2 cmp %eax,%edx 529d: 7c d1 jl 5270 <dump_codeview+0x700> 529f: 48 8b 44 24 18 mov 0x18(%rsp),%rax # rax = sourceModule + ofs ``` The `for` loop now is where it should be. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6780
From: Ruslan Garipov <ruslanngaripov(a)gmail.com> This stops GCC from optimizing out a for-loop inside the dump_cv_sst_src_module() function, when tools/winedump is compiled with optimization settings -O1 or higher. Because the baseSrcLn field of the 'struct OMFSourceFile' was declared as an array of one element **and it is an interior member** of the structure: typedef struct OMFSourceFile { unsigned short cSeg; unsigned short reserved; unsigned int baseSrcLn[1]; unsigned short cFName; char Name; } OMFSourceFile; The compiler/optimizer assumes that the baseSrcLn array may have only zero or one element. And generates the corresponding code. The following shows code that GCC 14.2.1 generates before and after this patch. Before patch: # this printf(3)'s "File table: ..." 524f: call 4160 <printf(a)plt> # r15 stores sourceFile->cSeg; assigned above. 5254: cmpw $0x0,(%r12) # r12 is sourceFile 525a: je 527e <dump_codeview+0x70e> 525c: lea 0x4(%r12,%r15,4),%rax #rax is seg_info_dw 5261: mov 0x4(%r12),%r8d # sourceFile->baseSrcLn[0] 5266: mov $0x1,%esi 526b: lea 0x402fe(%rip),%rdi # 45570 5272: mov 0x4(%rax),%ecx 5275: mov (%rax),%edx 5277: xor %eax,%eax 5279: call 4160 <printf(a)plt> 527e: movzbl (%r14),%eax # eax = sourceModule + ofs The code checks if the sourceFile->cSeg is zero or not, and if not it printf(3)'s information about the first element in the baseSrcLn and first offset pair. There is no for-loop there, only its first iteration. After patch: # this printf(3)'s "File table: ..." 525c: call 4160 <printf(a)plt> # r12 is seg_info_dw and r15 stores integer 1; both assigned above. # r15 is `i' -- the loop counter. 5261: cmpw $0x0,(%r14) # r14 is sourceFile 5266: je 529f <dump_codeview+0x72f> 5268: nopl 0x0(%rax,%rax,1) 526f: 5270: mov -0x8(%r12,%r15,8),%edx 5275: mov -0x4(%r12,%r15,8),%ecx 527a: mov %r15d,%esi 527d: xor %eax,%eax 527f: mov (%r14,%r15,4),%r8d # sourceFile->baseSrcLn[i] 5283: lea 0x402e6(%rip),%rdi # 45570 528a: add $0x1,%r15 528e: call 4160 <printf(a)plt> 5293: movzwl (%r14),%eax 5297: lea -0x1(%r15),%edx 529b: cmp %eax,%edx 529d: jl 5270 <dump_codeview+0x700> 529f: mov 0x18(%rsp),%rax # rax = sourceModule + ofs The for-loop now is where it should be. Signed-off-by: Ruslan Garipov <ruslanngaripov(a)gmail.com> --- tools/winedump/debug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/winedump/debug.c b/tools/winedump/debug.c index e62e4436e4d..5cf63391e06 100644 --- a/tools/winedump/debug.c +++ b/tools/winedump/debug.c @@ -285,7 +285,7 @@ static BOOL dump_cv_sst_src_module(const OMFDirEntry* omfde) for (i = 0; i < sourceFile->cSeg; i++) { printf (" Segment #%2d start = 0x%x, end = 0x%x, offset = 0x%x\n", - i + 1, seg_info_dw[i * 2], seg_info_dw[(i * 2) + 1], sourceFile->baseSrcLn[i]); + i + 1, seg_info_dw[i * 2], seg_info_dw[(i * 2) + 1], ((const unsigned int *)sourceFile->baseSrcLn)[i]); } /* add file name length */ ofs += *(const BYTE*)((const char*)sourceModule + ofs) + 1; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6780
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=149505 Your paranoid android. === debian11b (64 bit WoW report) === kernel32: comm.c:1586: Test failed: Unexpected time 1002, expected around 500
Seems to me, the later fields are unusable (as we'd have to calculate their true offset dynamically) and should be removed. The `OMFSourceLine` structure looks similar. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6780#note_86938
yes, and please use same structure + comment as already done elsewhere in mscvpdb.h (terminating the struct with 0-length char array + commenting the remaining serialized fields) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6780#note_86959
participants (5)
-
eric pouech (＠epo) -
Esme Povirk (＠madewokherd)
-
Marvin -
Ruslan Garipov
-
Ruslan Garipov (＠ruslangaripov)