July 2016 - Wine-Devel - list.winehq.org

Re: [PATCH 3/3] ntdll: For Mac 64-bit, poke NtCurrentTeb()->ThreadLocalStoragePointer to the corresponding offset from %gs.

by Sebastian Lackner

On 27.07.2016 18:27, Ken Thomases wrote: > On Jul 27, 2016, at 11:06 AM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: >> >> On 27.07.2016 17:42, Ken Thomases wrote: >>> 64-bit Windows apps have hard-coded accesses to %gs:0x58 baked into them. They >>> need to find the ThreadLocalStoragePointer there. >>> >>> Technically, the gsbase register and the memory it points to belong to the >>> pthread implementation on macOS. It's used for the pthread TLS implementation. >>> Slot 11 (offset 0x58) is currently used for the implementation of the ttyname() >>> system library function. We do not anticipate that Wine or any of the system >>> libraries or frameworks it uses will call ttyname(). Furthermore, Apple has >>> made it so that future releases of macOS will no longer use that slot. So, we >>> hijack it for our purposes. >>> >>> Signed-off-by: Ken Thomases <ken(a)codeweavers.com> >>> --- >>> dlls/ntdll/loader.c | 11 +++++++- >>> dlls/ntdll/signal_x86_64.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 72 insertions(+), 1 deletion(-) >> >> I'm not sure if I correctly understand the purpose of this patch. If you have to >> set %gs:0x58 to some specific value, why not just use an assembly instruction for that? >> > > alloc_tls_slot() is setting ThreadLocalStoragePointer for all threads, not just the current one. > > -Ken > Hm, I see. Do you know if the offset really changed in the past? I am asking because I assume the bruteforcing is an attempt to make the code more reliable, however on the other hand you still make the assumption that pthread keys directly correspond to indices into the %gs segment, which could also change. A check that it matches expected behavior is probably sufficient. Besides that, if the gsbase cannot be located, it probably would be preferred to skip this code on following attempts.

July 27, 2016

Re: [PATCH 1/3] ntdll: For Mac 64-bit, poke the TEB address to %gs:0x30 and re-enable the inlining of NtCurrentTeb().

by Ken Thomases

On Jul 27, 2016, at 11:38 AM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: > > On 27.07.2016 18:30, Ken Thomases wrote: >> On Jul 27, 2016, at 11:08 AM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: >>> >>> On 27.07.2016 17:42, Ken Thomases wrote: >>>> 64-bit Windows apps have hard-coded accesses to %gs:0x30 baked into them. They >>>> need to find the TEB self pointer there. >>>> >>>> Technically, the gsbase register and the memory it points to belong to the >>>> pthread implementation on macOS. It's used for the pthread TLS implementation. >>>> However, study of the sources and experimentation reveal that TLS slot 6 >>>> (offset 0x30) is not currently used. Furthermore, Apple has promised to not >>>> use that slot in the future. So, we hijack it for our purposes. >>>> >>>> This removes the implementation of NtCurrentTeb() based on pthread_key_create() >>>> and pthread_set/getspecific(). >>>> >>>> Signed-off-by: Ken Thomases <ken(a)codeweavers.com> >>>> --- >>>> dlls/ntdll/ntdll.spec | 2 +- >>>> dlls/ntdll/signal_x86_64.c | 34 +++------------------------------- >>>> include/winnt.h | 2 +- >>>> 3 files changed, 5 insertions(+), 33 deletions(-) >>> >>> Nice to hear that Apple agreed to keep that slot reserved. Would it make sense to >>> keep the NtCurrentTeb export neverthless, for compatibility with libraries compiled >>> against old Wine? >> >> Hmm. I don't know. I hadn't considered it. What sorts of libraries? Did you have some use case in mind? I guess I'd be surprised if there were such libraries around. >> >> -Ken >> > > I'm not sure if there are any important examples, but all libraries / executables > compiled with the old Wine headers will have references to the NtCurrentTeb export. > If users should still be able to use any previously compiled code, it would be > necessary to keep them. Thats also why we decided to keep it in our Staging patchset. Previously compiled code other than Wine itself, right? I.e. winelib apps. Which call NtCurrentTeb(). Built for Mac 64-bit. I guess I don't think that's an important case, but if others disagree I can change the patch. -Ken

