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

Re: [PATCH] d3dx9_36/tests: Fix a memory leak in test_createtext.

by Matteo Bruni

2017-02-27 7:52 GMT+01:00 Alex Henrie <alexhenrie24(a)gmail.com>: > diff --git a/dlls/d3dx9_36/tests/mesh.c b/dlls/d3dx9_36/tests/mesh.c > index 95dd06e466..64b7b8823c 100644 > --- a/dlls/d3dx9_36/tests/mesh.c > +++ b/dlls/d3dx9_36/tests/mesh.c > @@ -4340,8 +4340,7 @@ static void test_createtext(IDirect3DDevice9 *device, HDC hdc, const char *text, > if (!compute_text_mesh(&mesh, text, deviation, extrusion, otm.otmEMSquare, glyphs)) > { > skip("Couldn't create mesh\n"); > - d3dxmesh->lpVtbl->Release(d3dxmesh); > - return; > + goto error; > } > mesh.fvf = D3DFVF_XYZ | D3DFVF_NORMAL; I would go in the other direction and strip all the error checking branches and gotos from those tests. compute_text_mesh() returns FALSE only on allocation failure and that's not supposed to happen in the controlled conditions of the test. It's probably better to just fail and crash in those cases.

Feb. 28, 2017

Re: [PATCH 4/5] server: Added server side mamed pipe read and write implementation and use it for message mode pipes.

by Jacek Caban

On 28.02.2017 15:12, Alexandre Julliard wrote: > Sebastian Lackner <sebastian(a)fds-team.de> writes: > >> I don't think write_process_memory is the right way to do it. This function >> also has a lot of overhead, like suspending the thread, or if /proc is not >> available reading/writing is done in chunks of sizeof(long). I'm pretty sure >> it would be faster to just transfer back the result with the same wineserver >> call. Also please note that the client side already adds a reply buffer, >> so the changes would mostly be on the server side. > Agreed. write_process_memory has many issues, and should only be used > for debugging APIs. We don't want to make it a required part of the > server protocol. I never meant this to be required, we could still do everything without it using APCs. I also never meant it to be part of server protocol itself. I imagined it to be used instead of APCs in some cases (like setting IO_STATUS_BLOCK bytes, possibly more for data in read/ioctl) to avoid round-trips, falling back to APCs in case of any error or other problem. Back to the current patch, it just uses currently available mechanisms to achieve the task and it seems to work well. As I described earlier, I could implement that without APCs for immediate result, but we would still need some notification to server that data has been completely transferred. If we want to do it that way, the cleanest solution IMO would be a new server call done by clients in server_*_file() functions called for immediate results after setting iosb fields and receiving data. Would you prefer that? Thanks, Jacek

Feb. 28, 2017

Re: [PATCH 1/5] wined3d: Use the command stream state in wined3d_cs_exec_draw().

by Sebastian Lackner

On 28.02.2017 15:06, Henri Verbeet wrote: > On 28 February 2017 at 14:52, Sebastian Lackner <sebastian(a)fds-team.de> wrote: >> This patch causes corrupted graphics in multiple games. The problem is >> caused by uninitialized memory in the wined3d_state structs. Currently >> the struct is only cleared in wined3d_cs_exec_reset_state, but it >> probably should be cleared always when WINED3D_STATE_INIT_DEFAULT is >> passed, otherwise the CS and the main state can go out of sync. Could >> you please take a look? >> > It does look like we forget to clear the state structure in > wined3d_device_reset(). Is fixing that not enough? > Yes, it seems to be sufficient (at least for all games I am aware of). I have sent the patch used for testing to the ML. Best regards, Sebastian

Feb. 28, 2017

Re: [PATCH 4/5] server: Added server side mamed pipe read and write implementation and use it for message mode pipes.

by Jacek Caban

On 28.02.2017 14:40, Sebastian Lackner wrote: > On 28.02.2017 14:03, Jacek Caban wrote: >> As long as we transfer result over server socket, we don't have control >> in server over when iosb and result buffer are filled, so we need a >> signal from client when we can signal async completion (signal >> event/object, queue completion). I sent a patch that was more inline >> with how we do that for calls where I/O is implemented on client side >> [1], but Alexandre preferred that to be in server [2]. Having that in >> server is not only cleaner, but also gives us more control, like in >> which case to fill iosb or signal event. It will also make it possible >> to implement other solution (bellow). > I assume Alexandre was mainly referring to the event / APC stuff. I assume > he also would be fine with a code path to return the result immediately > when available. This could for example be done by marking the async object > as "in progress" to avoid queing unnecessary APCs. If a real result is > available at the end of the call, it could be transferred right away. This > would also help to fix the problem with "immediate returns" below. What about the problem of client having to signal server when it received and processed the data? When would you set the event in that case? We can transfer data immediately, but we still need to know when we can set event / queue APC. Currently we know that we can do that when APC_ASYNC_IO returns. >> As long as we need a way for client to signal that data is transferred, >> we need some sort of notification from client. We could add a new server >> call, that client would be responsible to call after receiving data, >> that we could use instead of wait handle and APCs, but I think that >> moving more logic to client is not the right solution. >> >> An other solution is not to transfer result over server socket, but use >> write_process_memory() instead. This would allow us to do everything in >> a single server call. Unfortunately, it seems to me that there will be >> cases (like writing to memory what has write watch), when we will need >> APC-based fallback anyway. This is a huge change on its own, it needs at >> least: >> [...] > I don't think write_process_memory is the right way to do it. This function > also has a lot of overhead, like suspending the thread, or if /proc is not > available reading/writing is done in chunks of sizeof(long). That's another reason for a good fallback. I imagine we could use APC fallback if /proc is not available. Suspending thread doesn't sound too bad, esp. given that the alternative is an extra server call. > I'm pretty sure > it would be faster to just transfer back the result with the same wineserver > call. Also please note that the client side already adds a reply buffer, > so the changes would mostly be on the server side. Yes, data transfer is probably faster this way, but, as I said, the problem is not really about transfer speed itself, but the fact that we need server round-trips to know when data is transferred. >>> Also please note that we need to distinguish "immediate returns" and >>> "async returns" to properly implement FileIoCompletionNotificationInformation. >>> This especially means that waiting internally is not an option. >> It's easy to distinguish and still have internal waits. I can't point >> the right solution without looking deeper at >> FileIoCompletionNotificationInformation, but just as an example, you >> could do: >> >> if (iosb->status != STATUS_PENDING) mark_async_as_immediate_result(); >> >> inside request handlers just after calling fd ops. Then you'd have all >> the info you need inside async object. Am I missing something? > With the currently proposed solution this would not work. The fd ops > already call async_terminate -> async_set_result -> add_async_completion -> > add_completion, but depending on the completion flags this should not be > done if it is an immediate result. There is no way to know if its supposed > to be an "immediate return" inside of the async_*() helpers, and checking > afterwards is too late (unless we introduce a flag to block async objects > from doing actions right away). Did you have a different idea how it should > be implemented? In cases covered by my patch, async_terminate doesn't call async_set_result, but queues APC instead, so that's not a problem. More generally speaking, if we had a code path like you described, the solution I proposed would still work, just reverse the logic. Mark all created asyncs as immediate returns, and change it to async where appropriate, something like: if (iosb->status == STATUS_PENDING) mark_async_as_async_result(); in read/write/ioctl/flush should do. register_async request could do that unconditionally. Thanks, Jacek

Feb. 28, 2017

Re: [PATCH 4/5] server: Added server side mamed pipe read and write implementation and use it for message mode pipes.

by Alexandre Julliard

Sebastian Lackner <sebastian(a)fds-team.de> writes: > I don't think write_process_memory is the right way to do it. This function > also has a lot of overhead, like suspending the thread, or if /proc is not > available reading/writing is done in chunks of sizeof(long). I'm pretty sure > it would be faster to just transfer back the result with the same wineserver > call. Also please note that the client side already adds a reply buffer, > so the changes would mostly be on the server side. Agreed. write_process_memory has many issues, and should only be used for debugging APIs. We don't want to make it a required part of the server protocol. -- Alexandre Julliard julliard(a)winehq.org

Feb. 28, 2017

Re: [PATCH 1/5] wined3d: Use the command stream state in wined3d_cs_exec_draw().

by Henri Verbeet

On 28 February 2017 at 14:52, Sebastian Lackner <sebastian(a)fds-team.de> wrote: > This patch causes corrupted graphics in multiple games. The problem is > caused by uninitialized memory in the wined3d_state structs. Currently > the struct is only cleared in wined3d_cs_exec_reset_state, but it > probably should be cleared always when WINED3D_STATE_INIT_DEFAULT is > passed, otherwise the CS and the main state can go out of sync. Could > you please take a look? > It does look like we forget to clear the state structure in wined3d_device_reset(). Is fixing that not enough?

Feb. 28, 2017

Re: [PATCH 1/5] wined3d: Use the command stream state in wined3d_cs_exec_draw().

by Sebastian Lackner

On 28.02.2017 06:11, Henri Verbeet wrote: > Signed-off-by: Henri Verbeet <hverbeet(a)codeweavers.com> > --- > dlls/wined3d/cs.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > This patch causes corrupted graphics in multiple games. The problem is caused by uninitialized memory in the wined3d_state structs. Currently the struct is only cleared in wined3d_cs_exec_reset_state, but it probably should be cleared always when WINED3D_STATE_INIT_DEFAULT is passed, otherwise the CS and the main state can go out of sync. Could you please take a look? Example app (~40 MB): https://archive.org/download/MotoRacer2Demo/MotoRacer2Demo.zip (sha256: 02c8523d61619b9f65d874e15af9838ebf78938a720c5b61504d97c0b7cc68c9) Best regards, Sebastian

Feb. 28, 2017

Re: [PATCH 4/5] server: Added server side mamed pipe read and write implementation and use it for message mode pipes.

by Sebastian Lackner

On 28.02.2017 14:03, Jacek Caban wrote: > Hi Sebastian, > > On 27.02.2017 19:42, Sebastian Lackner wrote: >> On 22.02.2017 14:51, Jacek Caban wrote: >>> Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> >>> --- >>> dlls/kernel32/tests/pipe.c | 28 ++- >>> dlls/ntdll/file.c | 7 +- >>> dlls/ntdll/tests/file.c | 2 +- >>> server/async.c | 9 + >>> server/file.h | 1 + >>> server/named_pipe.c | 446 >>> ++++++++++++++++++++++++++++++++++++++++++--- >>> 6 files changed, 447 insertions(+), 46 deletions(-) >> As I see this updated patchset allows to return the result immediately, >> but still uses a USR1 signal to transfer back the IO completion info. >> Wouldn't it be useful to get rid of that aswell? > I believe it would be useful, but I assumed it does not block this > patch. It's tricky, but I gave it some thoughts before sending patches > and intend to look at that as some point. Here are some thoughts: I'm not sure if this should be blocking the patch, but at least for me it was not really obvious how you were planning to fix it afterwards. > > As long as we transfer result over server socket, we don't have control > in server over when iosb and result buffer are filled, so we need a > signal from client when we can signal async completion (signal > event/object, queue completion). I sent a patch that was more inline > with how we do that for calls where I/O is implemented on client side > [1], but Alexandre preferred that to be in server [2]. Having that in > server is not only cleaner, but also gives us more control, like in > which case to fill iosb or signal event. It will also make it possible > to implement other solution (bellow). I assume Alexandre was mainly referring to the event / APC stuff. I assume he also would be fine with a code path to return the result immediately when available. This could for example be done by marking the async object as "in progress" to avoid queing unnecessary APCs. If a real result is available at the end of the call, it could be transferred right away. This would also help to fix the problem with "immediate returns" below. > > As long as we need a way for client to signal that data is transferred, > we need some sort of notification from client. We could add a new server > call, that client would be responsible to call after receiving data, > that we could use instead of wait handle and APCs, but I think that > moving more logic to client is not the right solution. > > An other solution is not to transfer result over server socket, but use > write_process_memory() instead. This would allow us to do everything in > a single server call. Unfortunately, it seems to me that there will be > cases (like writing to memory what has write watch), when we will need > APC-based fallback anyway. This is a huge change on its own, it needs at > least: > [...] I don't think write_process_memory is the right way to do it. This function also has a lot of overhead, like suspending the thread, or if /proc is not available reading/writing is done in chunks of sizeof(long). I'm pretty sure it would be faster to just transfer back the result with the same wineserver call. Also please note that the client side already adds a reply buffer, so the changes would mostly be on the server side. >> Also please note that we need to distinguish "immediate returns" and >> "async returns" to properly implement FileIoCompletionNotificationInformation. >> This especially means that waiting internally is not an option. > It's easy to distinguish and still have internal waits. I can't point > the right solution without looking deeper at > FileIoCompletionNotificationInformation, but just as an example, you > could do: > > if (iosb->status != STATUS_PENDING) mark_async_as_immediate_result(); > > inside request handlers just after calling fd ops. Then you'd have all > the info you need inside async object. Am I missing something? With the currently proposed solution this would not work. The fd ops already call async_terminate -> async_set_result -> add_async_completion -> add_completion, but depending on the completion flags this should not be done if it is an immediate result. There is no way to know if its supposed to be an "immediate return" inside of the async_*() helpers, and checking afterwards is too late (unless we introduce a flag to block async objects from doing actions right away). Did you have a different idea how it should be implemented? Best regards, Sebastian

Feb. 28, 2017

Re: [PATCH 4/5] server: Added server side mamed pipe read and write implementation and use it for message mode pipes.

by Jacek Caban

Hi Sebastian, On 27.02.2017 19:42, Sebastian Lackner wrote: > On 22.02.2017 14:51, Jacek Caban wrote: >> Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> >> --- >> dlls/kernel32/tests/pipe.c | 28 ++- >> dlls/ntdll/file.c | 7 +- >> dlls/ntdll/tests/file.c | 2 +- >> server/async.c | 9 + >> server/file.h | 1 + >> server/named_pipe.c | 446 >> ++++++++++++++++++++++++++++++++++++++++++--- >> 6 files changed, 447 insertions(+), 46 deletions(-) > As I see this updated patchset allows to return the result immediately, > but still uses a USR1 signal to transfer back the IO completion info. > Wouldn't it be useful to get rid of that aswell? I believe it would be useful, but I assumed it does not block this patch. It's tricky, but I gave it some thoughts before sending patches and intend to look at that as some point. Here are some thoughts: As long as we transfer result over server socket, we don't have control in server over when iosb and result buffer are filled, so we need a signal from client when we can signal async completion (signal event/object, queue completion). I sent a patch that was more inline with how we do that for calls where I/O is implemented on client side [1], but Alexandre preferred that to be in server [2]. Having that in server is not only cleaner, but also gives us more control, like in which case to fill iosb or signal event. It will also make it possible to implement other solution (bellow). As long as we need a way for client to signal that data is transferred, we need some sort of notification from client. We could add a new server call, that client would be responsible to call after receiving data, that we could use instead of wait handle and APCs, but I think that moving more logic to client is not the right solution. An other solution is not to transfer result over server socket, but use write_process_memory() instead. This would allow us to do everything in a single server call. Unfortunately, it seems to me that there will be cases (like writing to memory what has write watch), when we will need APC-based fallback anyway. This is a huge change on its own, it needs at least: - Use write_process_memory() in async_terminate() when we know we can do that for particular async - Since above will complicate wait handle logic, it would be nice to move this logic to read/write/ioctl/flush request handlers instead of duplicating that in fd ops. - The above will need invasive change for device IRP-based calls, which currently use irp_call, not async, for wait handles. - The above has a side quest: it would be nice to consider making device calls blocking more compatible. I believe that we should always wait until real driver completes request (even for non-blocking calls) and return STATUS_PENDING only if driver returned STATUS_PENDING. That's something to keep in mind while doing design decision, not strictly related. - Right now, user passed APCs are stored in client and transferred as APC_ASYNC_IO. This would need to be changed. I'm sure some of those tasks will need more smaller changes. Those would be much nicer to work on having named pipes in the tree first, so that we can at least test the code. And, since I think we will need APC+blocking as a fallback anyway, which we can use blocking as the main solution for the time being and have it well tested. That said, I agree it would be useful, I just think it shouldn't block the patch. > Also please note that we need to distinguish "immediate returns" and > "async returns" to properly implement FileIoCompletionNotificationInformation. > This especially means that waiting internally is not an option. It's easy to distinguish and still have internal waits. I can't point the right solution without looking deeper at FileIoCompletionNotificationInformation, but just as an example, you could do: if (iosb->status != STATUS_PENDING) mark_async_as_immediate_result(); inside request handlers just after calling fd ops. Then you'd have all the info you need inside async object. Am I missing something? >> + iosb = async_get_iosb( async ); >> + if ((blocking || iosb->status != STATUS_PENDING) >> + && !(handle = alloc_handle( current->process, async, SYNCHRONIZE, 0 ))) >> + async_terminate( async, get_error() ); > I don't think this will work as expected. async_terminate might queue an > APC, but the caller will immediately deallocate the async struct in server_write_file > because status != STATUS_PENDING. Good point, I will send an updated patch. Thanks for the review, Jacek [1] https://www.winehq.org/pipermail/wine-patches/2017-January/157013.html [2] https://www.winehq.org/pipermail/wine-devel/2017-February/116060.html

Feb. 28, 2017

Re: tools: remove wineinstall

by Dmitry Timoshkov

Austin English <austinenglish(a)gmail.com> wrote: > -2. QUICK START > - > -Whenever you compile from source, it is recommended to use the Wine > -Installer to build and install Wine. From the top-level directory > -of the Wine source (which contains this file), run: > - > -./tools/wineinstall > - > -Run programs as "wine program". For more information and problem > -resolution, read the rest of this file, the Wine man page, and > -especially the wealth of information found at http://www.winehq.org. Perhaps the 2nd part of this section should stay since it doesn't have anything to do with the wineinstall script. And regarding compilation it's worth to at least mention wiki/docs/forums I'd guess. -- Dmitry.

Feb. 28, 2017