June 2017 - Wine-Devel - list.winehq.org

Re: [5/7] windowscodecs: Add initial implementation of IWICMetadataQueryReader::GetMetadataByName.

by Dmitry Timoshkov

Vincent Povirk <madewokherd(a)gmail.com> wrote: > > + if (vt == VT_CLSID) > > + { > > + id->u.puuid = CoTaskMemAlloc(sizeof(GUID)); > > + if (!id->u.puuid) return E_OUTOFMEMORY; > > + > > + hr = UuidFromStringW(next_token.u.bstrVal, id->u.puuid); > > + } > > + else > > + hr = PropVariantChangeType(id, &next_token, 0, vt); > > + PropVariantClear(&next_token); > > + if (hr != S_OK) > > + { > > + PropVariantClear(id); > > + PropVariantClear(schema); > > + return hr; > > + } > > + > > + id->vt = vt; > > Assigning the vt at the end of this is a bit strange. I don't know any > situation here where PropVariantChangeType will succeed without giving > you the requested vt, and if it did, changing it would be unsafe. > > I can see you need it in the VT_CLSID case, but then it would make > more sense to set it there. In fact, this version will leak memory if > UuidFromStringW fails. Thanks for spotting this bug. > > + if (!elem.len) break; > > When can this happen? This could happen when the query ends with '/'. That's something that I noticed while playing with my test app, I have not included a test case for this in the test suite though. > + new_value.vt = VT_UNKNOWN; > + hr = MetadataQueryReader_CreateInstance(This->block, > root, (IWICMetadataQueryReader **)&new_value.u.punkVal); > > I think you need to clear new_value first here? Yep, thanks. -- Dmitry.

June 25, 2017

Re: [PATCH v2] configure: Avoid clobbering x18 on arm64 when building with clang

by Martin Storsjö

On Sat, 24 Jun 2017, André Hentschel wrote: > Am 23.06.2017 um 20:22 schrieb Martin Storsjö: >> On Fri, 23 Jun 2017, André Hentschel wrote: >> >>> Am 23.06.2017 um 11:38 schrieb Martin Storsjö: >>>> On Fri, 23 Jun 2017, Alexandre Julliard wrote: >>>> >>>>> Martin Storsjö <martin(a)martin.st> writes: >>>>> >>>> I'm not sure if that discussion resulted in something else somewhere - if I'm just lucky that it has happened to work for me so far (but could break randomly if gcc would happen to use x18 anywhere in the called system libraries, and just clang seems to be using it more often so I only ran into it with clang?), or if something really was changed elsewhere with respect to this. André, can you fill us in on this matter? >>> >>> So your Wine build crashes even when running Wines cmd/winecfg/...? Is that fixed by removing the assembler line in dlls/ntdll/signal_arm64.c:signal_init_thread ? > > While I signed off your patch, could you please try what happens without > reserve-x18 and without the assemblerline i referenced? Ah, sorry, I missed to test that. It still crashes - the crash is in a code segment like this: 0x0000000140011570: add x8, x18, #0x0 => 0x0000000140011574: ldr x19, [x8,#8] I haven't really identified where this piece of code comes from though. // Martin

June 24, 2017

Re: [PATCH] gdiplus: Write API doc for GdipAddPathEllipse and GdipAddPathEllipseI

by Clemens Tamme

ok, i've looked for all possible return values and traced the function calls back. fixed both 2017-06-23 23:09 GMT+02:00 Vincent Povirk <madewokherd(a)gmail.com>: > + * GenericError If conversion from arc to bezier curve fails > > When would this happen? > > Best I can tell, it isn't supposed to be possible, and the check is > just coding defensively. > > + * Ok If ecerything works out as expected > > Typo. > > >

June 24, 2017

Re: [PATCH v2] configure: Avoid clobbering x18 on arm64 when building with clang

by André Hentschel