July 27, 2016

Re: [PATCH 1/3] ntdll: For Mac 64-bit, poke the TEB address to %gs:0x30 and re-enable the inlining of NtCurrentTeb().

by Sebastian Lackner

On 27.07.2016 18:30, Ken Thomases wrote: > On Jul 27, 2016, at 11:08 AM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: >> >> On 27.07.2016 17:42, Ken Thomases wrote: >>> 64-bit Windows apps have hard-coded accesses to %gs:0x30 baked into them. They >>> need to find the TEB self pointer there. >>> >>> Technically, the gsbase register and the memory it points to belong to the >>> pthread implementation on macOS. It's used for the pthread TLS implementation. >>> However, study of the sources and experimentation reveal that TLS slot 6 >>> (offset 0x30) is not currently used. Furthermore, Apple has promised to not >>> use that slot in the future. So, we hijack it for our purposes. >>> >>> This removes the implementation of NtCurrentTeb() based on pthread_key_create() >>> and pthread_set/getspecific(). >>> >>> Signed-off-by: Ken Thomases <ken(a)codeweavers.com> >>> --- >>> dlls/ntdll/ntdll.spec | 2 +- >>> dlls/ntdll/signal_x86_64.c | 34 +++------------------------------- >>> include/winnt.h | 2 +- >>> 3 files changed, 5 insertions(+), 33 deletions(-) >> >> Nice to hear that Apple agreed to keep that slot reserved. Would it make sense to >> keep the NtCurrentTeb export neverthless, for compatibility with libraries compiled >> against old Wine? > > Hmm. I don't know. I hadn't considered it. What sorts of libraries? Did you have some use case in mind? I guess I'd be surprised if there were such libraries around. > > -Ken > I'm not sure if there are any important examples, but all libraries / executables compiled with the old Wine headers will have references to the NtCurrentTeb export. If users should still be able to use any previously compiled code, it would be necessary to keep them. Thats also why we decided to keep it in our Staging patchset.

July 27, 2016

Re: [PATCH 1/3] ntdll: For Mac 64-bit, poke the TEB address to %gs:0x30 and re-enable the inlining of NtCurrentTeb().

by Ken Thomases

On Jul 27, 2016, at 11:08 AM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: > > On 27.07.2016 17:42, Ken Thomases wrote: >> 64-bit Windows apps have hard-coded accesses to %gs:0x30 baked into them. They >> need to find the TEB self pointer there. >> >> Technically, the gsbase register and the memory it points to belong to the >> pthread implementation on macOS. It's used for the pthread TLS implementation. >> However, study of the sources and experimentation reveal that TLS slot 6 >> (offset 0x30) is not currently used. Furthermore, Apple has promised to not >> use that slot in the future. So, we hijack it for our purposes. >> >> This removes the implementation of NtCurrentTeb() based on pthread_key_create() >> and pthread_set/getspecific(). >> >> Signed-off-by: Ken Thomases <ken(a)codeweavers.com> >> --- >> dlls/ntdll/ntdll.spec | 2 +- >> dlls/ntdll/signal_x86_64.c | 34 +++------------------------------- >> include/winnt.h | 2 +- >> 3 files changed, 5 insertions(+), 33 deletions(-) > > Nice to hear that Apple agreed to keep that slot reserved. Would it make sense to > keep the NtCurrentTeb export neverthless, for compatibility with libraries compiled > against old Wine? Hmm. I don't know. I hadn't considered it. What sorts of libraries? Did you have some use case in mind? I guess I'd be surprised if there were such libraries around. -Ken

July 27, 2016

Re: [PATCH 3/3] ntdll: For Mac 64-bit, poke NtCurrentTeb()->ThreadLocalStoragePointer to the corresponding offset from %gs.

by Ken Thomases

On Jul 27, 2016, at 11:06 AM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: > > On 27.07.2016 17:42, Ken Thomases wrote: >> 64-bit Windows apps have hard-coded accesses to %gs:0x58 baked into them. They >> need to find the ThreadLocalStoragePointer there. >> >> Technically, the gsbase register and the memory it points to belong to the >> pthread implementation on macOS. It's used for the pthread TLS implementation. >> Slot 11 (offset 0x58) is currently used for the implementation of the ttyname() >> system library function. We do not anticipate that Wine or any of the system >> libraries or frameworks it uses will call ttyname(). Furthermore, Apple has >> made it so that future releases of macOS will no longer use that slot. So, we >> hijack it for our purposes. >> >> Signed-off-by: Ken Thomases <ken(a)codeweavers.com> >> --- >> dlls/ntdll/loader.c | 11 +++++++- >> dlls/ntdll/signal_x86_64.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 72 insertions(+), 1 deletion(-) > > I'm not sure if I correctly understand the purpose of this patch. If you have to > set %gs:0x58 to some specific value, why not just use an assembly instruction for that? > alloc_tls_slot() is setting ThreadLocalStoragePointer for all threads, not just the current one. -Ken

July 27, 2016

Re: [PATCH 1/3] ntdll: For Mac 64-bit, poke the TEB address to %gs:0x30 and re-enable the inlining of NtCurrentTeb().

by Sebastian Lackner

On 27.07.2016 17:42, Ken Thomases wrote: > 64-bit Windows apps have hard-coded accesses to %gs:0x30 baked into them. They > need to find the TEB self pointer there. > > Technically, the gsbase register and the memory it points to belong to the > pthread implementation on macOS. It's used for the pthread TLS implementation. > However, study of the sources and experimentation reveal that TLS slot 6 > (offset 0x30) is not currently used. Furthermore, Apple has promised to not > use that slot in the future. So, we hijack it for our purposes. > > This removes the implementation of NtCurrentTeb() based on pthread_key_create() > and pthread_set/getspecific(). > > Signed-off-by: Ken Thomases <ken(a)codeweavers.com> > --- > dlls/ntdll/ntdll.spec | 2 +- > dlls/ntdll/signal_x86_64.c | 34 +++------------------------------- > include/winnt.h | 2 +- > 3 files changed, 5 insertions(+), 33 deletions(-) Nice to hear that Apple agreed to keep that slot reserved. Would it make sense to keep the NtCurrentTeb export neverthless, for compatibility with libraries compiled against old Wine?

July 27, 2016

Re: [PATCH 3/3] ntdll: For Mac 64-bit, poke NtCurrentTeb()->ThreadLocalStoragePointer to the corresponding offset from %gs.

by Sebastian Lackner

On 27.07.2016 17:42, Ken Thomases wrote: > 64-bit Windows apps have hard-coded accesses to %gs:0x58 baked into them. They > need to find the ThreadLocalStoragePointer there. > > Technically, the gsbase register and the memory it points to belong to the > pthread implementation on macOS. It's used for the pthread TLS implementation. > Slot 11 (offset 0x58) is currently used for the implementation of the ttyname() > system library function. We do not anticipate that Wine or any of the system > libraries or frameworks it uses will call ttyname(). Furthermore, Apple has > made it so that future releases of macOS will no longer use that slot. So, we > hijack it for our purposes. > > Signed-off-by: Ken Thomases <ken(a)codeweavers.com> > --- > dlls/ntdll/loader.c | 11 +++++++- > dlls/ntdll/signal_x86_64.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 72 insertions(+), 1 deletion(-) I'm not sure if I correctly understand the purpose of this patch. If you have to set %gs:0x58 to some specific value, why not just use an assembly instruction for that?

July 27, 2016

Re: [v2 PATCH] ucrtbase: Implement _get_narrow_winmain_command_line/_get_wide_winmain_command_line

by Dmitry Timoshkov

