On Tue Oct 18 10:05:01 2022 +0000, Matteo Bruni wrote:
> You forgot to increment the "byte" pointer, you're only printing the
> first byte.
> BTW, I'd just get rid of this function and related machinery entirely.
> The hex prints do the job just fine.
Fixed the increment. Thanks for pointing it out, I was running in circles trying to fix the SetRawValue() implementation because of that. I'll remove the function in the final version of the patch. (It's helpful to me while working on it.)
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/979#note_11308
On Wed Oct 19 09:30:39 2022 +0000, Patrick Hibbs wrote:
> changed this line in [version 5 of the diff](/wine/wine/-/merge_requests/979/diffs?diff_id=14510&start_sha=9be4873a7477520f0eb74837f2a269f864a9ac13#6af8d13245d87d86339fd93c308f9d2332f22092_9173_9199)
Done in v3 of the patch.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/979#note_11307
On Wed Oct 19 09:30:41 2022 +0000, Patrick Hibbs wrote:
> changed this line in [version 5 of the diff](/wine/wine/-/merge_requests/979/diffs?diff_id=14510&start_sha=9be4873a7477520f0eb74837f2a269f864a9ac13#6af8d13245d87d86339fd93c308f9d2332f22092_9180_9216)
Done in v3 of the patch.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/979#note_11306
On Wed Oct 19 09:30:44 2022 +0000, Patrick Hibbs wrote:
> changed this line in [version 5 of the diff](/wine/wine/-/merge_requests/979/diffs?diff_id=14510&start_sha=9be4873a7477520f0eb74837f2a269f864a9ac13#6af8d13245d87d86339fd93c308f9d2332f22092_9185_9222)
Done in v3 of the patch.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/979#note_11305
On Wed Oct 19 09:30:46 2022 +0000, Patrick Hibbs wrote:
> changed this line in [version 5 of the diff](/wine/wine/-/merge_requests/979/diffs?diff_id=14510&start_sha=9be4873a7477520f0eb74837f2a269f864a9ac13#6af8d13245d87d86339fd93c308f9d2332f22092_9196_9222)
Done in v3 of the patch.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/979#note_11304
On Wed Oct 19 09:30:48 2022 +0000, Patrick Hibbs wrote:
> changed this line in [version 5 of the diff](/wine/wine/-/merge_requests/979/diffs?diff_id=14510&start_sha=9be4873a7477520f0eb74837f2a269f864a9ac13#6af8d13245d87d86339fd93c308f9d2332f22092_9004_9004)
Done in v3 of the patch.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/979#note_11303
On Wed Oct 19 09:30:54 2022 +0000, Patrick Hibbs wrote:
> changed this line in [version 5 of the diff](/wine/wine/-/merge_requests/979/diffs?diff_id=14510&start_sha=9be4873a7477520f0eb74837f2a269f864a9ac13#6af8d13245d87d86339fd93c308f9d2332f22092_9210_9259)
Done in v3 of the patch.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/979#note_11302
On Wed Oct 19 09:30:56 2022 +0000, Patrick Hibbs wrote:
> changed this line in [version 5 of the diff](/wine/wine/-/merge_requests/979/diffs?diff_id=14510&start_sha=9be4873a7477520f0eb74837f2a269f864a9ac13#6af8d13245d87d86339fd93c308f9d2332f22092_9225_9259)
Done in v3 of the patch.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/979#note_11301
On Wed Oct 19 09:30:58 2022 +0000, Patrick Hibbs wrote:
> changed this line in [version 5 of the diff](/wine/wine/-/merge_requests/979/diffs?diff_id=14510&start_sha=9be4873a7477520f0eb74837f2a269f864a9ac13#6af8d13245d87d86339fd93c308f9d2332f22092_9265_9259)
Done in v3 of the patch.
Should I respond to all of these if I think it's been done?
Should I mark the thread resolved if I think it's been done, or is that something the reviewer should do?
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/979#note_11300
On Tue Oct 18 10:04:52 2022 +0000, Matteo Bruni wrote:
> First of all, sorry for the delay.
> These are some good tests, in principle. They already show that the
> implementation is not correct (e.g. it doesn't sanitize D3DXPT_BOOL
> values to 0/1) but I guess you already knew that.
> Actually, in regard to that point specifically: assuming that
> SetRawValue() does in fact do what it says, it suggests that BOOL values
> are stored unchanged and are instead sanitized on use (or e.g. by
> GetValue()). That's probably something that we want to fix in our
> implementation, separately from SetRawValue() itself. Marking the
> relevant tests as todo_wine initially and fixing them afterwards is an option.
> The expected results used by the tests are muddied quite a bit by the
> fact that the parameter value is not cleared to 0 (or any other value)
> between individual tests. That also makes it more complicated than
> necessary to add more tests. I'd fix that by explicitly clearing the
> tested parameter before each test.
> WRT your questions:
> 1. I think you can simply compute the expected results beforehand and
> put them into static const variables. You can certainly e.g. use your
> test_effect_setrawvalue_init_floats() function with some additional
> traces to figure out what those values are supposed to be, but I don't
> see any reason to recalculate the expected results at runtime in the
> final tests. It might make sense to have a comment in the test
> explaining what those "weird" expected results actually mean, hard to
> say at this point.
> 2. If I understand the test correctly, there is no evidence that
> SetRawValue() can "overflow" the current parameter. Either way, I think
> it's a matter of cutting the value size to the maximum of bytes and
> param->bytes or something like that.
> 3. For sure :smile:. I think we used the /Fx output parameter to get
> DWORD data.
> 4. Testing that SetRawValue() doesn't affect the "next" parameter when
> used with larger offsets / sizes is probably interesting. I also have a
> bunch of comments that I'll mention inline. There's most likely more,
> it's not an exhaustive list.
> One more thing. I saw you just pushed a new version of the patches,
> fixing a couple issues I had noticed. Nice, but generally please wait to
> push updates while I have the MR assigned to me, it means I'm actively
> reviewing it and changing stuff at that time can be somewhat disruptive
> for me.
Sorry about the unexpected push, I'll be more careful next time.
With regards to the bool fixups, are we sure we want to just write the raw data given to us? I ask because I've also noticed that SetRawValue() has a tendency to promote datatypes when given a single value and a vector / matrix paramater. I.e. float and int become a float4 and int4 respectively for both vectors and matrices. (MSDN calls this behavior a "cast.") This results in the next 3 values in the paramater's buffer being set to zero.
Assuming SetRawValue() shouldn't fix the bool values itself, should it also avoid fixing the ints and floats? If so, how should we indicate the need for a fixup to other functions? There doesn't seem to be any infrastructure to support that in wine currently. (We could abuse the paramater block support for that, but see the speculation below.)
The float and int promotions were the reason I didn't clear the paramater after each test. As doing so would cause us to miss the writes to the next 3 values in the paramater. As they would already be set to zero before the call. Should I still go ahead and clear the paramaters after each test anyway?
As a dirty hack, I added a call to zero out the paramaters after each test with a 4x4 matrix of zero ints in v3 of the test. That call is commented out in the push, but if you enable it it will succeed for most of the tests under win10. (I disabled it before I fixed the string test crashes. So it may work fine now, I haven't checked.) I haven't tried checking the paramaters after the current one for alterations. I was going to put that in it's own set of tests.
This part is speculation but, if it is a valid use of SetRawValue() to set adjacent paramaters regardless of type with one call, then the entire set of paramaters for a given effect needs to be in the same memory block. (Which seems to be not the case in wine, as there are plenty of calls to heap_alloc() / heap_realloc() using the param->data pointer in effect.c.) That would also imply that there is a specific ordering of paramaters in memory as well. Which would need to be tested for. If this behavior is combined with the bool assumption above, then we'd also need to test for order of operations too.
Assuming all of that needs to be done, how do we split this up? Because I have a feeling that this patchset is going to be rather large in scope. (If all of the assumptions and speculation are true that is.)
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/979#note_11299
This change is adding DWARF (CFI) unwind information to the
hand-written assembly of the `__wine_syscall_dispatcher` function.
This enables unwinding through the dispatcher from the Linux stack
into (and through) the Windows stack.
The general idea is that the `syscall_frame` struct contains the
content of the callee-save registers before the function call
(in particular the stack pointer and the return address). At any
point of the execution, we have a pointer into the `syscall_frame`
in $rcx, $rbp or $rsp.
For the CFI codes the general idea is that we are defining the
computations of the callee-save registers based on the
`syscall_frame` using DWARF’s `breg` instruction, rather than
relative to CFA.
This change adds a bunch of convenience macros, to (hopefully)
improve readability of the CFI instructions.
Note: Those change was used with great success for unwinding through
the dispatcher using a modified LLDB shown in the
[“how-wine-works-101”](https://werat.dev/blog/how-wine-works-101/)
blog post as well as for in the [Orbit profiler](https://github.com/google/orbit),
that has mixed-callstack unwinding support.
Test: Inspect callstacks reported by the Orbit profiler while
running some Windows targets using the modified wine, as well as
verify debugging reports correct callstacks when stepping with our
modified LLDB through the dispatcher itself (so that we are able
to unwind through the dispatcher at any instruction).
--
v7: ntdll: Add CFI unwind info to __wine_syscall_dispatcher (x86_64)
https://gitlab.winehq.org/wine/wine/-/merge_requests/1065
On Tue Oct 18 23:34:47 2022 +0000, Rémi Bernon wrote:
> The cause of the failure was that we're using incorrect mutex names in
> some places for the `display_device_init` mutex. In some unlikely event,
> which becomes apparently more likely with the changes here,
> `D3DKMTOpenAdapterFromGdiDisplayName` was able to enter the mutex CS at
> the same time as the devices are refreshed, and fails to find the video
> device in `HKLM\\HARDWARE\\DEVICEMAP\\VIDEO`, returning an error which
> ultimately triggers an allocation failure.
> This should never happens as the device refresh should also be done
> while holding the same mutex, but because the names were wrong it was
> two different mutexes. I've opened
> https://gitlab.winehq.org/wine/wine/-/merge_requests/1099 to fix this.
> This is for the `d3drm` tests failure at least, I didn't look at the
> ddraw ones, hopefully it's the same problem.
Marvin reported the same `ddraw7` errors in !1099, so they may not be fixed. I'm not able to reproduce them locally so far, using the same kind of setup as Gitlab and running the tests in a loop.
These take a longer time, especially as I'm running the other d3d tests with ddraw in case they have an impact, so it's possible I'm simply being unlucky, or that they are somehow only happening on the testbot now. I'll probably stop looking for now and see how things improve with that MR.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/944#note_11293
On Wed Oct 19 09:30:52 2022 +0000, Patrick Hibbs wrote:
> changed this line in [version 5 of the diff](/wine/wine/-/merge_requests/979/diffs?diff_id=14510&start_sha=9be4873a7477520f0eb74837f2a269f864a9ac13#6af8d13245d87d86339fd93c308f9d2332f22092_9206_9250)
That was a leftover I missed from a previous iteration of the test. It's been removed in v3 of the patch.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/979#note_11292
On Wed Oct 19 09:30:34 2022 +0000, Patrick Hibbs wrote:
> changed this line in [version 5 of the diff](/wine/wine/-/merge_requests/979/diffs?diff_id=14510&start_sha=9be4873a7477520f0eb74837f2a269f864a9ac13#6af8d13245d87d86339fd93c308f9d2332f22092_9089_9004)
The same issue with strings occurs with Texture and shader types.
I.e. Paramater is rendered irretrievable. Calling it's relevant GetXXX() function causes a crash if it's a scalar. Calling GetParamaterByName() will return an invalid handle.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/979#note_11291
On Wed Oct 19 09:30:33 2022 +0000, Patrick Hibbs wrote:
> changed this line in [version 5 of the diff](/wine/wine/-/merge_requests/979/diffs?diff_id=14510&start_sha=9be4873a7477520f0eb74837f2a269f864a9ac13#6af8d13245d87d86339fd93c308f9d2332f22092_9077_9004)
Actually it seems that the strings are rendered irretrievable by the call to SetRawValue().
Calling GetString() on a scalar string after the destruction causes a crash in win10. While calling it with a string vector doesn't crash, but returns D3DERR_INVALIDCALL instead.
I've updated the tests in v3 of the patch to check for this.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/979#note_11290
This seems to be more correct than what we previously had with fewer lines of code, so I like that. I still don't really like the extra flag I had to add for Nt{Create,Open}Key, but I couldn't find a way to make things work without it. I also checked that Windows doesn't use a similar flag (by iterating over all bit masks).
--
v3: wine.inf: Put the Clients key in the right place.
ntdll/tests: Factor out the NtEnumerateKey() tests.
kernelbase: Remove special Wow64 handling for HKEY_CLASSES_ROOT.
kernelbase: Remove special Wow6432Node handling from RegCreateKeyEx().
kernelbase: Remove special Wow6432Node handling from RegOpenKeyEx().
server: Don't return the actual 32-bit Software\Classes key.
ntdll/tests: Add some some Software\Classes query and enumerate tests.
ntdll/tests: Test that NtCreateKeyEx() also recursively obtains the Wow6432Node parent.
server: Recursively obtain the Wow6432Node parent.
https://gitlab.winehq.org/wine/wine/-/merge_requests/966
This is the first in a series of MRs to get rid of the `HTMLDocument` basedoc struct and separate the `HTMLDocumentNode` and `HTMLDocumentObj`, as suggested. The other interfaces and fields will be done in follow-up MRs. It should be a no-op in general.
Some of the things are ugly but temporary (e.g. the chunk in `HTMLDocument_put_designMode` on first commit) until the entire transition is done, then they will be cleaned up.
For most of the interfaces, they all deal with doc objects in most cases, so the implementation on HTMLDocumentNode tends to just forward to that. There are exceptions of course, but care has been taken to have them mostly no-ops ("mostly" because there's cases where the previous code did not check for e.g. NULL node, so it would crash instead, which is bad and should be fixed now).
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/1078
The mutex is also used in user32, gdi32, and winevulkan, where it is
opened through kernel32, which opens it from the session directory.
I believe this will fix the d3drm spurious errors, and hopefully the
ddraw ones too, but I haven't investigated these as the test takes
much longer to run.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/1099
Some applications spam this fixme.
fixme:cryptasn:CRYPT_GetBuiltinDecoder Unsupported decoder for lpszStructType 1.3.6.1.4.1.311.2.1.4
This OID is supported in wintrust.dll which crypt32 uses, so this console fixme doesn't make sense.
The fixme will only appear if no support for the requested decoder is available.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53800
Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/1104