Closed wably closed 7 years ago
The compiler is working correctly.
if (value) ...
The condition is considered to be true and the code executed, for non-zero 'value'. That is how C is defined. The original source code was simply written incorrectly.
-Bill
Bill wrote:
The compiler is working correctly. [...] The original source code was simply written incorrectly.
I agree.
Also, while I'm certainly no VM or VM Assist expert by any stretch of the imagination, after reviewing the code, the other 3 if statements you mentioned:
if(!(EVM_IC(vmb+VMTRCTL) & (VMTRSVC|VMTRPRV|VMTRBRIN)))
if(B_VMPEND & (VMPERPND | VMPGPND))
if(!(B_VMTLEVEL & (VMTON | VMRON)))
seem okay to me.
When I checked the disassemblies of those other statements, the compiler was generating the same kind of code for them as well, which makes sense. For the two statements that have the !-not condition, the Jump instruction was reversed.
So these other statements are not actually correct, because there are other bit flags in the bytes being tested. If any of the other bit flags are set to one besides the actual bits being tested, then a non-zero value results and the Jump is taken (or not taken for the if-! nots). The code in the if-block must only be executed when the tested flags are on, regardless of the setting of the other flags in the same byte.
In my copy of the ecpsvm.c source, I changed these other three statements to:
if(!((EVM_IC(vmb+VMTRCTL) & (VMTRSVC|VMTRPRV|VMTRBRIN)) == (VMTRSVC|VMTRPRV|VMTRBRIN)))
if((B_VMPEND & (VMPERPND | VMPGPND)) == (VMPERPND | VMPGPND))
if(!((B_VMTLEVEL & (VMTON | VMRON)) == (VMTON | VMRON)))
Actually, this statement
if(!((B_VMTLEVEL & (VMTON | VMRON)) == (VMTON | VMRON)))
as I have changed it is incorrect as well. The flags VMTON and VMRON are mutually exclusive; they both cannot be on.
I will locate the corresponding code in CP that this assist represents and find out the true intention and post a corrected statement once I have it. I will cross check the others as well for completeness.
So these other statements are not actually correct, because there are other bit flags in the bytes being tested.
The x86 TEST instruction does not behave the way you think it does. It does not do a subtract; it does just what the name of the instruction implies: it tests the specified bits and sets the zero flag if all of the specified bits are off (i.e. if none of them are on):
The 'C' statement:
if(B_VMESTAT & (VMINVPAG | VMSHADT))
when compiled with Visual Studio 2008, results in the following unoptimized (Debug build) code:
; 946 : /* Invalidate Shadow Tables if necessary */
; 947 : if(B_VMESTAT & (VMINVPAG | VMSHADT))
07350 0f b6 85 c7 04
00 00 movzx eax, BYTE PTR B_VMESTAT$[rbp]
07357 25 81 00 00 00 and eax, 129 ; 00000081H
0735c 85 c0 test eax, eax
0735e 74 70 je SHORT $LN96@ecpsvm_do_@2
which, to me, appears absolutely correct.
The same optimized (Release/Retail build) code is:
; 946 : /* Invalidate Shadow Tables if necessary */
; 947 : if(B_VMESTAT & (VMINVPAG | VMSHADT))
013e9 40 8a 7d 00 mov dil, BYTE PTR B_VMESTAT$1$[rbp]
013ed 40 f6 c7 81 test dil, 129 ; 00000081H
013f1 74 69 je SHORT $LN96@ecpsvm_do_@2
which again, appears to me to be absolutely correct.
The 'C' statement itself is wrong, yes, since we agree the author's original intent was likely to only "do something" if both bits were on (which, as originally coded, it does not do), but the x86 code the compiler generated appears to be correct (or at least it does on my system using VS2008).
May we see the actual generated x86 assembler code for both an unoptimized "Debug" build as well as for an optimized "Release" build? If you're using makefile.bat
on Windows, simply add the -asm
option to your makefile.bat
command. The generated assembler code will be in the xxxx....cod
directory.
The flags VMTON and VMRON are mutually exclusive; they both cannot be on.
Then, as I said, the original 'C' statement, as originally coded, is correct.
The condition will only be false is neither bit is on.
If one or the other or both bits are on however (either situation), then the condition is true and the block of code will be executed (which as I originally said, seems to be what the original author intended and seems correct to me).
Fish,
I was incorrect to state that the TEST instruction used subtraction; my apologies.
For the original offending statement that caused me to open this issue, the retail build assembly code is:
; 1001 : if(B_VMESTAT & (VMINVPAG | VMSHADT))
01431 41 f6 c4 81 test r12b, 129 ; 00000081H
01435 74 23 je SHORT $LN114@ecpsvm_do_
The debug build assembly code is:
; 1001 : if(B_VMESTAT & (VMINVPAG | VMSHADT))
027ad 0f b6 44 24 3c movzx eax, BYTE PTR B_VMESTAT$[rsp]
027b2 25 81 00 00 00 and eax, 129 ; 00000081H
027b7 85 c0 test eax, eax
027b9 74 2b je SHORT $LN114@ecpsvm_do_
This means the original C statement as shown above is incorrect, because if either bit is 1, then the if-block will be executed. It should only be executed if BOTH bits are 1, according to the corresponding CP code (in DMKDSP):
CHKRUNE TM VMESTAT,VMINVPAG+VMSHADT SHADOW TABLES BAD ??
BNO SETCREGS IF NOT - READY TO DISPATCH HIM
CALL DMKVATAB INVALIDATE SHADOW PAGE TABLES
The essence of the call to DMKVATAB is what is inside the if-block. It must only be executed with both bits on.
Regarding the other statements I identified earlier, I am going to state what the CP code says how the bits must be set in order to execute the if-block (I am not stating whether the C statements are correct or incorrect).
For this original statement
if(!(B_VMTLEVEL & (VMTON | VMRON)))
both flags must be off in order to execute the if-block, per the corresponding CP code path.
For this next original statement:
if(B_VMPEND & (VMPERPND | VMPGPND))
per the CP code, if either bit is set to 1, the if-block must be executed.
For the last original statement:
if(!((EVM_IC(vmb+VMTRCTL) & (VMTRSVC|VMTRPRV|VMTRBRIN))
per the CP code, all of the bits must be off in order to execute the if-block.
Then we're in agreement.
Only the first statement you identified is wrong. The other three are okay. Yes?
SUMMARY:
Original code:
WRONG! if(B_VMESTAT & (VMINVPAG | VMSHADT))
OK: if(!(EVM_IC(vmb+VMTRCTL) & (VMTRSVC|VMTRPRV|VMTRBRIN)))
OK: if(B_VMPEND & (VMPERPND | VMPGPND))
OK: if(!(B_VMTLEVEL & (VMTON | VMRON)))
Corrected code:
FIXED: if((B_VMESTAT & (VMINVPAG | VMSHADT)) == (VMINVPAG|VMSHADT))
OK: if(!(EVM_IC(vmb+VMTRCTL) & (VMTRSVC|VMTRPRV|VMTRBRIN)))
OK: if(B_VMPEND & (VMPERPND | VMPGPND))
OK: if(!(B_VMTLEVEL & (VMTON | VMRON)))
Yes?
And your mention of the x86 code the compiler generated was an accidental/unintended temporary distraction. Yes?
Until I hear otherwise I'm going to presume my above understanding of the situation is correct and commit the corresponding changes.
THANK YOU, Peter, for taking the time and effort to look into this for us!
Fish,
I am in agreement. I will reverse my changes to the other three statements that you marked as OK above.
And yes, I would call my mention of the compiler generated code an accident since I misunderstood the way the TEST instruction worked.
Thank you, and you Peter, for taking the time to straighten this out!
THANK YOU, Peter, for taking the time and effort to look into this for us!
(Oops!) A thousand pardons, Bob!
I forgot who I was talking to! (Doh!)
That of course should be:
"THANK YOU, Bob, for taking the time and effort to look into this for us!"
My apologies.
On 1/30/2017 5:30 PM, Fish-Git wrote:
"/THANK YOU, Bob/, for taking the time and effort to look into this for us!" I'll add my support here !
I can only thank someone looking at my feeble attempt at ECPS:VM... DISP1 and DISP2 were a headscratcher from the start ;) DMKFREEX and DMKFRETX weren't that hard !
My only reassurance is that the original developer of ECPS:VM (Lynn Wheeler) indicated in a forum that I was on the right track - basically implementing whatever is done in CP (for CPA).
I'm glad you are looking into it - because one thing I never tested was CPA/VMA with a virtual machine not running CMS ! (Never tried VM under VM or OS/VS under VM or DOS under VM).
One last thing is Interval Timer assist... But it's another story overall !
--Ivan
closing; fixed by commit of 3/4/2017
The ECPS supported dispatcher assist DISP2 is incorrectly returning to CP rather than completing the dispatch of the run user. This condition occurs with virtual machines that themselves have virtual storage and therefore require shadow page tables. The DISP2 assist has code that checks for the presence of shadow tables and is supposed to exit back to CP if those shadow tables need to be invalidated.
However, that is not what is happening. DISP2 is exiting if shadow tables are present, regardless of whether they need to be invalidated. This means that DISP2 will never complete its assist for guest operating systems in a virtual machine.
This is not so much an issue in the original code as written in ecpsvm.c; the intention is correct. The problem is what is questionable code generation by the compiler in my mind. The assist contains this statement:
The intention here is to execute the if-block when shadow tables are present AND they need to be invalidated. If they do not need to be invalidated the if-block should fall through. The compiler (Visual Studio 2012) is generating the following two lines of code:
The 81h is the combined value of the two flags VMINVPAG and VMSHADT. On an x86/x64 platform the TEST instruction works by subtracting the specified bits from the register and setting flags based on the result. If the resulting byte is all zeros, the zero flag (ZF) is set. The JE instruction takes the branch if ZF is set. A common value for B_VMESTAT during debugging is x'90'. Applying the TEST rules, the resulting byte is non-zero, and so the JE instruction will not branch thereby executing the if-block content. This is exactly the opposite of what the original intention is! And as a result DISP2 exits to CP (that's what is in the if-block).
This issue can be corrected by simply reworking the if-statement:
which causes the compiler to generate proper code to analyze the correct bits and branch accordingly.
The original author does have other if-statements coded in the correct way. However, I found three more statements which were of the incorrect form and they should also be fixed. Note that I did not actually experience an issue with these other if-statements as I was not using the facilities that they pertain to. The other if-statements are: