[PATCH 0/5] MR3256: Use nameless structs/unions for VARIANT struct (Clang build fixes)
Recent nameless struct/union changes broke MSVC Clang builds. That's because `VARIANT` still doesn't skip names when `__STDC__` is defined, which is true for most configs. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3256
From: Jacek Caban <jacek(a)codeweavers.com> --- dlls/dbghelp/msc.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/dlls/dbghelp/msc.c b/dlls/dbghelp/msc.c index 5abd5b577a1..6af3b415bf8 100644 --- a/dlls/dbghelp/msc.c +++ b/dlls/dbghelp/msc.c @@ -2582,8 +2582,7 @@ static BOOL codeview_snarf(const struct msc_debug_info* msc_dbg, name = (const struct p_string*)((const char*)&sym->constant_v1.cvalue + vlen); se = codeview_get_type(sym->constant_v1.type, FALSE); - TRACE("S-Constant-V1 %u %s %x\n", - v.n1.n2.n3.intVal, terminate_string(name), sym->constant_v1.type); + TRACE("S-Constant-V1 %u %s %x\n", V_INT(&v), terminate_string(name), sym->constant_v1.type); symt_new_constant(msc_dbg->module, compiland, terminate_string(name), se, &v); } @@ -2599,8 +2598,7 @@ static BOOL codeview_snarf(const struct msc_debug_info* msc_dbg, name = (const struct p_string*)((const char*)&sym->constant_v2.cvalue + vlen); se = codeview_get_type(sym->constant_v2.type, FALSE); - TRACE("S-Constant-V2 %u %s %x\n", - v.n1.n2.n3.intVal, terminate_string(name), sym->constant_v2.type); + TRACE("S-Constant-V2 %u %s %x\n", V_INT(&v), terminate_string(name), sym->constant_v2.type); symt_new_constant(msc_dbg->module, compiland, terminate_string(name), se, &v); } @@ -2616,8 +2614,7 @@ static BOOL codeview_snarf(const struct msc_debug_info* msc_dbg, name = (const char*)&sym->constant_v3.cvalue + vlen; se = codeview_get_type(sym->constant_v3.type, FALSE); - TRACE("S-Constant-V3 %u %s %x\n", - v.n1.n2.n3.intVal, name, sym->constant_v3.type); + TRACE("S-Constant-V3 %u %s %x\n", V_INT(&v), name, sym->constant_v3.type); /* FIXME: we should add this as a constant value */ symt_new_constant(msc_dbg->module, compiland, name, se, &v); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3256
From: Jacek Caban <jacek(a)codeweavers.com> --- dlls/oleaut32/tests/vartest.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/dlls/oleaut32/tests/vartest.c b/dlls/oleaut32/tests/vartest.c index 34d0d1f269b..cf93e4cd5fc 100644 --- a/dlls/oleaut32/tests/vartest.c +++ b/dlls/oleaut32/tests/vartest.c @@ -248,7 +248,7 @@ typedef struct IRecordInfoImpl unsigned int recordclear; unsigned int getsize; unsigned int recordcopy; - struct __tagBRECORD *rec; + VARIANT *v; } IRecordInfoImpl; static inline IRecordInfoImpl *impl_from_IRecordInfo(IRecordInfo *iface) @@ -298,7 +298,7 @@ static HRESULT WINAPI RecordInfo_RecordClear(IRecordInfo *iface, void *data) { IRecordInfoImpl* This = impl_from_IRecordInfo(iface); This->recordclear++; - This->rec->pvRecord = NULL; + V_RECORD(This->v) = NULL; return S_OK; } @@ -766,7 +766,6 @@ static test_VariantClearImpl test_myVariantClearImpl = {{&test_VariantClear_vtbl static void test_VariantClear(void) { - struct __tagBRECORD *rec; IRecordInfoImpl *recinfo; HRESULT hres; VARIANTARG v; @@ -891,15 +890,14 @@ static void test_VariantClear(void) /* RECORD */ recinfo = get_test_recordinfo(); V_VT(&v) = VT_RECORD; - rec = &V_UNION(&v, brecVal); - rec->pRecInfo = &recinfo->IRecordInfo_iface; - rec->pvRecord = (void*)0xdeadbeef; + V_RECORDINFO(&v) = &recinfo->IRecordInfo_iface; + V_RECORD(&v) = (void*)0xdeadbeef; recinfo->recordclear = 0; recinfo->ref = 2; - recinfo->rec = rec; + recinfo->v = &v; hres = VariantClear(&v); ok(hres == S_OK, "ret %08lx\n", hres); - ok(rec->pvRecord == NULL, "got %p\n", rec->pvRecord); + ok(V_RECORD(&v) == NULL, "got %p\n", V_RECORD(&v)); ok(recinfo->recordclear == 1, "got %d\n", recinfo->recordclear); ok(recinfo->ref == 1, "got %ld\n", recinfo->ref); IRecordInfo_Release(&recinfo->IRecordInfo_iface); @@ -907,7 +905,6 @@ static void test_VariantClear(void) static void test_VariantCopy(void) { - struct __tagBRECORD *rec; IRecordInfoImpl *recinfo; VARIANTARG vSrc, vDst; VARTYPE vt; @@ -1032,20 +1029,18 @@ static void test_VariantCopy(void) V_VT(&vDst) = VT_EMPTY; V_VT(&vSrc) = VT_RECORD; - rec = &V_UNION(&vSrc, brecVal); - rec->pRecInfo = &recinfo->IRecordInfo_iface; - rec->pvRecord = (void*)0xdeadbeef; + V_RECORDINFO(&vSrc) = &recinfo->IRecordInfo_iface; + V_RECORD(&vSrc) = (void*)0xdeadbeef; recinfo->recordclear = 0; recinfo->recordcopy = 0; recinfo->getsize = 0; - recinfo->rec = rec; + recinfo->v = &vSrc; hres = VariantCopy(&vDst, &vSrc); ok(hres == S_OK, "ret %08lx\n", hres); - rec = &V_UNION(&vDst, brecVal); - ok(rec->pvRecord != (void*)0xdeadbeef && rec->pvRecord != NULL, "got %p\n", rec->pvRecord); - ok(rec->pRecInfo == &recinfo->IRecordInfo_iface, "got %p\n", rec->pRecInfo); + ok(V_RECORD(&vDst) != (void*)0xdeadbeef && V_RECORD(&vDst) != NULL, "got %p\n", V_RECORD(&vDst)); + ok(V_RECORDINFO(&vDst) == &recinfo->IRecordInfo_iface, "got %p\n", V_RECORDINFO(&vDst)); ok(recinfo->getsize == 1, "got %d\n", recinfo->recordclear); ok(recinfo->recordcopy == 1, "got %d\n", recinfo->recordclear); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3256
From: Jacek Caban <jacek(a)codeweavers.com> --- dlls/oleaut32/variant.c | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/dlls/oleaut32/variant.c b/dlls/oleaut32/variant.c index 85fef3113b6..fa4492b968a 100644 --- a/dlls/oleaut32/variant.c +++ b/dlls/oleaut32/variant.c @@ -585,11 +585,11 @@ HRESULT VARIANT_ClearInd(VARIANTARG *pVarg) case VT_RECORD: case VT_RECORD | VT_BYREF: { - struct __tagBRECORD* pBr = &V_UNION(pVarg,brecVal); - if (pBr->pRecInfo) + IRecordInfo *rec_info = V_RECORDINFO(pVarg); + if (rec_info) { - IRecordInfo_RecordClear(pBr->pRecInfo, pBr->pvRecord); - IRecordInfo_Release(pBr->pRecInfo); + IRecordInfo_RecordClear(rec_info, V_RECORD(pVarg)); + IRecordInfo_Release(rec_info); } break; } @@ -645,11 +645,11 @@ HRESULT WINAPI DECLSPEC_HOTPATCH VariantClear(VARIANTARG* pVarg) } else if (V_VT(pVarg) == VT_RECORD) { - struct __tagBRECORD* pBr = &V_UNION(pVarg,brecVal); - if (pBr->pRecInfo) + IRecordInfo *rec_info = V_RECORDINFO(pVarg); + if (rec_info) { - IRecordInfo_RecordClear(pBr->pRecInfo, pBr->pvRecord); - IRecordInfo_Release(pBr->pRecInfo); + IRecordInfo_RecordClear(rec_info, V_RECORD(pVarg)); + IRecordInfo_Release(rec_info); } } else if (V_VT(pVarg) == VT_DISPATCH || @@ -669,31 +669,28 @@ HRESULT WINAPI DECLSPEC_HOTPATCH VariantClear(VARIANTARG* pVarg) */ static HRESULT VARIANT_CopyIRecordInfo(VARIANT *dest, const VARIANT *src) { - struct __tagBRECORD *dest_rec = &V_UNION(dest, brecVal); - const struct __tagBRECORD *src_rec = &V_UNION(src, brecVal); + IRecordInfo *src_info = V_RECORDINFO(src); HRESULT hr = S_OK; ULONG size; - if (!src_rec->pRecInfo) + if (!src_info) { - if (src_rec->pvRecord) return E_INVALIDARG; + if (V_RECORD(src)) return E_INVALIDARG; return S_OK; } - hr = IRecordInfo_GetSize(src_rec->pRecInfo, &size); + hr = IRecordInfo_GetSize(src_info, &size); if (FAILED(hr)) return hr; /* Windows does not use RecordCreate() here, memory should be allocated in compatible way so RecordDestroy() could free it later. */ - dest_rec->pvRecord = CoTaskMemAlloc(size); - if (!dest_rec->pvRecord) return E_OUTOFMEMORY; + V_RECORD(dest) = CoTaskMemAlloc(size); + if (!V_RECORD(dest)) return E_OUTOFMEMORY; if (size) - memset(dest_rec->pvRecord, 0, size); + memset(V_RECORD(dest), 0, size); - dest_rec->pRecInfo = src_rec->pRecInfo; - IRecordInfo_AddRef(src_rec->pRecInfo); - - return IRecordInfo_RecordCopy(src_rec->pRecInfo, src_rec->pvRecord, dest_rec->pvRecord); + IRecordInfo_AddRef(V_RECORDINFO(dest) = src_info); + return IRecordInfo_RecordCopy(src_info, V_RECORD(src), V_RECORD(dest)); } /****************************************************************************** -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3256
From: Jacek Caban <jacek(a)codeweavers.com> --- dlls/oleaut32/usrmarshal.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/dlls/oleaut32/usrmarshal.c b/dlls/oleaut32/usrmarshal.c index ae4d11b7a0c..eed0f3ac0aa 100644 --- a/dlls/oleaut32/usrmarshal.c +++ b/dlls/oleaut32/usrmarshal.c @@ -399,11 +399,11 @@ unsigned char * WINAPI VARIANT_UserMarshal(ULONG *pFlags, unsigned char *Buffer, header->clSize = 0; /* fixed up at the end */ header->rpcReserved = 0; - header->vt = pvar->n1.n2.vt; + header->vt = V_VT(pvar); header->wReserved1 = pvar->n1.n2.wReserved1; header->wReserved2 = pvar->n1.n2.wReserved2; header->wReserved3 = pvar->n1.n2.wReserved3; - header->switch_is = pvar->n1.n2.vt; + header->switch_is = V_VT(pvar); if(header->switch_is & VT_ARRAY) header->switch_is &= ~VT_TYPEMASK; @@ -418,7 +418,7 @@ unsigned char * WINAPI VARIANT_UserMarshal(ULONG *pFlags, unsigned char *Buffer, Pos += 4; if((header->vt & VT_TYPEMASK) != VT_VARIANT) { - memcpy(Pos, pvar->n1.n2.n3.byref, type_size); + memcpy(Pos, V_BYREF(pvar), type_size); Pos += type_size; } else @@ -432,7 +432,7 @@ unsigned char * WINAPI VARIANT_UserMarshal(ULONG *pFlags, unsigned char *Buffer, if((header->vt & VT_TYPEMASK) == VT_DECIMAL) memcpy(Pos, pvar, type_size); else - memcpy(Pos, &pvar->n1.n2.n3, type_size); + memcpy(Pos, &V_UI8(pvar), type_size); Pos += type_size; } @@ -561,11 +561,11 @@ unsigned char * WINAPI VARIANT_UserUnmarshal(ULONG *pFlags, unsigned char *Buffe else if((header->vt & VT_TYPEMASK) == VT_DECIMAL) memcpy(pvar, Pos, type_size); else - memcpy(&pvar->n1.n2.n3, Pos, type_size); + memcpy(&V_UI8(pvar), Pos, type_size); Pos += type_size; } - pvar->n1.n2.vt = header->vt; + V_VT(pvar) = header->vt; pvar->n1.n2.wReserved1 = header->wReserved1; pvar->n1.n2.wReserved2 = header->wReserved2; pvar->n1.n2.wReserved3 = header->wReserved3; @@ -621,7 +621,7 @@ void WINAPI VARIANT_UserFree(ULONG *pFlags, VARIANT *pvar) TRACE("%#lx, %p.\n", *pFlags, pvar); TRACE("vt=%04x\n", V_VT(pvar)); - if (vt & VT_BYREF) ref = pvar->n1.n2.n3.byref; + if (vt & VT_BYREF) ref = V_BYREF(pvar); VariantClear(pvar); if (!ref) return; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3256
From: Jacek Caban <jacek(a)codeweavers.com> --- dlls/oleaut32/tests/usrmarshal.c | 24 +++++++++--------------- dlls/oleaut32/usrmarshal.c | 12 ++++++------ include/oaidl.idl | 2 +- include/oleauto.h | 4 ++-- 4 files changed, 18 insertions(+), 24 deletions(-) diff --git a/dlls/oleaut32/tests/usrmarshal.c b/dlls/oleaut32/tests/usrmarshal.c index 6559cdcd865..fee8097fac8 100644 --- a/dlls/oleaut32/tests/usrmarshal.c +++ b/dlls/oleaut32/tests/usrmarshal.c @@ -30,12 +30,6 @@ #include "wine/test.h" -#if (__STDC__ && !defined(_FORCENAMELESSUNION)) || defined(NONAMELESSUNION) -# define V_U2(A) ((A)->n1.n2) -#else -# define V_U2(A) (*(A)) -#endif - typedef struct { IUnknown IUnknown_iface; @@ -870,9 +864,9 @@ static DWORD *check_variant_header(DWORD *wirev, VARIANT *v, ULONG size) ok(header->clSize == (size + 7) >> 3, "wv[0] %08lx, expected %08lx\n", header->clSize, (size + 7) >> 3); ok(header->rpcReserved == 0, "wv[1] %08lx\n", header->rpcReserved); ok(header->vt == V_VT(v), "vt %04x expected %04x\n", header->vt, V_VT(v)); - ok(header->wReserved1 == V_U2(v).wReserved1, "res1 %04x expected %04x\n", header->wReserved1, V_U2(v).wReserved1); - ok(header->wReserved2 == V_U2(v).wReserved2, "res2 %04x expected %04x\n", header->wReserved2, V_U2(v).wReserved2); - ok(header->wReserved3 == V_U2(v).wReserved3, "res3 %04x expected %04x\n", header->wReserved3, V_U2(v).wReserved3); + ok(header->wReserved1 == v->wReserved1, "res1 %04x expected %04x\n", header->wReserved1, v->wReserved1); + ok(header->wReserved2 == v->wReserved2, "res2 %04x expected %04x\n", header->wReserved2, v->wReserved2); + ok(header->wReserved3 == v->wReserved3, "res3 %04x expected %04x\n", header->wReserved3, v->wReserved3); switch_is = V_VT(v); if(switch_is & VT_ARRAY) @@ -938,9 +932,9 @@ static void test_marshal_VARIANT(void) /* check_variant_header tests wReserved[123], so initialize to unique values. * (Could probably also do this by setting the variant to a known DECIMAL.) */ - V_U2(&v).wReserved1 = 0x1234; - V_U2(&v).wReserved2 = 0x5678; - V_U2(&v).wReserved3 = 0x9abc; + v.wReserved1 = 0x1234; + v.wReserved2 = 0x5678; + v.wReserved3 = 0x9abc; /* Variants have an alignment of 8 */ rpcMsg.BufferLength = stubMsg.BufferLength = VARIANT_UserSize(&umcb.Flags, 1, &v); @@ -1306,9 +1300,9 @@ static void test_marshal_VARIANT(void) /* check_variant_header tests wReserved[123], so initialize to unique values. * (Could probably also do this by setting the variant to a known DECIMAL.) */ - V_U2(&v2).wReserved1 = 0x0123; - V_U2(&v2).wReserved2 = 0x4567; - V_U2(&v2).wReserved3 = 0x89ab; + v2.wReserved1 = 0x0123; + v2.wReserved2 = 0x4567; + v2.wReserved3 = 0x89ab; stubMsg.Buffer = buffer; next = VARIANT_UserUnmarshal(&umcb.Flags, buffer, &v2); diff --git a/dlls/oleaut32/usrmarshal.c b/dlls/oleaut32/usrmarshal.c index eed0f3ac0aa..25bce3155bf 100644 --- a/dlls/oleaut32/usrmarshal.c +++ b/dlls/oleaut32/usrmarshal.c @@ -400,9 +400,9 @@ unsigned char * WINAPI VARIANT_UserMarshal(ULONG *pFlags, unsigned char *Buffer, header->clSize = 0; /* fixed up at the end */ header->rpcReserved = 0; header->vt = V_VT(pvar); - header->wReserved1 = pvar->n1.n2.wReserved1; - header->wReserved2 = pvar->n1.n2.wReserved2; - header->wReserved3 = pvar->n1.n2.wReserved3; + header->wReserved1 = pvar->wReserved1; + header->wReserved2 = pvar->wReserved2; + header->wReserved3 = pvar->wReserved3; header->switch_is = V_VT(pvar); if(header->switch_is & VT_ARRAY) header->switch_is &= ~VT_TYPEMASK; @@ -566,9 +566,9 @@ unsigned char * WINAPI VARIANT_UserUnmarshal(ULONG *pFlags, unsigned char *Buffe } V_VT(pvar) = header->vt; - pvar->n1.n2.wReserved1 = header->wReserved1; - pvar->n1.n2.wReserved2 = header->wReserved2; - pvar->n1.n2.wReserved3 = header->wReserved3; + pvar->wReserved1 = header->wReserved1; + pvar->wReserved2 = header->wReserved2; + pvar->wReserved3 = header->wReserved3; if(header->vt & VT_ARRAY) { diff --git a/include/oaidl.idl b/include/oaidl.idl index 37da734baf7..c37ae0c70d1 100644 --- a/include/oaidl.idl +++ b/include/oaidl.idl @@ -142,7 +142,7 @@ const USHORT FADF_CREATEVECTOR = 0x2000; /* This array is a vector */ /* Variant */ -cpp_quote("#if (__STDC__ && !defined(_FORCENAMELESSUNION)) || defined(NONAMELESSUNION)") +cpp_quote("#if (__STDC__ && !defined(_FORCENAMELESSUNION) && !defined(__WINESRC__)) || defined(NONAMELESSUNION)") cpp_quote("#define __VARIANT_NAME_1 n1") cpp_quote("#define __VARIANT_NAME_2 n2") cpp_quote("#define __VARIANT_NAME_3 n3") diff --git a/include/oleauto.h b/include/oleauto.h index 657986e399f..badf2b4258b 100644 --- a/include/oleauto.h +++ b/include/oleauto.h @@ -112,7 +112,7 @@ WINOLEAUTAPI HRESULT WINAPI GetRecordInfoFromGuids(REFGUID,ULONG,ULONG,LCID,REFG */ /* Macros for accessing the fields of the VARIANT type */ -#if (__STDC__ && !defined(_FORCENAMELESSUNION)) || defined(NONAMELESSUNION) +#if (__STDC__ && !defined(_FORCENAMELESSUNION) && !defined(__WINESRC__)) || defined(NONAMELESSUNION) #define V_VT(A) ((A)->n1.n2.vt) #define V_UNION(A,B) ((A)->n1.n2.n3.B) #define V_RECORD(A) (V_UNION(A,brecVal).pvRecord) @@ -140,7 +140,7 @@ WINOLEAUTAPI HRESULT WINAPI GetRecordInfoFromGuids(REFGUID,ULONG,ULONG,LCID,REFG #define V_CYREF(A) V_UNION(A,pcyVal) #define V_DATE(A) V_UNION(A,date) #define V_DATEREF(A) V_UNION(A,pdate) -#if (__STDC__ && !defined(_FORCENAMELESSUNION)) || defined(NONAMELESSUNION) +#if (__STDC__ && !defined(_FORCENAMELESSUNION) && !defined(__WINESRC__)) || defined(NONAMELESSUNION) #define V_DECIMAL(A) ((A)->n1.decVal) #else #define V_DECIMAL(A) ((A)->decVal) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3256
Recent nameless struct/union changes broke MSVC Clang builds. That's because `VARIANT` still doesn't skip names when `__STDC__` is defined, which is true for most configs.
I'd argue that it should be true in all configs. MSVC clang not defining `__STDC__` is a bug IMHO. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3256#note_38143
On Thu Jul 6 13:44:11 2023 +0000, Alexandre Julliard wrote:
Recent nameless struct/union changes broke MSVC Clang builds. That's because `VARIANT` still doesn't skip names when `__STDC__` is defined, which is true for most configs. I'd argue that it should be true in all configs. MSVC clang not defining `__STDC__` is a bug IMHO. MSVC does not define it by default or even if you specify -std:c*, there is a separated option to enable it [1] (only in new MSVC and clang does not yet seem to implement it). so clang is compatible in this aspect and that's what MSVC-aware code expects. I think that the current behavior would not be considered a bug upstream.
Would you prefer if I change the logic to make clang use current mingw behavior (instead of the other way around)? [1] https://learn.microsoft.com/en-us/cpp/build/reference/zc-stdc?view=msvc-170 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3256#note_38144
MSVC does not define it by default or even if you specify -std:c\*, there is a separated option to enable it \[1\] (only in new MSVC and clang does not yet seem to implement it). so clang is compatible in this aspect and that's what MSVC-aware code expects. I think that the current behavior would not be considered a bug upstream.
Yes, it's compatible with Microsoft's broken definition of `__STDC__`, but that doesn't mean it's a good idea.
Would you prefer if I change the logic to make clang use current mingw behavior (instead of the other way around)?
The code changes are fine, but I think in the headers we may as well stop checking `__STDC__` altogether, since MS has made that meaningless. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3256#note_38148
participants (3)
-
Alexandre Julliard (＠julliard) -
Jacek Caban
-
Jacek Caban (＠jacek)