[PATCH v10 0/5] MR10673: oleaut32: Fix Null handling in three-valued logical ops.
VarAnd only inspected the non-Null operand one way and left V_BOOL/V_I4 uninitialized. VarImp treated VT_UI1 0xFF as VARIANT_TRUE (it isn't) and its VT_CY -1 check compared unscaled .int64 to -1 instead of -10000. props to @jsm174 for this one: https://github.com/vpinball/libwinevbs/commit/7e877fafe34483623ca33d26ab76fd... -- v10: vbscript/tests: Add Null handling smoke tests for logical operators. oleaut32: Fix VT_DATE Null handling in VarImp. vbscript: Match native UI1 Imp Null behavior. oleaut32: Fix VT_CY Null handling in VarImp. oleaut32: Fix Null handling in VarAnd. https://gitlab.winehq.org/wine/wine/-/merge_requests/10673
From: Francis De Brabandere <francisdb@gmail.com> Handle both operand orderings symmetrically and always write the result payload. Convert VARAND_TODOCOMM back to VARAND for the cases the fix makes commutative, and drop the now-unused macro. --- dlls/oleaut32/tests/vartest.c | 43 +++++------- dlls/oleaut32/variant.c | 121 +++++++++++++++++++++++----------- 2 files changed, 98 insertions(+), 66 deletions(-) diff --git a/dlls/oleaut32/tests/vartest.c b/dlls/oleaut32/tests/vartest.c index db357cf77ff..c362f9854fe 100644 --- a/dlls/oleaut32/tests/vartest.c +++ b/dlls/oleaut32/tests/vartest.c @@ -55,8 +55,6 @@ static WCHAR sz12_true[32]; /* Has I8/UI8 data type? */ static BOOL has_i8; -static BOOL is_win; - /* When comparing floating point values we cannot expect an exact match * because the rounding errors depend on the exact algorithm. */ @@ -434,8 +432,6 @@ static void init(void) BSTR bstr; HRESULT res; - is_win = !strcmp(winetest_platform, "windows"); - res = VarBstrFromBool(VARIANT_TRUE, LANG_USER_DEFAULT, VAR_LOCALBOOL, &bstr); ok(res == S_OK && bstr[0], "Expected localized string for 'True'\n"); /* lstrcpyW / lstrcatW do not work on win95 */ @@ -6800,15 +6796,6 @@ static HRESULT (WINAPI *pVarAnd)(LPVARIANT,LPVARIANT,LPVARIANT); V_VT(&exp) = VT_##rvt; V_##rvt(&exp) = rval; \ test_var_call2_commutative( __LINE__, pVarAnd, &left, &right, &exp ) -#define VARAND_TODOCOMM(vt1,val1,vt2,val2,rvt,rval) \ - V_VT(&left) = VT_##vt1; V_##vt1(&left) = val1; \ - V_VT(&right) = VT_##vt2; V_##vt2(&right) = val2; \ - V_VT(&exp) = VT_##rvt; V_##rvt(&exp) = rval; \ - if (is_win) \ - test_var_call2_commutative( __LINE__, pVarAnd, &left, &right, &exp ); \ - else \ - test_var_call2( __LINE__, pVarAnd, &left, &right, &exp ); \ - #define VARANDCY(vt1,val1,val2,rvt,rval) \ V_VT(&left) = VT_##vt1; V_##vt1(&left) = val1; \ V_VT(&right) = VT_CY; V_CY(&right).int64 = val2; \ @@ -6990,35 +6977,35 @@ static void test_VarAnd(void) VARAND(NULL,0,NULL,0,NULL,0); VARAND(NULL,1,NULL,0,NULL,0); VARAND(NULL,0,I1,0,I4,0); - VARAND_TODOCOMM(NULL,0,I1,1,NULL,0); + VARAND(NULL,0,I1,1,NULL,0); VARAND(NULL,0,UI1,0,UI1,0); - VARAND_TODOCOMM(NULL,0,UI1,1,NULL,0); + VARAND(NULL,0,UI1,1,NULL,0); VARAND(NULL,0,I2,0,I2,0); - VARAND_TODOCOMM(NULL,0,I2,1,NULL,0); + VARAND(NULL,0,I2,1,NULL,0); VARAND(NULL,0,UI2,0,I4,0); - VARAND_TODOCOMM(NULL,0,UI2,1,NULL,0); + VARAND(NULL,0,UI2,1,NULL,0); VARAND(NULL,0,I4,0,I4,0); - VARAND_TODOCOMM(NULL,0,I4,1,NULL,0); + VARAND(NULL,0,I4,1,NULL,0); VARAND(NULL,0,UI4,0,I4,0); - VARAND_TODOCOMM(NULL,0,UI4,1,NULL,0); + VARAND(NULL,0,UI4,1,NULL,0); if (has_i8) { VARAND(NULL,0,I8,0,I8,0); - VARAND_TODOCOMM(NULL,0,I8,1,NULL,0); + VARAND(NULL,0,I8,1,NULL,0); VARAND(NULL,0,UI8,0,I4,0); - VARAND_TODOCOMM(NULL,0,UI8,1,NULL,0); + VARAND(NULL,0,UI8,1,NULL,0); } VARAND(NULL,0,INT,0,I4,0); - VARAND_TODOCOMM(NULL,0,INT,1,NULL,0); + VARAND(NULL,0,INT,1,NULL,0); VARAND(NULL,0,UINT,0,I4,0); - VARAND_TODOCOMM(NULL,0,UINT,1,NULL,0); - VARAND_TODOCOMM(NULL,0,BOOL,0,BOOL,0); - VARAND_TODOCOMM(NULL,0,BOOL,1,NULL,0); + VARAND(NULL,0,UINT,1,NULL,0); + VARAND(NULL,0,BOOL,0,BOOL,0); + VARAND(NULL,0,BOOL,1,NULL,0); VARAND(NULL,0,R4,0,I4,0); - VARAND_TODOCOMM(NULL,0,R4,1,NULL,0); + VARAND(NULL,0,R4,1,NULL,0); VARAND(NULL,0,R8,0,I4,0); - VARAND_TODOCOMM(NULL,0,R8,1,NULL,0); - VARAND_TODOCOMM(NULL,0,BSTR,false_str,BOOL,0); + VARAND(NULL,0,R8,1,NULL,0); + VARAND(NULL,0,BSTR,false_str,BOOL,0); VARAND(NULL,0,BSTR,true_str,NULL,VARIANT_FALSE); VARANDCY(NULL,0,10000,NULL,0); VARANDCY(NULL,0,0,I4,0); diff --git a/dlls/oleaut32/variant.c b/dlls/oleaut32/variant.c index 48851ba05ec..f6fbdfbf5b4 100644 --- a/dlls/oleaut32/variant.c +++ b/dlls/oleaut32/variant.c @@ -3048,52 +3048,97 @@ HRESULT WINAPI VarAnd(LPVARIANT left, LPVARIANT right, LPVARIANT result) if (leftvt == VT_NULL || rightvt == VT_NULL) { - /* - * Special cases for when left variant is VT_NULL - * (VT_NULL & 0 = VT_NULL, VT_NULL & value = value) - */ + /* Three-valued logic for `And` with Null: + * zero And Null = zero (typed by the resvt computed above) + * nonzero And Null = Null + * Null And Null = Null + * + * Both orderings must produce the same result. */ + VARIANT *other; + VARTYPE othervt; + BOOL nonzero = FALSE; + + if (leftvt == VT_NULL && rightvt == VT_NULL) + { + V_VT(result) = VT_NULL; + goto VarAnd_Exit; + } + if (leftvt == VT_NULL) + { + other = right; + othervt = rightvt; + } + else + { + other = left; + othervt = leftvt; + } + + switch (othervt) + { + case VT_EMPTY: break; + case VT_I1: nonzero = V_I1(other) != 0; break; + case VT_UI1: nonzero = V_UI1(other) != 0; break; + case VT_I2: nonzero = V_I2(other) != 0; break; + case VT_UI2: nonzero = V_UI2(other) != 0; break; + case VT_I4: nonzero = V_I4(other) != 0; break; + case VT_UI4: nonzero = V_UI4(other) != 0; break; + case VT_I8: nonzero = V_I8(other) != 0; break; + case VT_UI8: nonzero = V_UI8(other) != 0; break; + case VT_INT: nonzero = V_INT(other) != 0; break; + case VT_UINT: nonzero = V_UINT(other) != 0; break; + case VT_BOOL: nonzero = V_BOOL(other) != VARIANT_FALSE; break; + case VT_R4: nonzero = V_R4(other) != 0.0f; break; + case VT_R8: nonzero = V_R8(other) != 0.0; break; + case VT_DATE: nonzero = V_DATE(other) != 0.0; break; + case VT_CY: nonzero = V_CY(other).int64 != 0; break; + case VT_DECIMAL: nonzero = V_DECIMAL(other).Hi32 || V_DECIMAL(other).Lo64; break; + case VT_BSTR: { VARIANT_BOOL b; - switch(rightvt) - { - case VT_I1: if (V_I1(right)) resvt = VT_NULL; break; - case VT_UI1: if (V_UI1(right)) resvt = VT_NULL; break; - case VT_I2: if (V_I2(right)) resvt = VT_NULL; break; - case VT_UI2: if (V_UI2(right)) resvt = VT_NULL; break; - case VT_I4: if (V_I4(right)) resvt = VT_NULL; break; - case VT_UI4: if (V_UI4(right)) resvt = VT_NULL; break; - case VT_I8: if (V_I8(right)) resvt = VT_NULL; break; - case VT_UI8: if (V_UI8(right)) resvt = VT_NULL; break; - case VT_INT: if (V_INT(right)) resvt = VT_NULL; break; - case VT_UINT: if (V_UINT(right)) resvt = VT_NULL; break; - case VT_BOOL: if (V_BOOL(right)) resvt = VT_NULL; break; - case VT_R4: if (V_R4(right)) resvt = VT_NULL; break; - case VT_R8: if (V_R8(right)) resvt = VT_NULL; break; - case VT_CY: - if(V_CY(right).int64) - resvt = VT_NULL; - break; - case VT_DECIMAL: - if (V_DECIMAL(right).Hi32 || V_DECIMAL(right).Lo64) - resvt = VT_NULL; - break; - case VT_BSTR: - hres = VarBoolFromStr(V_BSTR(right), + hres = VarBoolFromStr(V_BSTR(other), LOCALE_USER_DEFAULT, VAR_LOCALBOOL, &b); - if (FAILED(hres)) - return hres; - else if (b) - V_VT(result) = VT_NULL; - else - { - V_VT(result) = VT_BOOL; - V_BOOL(result) = b; - } + if (FAILED(hres)) goto VarAnd_Exit; + if (b) + V_VT(result) = VT_NULL; + else + { + V_VT(result) = VT_BOOL; + V_BOOL(result) = VARIANT_FALSE; } + goto VarAnd_Exit; + } + default: + V_VT(result) = VT_NULL; + goto VarAnd_Exit; + } + + if (nonzero) + { + V_VT(result) = VT_NULL; + goto VarAnd_Exit; } + V_VT(result) = resvt; + switch (resvt) + { + case VT_BOOL: V_BOOL(result) = VARIANT_FALSE; break; + case VT_I1: V_I1(result) = 0; break; + case VT_UI1: V_UI1(result) = 0; break; + case VT_I2: V_I2(result) = 0; break; + case VT_UI2: V_UI2(result) = 0; break; + case VT_I4: V_I4(result) = 0; break; + case VT_UI4: V_UI4(result) = 0; break; + case VT_I8: V_I8(result) = 0; break; + case VT_UI8: V_UI8(result) = 0; break; + case VT_INT: V_INT(result) = 0; break; + case VT_UINT: V_UINT(result) = 0; break; + default: + V_VT(result) = VT_NULL; + break; + } goto VarAnd_Exit; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10673
From: Francis De Brabandere <francisdb@gmail.com> VT_CY stores values scaled by 10000 in .int64, so the all-ones-Imp- Null check must compare to -10000 (not -1) for CCur(-1). Add right- Null conformance rows for I2/I4/R8/UI1 and a manual CY -1 case. --- dlls/oleaut32/tests/vartest.c | 20 ++++++++++++++++++++ dlls/oleaut32/variant.c | 3 ++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/dlls/oleaut32/tests/vartest.c b/dlls/oleaut32/tests/vartest.c index c362f9854fe..dc85f04c0ae 100644 --- a/dlls/oleaut32/tests/vartest.c +++ b/dlls/oleaut32/tests/vartest.c @@ -9759,6 +9759,26 @@ static void test_VarImp(void) V_I8(&result), -3); } + /* All-ones left Imp Null returns Null for every numeric type: the + * three-valued "True Imp unknown = unknown" rule applies to any bit + * pattern that matches VARIANT_TRUE in the operand's width, including + * VT_UI1 0xFF (despite being unsigned). */ + VARIMP(I2,-1,NULL,0,NULL,0); + VARIMP(I4,-1,NULL,0,NULL,0); + VARIMP(R8,-1.0,NULL,0,NULL,0); + VARIMP(UI1,255,NULL,0,NULL,0); + + /* VT_CY stores values scaled by 10000 in .int64, so CY -1 has .int64 of + * -10000 (not -1). Previously Wine's all-ones check compared against -1 + * and never fired, producing a bitwise result instead of Null. */ + V_VT(&left) = VT_CY; + V_CY(&left).int64 = -10000; + V_VT(&right) = VT_NULL; + V_I4(&right) = 0; + V_VT(&exp) = VT_NULL; + V_I4(&exp) = 0; + test_var_call2(__LINE__, pVarImp, &left, &right, &exp); + SysFreeString(false_str); SysFreeString(true_str); } diff --git a/dlls/oleaut32/variant.c b/dlls/oleaut32/variant.c index f6fbdfbf5b4..3fbc7730926 100644 --- a/dlls/oleaut32/variant.c +++ b/dlls/oleaut32/variant.c @@ -5957,7 +5957,8 @@ HRESULT WINAPI VarImp(LPVARIANT left, LPVARIANT right, LPVARIANT result) case VT_BOOL: if (V_BOOL(left) == VARIANT_TRUE) resvt = VT_NULL; break; case VT_R4: if (V_R4(left) == -1.0) resvt = VT_NULL; break; case VT_R8: if (V_R8(left) == -1.0) resvt = VT_NULL; break; - case VT_CY: if (V_CY(left).int64 == -1) resvt = VT_NULL; break; + /* VT_CY stores values scaled by 10000, so -1 is -10000 in .int64. */ + case VT_CY: if (V_CY(left).int64 == -10000) resvt = VT_NULL; break; case VT_DECIMAL: if (V_DECIMAL(left).Hi32 == 0xffffffff) resvt = VT_NULL; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10673
From: Francis De Brabandere <francisdb@gmail.com> Native VarImp returns VT_NULL for UI1 0xFF Imp Null via the three- valued all-ones rule, but native VBScript keeps UI1 width and returns the bitwise ~left. Handle the narrow case directly in interp_imp. --- dlls/vbscript/interp.c | 13 ++++++++++++- dlls/vbscript/tests/lang.vbs | 10 ++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/dlls/vbscript/interp.c b/dlls/vbscript/interp.c index ba530fca38c..c92006b15d6 100644 --- a/dlls/vbscript/interp.c +++ b/dlls/vbscript/interp.c @@ -2103,7 +2103,18 @@ static HRESULT interp_imp(exec_ctx_t *ctx) hres = stack_pop_val(ctx, &l); if(SUCCEEDED(hres)) { - hres = VarImp(l.v, r.v, &v); + /* Native VarImp returns VT_NULL for UI1 0xFF Imp Null under the + * three-valued "all-ones Imp unknown = unknown" rule, but native + * VBScript keeps UI1 width and returns the bitwise complement of + * the left operand. Handle UI1 Imp Null directly. */ + if ((V_VT(l.v) & VT_TYPEMASK) == VT_UI1 && + (V_VT(r.v) & VT_TYPEMASK) == VT_NULL) { + V_VT(&v) = VT_UI1; + V_UI1(&v) = ~V_UI1(l.v); + hres = S_OK; + } else { + hres = VarImp(l.v, r.v, &v); + } release_val(&l); } release_val(&r); diff --git a/dlls/vbscript/tests/lang.vbs b/dlls/vbscript/tests/lang.vbs index 7ff524461c6..1161545b79c 100644 --- a/dlls/vbscript/tests/lang.vbs +++ b/dlls/vbscript/tests/lang.vbs @@ -196,6 +196,16 @@ call ok(false imp false, "false does not imp false?") call ok(not (true imp false), "true imp false?") call ok(false imp null, "false imp null is false?") +' For VT_UI1 Imp VT_NULL, native VBScript keeps UI1 width and returns +' the bitwise complement of the left operand, rather than applying +' VarImp's three-valued all-ones rule. interp_imp has a narrow special +' case to match this native behavior. +Call ok((CByte(0) Imp Null) = 255, "CByte(0) Imp Null is not 255") +Call ok(getVT(CByte(0) Imp Null) = "VT_UI1", "getVT(CByte(0) Imp Null) = " & getVT(CByte(0) Imp Null)) +Call ok((CByte(170) Imp Null) = 85, "CByte(170) Imp Null is not 85") +Call ok((CByte(255) Imp Null) = 0, "CByte(255) Imp Null is not 0") +Call ok(getVT(CByte(255) Imp Null) = "VT_UI1", "getVT(CByte(255) Imp Null) = " & getVT(CByte(255) Imp Null)) + Call ok(2 >= 1, "! 2 >= 1") Call ok(2 >= 2, "! 2 >= 2") Call ok(2 => 1, "! 2 => 1") -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10673
From: Francis De Brabandere <francisdb@gmail.com> VarImp's right-Null switch was missing a VT_DATE case, so CDate(-1) Imp Null fell through to the main computation and returned VT_I4 0 instead of Null. Add the all-ones check using V_DATE. --- dlls/oleaut32/tests/vartest.c | 1 + dlls/oleaut32/variant.c | 1 + 2 files changed, 2 insertions(+) diff --git a/dlls/oleaut32/tests/vartest.c b/dlls/oleaut32/tests/vartest.c index dc85f04c0ae..e5bbecbdd3f 100644 --- a/dlls/oleaut32/tests/vartest.c +++ b/dlls/oleaut32/tests/vartest.c @@ -9766,6 +9766,7 @@ static void test_VarImp(void) VARIMP(I2,-1,NULL,0,NULL,0); VARIMP(I4,-1,NULL,0,NULL,0); VARIMP(R8,-1.0,NULL,0,NULL,0); + VARIMP(DATE,-1.0,NULL,0,NULL,0); VARIMP(UI1,255,NULL,0,NULL,0); /* VT_CY stores values scaled by 10000 in .int64, so CY -1 has .int64 of diff --git a/dlls/oleaut32/variant.c b/dlls/oleaut32/variant.c index 3fbc7730926..c24b32fa9f0 100644 --- a/dlls/oleaut32/variant.c +++ b/dlls/oleaut32/variant.c @@ -5957,6 +5957,7 @@ HRESULT WINAPI VarImp(LPVARIANT left, LPVARIANT right, LPVARIANT result) case VT_BOOL: if (V_BOOL(left) == VARIANT_TRUE) resvt = VT_NULL; break; case VT_R4: if (V_R4(left) == -1.0) resvt = VT_NULL; break; case VT_R8: if (V_R8(left) == -1.0) resvt = VT_NULL; break; + case VT_DATE: if (V_DATE(left) == -1.0) resvt = VT_NULL; break; /* VT_CY stores values scaled by 10000, so -1 is -10000 in .int64. */ case VT_CY: if (V_CY(left).int64 == -10000) resvt = VT_NULL; break; case VT_DECIMAL: -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10673
From: Francis De Brabandere <francisdb@gmail.com> Verify that VBScript's And/Or/Xor/Eqv/Imp/Not operators propagate the oleaut32 VarAnd/VarOr/... results correctly through the interpreter for a few representative Null combinations. --- dlls/vbscript/tests/lang.vbs | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/dlls/vbscript/tests/lang.vbs b/dlls/vbscript/tests/lang.vbs index 1161545b79c..229ffcdf78d 100644 --- a/dlls/vbscript/tests/lang.vbs +++ b/dlls/vbscript/tests/lang.vbs @@ -196,10 +196,31 @@ call ok(false imp false, "false does not imp false?") call ok(not (true imp false), "true imp false?") call ok(false imp null, "false imp null is false?") -' For VT_UI1 Imp VT_NULL, native VBScript keeps UI1 width and returns -' the bitwise complement of the left operand, rather than applying -' VarImp's three-valued all-ones rule. interp_imp has a narrow special -' case to match this native behavior. +' Smoke check that VBScript's `And` operator reaches VarAnd correctly and +' propagates the result payload. The full VarAnd+Null conformance table +' lives in dlls/oleaut32/tests/vartest.c. +Call ok((False And Null) = False, "False And Null is not False") +Call ok(isNull(True And Null), "True And Null is not Null") +Call ok(isNull(Null And Null), "Null And Null is not Null") +Call ok((CInt(0) And Null) = 0, "CInt(0) And Null is not 0") +Call ok(getVT(CInt(0) And Null) = "VT_I2", "getVT(CInt(0) And Null) = " & getVT(CInt(0) And Null)) +Call ok(isNull(CInt(5) And Null), "CInt(5) And Null is not Null") + +' Smoke checks that VBScript's sibling logical operators reach Var* +' correctly and propagate the result payload. The full conformance tables +' live in dlls/oleaut32/tests/vartest.c. +Call ok((True Or Null) = True, "True Or Null is not True") +Call ok(isNull(False Or Null), "False Or Null is not Null") +Call ok(isNull(CInt(5) Xor Null), "CInt(5) Xor Null is not Null") +Call ok(isNull(CInt(5) Eqv Null), "CInt(5) Eqv Null is not Null") +Call ok(isNull(CDate(-1) Imp Null), "CDate(-1) Imp Null is not Null") +Call ok((Not CLng(0)) = -1, "Not CLng(0) is not -1") + +' VBScript-specific: for VT_UI1 Imp VT_NULL, native VBScript keeps UI1 +' width and returns the bitwise complement of the left operand, rather +' than applying VarImp's three-valued "all-ones Imp unknown = unknown" +' rule (which returns VT_NULL at the C level for UI1 0xFF). interp_imp +' has a narrow special case to match this native behavior. Call ok((CByte(0) Imp Null) = 255, "CByte(0) Imp Null is not 255") Call ok(getVT(CByte(0) Imp Null) = "VT_UI1", "getVT(CByte(0) Imp Null) = " & getVT(CByte(0) Imp Null)) Call ok((CByte(170) Imp Null) = 85, "CByte(170) Imp Null is not 85") -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10673
participants (2)
-
Francis De Brabandere -
Francis De Brabandere (＠francisdb)