Piotr Caban <piotr.caban(a)gmail.com> wrote: > >>> Once again: > >>> Where did you get an idea of such an implementation? Is this copied from > >>> some place or based on some other implementation? Why for instance simple > >>> 'return GetCommandLineA()' is not enough as an initial implementation? > >> > >> Probably I'm just stating obvious but the function skips executable name > >> from the command line. That's why returning what GetCommandLineA returns > >> is incorrect. > > > > From what documentation/description is that obvious? That's not a common > > practice to send patches for not very obvious things without any tests. > > Why this implementation treats specially tabs and spaces but not other > > white space characters? Is that also obvious? How about the tests for > > quotes/embedded quotes? > > > I was not asking Nikolay for the tests because I think it's not easy to > write them. Some interesting tests needs executable name with ' ' in its > name. I also don't like the idea of starting many processes just to test > the function. On the other hand the initial test executable may be > started in many different ways (like e.g. passing first argument quoted > makes it impossible to check the return with simple comparison). > > That doesn't mean that I'm against adding a test or 2 for that. Anyway I > think this implementation is good after doing some tests myself. Testing on your own is good, however that doesn't count like an answer to my questions above which still left unanswered. If Wine development would follow this kind of quality control it wouldn't get that far as it is now. -- Dmitry.

July 27, 2016

Re: [v2 PATCH] ucrtbase: Implement _get_narrow_winmain_command_line/_get_wide_winmain_command_line

by Piotr Caban

On 07/27/16 15:29, Dmitry Timoshkov wrote: > Piotr Caban <piotr.caban(a)gmail.com> wrote: > >>> Once again: >>> Where did you get an idea of such an implementation? Is this copied from >>> some place or based on some other implementation? Why for instance simple >>> 'return GetCommandLineA()' is not enough as an initial implementation? >> >> Probably I'm just stating obvious but the function skips executable name >> from the command line. That's why returning what GetCommandLineA returns >> is incorrect. > > From what documentation/description is that obvious? That's not a common > practice to send patches for not very obvious things without any tests. > Why this implementation treats specially tabs and spaces but not other > white space characters? Is that also obvious? How about the tests for > quotes/embedded quotes? > I was not asking Nikolay for the tests because I think it's not easy to write them. Some interesting tests needs executable name with ' ' in its name. I also don't like the idea of starting many processes just to test the function. On the other hand the initial test executable may be started in many different ways (like e.g. passing first argument quoted makes it impossible to check the return with simple comparison). That doesn't mean that I'm against adding a test or 2 for that. Anyway I think this implementation is good after doing some tests myself.

July 27, 2016

Re: [PATCH 1/6] amstream: Fix AMAudioData::QueryInterface.

by Nikolay Sivov

On 27.07.2016 15:51, Anton Baskanov wrote: > Signed-off-by: Anton Baskanov <baskanov(a)gmail.com> > --- > dlls/amstream/audiodata.c | 1 - > dlls/amstream/tests/Makefile.in | 3 +- > dlls/amstream/tests/audiodata.c | 67 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 69 insertions(+), 2 deletions(-) > create mode 100755 dlls/amstream/tests/audiodata.c Hi, Anton. Please keep tests in existing amstream.c file. + > +static void test_query_interface(void) > +{ > + IUnknown *unknown = create_audio_data(); > + IMemoryData *memory_data = NULL; > + IAudioData *audio_data = NULL; > + > + HRESULT result = S_OK; result initialization is not needed; > + > + result = IUnknown_QueryInterface(unknown, &IID_IMemoryData, (void **)&memory_data); > + ok(E_NOINTERFACE == result, "got 0x%08x\n", result); > + if (S_OK == result) > + { > + IMemoryData_Release(memory_data); > + } If it always fails, there's no reason add conditional Release. > + > + result = IUnknown_QueryInterface(unknown, &IID_IAudioData, (void **)&audio_data); > + ok(S_OK == result, "got 0x%08x\n", result); > + if (S_OK == result) > + { > + IAudioData_Release(audio_data); > + } > + > + IUnknown_Release(unknown); > +} Could you check if you can query IMemoryData from IAudioData? It seems really weird if parent interface is not supported at all.

July 27, 2016