[PATCH] gdi32: Add DECLSPEC_HOTPATCH to GetDIBits
Signed-off-by: Alex Henrie <alexhenrie24(a)gmail.com> --- Fixes https://bugs.winehq.org/show_bug.cgi?id=45346 Although this patch fixes the bug in question, Zeb and I would like to know: Since all exported Windows API functions support hotpatching,[1] why do we add __attribute__((__ms_hook_prologue__)) attribute to functions ad hoc instead of making it part of the definitions of __stdcall and __cdecl in windef.h? [1] https://blogs.msdn.microsoft.com/oldnewthing/20110921-00/?p=9583 --- dlls/gdi32/dib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/gdi32/dib.c b/dlls/gdi32/dib.c index c7dcdb122c..1312bf0533 100644 --- a/dlls/gdi32/dib.c +++ b/dlls/gdi32/dib.c @@ -1208,7 +1208,7 @@ BITMAPINFO *copy_packed_dib( const BITMAPINFO *src_info, UINT usage ) * Success: Number of scan lines copied from bitmap * Failure: 0 */ -INT WINAPI GetDIBits( +INT WINAPI DECLSPEC_HOTPATCH GetDIBits( HDC hdc, /* [in] Handle to device context */ HBITMAP hbitmap, /* [in] Handle to bitmap */ UINT startscan, /* [in] First scan line to set in dest bitmap */ -- 2.18.0
Am 28.06.2018 um 17:28 schrieb Alex Henrie <alexhenrie24(a)gmail.com>:
Although this patch fixes the bug in question, Zeb and I would like to know: Since all exported Windows API functions support hotpatching,[1] why do we add __attribute__((__ms_hook_prologue__)) attribute to functions ad hoc instead of making it part of the definitions of __stdcall and __cdecl in windef.h? Alex asked me at Wineconf to comment on this.
When I added __ms_hook_prologue__ to gcc a long time ago we did not anticipate a high number of functions to need it. In 32 bit x86 code it adds minor overhead - there is a 2 byte nop at the start of every function that uses it. Having it on all functions was considered unnecessary and wasteful. On x64 this nop is 8 bytes long. At the time I worked on this Windows only made regular exported functions hotpatchable (what we have in our .spec files), but not e.g. COM methods. But Windows applications hook COM methods too, we wanted to be able to make static functions hotpatchable. On Windows this is a compiler option: https://docs.microsoft.com/en-us/cpp/build/reference/hotpatch-create-hotpatc... <https://docs.microsoft.com/en-us/cpp/build/reference/hotpatch-create-hotpatchable-image> . Doing the same thing would not work for us because we still have to make sure that e.g. IDirect3DDevice9::Present does not contain code that Steam et all cannot handle.
On 30/06/18 11:34, Stefan Dösinger wrote:
Am 28.06.2018 um 17:28 schrieb Alex Henrie <alexhenrie24(a)gmail.com <mailto:alexhenrie24(a)gmail.com>>:
Although this patch fixes the bug in question, Zeb and I would like to know: Since all exported Windows API functions support hotpatching,[1] why do we add __attribute__((__ms_hook_prologue__)) attribute to functions ad hoc instead of making it part of the definitions of __stdcall and __cdecl in windef.h? Alex asked me at Wineconf to comment on this.
When I added __ms_hook_prologue__ to gcc a long time ago we did not anticipate a high number of functions to need it. In 32 bit x86 code it adds minor overhead - there is a 2 byte nop at the start of every function that uses it. Having it on all functions was considered unnecessary and wasteful. On x64 this nop is 8 bytes long.
At the time I worked on this Windows only made regular exported functions hotpatchable (what we have in our .spec files), but not e.g. COM methods. But Windows applications hook COM methods too, we wanted to be able to make static functions hotpatchable. On Windows this is a compiler option: https://docs.microsoft.com/en-us/cpp/build/reference/hotpatch-create-hotpatc... . Doing the same thing would not work for us because we still have to make sure that e.g. IDirect3DDevice9::Present does not contain code that Steam et all cannot handle.
Sorry, I'm confused. Is the issue that static (COM) methods are hotpatchable and shouldn't be, or that they aren't and should be? In the latter case presumably we'd be adding it to the definition of __stdcall, so that declaration would be carried through to the requisite WINAPI flag. In the former case, firstly, are there really applications that choke on "mov %edi,%edi", and, secondly, since GCC currently has freedom to put whatever it wants to there (like the PC thunk) wouldn't we need some way to guarantee it's compatible anyway?
Am 2018-07-01 um 10:07 schrieb Zebediah Figura:
Sorry, I'm confused. Is the issue that static (COM) methods are hotpatchable and shouldn't be, or that they aren't and should be? When I implemented this in 2009 Microsoft did not make them hotpatchable. We decided we want to do be able to make them hotpatchable though.
Windows applications that hotpatch 32 bit code need to be able to process code that is different from the standard "8b-ff 55 8b-ec" instruction sequence, or at least used to in 2009. Windows before XP Service Pack 2 did not have the 2 byte NOP. This is why some apps hotpatch functions without DECLSPEC_HOTPATCH successfully sometimes. But because there are many, many possible opcodes, no Windows application will handle all possible code that is out there.
are there really applications that choke on "mov %edi,%edi", and, secondly, since GCC currently has freedom to put whatever it wants to there (like the PC thunk) wouldn't we need some way to guarantee it's compatible anyway? I don't know anything that chokes on the mov edi, edi (8b ff). This is why we had no problem with using DECLSPEC_HOTPATCH for things that are not technically hotpatchable on Windows. Eliminating everything Steam et all don't understand is hard (we don't know what it understands and what it doesn't), so putting something there that it understands is a way easier solution - even if we know that it technically incorrect.
(There are "easier" ways to hook COM methods than hotpatching bytecode - create a proxy object, replace the vtable in the object, overwrite the function pointer in the vtable. Afaik no Windows application goes that route though) I still think adding a 2 byte (or 8 byte) NOP to every WINAPI or CDECL function is wasteful.
Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
I still think adding a 2 byte (or 8 byte) NOP to every WINAPI or CDECL function is wasteful.
Just try to look at the problem from the other point of view: compare effort needed to add a 2 bytes nop to the efforts of investigating every bug out there that at least slightly smells like a hot patching issue. On the other hand entry point stack alignment also could be treated as a case by case thing, but in the end it was decided to add forcing stack alignment spec to the __stdcall definition. -- Dmitry.
Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
I still think adding a 2 byte (or 8 byte) NOP to every WINAPI or CDECL function is wasteful.
Just try to look at the problem from the other point of view: compare effort needed to add a 2 bytes nop to the efforts of investigating every bug out there that at least slightly smells like a hot patching issue. On the other hand entry point stack alignment also could be treated as a case by case thing, but in the end it was decided to add forcing stack alignment spec to the __stdcall definition.
Agreed, if there are no known cases where the hotpatch prefix breaks something, then we might as well use it for all functions. -- Alexandre Julliard julliard(a)winehq.org
Alexandre, would you please reconsider this patch? GCC currently does not give us any good way to make all WINAPI functions hotpatchable, and the next best thing is to add DCLSPEC_HOTPATCH when it fixes an application. -Alex
participants (5)
-
Alex Henrie -
Alexandre Julliard -
Dmitry Timoshkov -
Stefan Dösinger -
Zebediah Figura