Am 23.06.2017 um 20:22 schrieb Martin Storsjö: > On Fri, 23 Jun 2017, André Hentschel wrote: > >> Am 23.06.2017 um 11:38 schrieb Martin Storsjö: >>> On Fri, 23 Jun 2017, Alexandre Julliard wrote: >>> >>>> Martin Storsjö <martin(a)martin.st> writes: >>>> >>> I'm not sure if that discussion resulted in something else somewhere - if I'm just lucky that it has happened to work for me so far (but could break randomly if gcc would happen to use x18 anywhere in the called system libraries, and just clang seems to be using it more often so I only ran into it with clang?), or if something really was changed elsewhere with respect to this. André, can you fill us in on this matter? >> >> So your Wine build crashes even when running Wines cmd/winecfg/...? Is that fixed by removing the assembler line in dlls/ntdll/signal_arm64.c:signal_init_thread ? While I signed off your patch, could you please try what happens without reserve-x18 and without the assemblerline i referenced?

June 24, 2017

Re: [3/3] user32/tests: Add test for GetWindowDisplayAffinity.

by Andrew Wesie

Windows 7 apparently requires clearing the WS_EX_LAYERED style to call SetWindowDisplayAffinity. This behavior differs from win8+. Absent any good way of having different test code for different Windows versions, I can just add a call to SetWindowLong to clear that style. -Andrew On Fri, Jun 23, 2017 at 6:11 PM, Marvin <testbot(a)winehq.org> wrote: > Hi, > > While running your changed tests on Windows, I think I found new failures. > Being a bot and all I'm not very good at pattern recognition, so I might be > wrong, but could you please double-check? > Full results can be found at > https://testbot.winehq.org/JobDetails.pl?Key=31812 > > Your paranoid android. > > > === w7u (32 bit win) === > win.c:9858: Test failed: SetWindowDisplayAffinity returned with 8 > win.c:9862: Test failed: GetWindowDisplayAffinity returned unexpected > affinity 0 > win.c:9865: Test failed: SetWindowDisplayAffinity returned with 8 > > === w7pro64 (32 bit win) === > win.c:9858: Test failed: SetWindowDisplayAffinity returned with 8 > win.c:9862: Test failed: GetWindowDisplayAffinity returned unexpected > affinity 0 > win.c:9865: Test failed: SetWindowDisplayAffinity returned with 8 > > === w7pro64 (64 bit win) === > win.c:9858: Test failed: SetWindowDisplayAffinity returned with 8 > win.c:9862: Test failed: GetWindowDisplayAffinity returned unexpected > affinity 0 > win.c:9865: Test failed: SetWindowDisplayAffinity returned with 8 >

June 23, 2017

Re: [3/3] user32/tests: Add test for GetWindowDisplayAffinity.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=31812 Your paranoid android. === w7u (32 bit win) === win.c:9858: Test failed: SetWindowDisplayAffinity returned with 8 win.c:9862: Test failed: GetWindowDisplayAffinity returned unexpected affinity 0 win.c:9865: Test failed: SetWindowDisplayAffinity returned with 8 === w7pro64 (32 bit win) === win.c:9858: Test failed: SetWindowDisplayAffinity returned with 8 win.c:9862: Test failed: GetWindowDisplayAffinity returned unexpected affinity 0 win.c:9865: Test failed: SetWindowDisplayAffinity returned with 8 === w7pro64 (64 bit win) === win.c:9858: Test failed: SetWindowDisplayAffinity returned with 8 win.c:9862: Test failed: GetWindowDisplayAffinity returned unexpected affinity 0 win.c:9865: Test failed: SetWindowDisplayAffinity returned with 8

June 23, 2017

Re: [PATCH] gdiplus: Write API doc for GdipAddPathEllipse and GdipAddPathEllipseI

by Vincent Povirk

+ * GenericError If conversion from arc to bezier curve fails When would this happen? Best I can tell, it isn't supposed to be possible, and the check is just coding defensively. + * Ok If ecerything works out as expected Typo.

June 23, 2017

Re: [6/7] windowscodecs: Add some tests for IWICMetadataQueryReader.

by Vincent Povirk

