(This is a little long, but I wanted to show my steps so I could be corrected) So after spending a very long time looking at it (probably too long), I think I have a solution. Looking at the code snippet from /dlls/atl100/tests/atl.c
hwnd = create_container_window(); SetWindowLongW(hwnd, GWLP_USERDATA, 0xdeadbeef); container = NULL; hr = AtlAxAttachControl(control, hwnd, &container); ok(1==0, "Leaving AtlAxAttachControl second 0x%08x, 0x%08x\n",container, &container); ok(container != NULL, "Expected not NULL!\n"); val = GetWindowLongW(hwnd, GWLP_USERDATA); ok(val == 0xdeadbeef, "Expected unchanged, returned %08x\n", val); DestroyWindow(hwnd);
The data that was never freed in this case was the container which got assigned in the AtlAxAttatchControl function
==8187== 64 bytes in 1 blocks are definitely lost in loss record 271 of 577 ==8187== at 0x7BC519A5: RtlAllocateHeap (heap.c:260) ==8187== by 0x4D27844: IOCS_Create (atl_ax.c:953) ==8187== by 0x4D27844: AtlAxAttachControl (???:0) ==8187== by 0x487D35A: func_atl (in /home/kduggan15/Documents/Wine/wine-valgrind/wine/dlls/atl100/tests/ atl100_test-stripped.exe.so) ==8187== by 0x487D66E: ??? (in /home/kduggan15/Documents/Wine/wine-valgrind/wine/dlls/atl100/tests/ atl100_test-stripped.exe.so) ==8187== by 0x487CDD8: main (in /home/kduggan15/Documents/Wine/wine-valgrind/wine/dlls/atl100/tests/ atl100_test-stripped.exe.so) ==8187==
The hard part of this for me was figuring out who was responsible for freeing container. After some time I came to the conclusion that the caller of the AtlAxAttachControl function was intended to free container. I namely came to this conclusion because I couldn't really see how the DestroyWindow function would be able to free the container. If DestoyWindow is supposed to be responsible for freeing container, I just wasn't smart enough to figure it out and will have too look again.
To fix it, I just added "IUnknown_Release(container);" after DestroyWindow(hwnd) so the new code would look like this
hwnd = create_container_window(); SetWindowLongW(hwnd, GWLP_USERDATA, 0xdeadbeef); container = NULL; hr = AtlAxAttachControl(control, hwnd, &container); ok(1==0, "Leaving AtlAxAttachControl second 0x%08x, 0x%08x\n",container, &container); ok(container != NULL, "Expected not NULL!\n"); val = GetWindowLongW(hwnd, GWLP_USERDATA); ok(val == 0xdeadbeef, "Expected unchanged, returned %08x\n", val); DestroyWindow(hwnd); + IUnknown_Release(container);
Valgrind shows that this does remove the memory leak If this looks good enough, then I'll go ahead and submit it as a patch. I just wanted to get opinions here before I did
Thank you all for your assistance up to this point. I've never worked with a project as large as Wine so exploring the source code and learning the debugging tools has been onerous, but I feel like I've learned a lot and feel accomplished (even if this attempt is incorrect).
On Sat, Mar 24, 2018 at 5:14 PM, Austin English austinenglish@gmail.com wrote:
On mobile, so can't verify, but look at: ==9846== by 0x4B07572: AtlAxAttachControl (atl_ax.c:1156) Line 1156 in atl_ax. And ==9846== by 0x4804025: test_AtlAxAttachControl (atl.c:902) Line 902 atl.c
You can use `find -name atl.c` to find its location in the source checkout.
On Sat, Mar 24, 2018, 13:52 Kieran Duggan kieranduggan15@gmail.com wrote:
If that's the case, Is there a way that I can see where the AtlAxAttachControl was called from? It's not necessary technically, but knowing which test case caused a memory leak would be useful When I ran the full test suite I got something like this
==9846== 64 bytes in 1 blocks are definitely lost in loss record 1,255 of 2,380 ==9846== at 0x7BC46859: notify_alloc (heap.c:260) ==9846== by 0x7BC49D56: RtlAllocateHeap (heap.c:1726) ==9846== by 0x4B06D40: IOCS_Create (atl_ax.c:952) ==9846== by 0x4B07572: AtlAxAttachControl (atl_ax.c:1156) ==9846== by 0x4804025: test_AtlAxAttachControl (atl.c:902) ==9846== by 0x4805130: func_atl (atl.c:1092) ==9846== by 0x4805485: run_test (test.h:603) ==9846== by 0x4805EDC: main (test.h:687) ==9846==
Because I can see what parameters the method was called with I get a little more useful information For example this one shows that a specific test case is where the function causes If a leak f I look around the logs a little more I don't see the other test cases breaking. So if there is a way to see where AtlAxAttachControl is being called from while running the test that would be useful.
On Sat, Mar 24, 2018 at 2:49 AM, Austin English austinenglish@gmail.com wrote:
On Sat, Mar 24, 2018 at 1:34 AM, Kieran Duggan kieranduggan15@gmail.com wrote:
So I'm attempting to run individual tests now. What exactly should I do after I "make service.ok"? I tried just running valgrind the way I
would
expect to use it i.e.
valgrind --trace-children=yes --track-origins=yes --leak-check=full --num-callers=20 --workaround-gcc296-bugs=yes --log-file="filename.log" ./wine /home/kduggan15/Documents/Wine/wine-valgrind/dlls/atl100/tests/
atl100_test-stripped.exe.so
I get
64 bytes in 1 blocks are definitely lost in loss record 1,250 of 2,380 ==11711== at 0x7BC46859: notify_alloc (heap.c:260) ==11711== by 0x7BC49D56: RtlAllocateHeap (heap.c:1726) ==11711== by 0x4D16D40: IOCS_Create (atl_ax.c:952) ==11711== by 0x4D17572: AtlAxAttachControl (atl_ax.c:1156) ==11711== by 0x4804025: ??? (in /home/kduggan15/Documents/Wine/wine-valgrind/dlls/atl100/tests/
atl100_test-stripped.exe.so)
The issue being that the atl100_test-stripped.exe.so doesn't have any
debug
symbols Also I initially tried make service.ok and I just get something
likemake
service is up to date or it just builds the tests without running
valgrind.
If it does run valgrind, then I don't know where the log is going (and
I
checked wine-valgrind/logs of course)
That's basically all I got. Sorry if these questions are a bit basic.
I'm
just running into a few roadblocks and could use the assistance. I
really
appreciate the help
Hi Kieran,
You've got it. Notice how the output is prefixed with the pid of the wine process; that's valgrind's output. The source code lines on the end of the line, like a backtrace.
So that output shows that there's a leak in atl100, allocated via heap.c's notify_alloc, then RtlAllocateHeap, then IOCS_Create, then AtlAxAttachControl).
Your task would be to find that leak, then fix it, i.e., free the memory where it should have been, but wasn't.
FYI, after running the tests, you may need to run 'rm service.ok', otherwise the test won't run (only if it succeeded beforehand).
Am 26.03.2018 um 06:12 schrieb Kieran Duggan kieranduggan15@gmail.com:
The hard part of this for me was figuring out who was responsible for freeing container. After some time I came to the conclusion that the caller of the AtlAxAttachControl function was intended to free container. I namely came to this conclusion because I couldn't really see how the DestroyWindow function would be able to free the container. If DestoyWindow is supposed to be responsible for freeing container, I just wasn't smart enough to figure it out and will have too look again.
As far as I can tell (and I haven't touched the atl code myself before) your conclusion is correct. AtlAxAttachControl creates the container object and returns the interface, so the caller is responsible for eventually destroying it. I would say submit your patch :-) . And don't forget to submit your gsoc proposal in time on the google website!
Ignore the following if it confuses you. It's some semi-educated guesswork on my part:
DestroyWindow doesn't know anything about COM or atl, so the DestroyWindow implementation is certainly not the right place. However, one thing is theoretically possible: DestroyWindow will send WM_DESTROY to the window callback procedure, which could in theory be responsible for releasing the container. AtlAxAttachControl appears to overwrite the wndproc (in IOCS_Attach). IOCS_Detach might be a candidate for releasing the container, it is called on WM_DESTROY.
However, I think this is unlikely because AtlAxAttachControl returns the interface to the caller. And convention says that the caller that receives an interface must release it once it is done. Of course Microsoft screws up its own COM rules a lot.
You can try to do to find out by looking at how Windows behaves when it is running this test: You can read the reference count by calling AddRef() followed by Release(). (there is a function get_refcount in numerous places, e.g. dlls/ddraw/tests). After AtlAxAttachControl I'd expect it to be 1. If it is still 1 after DestroyWindow(), the window callbacks should leave the refcount alone. If trying to call AddRef() after DestroyWindow crashes, or the refcount is zero, something in the window procedure released the container.
So I took another look at the ideas list and I thought that writing micro benchmarks for the D3D components would be about my speed and also fit into my interests. My only issue is that I am not sure which operations to test.
While I was looking for inspiration I came across this project[1] and thought that it could be a good focus for a GSoC project. That is, specifically writing micro benchmarks to measure the improvements of components effected by changes in wine-pba. I'm very uncertain about this however because it isn't officially in the master branch or even submitted at all. As far as I can tell the developer isn't directly associated with Wine. On the other hand having conformance tests and benchmarks made would save the developer time and allow his patch to be moved through quicker. But really it just looks interesting so I thought I would bring it up.
If this isn't a possibility, then I could use some help finding operations that I can work on.
I know this is very close to the deadline, so I apologize for my poor timing. I underestimated how long it would take me to submit a patch and ended up investing not enough time on the proposal. I hope that I can make up for this in the final hours. Again, thank you for your assistance.
[1] https://comminos.com/posts/2018-02-21-wined3d-profiling.html
On Mon, Mar 26, 2018 at 10:02 AM, Stefan Dösinger <stefandoesinger@gmail.com
wrote:
Am 26.03.2018 um 06:12 schrieb Kieran Duggan kieranduggan15@gmail.com:
The hard part of this for me was figuring out who was responsible for freeing container. After some time I came to the conclusion that the caller of the AtlAxAttachControl function was intended to free container. I namely came to this conclusion because I couldn't really see how the DestroyWindow function would be able to free the container. If DestoyWindow is supposed to be responsible for freeing container, I just wasn't smart enough to figure it out and will have too look again.
As far as I can tell (and I haven't touched the atl code myself before) your conclusion is correct. AtlAxAttachControl creates the container object and returns the interface, so the caller is responsible for eventually destroying it. I would say submit your patch :-) . And don't forget to submit your gsoc proposal in time on the google website!
Ignore the following if it confuses you. It's some semi-educated guesswork on my part:
DestroyWindow doesn't know anything about COM or atl, so the DestroyWindow implementation is certainly not the right place. However, one thing is theoretically possible: DestroyWindow will send WM_DESTROY to the window callback procedure, which could in theory be responsible for releasing the container. AtlAxAttachControl appears to overwrite the wndproc (in IOCS_Attach). IOCS_Detach might be a candidate for releasing the container, it is called on WM_DESTROY.
However, I think this is unlikely because AtlAxAttachControl returns the interface to the caller. And convention says that the caller that receives an interface must release it once it is done. Of course Microsoft screws up its own COM rules a lot.
You can try to do to find out by looking at how Windows behaves when it is running this test: You can read the reference count by calling AddRef() followed by Release(). (there is a function get_refcount in numerous places, e.g. dlls/ddraw/tests). After AtlAxAttachControl I'd expect it to be 1. If it is still 1 after DestroyWindow(), the window callbacks should leave the refcount alone. If trying to call AddRef() after DestroyWindow crashes, or the refcount is zero, something in the window procedure released the container.
On Mon, Mar 26, 2018 at 11:35 PM, Kieran Duggan kieranduggan15@gmail.com wrote:
So I took another look at the ideas list and I thought that writing micro benchmarks for the D3D components would be about my speed and also fit into my interests. My only issue is that I am not sure which operations to test.
While I was looking for inspiration I came across this project[1] and thought that it could be a good focus for a GSoC project. That is, specifically writing micro benchmarks to measure the improvements of components effected by changes in wine-pba. I'm very uncertain about this however because it isn't officially in the master branch or even submitted at all. As far as I can tell the developer isn't directly associated with Wine. On the other hand having conformance tests and benchmarks made would save the developer time and allow his patch to be moved through quicker. But really it just looks interesting so I thought I would bring it up.
If this isn't a possibility, then I could use some help finding operations that I can work on.
I know this is very close to the deadline, so I apologize for my poor timing. I underestimated how long it would take me to submit a patch and ended up investing not enough time on the proposal. I hope that I can make up for this in the final hours. Again, thank you for your assistance.
[1] https://comminos.com/posts/2018-02-21-wined3d-profiling.html
On Mon, Mar 26, 2018 at 10:02 AM, Stefan Dösinger stefandoesinger@gmail.com wrote:
Am 26.03.2018 um 06:12 schrieb Kieran Duggan kieranduggan15@gmail.com:
The hard part of this for me was figuring out who was responsible for freeing container. After some time I came to the conclusion that the caller of the AtlAxAttachControl function was intended to free container. I namely came to this conclusion because I couldn't really see how the DestroyWindow function would be able to free the container. If DestoyWindow is supposed to be responsible for freeing container, I just wasn't smart enough to figure it out and will have too look again.
As far as I can tell (and I haven't touched the atl code myself before) your conclusion is correct. AtlAxAttachControl creates the container object and returns the interface, so the caller is responsible for eventually destroying it. I would say submit your patch :-) . And don't forget to submit your gsoc proposal in time on the google website!
Ignore the following if it confuses you. It's some semi-educated guesswork on my part:
DestroyWindow doesn't know anything about COM or atl, so the DestroyWindow implementation is certainly not the right place. However, one thing is theoretically possible: DestroyWindow will send WM_DESTROY to the window callback procedure, which could in theory be responsible for releasing the container. AtlAxAttachControl appears to overwrite the wndproc (in IOCS_Attach). IOCS_Detach might be a candidate for releasing the container, it is called on WM_DESTROY.
However, I think this is unlikely because AtlAxAttachControl returns the interface to the caller. And convention says that the caller that receives an interface must release it once it is done. Of course Microsoft screws up its own COM rules a lot.
You can try to do to find out by looking at how Windows behaves when it is running this test: You can read the reference count by calling AddRef() followed by Release(). (there is a function get_refcount in numerous places, e.g. dlls/ddraw/tests). After AtlAxAttachControl I'd expect it to be 1. If it is still 1 after DestroyWindow(), the window callbacks should leave the refcount alone. If trying to call AddRef() after DestroyWindow crashes, or the refcount is zero, something in the window procedure released the container.
I don't think making a project about a wine fork (that's not upstreaming it) is a useful use of GSOC resources.
That's what I figured. I thought I would try anyway.
On Tue, Mar 27, 2018, 12:45 AM Austin English austinenglish@gmail.com wrote:
On Mon, Mar 26, 2018 at 11:35 PM, Kieran Duggan kieranduggan15@gmail.com wrote:
So I took another look at the ideas list and I thought that writing micro benchmarks for the D3D components would be about my speed and also fit
into
my interests. My only issue is that I am not sure which operations to test.
While I was looking for inspiration I came across this project[1] and thought that it could be a good focus for a GSoC project. That is, specifically writing micro benchmarks to measure the
improvements
of components effected by changes in wine-pba. I'm very uncertain about
this
however because it isn't officially in the master branch or even submitted at all. As far as I can tell the developer isn't directly associated with Wine. On the other hand having conformance tests and benchmarks made would save the developer time and allow his patch to be moved through quicker. But really it just looks interesting so I thought I would bring it up.
If this isn't a possibility, then I could use some help finding
operations
that I can work on.
I know this is very close to the deadline, so I apologize for my poor timing. I underestimated how long it would take me to submit a patch and ended up investing not enough time on the proposal. I hope that I can make up for this in the final
hours.
Again, thank you for your assistance.
[1] https://comminos.com/posts/2018-02-21-wined3d-profiling.html
On Mon, Mar 26, 2018 at 10:02 AM, Stefan Dösinger stefandoesinger@gmail.com wrote:
Am 26.03.2018 um 06:12 schrieb Kieran Duggan <kieranduggan15@gmail.com
:
The hard part of this for me was figuring out who was responsible for freeing container. After some time I came to the conclusion that the
caller
of the AtlAxAttachControl function was intended to free container. I namely came to this conclusion because I couldn't really see how the
DestroyWindow
function would be able to free the container. If DestoyWindow is
supposed
to be responsible for freeing container, I just wasn't smart enough to figure it out and will have too look again.
As far as I can tell (and I haven't touched the atl code myself before) your conclusion is correct. AtlAxAttachControl creates the container
object
and returns the interface, so the caller is responsible for eventually destroying it. I would say submit your patch :-) . And don't forget to submit your gsoc proposal in time on the google website!
Ignore the following if it confuses you. It's some semi-educated
guesswork
on my part:
DestroyWindow doesn't know anything about COM or atl, so the
DestroyWindow
implementation is certainly not the right place. However, one thing is theoretically possible: DestroyWindow will send WM_DESTROY to the window callback procedure, which could in theory be responsible for releasing
the
container. AtlAxAttachControl appears to overwrite the wndproc (in IOCS_Attach). IOCS_Detach might be a candidate for releasing the
container,
it is called on WM_DESTROY.
However, I think this is unlikely because AtlAxAttachControl returns the interface to the caller. And convention says that the caller that
receives
an interface must release it once it is done. Of course Microsoft
screws up
its own COM rules a lot.
You can try to do to find out by looking at how Windows behaves when it
is
running this test: You can read the reference count by calling AddRef() followed by Release(). (there is a function get_refcount in numerous
places,
e.g. dlls/ddraw/tests). After AtlAxAttachControl I'd expect it to be 1.
If
it is still 1 after DestroyWindow(), the window callbacks should leave
the
refcount alone. If trying to call AddRef() after DestroyWindow crashes,
or
the refcount is zero, something in the window procedure released the container.
I don't think making a project about a wine fork (that's not upstreaming it) is a useful use of GSOC resources.
-- -Austin GPG: 267B CC1F 053F 0749 (expires 2021/02/18)
On 27 March 2018 at 06:44, Austin English austinenglish@gmail.com wrote:
While I was looking for inspiration I came across this project[1] and thought that it could be a good focus for a GSoC project. That is, specifically writing micro benchmarks to measure the improvements of components effected by changes in wine-pba. I'm very uncertain about this however because it isn't officially in the master branch or even submitted at all. As far as I can tell the developer isn't directly associated with Wine. On the other hand having conformance tests and benchmarks made would save the developer time and allow his patch to be moved through quicker. But really it just looks interesting so I thought I would bring it up.
[1] https://comminos.com/posts/2018-02-21-wined3d-profiling.html
I don't think making a project about a wine fork (that's not upstreaming it) is a useful use of GSOC resources.
I'm not a GSoC mentor, so my opinion here is of limited value, but I did occasionally touch some Wine Direct3D code over the years. As a general principle, I think there's value in understanding performance differences between Wine-proper and its various forks and/or competitors. Since Stefan would be the likely mentor for such a project, ultimately that's up to him, of course.
Am 27.03.2018 um 06:35 schrieb Kieran Duggan kieranduggan15@gmail.com:
So I took another look at the ideas list and I thought that writing micro benchmarks for the D3D components would be about my speed and also fit into my interests. My only issue is that I am not sure which operations to test.
I have benchmarks for draws (just calling drawPrimitive in a loop), Clears, SetStreamSource + drawPrimitive, dynamic vertex buffer uploads and SetVertexShader. A few quick ideas that pop into my mind are Set*ShaderConstantF, dynamic textures, UpdateTexture/UpdateSurface, StretchRect. D3D versions other than d3d9 are interesting too.
Unfortunately I am no longer running my nightly tests after a change in Steam made it impossible to automate Steam games. At some point I should put some effort into adding more (full) games and running the benchmarks on a regular basis again.
-
Austin English -
Henri Verbeet -
Kieran Duggan -
Stefan Dösinger