> + if (CompareStringW(LOCALE_NEUTRAL, NORM_IGNORECASE, queryW, len-1, name, len-1) == CSTR_EQUAL && decoder_data[j].query[len - 1] != 0) > + { > + if (winetest_debug > 1) > + trace("query: %s\n", wine_dbgstr_w(queryW + len - 1)); > + PropVariantInit(&value); Shouldn't this be PropVariantClear?

June 23, 2017

Re: [5/7] windowscodecs: Add initial implementation of IWICMetadataQueryReader::GetMetadataByName.

by Vincent Povirk

> + if (vt == VT_CLSID) > + { > + id->u.puuid = CoTaskMemAlloc(sizeof(GUID)); > + if (!id->u.puuid) return E_OUTOFMEMORY; > + > + hr = UuidFromStringW(next_token.u.bstrVal, id->u.puuid); > + } > + else > + hr = PropVariantChangeType(id, &next_token, 0, vt); > + PropVariantClear(&next_token); > + if (hr != S_OK) > + { > + PropVariantClear(id); > + PropVariantClear(schema); > + return hr; > + } > + > + id->vt = vt; Assigning the vt at the end of this is a bit strange. I don't know any situation here where PropVariantChangeType will succeed without giving you the requested vt, and if it did, changing it would be unsafe. I can see you need it in the VT_CLSID case, but then it would make more sense to set it there. In fact, this version will leak memory if UuidFromStringW fails. + if (!elem.len) break; When can this happen? + new_value.vt = VT_UNKNOWN; + hr = MetadataQueryReader_CreateInstance(This->block, root, (IWICMetadataQueryReader **)&new_value.u.punkVal); I think you need to clear new_value first here?

June 23, 2017

Re: [PATCH v2] configure: Avoid clobbering x18 on arm64 when building with clang

by Martin Storsjö

On Fri, 23 Jun 2017, André Hentschel wrote: > Am 23.06.2017 um 11:38 schrieb Martin Storsjö: >> On Fri, 23 Jun 2017, Alexandre Julliard wrote: >> >>> Martin Storsjö <martin(a)martin.st> writes: >>> >> I'm not sure if that discussion resulted in something else somewhere - if I'm just lucky that it has happened to work for me so far (but could break randomly if gcc would happen to use x18 anywhere in the called system libraries, and just clang seems to be using it more often so I only ran into it with clang?), or if something really was changed elsewhere with respect to this. André, can you fill us in on this matter? > > So your Wine build crashes even when running Wines cmd/winecfg/...? Is that fixed by removing the assembler line in dlls/ntdll/signal_arm64.c:signal_init_thread ? > As you already mentioned, if x18 is not treated special it should be a normal scratch register and one should be able to write to it nearly always without producing a crash... No, plain "wine64 cmd" runs fine (iirc), with wine built with both gcc and clang - with a debian jessie userland. (I've built wine without most extra libraries, so no X output or anything such, so I didn't try winecfg or anything like that.) Now with a gcc-built wine, if I try "wine64 uuidgen.exe", it runs fine. If I instead try "wine64 midl.exe", I get similar crashes as you have in https://bugs.winehq.org/show_bug.cgi?id=38886. If I instead switch to clang-built wine, "wine64 cmd" runs fine, but uuidgen.exe crashes before printing anything, with a crash point indicating that x18 had the wrong value. Now if I rebuild wine with clang, with "-Xclang -target-feature -Xclang +reserve-x18", uuidgen.exe works just as with gcc. And later with https://reviews.llvm.org/D34474 and https://reviews.llvm.org/D34475 and some trivial hookups of __builtin_ms_va_list and __attribute__((ms_abi)), midl.exe also gives working printouts. That's about as far as I've gone. > Thanks for your work on it! I'm also interested in doing the ARM64 work > in mingw-w64 again (as soon as clang is ready for that) I guess clang support for actually targeting win/arm64 is much further away (especially since there's no public msvc tools yet to compare to); the only thing so far is my RFC for handling their va_list. > See also: > https://bugs.winehq.org/show_bug.cgi?id=38780 > https://bugs.winehq.org/show_bug.cgi?id=38886 Thanks, these are good pointers to follow up from! // Martin

June 23, 2017