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

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 14.03.2017 16:20, Sebastian Lackner wrote: > On 13.03.2017 13:31, Jacek Caban wrote: >> I drafted an optimization discussed in this thread. It's just a >> proof-of-concept. A better implementation will store user APCs on server >> side when the async is created and I'd consider moving handling of >> returned data and wait handle out of fd-specific ops. I ran it on my >> benchmark from last year [1] and it was 2 times slower than what I >> called "plain wine+msgmode patchset+optimizations". It's expected, since >> with that we need two server calls (that one needed one). Going down to >> one server round-trip for simple cases is easy with that and should give >> about equal results. >> >> >> The point is, named pipe code needs just small changes for such >> optimization, no refactoring. I hope that addresses your concerns. >> >> >> Jacek >> >> >> [1] https://www.winehq.org/pipermail/wine-devel/2016-October/115071.html >> >> > Thanks for sharing the draft patch. All of the ideas you mentioned > sound reasonable, but if you already know how to solve these problems, > I am not sure why you insist on merging your named pipe implementation > first. The goal should be to keep the risk of regressions (this also > includes performance regressions - or weird issues because USR1 > signals arrive on other threads, possibly interrupting libraries > which cannot deal with it) as small as possible. Imho it would be > preferred to fix the limitations first, then adding the named pipe > implementation. I guess I look at this from different perspective. Adding different way of returning data before named pipes would introduce a dead and untested code, which would be enabled by the patch changing named pipes. I tried to construct the whole series in a way that would make the single most dangerous patch not only as small in size as possible (and it's still pretty long), but also reduce its impact. That's a way to actually reduce regression risk, in my opinion. This way: - in case of regression in new transport code, bisect will show the fault patch instead of named pipe commit (that just enables previously dead code) - the first version of named pipes changes uses single, simpler and already tested way of transport, ensuring that this works well - APCs, which are needed for async results anyway, are better tested - once we start improving performance, the new code will be immediately covered by both Wine tests and user testing The way I see it, a temporary performance degradation is a cost of better incremental changes that should reduce regression risk. > So far I'm also not sure yet if this separate async_complete > wineserver call really is the best approach. What about just using the > existing wait mechanism? Either read/write immediately starts a wait > and the ntdll side would look similar to server_select(). That's interesting, but note that we can't finish the async on any wait from given thread, because a signal may happen before the data is actually read. But it should be doable. > Alternatively it would also be sufficient to block USR1 signals until > ntdll has reached NtWaitForSingleObject. That alone would be slower, it would need 3 server calls. But it's a nice idea to do that for calls ending with wait anyway. Thanks, Jacek

March 15, 2017

Re: [4/4] kernel32: Add localized version resource.

by Alexandre Julliard

Akihiro Sagawa <sagawa.aki(a)gmail.com> writes: > diff --git a/dlls/kernel32/version.rc b/dlls/kernel32/version.rc > index 411bd6e..dbaee67 100644 > --- a/dlls/kernel32/version.rc > +++ b/dlls/kernel32/version.rc > @@ -16,6 +16,13 @@ > * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA > */ > > +#include "winresrc.h" > + > +#pragma makedep po > + > +LANGUAGE LANG_ENGLISH, SUBLANG_DEFAULT > + > +#define WINE_FILEDESCRIPTION_STR "#msgctxt#kernel32#Wine Core DLL" > #define WINE_FILENAME_STR "kernel32.dll" Is there a reason that this needs to be kernel32 specific? Why couldn't other dlls use the translation too? -- Alexandre Julliard julliard(a)winehq.org

March 15, 2017

Re: Outreachy applicant

by Stefan Dösinger

Hi Prerna, Thank you for your interest in Wine! I assume you are familiar with https://wiki.winehq.org/Summer_Of_Code <https://wiki.winehq.org/Summer_Of_Code> . We will run Outreachy very similarly to GSoC, and the same preconditions apply. Please be aware of the requirement to submit a patch to Wine before the end of the application deadline. For the Winetest interface I recommend to build Wine and run the tests yourself, and have a look at the existing winetest program and the UI at test.winehq.org <http://test.winehq.org/>. Best regards, Stefan > Am 13.03.2017 um 16:21 schrieb Prerna Garg <Prerna.Garg(a)live.com>: > > Hi, > > I would like to participate in the current round of Outreachy. I am new to the world of open source. I am interested to contribute in the project Winetest Graphical User Interface. Can somebody please guide me as to how I should go about it. > > Regards, > Prerna Garg > B. Tech. Computer Science > NIT Hamirpur > India

March 14, 2017

Re: appwiz.cpl: Use sha256 checksums to validate Mono/Gecko downloads.

by Alexandre Julliard

Sebastian Lackner <sebastian(a)fds-team.de> writes: > On 14.03.2017 20:29, Alexandre Julliard wrote: >> Sebastian Lackner <sebastian(a)fds-team.de> writes: >> >>> This patch also replaces the memory mapping logic with ReadFile. >> >> That's noticeably slower, any particular reason for doing this? >> > > I did the change for security reasons. If the file is truncated by > another process while we are computing the checksum it could lead > to a SIGBUS. Nevertheless, I wouldn't really expect that it is much > slower. How big is the difference in your case? If it is a problem > we could increase the buffer size and/or switch to sha512, which > is a bit faster afaik. The tiny buffer probably doesn't help, but it's about 10% slower (650ms vs. 600ms). I have to admit I don't see a security issue here. If another process is modifying the file while we are loading it, then I don't think a SIGBUS is necessarily worse than a corrupted install. -- Alexandre Julliard julliard(a)winehq.org

March 14, 2017

Re: appwiz.cpl: Use sha256 checksums to validate Mono/Gecko downloads.

by Sebastian Lackner

On 14.03.2017 20:29, Alexandre Julliard wrote: > Sebastian Lackner <sebastian(a)fds-team.de> writes: > >> This patch also replaces the memory mapping logic with ReadFile. > > That's noticeably slower, any particular reason for doing this? > I did the change for security reasons. If the file is truncated by another process while we are computing the checksum it could lead to a SIGBUS. Nevertheless, I wouldn't really expect that it is much slower. How big is the difference in your case? If it is a problem we could increase the buffer size and/or switch to sha512, which is a bit faster afaik. Best regards, Sebastian

March 14, 2017

Re: appwiz.cpl: Use sha256 checksums to validate Mono/Gecko downloads.

by Alexandre Julliard

Sebastian Lackner <sebastian(a)fds-team.de> writes: > This patch also replaces the memory mapping logic with ReadFile. That's noticeably slower, any particular reason for doing this? -- Alexandre Julliard julliard(a)winehq.org

March 14, 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: > Thanks for sharing the draft patch. All of the ideas you mentioned > sound reasonable, but if you already know how to solve these problems, > I am not sure why you insist on merging your named pipe implementation > first. The goal should be to keep the risk of regressions (this also > includes performance regressions - or weird issues because USR1 > signals arrive on other threads, possibly interrupting libraries > which cannot deal with it) as small as possible. Imho it would be > preferred to fix the limitations first, then adding the named pipe > implementation. > > So far I'm also not sure yet if this separate async_complete > wineserver call really is the best approach. What about just using the > existing wait mechanism? Either read/write immediately starts a wait > and the ntdll side would look similar to server_select(). > Alternatively it would also be sufficient to block USR1 signals until > ntdll has reached NtWaitForSingleObject. If you decide to add such a > flag, it could probably also be reused to fix another bug. > > [@ Alexandre: Do you think those concerns are valid, or do you think > the patches would be acceptable despite the problems?] I think we can do it either way. I'm not necessarily opposed to merging the named pipe support first and improving the APC mechanism afterwards. I would like to see smaller patches though... -- Alexandre Julliard julliard(a)winehq.org

March 14, 2017

Re: [2/2] include: Introduce a with_context macro and use it to simplify vcomp tests. [RFC]

by Bruno Jesus

On Tue, Mar 14, 2017 at 12:27 PM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: >> On 7 March 2017 at 18:29, Sebastian Lackner <sebastian(a)fds-team.de> wrote: >>> It can also be combined with a loop like: >>> >>> for (i = 0; i < 100; i++) >>> with_context("i = %d", i) >>> { >>> ... >>> } >>> >> But e.g. "break" would be problematic inside such a loop, right? >> > > Yes, break would be problematic, but continue for example would work > fine. As a workaround you could add a failing ok call before leaving the test explaining about the wrong usage of the macro (the context %s is still open, please fix). So it will never be commited upstream with incorrect usage. Too bad?

March 14, 2017

Re: [2/2] include: Introduce a with_context macro and use it to simplify vcomp tests. [RFC]

by Sebastian Lackner

On 14.03.2017 11:45, Henri Verbeet wrote: > On 14 March 2017 at 01:56, Bruno Jesus <00cpxxx(a)gmail.com> wrote: >> Hi, Sebastian. I like this idea, it would be specially useful for >> table struct tests (eg ws2_32) and functions that are called more than >> once for different tests (eg rsaenh). Each maintainer/helper could >> slowly update the tests as they think it fits and new tests could >> start benefiting from it. I hope more people join this conversation as >> it clearly took a long time to produce the patch and letting it drop >> from the list without talk would not be nice ;-) >> > Well, for what it's worth, I'd welcome the functionality, although > personally I'd be tempted to go for > winetest_push_context()/winetest_pop_context() and avoid the extra > indentation. Thanks for the feedback. I will send a modified version without the macro if you think it is preferred. > > On 7 March 2017 at 18:29, Sebastian Lackner <sebastian(a)fds-team.de> wrote: >> It can also be combined with a loop like: >> >> for (i = 0; i < 100; i++) >> with_context("i = %d", i) >> { >> ... >> } >> > But e.g. "break" would be problematic inside such a loop, right? > Yes, break would be problematic, but continue for example would work fine. Best regards, Sebastian

March 14, 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 13.03.2017 13:31, Jacek Caban wrote: > I drafted an optimization discussed in this thread. It's just a > proof-of-concept. A better implementation will store user APCs on server > side when the async is created and I'd consider moving handling of > returned data and wait handle out of fd-specific ops. I ran it on my > benchmark from last year [1] and it was 2 times slower than what I > called "plain wine+msgmode patchset+optimizations". It's expected, since > with that we need two server calls (that one needed one). Going down to > one server round-trip for simple cases is easy with that and should give > about equal results. > > > The point is, named pipe code needs just small changes for such > optimization, no refactoring. I hope that addresses your concerns. > > > Jacek > > > [1] https://www.winehq.org/pipermail/wine-devel/2016-October/115071.html > > Thanks for sharing the draft patch. All of the ideas you mentioned sound reasonable, but if you already know how to solve these problems, I am not sure why you insist on merging your named pipe implementation first. The goal should be to keep the risk of regressions (this also includes performance regressions - or weird issues because USR1 signals arrive on other threads, possibly interrupting libraries which cannot deal with it) as small as possible. Imho it would be preferred to fix the limitations first, then adding the named pipe implementation. So far I'm also not sure yet if this separate async_complete wineserver call really is the best approach. What about just using the existing wait mechanism? Either read/write immediately starts a wait and the ntdll side would look similar to server_select(). Alternatively it would also be sufficient to block USR1 signals until ntdll has reached NtWaitForSingleObject. If you decide to add such a flag, it could probably also be reused to fix another bug. [@ Alexandre: Do you think those concerns are valid, or do you think the patches would be acceptable despite the problems?] Best regards, Sebastian

March 14, 2017

Re: [PATCH 8/8] d3d9/tests: Add test for vertex textures.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=29507 Your paranoid android. === w8 (32 bit visual) === visual.c:8099: Test failed: Got unexpected color 0x00ff00ff for no color attribute test. === w864 (64 bit visual) === visual.c:8099: Test failed: Got unexpected color 0x00ff00ff for no color attribute test.

March 14, 2017

Re: [PATCH 3/3] msvcrt100/tests: Add tests for concurrency::event

by Piotr Caban

Hi, On 03/13/17 21:53, Daniel Lehman wrote: > +static void test_event(void) > +{ > + int i; > + int ret; > + int left; > + int wait; > + event evt; > + event *evts[70]; > + HANDLE thread; > + HANDLE threads[NUMELMS(evts)]; > + > + call_func1(p_event_ctor, &evt); > + > + ret = call_func2(p_event_wait, &evt, 100); > + ok(ret == -1, "expected -1, got %d\n", ret); > + > + call_func1(p_event_set, &evt); > + ret = call_func2(p_event_wait, &evt, 100); > + ok(!ret, "expected 0, got %d\n", ret); Could you please add following test here (to show that event is not reset after wait): ret = call_func2(p_event_wait, &evt, 100); ok(!ret, "expected 0, got %d\n", ret); > + i = 0; > + left = NUMELMS(evts); > + while (left) { > + wait = min(left, MAXIMUM_WAIT_OBJECTS); > + WaitForMultipleObjects(wait, &threads[i], TRUE, 5000); > + left -= wait; > + i += wait; > + } > + > + for (i = 0; i < NUMELMS(evts); i++) > + CloseHandle(threads[i]); I think it's cleaner if you change the while and for loop to something like: for (i = 0; i < NUMELMS(threads); i++) { WaitForSingleObject(threads[i], INFINITE); CloseHandle(threads[i]); } > + /* reset and test wait for multiple with any */ > + for (i = 0; i < NUMELMS(evts); i++) > + call_func1(p_event_reset, evts[i]); > + > + for (i = 0; i < NUMELMS(evts); i++) > + threads[i] = CreateThread(NULL, 0, test_event_thread, (void*)evts[i], 0, NULL); > + > + ret = p_event_wait_for_multiple(evts, NUMELMS(evts), FALSE, 5000); > + ok(ret != -1, "didn't expect -1\n"); Could you please start only one thread in this case and check exact return value from p_event_wait_for_multiple. Could you please also add following test: static DWORD WINAPI multiple_events_thread(void *arg) { event **events = arg; Sleep(50); call_func1(p_event_set, events[0]); call_func1(p_event_reset, events[0]); call_func1(p_event_set, events[1]); call_func1(p_event_reset, events[1]); return 0; } call_func1(p_event_reset, evts[0]); call_func1(p_event_reset, evts[1]); thread = CreateThread(NULL, 0, multiple_events_thread, (void*)evts, 0, NULL); ret = p_event_wait_for_multiple(evts, 2, TRUE, 500); ok(ret == -1, "expect -1, got %d\n", ret); WaitForSingleObject(thread, INFINITE); CloseHandle(thread); Thanks, Piotr

March 14, 2017

Re: 1.8.7 maintenance release?

by Michael Stefaniuc

Hello Jens, On 03/13/2017 05:01 PM, Jens Reyer wrote: > it's long overdue: thanks for your work on stable releases! > > > On 03/03/2017 10:56 AM, Michael Stefaniuc wrote: >> On 03/02/2017 07:50 PM, Nathan Schulte wrote: >> - Current supported distribution versions won't upgrade from 1.8.x to >> 2.0. They will (hopefully) do the switch in their next version. > > Indeed Wine 2.0 was deemed as too late for the main Wine packages in > Debian Stretch (currently in freeze). So I'm trying to get 1.8.7 in > there, for now I uploaded it to experimental. > > If this works, I'd also try to get Wine stable updates in Debian stable > updates in the future. However this would first happen with maybe Wine > 4.0.x for Debian Buster (2019). > > >> - Ubuntu 16.10 LTS came out with Wine 1.6.2 even though 1.8.3 was out >> already. So I wouldn't wonder if they'll use 1.8.7 in their next LTS... >> I don't particularly care about Ubuntu but that's how most of our new >> users make their first contact with Wine. > > LTS was 16.04, not 16.10. AIUI 16.04 was released with 1.6.2 because I was already thinking ahead that 16.10 moved to the 1.8.x branch while my fingers were still typing above sentence. > the old Ubuntu maintainer(s) stopped working on Wine. > > Then several people started to work on bringing the Debian packages to > Ubuntu. I'm working closely with them, and since 16.10 we succeeded in > having the up to date Debian packages in Ubuntu. Work is also going on > to get older versions updated, but progress is slow there. > > 17.04 will most probably use 1.8.7 (currently 1.8.6), even if they have > to pick it from Debian experimental, not Debian testing/unstable. > Eventually we might even get 2.0 there, but with Debian being in freeze > that's not trivial - so thanks again for 1.8.7. You're welcome, >> - It is very easy for me to do an old stable release after the new >> stable was cut. With the code freeze prior to a new stable release only >> small bug fixes and most importantly regression fixes go in. So there is >> a high density of good commits to cherry pick for the old stable. > > This makes me a lot more comfortable in pushing 1.8.7 now :) > > Currently the bug list for target_milestone=1.8.x is empty.[1] Would Correct. I removed all the 1.8.x milestones as those were all bugs that I kept deferring from one release to another. For various reasons: too complex, introduced a regression, too risky, etc. > you tag any regression in that way (even for the now "finished" 1.8.x > series), or what's the best way for me to learn about regressions in the > stable releases? I don't mind if people tag regressions that are specific to 1.8.7 with the 1.8.x milestone. But a search like this could work too: https://bugs.winehq.org/buglist.cgi?bug_status=UNCONFIRMED&bug_status=NEW&b… If there is interest from other distributions too I can keep an 1.8.x branch on my stable git tree at https://github.com/mstefani/wine-stable as a synchronization point with patches that various distributions add on top of 1.8.7. But if it is just Debian and Ubuntu, I guess the Debian wine-1.8.7.deb will be good enough. > Do you plan to roughly stick with the bi-monthly release schedule? So > I'd roughly assume a 2.0.1 in April? Heh, I kinda initially planned for a 4-6 weeks cadence for 1.8.x but due to time constraints it ended up being a bi-monthly schedule. As that seems to have worked quite well I'll stick to a 6-10 weeks release schedule. Lower bound if I have enough (~50) interesting (aka not stubs, GPU card additions, documentation, small translation) patches. The upper bound is a timeout to release whatever I have at that point. bye michael

March 14, 2017

Re: [PATCH 1/3] msvcrt: Add Concurrency::event stubs

by Piotr Caban

Hi Daniel, On 03/13/17 21:53, Daniel Lehman wrote: > include/concrt.h | 25 ++++++++++++++++ The c-runtimes related headers are defined in include/msvcrt/*. In this case I think that it will be better to define needed constants in dlls/msvcrt/msvcrt.h because the concrt.h header is not useful. Thanks, Piotr

March 14, 2017

Re: [2/2] include: Introduce a with_context macro and use it to simplify vcomp tests. [RFC]

by Henri Verbeet

On 14 March 2017 at 01:56, Bruno Jesus <00cpxxx(a)gmail.com> wrote: > Hi, Sebastian. I like this idea, it would be specially useful for > table struct tests (eg ws2_32) and functions that are called more than > once for different tests (eg rsaenh). Each maintainer/helper could > slowly update the tests as they think it fits and new tests could > start benefiting from it. I hope more people join this conversation as > it clearly took a long time to produce the patch and letting it drop > from the list without talk would not be nice ;-) > Well, for what it's worth, I'd welcome the functionality, although personally I'd be tempted to go for winetest_push_context()/winetest_pop_context() and avoid the extra indentation. On 7 March 2017 at 18:29, Sebastian Lackner <sebastian(a)fds-team.de> wrote: > It can also be combined with a loop like: > > for (i = 0; i < 100; i++) > with_context("i = %d", i) > { > ... > } > But e.g. "break" would be problematic inside such a loop, right?

March 14, 2017

Re: [PATCH] user32: Removed 16-bit windows position and size constraints

by Alexandre Julliard

Kees Beets <kbeets68(a)gmail.com> writes: > Fixes https://bugs.winehq.org/show_bug.cgi?id=38231 > > Removed 16-bit windows position and size constraints > introduced in 2006. Only kept truncation to 16-bit > in windows messaging to keep same behaviour as windows. I'd find it surprising that the messages wouldn't match the actual window sizes. This would need more test cases. In fact we already had some window size tests, but they were commented out. I've re-enabled them now, so if you try your patch against latest git you'll see new failures, indicating that fixing up the messages is not correct. -- Alexandre Julliard julliard(a)winehq.org

March 14, 2017

Oureachy Applicant Introduction

by chhavi gupta

Hello! I am Chhavi Gupta, a final year Computer Science Student with an interest in applying for the next round of Outreachy with Wine as some of the ideas have really caught my attention. I am familiar with C and have taken a course in Computer Graphics during my third year in college. Github Profile: https://github.com/chhavip I am interested in the following projects: - Winetest Graphical User Interface - Winecfg / winemenubuilder - enhance MIME type handling - WineCE: Implement the CommandBar I am quite new to Windows programming hence any guidance on how to get started will be extremely helpful! Thank You Chhavi Gupta

March 14, 2017

RE: [PATCH 2/3] msvcrt: Implement Concurrency::event

by Daniel Lehman

> Nevertheless, after more careful reviewing in your patch, it looks like handles are never really exposed to the application, is that correct? correct > I'm not really sure why you need a separate thread then. Why does event_set for example not wake up the the pending waits directly? You have to be careful when locking multiple critical sections, but I do not see any reason why it shouldn't be possible. i'll send one without

March 13, 2017

Re: [2/2] include: Introduce a with_context macro and use it to simplify vcomp tests. [RFC]

by Bruno Jesus

On Tue, Mar 7, 2017 at 2:29 PM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: > Based on ideas by Mark Jansen. > ... > > The with_context macro allows to specify a format string which is printed in > case of test failures and for trace messages. Currently, to track down the > origin of a test failure, it is necessary to write down all relevant variables > in each ok message. With the new macro it is much easier: > ... Hi, Sebastian. I like this idea, it would be specially useful for table struct tests (eg ws2_32) and functions that are called more than once for different tests (eg rsaenh). Each maintainer/helper could slowly update the tests as they think it fits and new tests could start benefiting from it. I hope more people join this conversation as it clearly took a long time to produce the patch and letting it drop from the list without talk would not be nice ;-)

March 13, 2017

Re: [PATCH 3/3] d3dx9: Apply changed states only in CommitChanges.

by Paul Gofman

On 03/14/2017 12:47 AM, Matteo Bruni wrote: > 2017-03-09 23:15 GMT+01:00 Paul Gofman <gofmanp(a)gmail.com>: >> Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> >> --- >> >> >> struct d3dx_state *states; >> struct d3dx_parameter *annotations; >> + >> + D3DLIGHT9 current_light[8]; >> + D3DMATERIAL9 current_material; >> }; > I think these might be stored in the effect struct instead. Yes actually, too passes can't be active at a time and each BeginPass should initialize this in effect structure correctly. >> +static void clear_dirty_all(struct d3dx9_base_effect *base) > clear_dirty_params() or something like that would be a clearer name. I will change the name. >> @@ -1570,8 +1587,12 @@ static HRESULT d3dx9_base_effect_set_int(struct d3dx9_base_effect *base, D3DXHAN >> { >> if (param->rows == 1 && param->columns == 1) >> { >> - set_number(param->data, param->type, &n, D3DXPT_INT); >> - set_dirty(param); >> + DWORD value; >> + >> + set_number(&value, param->type, &n, D3DXPT_INT); >> + if (value != *(DWORD *)param->data) >> + set_dirty(param); >> + *(DWORD *)param->data = value; > Shouldn't this go in the previous patch? > Yes, sure. There is actually another fix I made in the next patches which makes SetLight go in one call for all light attributes, probably I should bring it here at once too? Just the test for that goes later in my patches when I test & implement effect state manager.

March 13, 2017

Re: [PATCH 2/3] d3dx9: Set dirty flag on parameter change.

by Paul Gofman

Yes, arrays and structs are always considered completely dirty, it is in my test (for both shader constants and states). Setting dirty each element here is actually a leftover from early implementation when I did not know that, I will fix it (just put it off the loop). On 03/14/2017 12:42 AM, Matteo Bruni wrote: > 2017-03-09 23:15 GMT+01:00 Paul Gofman <gofmanp(a)gmail.com>: > >> @@ -2021,9 +2055,11 @@ static HRESULT d3dx9_base_effect_set_matrix_array(struct d3dx9_base_effect *base >> switch (param->class) >> { >> case D3DXPC_MATRIX_ROWS: >> + set_dirty(param); >> for (i = 0; i < count; ++i) >> { >> set_matrix(&param->members[i], &matrix[i]); >> + set_dirty(&param->members[i]); >> } >> return D3D_OK; >> > Here and below it should be possible to avoid marking dirty each array > element individually. IIRC I had some test showing that arrays are > always considered completely dirty or clean on native i.e. > CommitChanges updates the whole array even if you only change a subset > of the elements. >

March 13, 2017

Re: [PATCH 1/3] d3dx9/tests: Add test for CommitChanges in effect.

by Paul Gofman

Thanks, sure, I will do that. On 03/14/2017 12:36 AM, Matteo Bruni wrote: > 2017-03-09 23:15 GMT+01:00 Paul Gofman <gofmanp(a)gmail.com>: >> Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> >> --- >> dlls/d3dx9_36/tests/effect.c | 1267 +++++++++++++++++++++++++++++++----------- >> 1 file changed, 930 insertions(+), 337 deletions(-) > First of all, please split up this patch, this is way too large and > does multiple things at once (I see that it extends the test effect > and checks the new states, factors out two helper functions from > test_effect_preshader() and it also adds the CommitChanges test in the > subject). Those are all good changes but when they are squeezed in one > patch it becomes hard to review. > > I haven't looked in too much detail but anyway there are a few > additional comments below. > >> +#define TEST_EFFECT_PRES_NOPTESTS ARRAY_SIZE(test_effect_preshader_op_results) > I know this isn't new but I don't think this define is very useful. If > you really want to keep it, please name it xxx_TEST_COUNT. > >> + else if (!state_updated[i]) >> + { >> + todo_wine_if(memcmp(v, empty_v, sizeof(float) * 4)) >> + ok_(__FILE__, line)(!memcmp(v, empty_v, sizeof(float) * 4), >> + "Parameter %s, test %d, operation %s, state updated unexpectedly.\n", >> + updated_param, i, test_effect_preshader_op_results[i].comment); > Eh, this isn't really all that nice. I don't see an obvious way to > make this any better though so it probably makes sense to move this > part (or even all the tests) to the end of the patch series and avoid > the problem altogether. > > BTW, if you define empty_v as an array you can use sizeof(empty_v) here. > >> + } >> + } >> +} >> + >> +static const D3DXVECTOR4 fvect_empty = {-9999.0f, -9999.0f, -9999.0f, -9999.0f}; >> + >> +static void test_effect_preshader_clear_vconsts(IDirect3DDevice9 *device) >> +{ >> + int i; > unsigned int i; > >> + static const struct >> + { >> + const char *param_name; >> + BOOL const_updated[TEST_EFFECT_PRES_NFLOATV]; >> + } >> + check_vconsts_parameters[] = >> + { >> + {"g_Selector", >> + {FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, >> + FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, >> + FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, >> + FALSE, FALSE, FALSE, TRUE}}, > It would be nicer to represent these as 32-bit hexadecimal bitmasks. > >> + for (i = 0; i < sizeof(check_op_parameters) / sizeof(*check_op_parameters); ++i) >> + { >> + int j; > unsigned int j; > Also please leave a blank line between declarations and code. > >> + for (j = 0; j < 8; j++) > Consistency, please stick to the '++j' form. >

March 13, 2017

Re: [PATCH 3/7] d3d9/tests: Fix test_updatetexture() on GPUs not supporting WINED3DFMT_R32_FLOAT.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=29498 Your paranoid android. === w1064 (32 bit visual) === visual.c:7995: Test failed: Got unexpected color 0x00008080 for quad 2 (different colors). visual.c:7995: Test failed: Got unexpected color 0x00007580 for quad 2 (different colors).

March 13, 2017

Re: [PATCH 1/7] d3d9/tests: Ignore test_vshader_input() "no color attribute" test result on WARP.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=29497 Your paranoid android. === w1064 (32 bit visual) === visual.c:7995: Test failed: Got unexpected color 0x00008080 for quad 2 (different colors). === w1064 (64 bit visual) === visual.c:7995: Test failed: Got unexpected color 0x00007580 for quad 2 (different colors).

March 13, 2017

Re: [PATCH 2/3] msvcrt: Implement Concurrency::event

by Sebastian Lackner

On 13.03.2017 22:33, Daniel Lehman wrote: >> Not sure if its suitable here, but please note that Wines threadpool implementation also supports waiting on more than 64 objects (using similar tricks). If possible, it would be better to avoid the second implementation. > > Not sure what you mean. Are you talking about timerqueue_thread_proc? > I'm talking about CreateThreadpoolWait/SetThreadpoolWait or the corresponding ntdll functions TpAllocWait/TpSetWait. Each wait handle would have to be added separately, but there is no limitation for the number of handles. Also see dlls/ntdll/tests/threadpool.c:test_tp_multi_wait for an example how it can be used. Nevertheless, after more careful reviewing in your patch, it looks like handles are never really exposed to the application, is that correct? I'm not really sure why you need a separate thread then. Why does event_set for example not wake up the the pending waits directly? You have to be careful when locking multiple critical sections, but I do not see any reason why it shouldn't be possible. Best regards, Sebastian

March 13, 2017

Re: [PATCH 3/3] d3dx9: Apply changed states only in CommitChanges.

by Matteo Bruni

2017-03-09 23:15 GMT+01:00 Paul Gofman <gofmanp(a)gmail.com>: > Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> > --- > dlls/d3dx9_36/d3dx9_private.h | 8 ++- > dlls/d3dx9_36/effect.c | 146 ++++++++++++++++++++++++++++-------------- > dlls/d3dx9_36/preshader.c | 40 ++++++++++-- > dlls/d3dx9_36/tests/effect.c | 13 ---- > 4 files changed, 137 insertions(+), 70 deletions(-) > > diff --git a/dlls/d3dx9_36/d3dx9_private.h b/dlls/d3dx9_36/d3dx9_private.h > index 27b91a7..53cf99f 100644 > --- a/dlls/d3dx9_36/d3dx9_private.h > +++ b/dlls/d3dx9_36/d3dx9_private.h > @@ -222,6 +222,11 @@ struct d3dx_parameter > DWORD *dirty_flag_ptr; > }; > > +static inline BOOL is_param_dirty(struct d3dx_parameter *param) > +{ > + return (*param->dirty_flag_ptr & PARAMETER_FLAG_DIRTY); > +} > + > struct d3dx9_base_effect; > > struct d3dx_parameter *get_parameter_by_name(struct d3dx9_base_effect *base, > @@ -233,6 +238,7 @@ void d3dx_free_param_eval(struct d3dx_param_eval *peval) DECLSPEC_HIDDEN; > HRESULT d3dx_evaluate_parameter(struct d3dx_param_eval *peval, > const struct d3dx_parameter *param, void *param_value) DECLSPEC_HIDDEN; > HRESULT d3dx_param_eval_set_shader_constants(struct IDirect3DDevice9 *device, > - struct d3dx_param_eval *peval) DECLSPEC_HIDDEN; > + struct d3dx_param_eval *peval, BOOL update_all) DECLSPEC_HIDDEN; > +BOOL is_param_eval_input_dirty(struct d3dx_param_eval *peval) DECLSPEC_HIDDEN; > > #endif /* __WINE_D3DX9_PRIVATE_H */ > diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c > index ada78f9..b18465c 100644 > --- a/dlls/d3dx9_36/effect.c > +++ b/dlls/d3dx9_36/effect.c > @@ -123,6 +123,9 @@ struct d3dx_pass > > struct d3dx_state *states; > struct d3dx_parameter *annotations; > + > + D3DLIGHT9 current_light[8]; > + D3DMATERIAL9 current_material; > }; I think these might be stored in the effect struct instead. > +static void clear_dirty_all(struct d3dx9_base_effect *base) clear_dirty_params() or something like that would be a clearer name. > @@ -1570,8 +1587,12 @@ static HRESULT d3dx9_base_effect_set_int(struct d3dx9_base_effect *base, D3DXHAN > { > if (param->rows == 1 && param->columns == 1) > { > - set_number(param->data, param->type, &n, D3DXPT_INT); > - set_dirty(param); > + DWORD value; > + > + set_number(&value, param->type, &n, D3DXPT_INT); > + if (value != *(DWORD *)param->data) > + set_dirty(param); > + *(DWORD *)param->data = value; Shouldn't this go in the previous patch?

March 13, 2017

Re: [PATCH 2/3] d3dx9: Set dirty flag on parameter change.

by Matteo Bruni

2017-03-09 23:15 GMT+01:00 Paul Gofman <gofmanp(a)gmail.com>: > @@ -2021,9 +2055,11 @@ static HRESULT d3dx9_base_effect_set_matrix_array(struct d3dx9_base_effect *base > switch (param->class) > { > case D3DXPC_MATRIX_ROWS: > + set_dirty(param); > for (i = 0; i < count; ++i) > { > set_matrix(&param->members[i], &matrix[i]); > + set_dirty(&param->members[i]); > } > return D3D_OK; > Here and below it should be possible to avoid marking dirty each array element individually. IIRC I had some test showing that arrays are always considered completely dirty or clean on native i.e. CommitChanges updates the whole array even if you only change a subset of the elements.

March 13, 2017

Re: [PATCH 1/3] d3dx9/tests: Add test for CommitChanges in effect.

by Matteo Bruni

2017-03-09 23:15 GMT+01:00 Paul Gofman <gofmanp(a)gmail.com>: > Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> > --- > dlls/d3dx9_36/tests/effect.c | 1267 +++++++++++++++++++++++++++++++----------- > 1 file changed, 930 insertions(+), 337 deletions(-) First of all, please split up this patch, this is way too large and does multiple things at once (I see that it extends the test effect and checks the new states, factors out two helper functions from test_effect_preshader() and it also adds the CommitChanges test in the subject). Those are all good changes but when they are squeezed in one patch it becomes hard to review. I haven't looked in too much detail but anyway there are a few additional comments below. > +#define TEST_EFFECT_PRES_NOPTESTS ARRAY_SIZE(test_effect_preshader_op_results) I know this isn't new but I don't think this define is very useful. If you really want to keep it, please name it xxx_TEST_COUNT. > + else if (!state_updated[i]) > + { > + todo_wine_if(memcmp(v, empty_v, sizeof(float) * 4)) > + ok_(__FILE__, line)(!memcmp(v, empty_v, sizeof(float) * 4), > + "Parameter %s, test %d, operation %s, state updated unexpectedly.\n", > + updated_param, i, test_effect_preshader_op_results[i].comment); Eh, this isn't really all that nice. I don't see an obvious way to make this any better though so it probably makes sense to move this part (or even all the tests) to the end of the patch series and avoid the problem altogether. BTW, if you define empty_v as an array you can use sizeof(empty_v) here. > + } > + } > +} > + > +static const D3DXVECTOR4 fvect_empty = {-9999.0f, -9999.0f, -9999.0f, -9999.0f}; > + > +static void test_effect_preshader_clear_vconsts(IDirect3DDevice9 *device) > +{ > + int i; unsigned int i; > + static const struct > + { > + const char *param_name; > + BOOL const_updated[TEST_EFFECT_PRES_NFLOATV]; > + } > + check_vconsts_parameters[] = > + { > + {"g_Selector", > + {FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, > + FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, > + FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, > + FALSE, FALSE, FALSE, TRUE}}, It would be nicer to represent these as 32-bit hexadecimal bitmasks. > + for (i = 0; i < sizeof(check_op_parameters) / sizeof(*check_op_parameters); ++i) > + { > + int j; unsigned int j; Also please leave a blank line between declarations and code. > + for (j = 0; j < 8; j++) Consistency, please stick to the '++j' form.

March 13, 2017

RE: [PATCH 2/3] msvcrt: Implement Concurrency::event

by Daniel Lehman

> Not sure if its suitable here, but please note that Wines threadpool implementation also supports waiting on more than 64 objects (using similar tricks). If possible, it would be better to avoid the second implementation. Not sure what you mean. Are you talking about timerqueue_thread_proc?

March 13, 2017

Re: [v6 PATCH 00/21] x86: Enable User-Mode Instruction Prevention

by Stas Sergeev

11.03.2017 02:59, Ricardo Neri пишет: > On Fri, 2017-03-10 at 14:33 +0300, Stas Sergeev wrote: > >> Why would you need one? >> Or do you really want to allow these instructions >> in v86 by the means of emulation? If so - this wasn't >> clearly stated in the patch description, neither it was >> properly discussed, it seems. > It str and sldt can be emulated in vm86 but as Andy mention, the > behavior sould be the same with and without emulation. Why would you do that? I looked up the dosemu2 CPU simulator code that is used under x86-64. It says this: --- CODE_FLUSH(); if (REALMODE()) goto illegal_op; PC += ModRMSim(PC+1, mode) + 1; error("SLDT not implemented\n"); break; case 1: /* STR */ /* Store Task Register */ CODE_FLUSH(); if (REALMODE()) goto illegal_op; PC += ModRMSim(PC+1, mode) + 1; error("STR not implemented\n"); break; ... case 0: /* SGDT */ /* Store Global Descriptor Table Register */ PC++; PC += ModRM(opc, PC, mode|DATA16|MSTORE); error("SGDT not implemented\n"); break; case 1: /* SIDT */ /* Store Interrupt Descriptor Table Register */ PC++; PC += ModRM(opc, PC, mode|DATA16|MSTORE); error("SIDT not implemented\n"); break; --- It only implements smsw. So maybe you can make your code much simpler and remove the unneeded emulation? Same is for prot mode. You know the wine's requirements now - they are very small. And dosemu doesn't need anything at all but smsw. And even smsw is very rare.

March 13, 2017

Re: [PATCH 2/3] msvcrt: Implement Concurrency::event

by Sebastian Lackner

On 13.03.2017 21:53, Daniel Lehman wrote: > > From 44d603b75fe10d51377c20e4c3c2a1bf031c4fcf Mon Sep 17 00:00:00 2001 > From: Daniel Lehman <dlehman(a)esri.com> > Date: Mon, 12 Dec 2016 16:44:44 -0800 > Subject: [PATCH 2/3] msvcrt: Implement Concurrency::event > > wait_for_multiple can handle more than WaitForMultipleObjects' limit of 64 > a separate thread is used to handle threads that wait on multiple handles > and is loosely modeled after wineserver's select request handler > a very simple command structure is used to communicate with it over a single shared pipe > > Signed-off-by: Daniel Lehman <dlehman(a)esri.com> > --- > dlls/msvcrt/lock.c | 387 +++++++++++++++++++++++++++++++++++++++++++++++++-- > dlls/msvcrt/main.c | 3 + > dlls/msvcrt/msvcrt.h | 18 +++ > 3 files changed, 397 insertions(+), 11 deletions(-) Not sure if its suitable here, but please note that Wines threadpool implementation also supports waiting on more than 64 objects (using similar tricks). If possible, it would be better to avoid the second implementation.

March 13, 2017

Re: [PATCH] ddraw/tests: Rewrite SrcColorKey32BlitTest (v2).

by Stefan Dösinger

Am 2017-03-09 um 11:00 schrieb Henri Verbeet: > On 8 March 2017 at 20:29, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote: >> *) Accept the Nvidia XRGB color key results with X != 0. I am open to >> removing this test entirely as we're accepting a lot of different >> outcomes now. > Yeah, probably. It does seem consistent with the idea that the "X" > channel is undefined, and I don't think that particular test was > motivated by application behaviour. https://www.winehq.org/pipermail/wine-patches/2005-August/019938.html mentions the game Black Mirror, and it seems that the game requires that the X channel is ignored for color keying - this behavior makes sense, but one of our broken() results is different. It appears that the behavior I've seen on system surfaces on Windows would break the game. I originally thought it's possible that the game uses 16 bit surfaces and sets higher bits in the color key, but apparently this is not the case. I'll spend a few minutes to try to break the game's transparency on Windows, simply running it with the basic display driver might do the job. But I think since the game requires a certain behavior re the X channel we should keep the test. The other 3 behaviors (X doesn't influence color keying, but may be copied or set to 0x00 or 0xff) are all fine for the game.

March 13, 2017

Outreachy applicant

by Prerna Garg

Hi, I would like to participate in the current round of Outreachy. I am new to the world of open source. I am interested to contribute in the project Winetest Graphical User Interface. Can somebody please guide me as to how I should go about it. Regards, Prerna Garg B. Tech. Computer Science NIT Hamirpur India

March 13, 2017

Re: 1.8.7 maintenance release?

by Jens Reyer

Hi Michael, it's long overdue: thanks for your work on stable releases! On 03/03/2017 10:56 AM, Michael Stefaniuc wrote: > On 03/02/2017 07:50 PM, Nathan Schulte wrote: > - Current supported distribution versions won't upgrade from 1.8.x to > 2.0. They will (hopefully) do the switch in their next version. Indeed Wine 2.0 was deemed as too late for the main Wine packages in Debian Stretch (currently in freeze). So I'm trying to get 1.8.7 in there, for now I uploaded it to experimental. If this works, I'd also try to get Wine stable updates in Debian stable updates in the future. However this would first happen with maybe Wine 4.0.x for Debian Buster (2019). > - Ubuntu 16.10 LTS came out with Wine 1.6.2 even though 1.8.3 was out > already. So I wouldn't wonder if they'll use 1.8.7 in their next LTS... > I don't particularly care about Ubuntu but that's how most of our new > users make their first contact with Wine. LTS was 16.04, not 16.10. AIUI 16.04 was released with 1.6.2 because the old Ubuntu maintainer(s) stopped working on Wine. Then several people started to work on bringing the Debian packages to Ubuntu. I'm working closely with them, and since 16.10 we succeeded in having the up to date Debian packages in Ubuntu. Work is also going on to get older versions updated, but progress is slow there. 17.04 will most probably use 1.8.7 (currently 1.8.6), even if they have to pick it from Debian experimental, not Debian testing/unstable. Eventually we might even get 2.0 there, but with Debian being in freeze that's not trivial - so thanks again for 1.8.7. > - It is very easy for me to do an old stable release after the new > stable was cut. With the code freeze prior to a new stable release only > small bug fixes and most importantly regression fixes go in. So there is > a high density of good commits to cherry pick for the old stable. This makes me a lot more comfortable in pushing 1.8.7 now :) Currently the bug list for target_milestone=1.8.x is empty.[1] Would you tag any regression in that way (even for the now "finished" 1.8.x series), or what's the best way for me to learn about regressions in the stable releases? Do you plan to roughly stick with the bi-monthly release schedule? So I'd roughly assume a 2.0.1 in April? Greets! jre [1]: https://bugs.winehq.org/buglist.cgi?target_milestone=1.8.x

March 13, 2017

[PATCH] user32: Fix handling of internal messages.

by Zebediah Figura

I'm submitting this to wine-devel because the code is probably messy and I'm not sure if this is the right approach to solving the problem. This patch adds a new flag and a server reply, allowing GetQueueStatus to ignore internal messages, and MsgWaitForMultipleObjects to process internal messages until a real sent message is found. Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com> --- dlls/user32/message.c | 129 +++++++++++++++++++++++++++++++++++++++-- dlls/user32/tests/msg.c | 8 +-- include/wine/server_protocol.h | 4 +- include/winuser.h | 3 + server/protocol.def | 1 + server/queue.c | 14 ++++- server/request.h | 3 +- server/trace.c | 1 + 8 files changed, 150 insertions(+), 13 deletions(-) diff --git a/dlls/user32/message.c b/dlls/user32/message.c index 9bfb453..92caf64 100644 --- a/dlls/user32/message.c +++ b/dlls/user32/message.c @@ -2964,6 +2964,98 @@ static BOOL peek_message( MSG *msg, HWND hwnd, UINT first, UINT last, UINT flags /*********************************************************************** + * receive_internal_message + * + * Retrieves and processes a single sent internal message. + */ +static DWORD receive_internal_message(size_t buffer_size) +{ + LRESULT result; + struct user_thread_info *thread_info = get_user_thread_info(); + struct received_message_info info, *old_info; + void *buffer; + size_t size = 0; + + buffer = HeapAlloc(GetProcessHeap(), 0, buffer_size); + + SERVER_START_REQ( get_message ) + { + NTSTATUS res; + + req->flags = PM_REMOVE | PM_QS_SENDMESSAGE; + req->get_win = 0; + req->get_first = 0; + req->get_last = 0; + req->hw_id = 0; + req->wake_mask = 0; + req->changed_mask = 0; + wine_server_set_reply( req, buffer, buffer_size ); + + if ((res = wine_server_call( req )) != STATUS_SUCCESS) + { + HeapFree(GetProcessHeap(), 0, buffer); + if (res == STATUS_BUFFER_OVERFLOW) + return receive_internal_message(reply->total); + + return RtlNtStatusToDosError(res); + } + + size = wine_server_reply_size( reply ); + info.type = reply->type; + info.msg.hwnd = wine_server_ptr_handle( reply->win ); + info.msg.message = reply->msg; + info.msg.wParam = reply->wparam; + info.msg.lParam = reply->lparam; + info.msg.time = reply->time; + info.msg.pt.x = reply->x; + info.msg.pt.y = reply->y; + thread_info->active_hooks = reply->active_hooks; + } + SERVER_END_REQ; + + switch(info.type) + { + case MSG_ASCII: + case MSG_UNICODE: + info.flags = ISMEX_SEND; + break; + case MSG_NOTIFY: + info.flags = ISMEX_NOTIFY; + break; + case MSG_CALLBACK: + info.flags = ISMEX_CALLBACK; + break; + case MSG_HOOK_LL: + info.flags = ISMEX_SEND; + reply_message(&info, 0, TRUE); + return STATUS_SUCCESS; + case MSG_OTHER_PROCESS: + info.flags = ISMEX_SEND; + if (!unpack_message(info.msg.hwnd, info.msg.message, &info.msg.wParam, + &info.msg.lParam, &buffer, size)) + { + /* ignore it */ + reply_message(&info, 0, TRUE); + return ERROR_SUCCESS; + } + break; + default: + ERR("unexpected type %d for internal message %08x\n", info.type, info.msg.message); + } + + old_info = thread_info->receive_info; + thread_info->receive_info = &info; + result = handle_internal_message(info.msg.hwnd, info.msg.message, + info.msg.wParam, info.msg.lParam); + reply_message( &info, result, TRUE ); + thread_info->receive_info = old_info; + + HeapFree(GetProcessHeap(), 0, buffer); + return ERROR_SUCCESS; +} + + +/*********************************************************************** * process_sent_messages * * Process all pending sent messages. @@ -3811,7 +3903,7 @@ BOOL WINAPI DECLSPEC_HOTPATCH PeekMessageA( MSG *msg, HWND hwnd, UINT first, UIN BOOL WINAPI DECLSPEC_HOTPATCH GetMessageW( MSG *msg, HWND hwnd, UINT first, UINT last ) { HANDLE server_queue = get_server_queue_handle(); - unsigned int mask = QS_POSTMESSAGE | QS_SENDMESSAGE; /* Always selected */ + unsigned int mask = QS_POSTMESSAGE | QS_SENDMESSAGE | QS_WINE_SENDMESSAGE; /* Always selected */ USER_CheckNotLock(); check_for_driver_events( 0 ); @@ -3825,7 +3917,7 @@ BOOL WINAPI DECLSPEC_HOTPATCH GetMessageW( MSG *msg, HWND hwnd, UINT first, UINT if ((first <= WM_SYSTIMER) && (last >= WM_SYSTIMER)) mask |= QS_TIMER; if ((first <= WM_PAINT) && (last >= WM_PAINT)) mask |= QS_PAINT; } - else mask = QS_ALLINPUT; + else mask |= QS_ALLINPUT; while (!peek_message( msg, hwnd, first, last, PM_REMOVE | (mask << 16), mask )) { @@ -4142,7 +4234,7 @@ DWORD WINAPI MsgWaitForMultipleObjectsEx( DWORD count, const HANDLE *pHandles, DWORD timeout, DWORD mask, DWORD flags ) { HANDLE handles[MAXIMUM_WAIT_OBJECTS]; - DWORD i; + DWORD i, res; if (count > MAXIMUM_WAIT_OBJECTS-1) { @@ -4154,8 +4246,37 @@ DWORD WINAPI MsgWaitForMultipleObjectsEx( DWORD count, const HANDLE *pHandles, for (i = 0; i < count; i++) handles[i] = pHandles[i]; handles[count] = get_server_queue_handle(); - return wait_objects( count+1, handles, timeout, + mask |= QS_WINE_SENDMESSAGE; + + for (;;) + { + res = wait_objects( count+1, handles, timeout, (flags & MWMO_INPUTAVAILABLE) ? mask : 0, mask, flags ); + if (res == WAIT_OBJECT_0+count) + { + NTSTATUS server_res; + UINT msg; + SERVER_START_REQ( get_queue_status ) + { + req->clear_bits = 0; + if ((server_res = wine_server_call( req )) != STATUS_SUCCESS) + { + SetLastError(RtlNtStatusToDosError(server_res)); + return WAIT_FAILED; + } + msg = reply->first_sent_msg; + } + SERVER_END_REQ; + + if (msg & 0x80000000) + { + /* If it's a WINE internal message, process it and keep waiting */ + receive_internal_message(256); + continue; + } + } + return res; + } } diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index 19a1f31..a682b70 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -16332,7 +16332,7 @@ static void test_SendMessage_other_thread(int thread_n) GetMessageA(&msg, 0, 0, 0); ok(msg.message == WM_USER, "expected WM_USER, got %04x\n", msg.message); DispatchMessageA(&msg); - ok_sequence(send_message_1, "SendMessage from other thread 1", thread_n == 2); + ok_sequence(send_message_1, "SendMessage from other thread 1", FALSE); /* intentionally yield */ MsgWaitForMultipleObjects(0, NULL, FALSE, 100, qs_all_input); @@ -16347,7 +16347,7 @@ static void test_SendMessage_other_thread(int thread_n) trace("main: call PeekMessage\n"); ok(PeekMessageA(&msg, 0, 0, 0, PM_NOREMOVE), "PeekMessage should not fail\n"); ok(msg.message == WM_USER+1, "expected WM_USER+1, got %04x\n", msg.message); - ok_sequence(send_message_3, "SendMessage from other thread 3", thread_n == 2); + ok_sequence(send_message_3, "SendMessage from other thread 3", FALSE); trace("main: call PeekMessage\n"); ok(PeekMessageA(&msg, 0, 0, 0, PM_REMOVE), "PeekMessage should not fail\n"); @@ -16359,13 +16359,11 @@ static void test_SendMessage_other_thread(int thread_n) MsgWaitForMultipleObjects(0, NULL, FALSE, 100, qs_all_input); ret = GetQueueStatus(QS_SENDMESSAGE|QS_POSTMESSAGE); - /* FIXME: remove once Wine is fixed */ -todo_wine_if (thread_n == 2) ok(ret == 0, "wrong status %08x\n", ret); trace("main: call PeekMessage\n"); ok(!PeekMessageA(&msg, 0, 0, 0, PM_REMOVE), "PeekMessage should fail\n"); - ok_sequence(WmEmptySeq, "SendMessage from other thread 5", thread_n == 2); + ok_sequence(WmEmptySeq, "SendMessage from other thread 5", FALSE); ret = GetQueueStatus(QS_SENDMESSAGE|QS_POSTMESSAGE); ok(ret == 0, "wrong status %08x\n", ret); diff --git a/include/wine/server_protocol.h b/include/wine/server_protocol.h index ee9fdd1..4417cb6 100644 --- a/include/wine/server_protocol.h +++ b/include/wine/server_protocol.h @@ -2941,6 +2941,8 @@ struct get_queue_status_reply struct reply_header __header; unsigned int wake_bits; unsigned int changed_bits; + unsigned int first_sent_msg; + char __pad_20[4]; }; @@ -6412,6 +6414,6 @@ union generic_reply struct terminate_job_reply terminate_job_reply; }; -#define SERVER_PROTOCOL_VERSION 524 +#define SERVER_PROTOCOL_VERSION 525 #endif /* __WINE_WINE_SERVER_PROTOCOL_H */ diff --git a/include/winuser.h b/include/winuser.h index 2b8331c..d10caba 100644 --- a/include/winuser.h +++ b/include/winuser.h @@ -2898,6 +2898,9 @@ typedef struct tagTRACKMOUSEEVENT { /* Extra (undocumented) queue wake bits - see "Undoc. Windows" */ #define QS_SMRESULT 0x8000 +/* Internal WINE message */ +#define QS_WINE_SENDMESSAGE 0x80000000 + /* InSendMessageEx flags */ #define ISMEX_NOSEND 0x00000000 #define ISMEX_SEND 0x00000001 diff --git a/server/protocol.def b/server/protocol.def index 60865a6..7b797af 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -2162,6 +2162,7 @@ enum char_info_mode @REPLY unsigned int wake_bits; /* wake bits */ unsigned int changed_bits; /* changed bits since last time */ + unsigned int first_sent_msg; /* first message in the sent message queue */ @END diff --git a/server/queue.c b/server/queue.c index c479b38..ea6d5e6 100644 --- a/server/queue.c +++ b/server/queue.c @@ -744,7 +744,8 @@ static void receive_message( struct msg_queue *queue, struct message *msg, queue->recv_result = result; } free( msg ); - if (list_empty( &queue->msg_list[SEND_MESSAGE] )) clear_queue_bits( queue, QS_SENDMESSAGE ); + if (list_empty( &queue->msg_list[SEND_MESSAGE] )) + clear_queue_bits( queue, QS_SENDMESSAGE | QS_WINE_SENDMESSAGE ); } /* set the result of the current received message */ @@ -2272,11 +2273,17 @@ DECL_HANDLER(set_queue_mask) DECL_HANDLER(get_queue_status) { struct msg_queue *queue = current->queue; + struct list *ptr; if (queue) { reply->wake_bits = queue->wake_bits; reply->changed_bits = queue->changed_bits; queue->changed_bits &= ~req->clear_bits; + + if ((ptr = list_head( &queue->msg_list[SEND_MESSAGE] ))) + reply->first_sent_msg = LIST_ENTRY( ptr, struct message, entry )->msg; + else + reply->first_sent_msg = 0; } else reply->wake_bits = reply->changed_bits = 0; } @@ -2339,7 +2346,10 @@ DECL_HANDLER(send_message) /* fall through */ case MSG_NOTIFY: list_add_tail( &recv_queue->msg_list[SEND_MESSAGE], &msg->entry ); - set_queue_bits( recv_queue, QS_SENDMESSAGE ); + if (msg->msg & 0x80000000) /* Wine internal message */ + set_queue_bits( recv_queue, QS_WINE_SENDMESSAGE ); + else + set_queue_bits( recv_queue, QS_SENDMESSAGE ); break; case MSG_POSTED: list_add_tail( &recv_queue->msg_list[POST_MESSAGE], &msg->entry ); diff --git a/server/request.h b/server/request.h index cd396ee..374150d 100644 --- a/server/request.h +++ b/server/request.h @@ -1480,7 +1480,8 @@ C_ASSERT( FIELD_OFFSET(struct get_queue_status_request, clear_bits) == 12 ); C_ASSERT( sizeof(struct get_queue_status_request) == 16 ); C_ASSERT( FIELD_OFFSET(struct get_queue_status_reply, wake_bits) == 8 ); C_ASSERT( FIELD_OFFSET(struct get_queue_status_reply, changed_bits) == 12 ); -C_ASSERT( sizeof(struct get_queue_status_reply) == 16 ); +C_ASSERT( FIELD_OFFSET(struct get_queue_status_reply, first_sent_msg) == 16 ); +C_ASSERT( sizeof(struct get_queue_status_reply) == 24 ); C_ASSERT( FIELD_OFFSET(struct get_process_idle_event_request, handle) == 12 ); C_ASSERT( sizeof(struct get_process_idle_event_request) == 16 ); C_ASSERT( FIELD_OFFSET(struct get_process_idle_event_reply, event) == 8 ); diff --git a/server/trace.c b/server/trace.c index e0ce45a..5ba65da 100644 --- a/server/trace.c +++ b/server/trace.c @@ -2698,6 +2698,7 @@ static void dump_get_queue_status_reply( const struct get_queue_status_reply *re { fprintf( stderr, " wake_bits=%08x", req->wake_bits ); fprintf( stderr, ", changed_bits=%08x", req->changed_bits ); + fprintf( stderr, ", first_sent_msg=%08x", req->first_sent_msg ); } static void dump_get_process_idle_event_request( const struct get_process_idle_event_request *req ) -- 2.7.4

March 13, 2017

Re: [PATCH 1/5] wined3d: Get rid of "texture_stages" variable in draw_primitive_immediate_mode().

by Henri Verbeet

On 13 March 2017 at 11:57, Józef Kucia <jkucia(a)codeweavers.com> wrote: > @@ -281,8 +281,7 @@ static void draw_primitive_immediate_mode(struct wined3d_context *context, const > GL_EXTCALL(glSecondaryColor3fEXT)(0.0f, 0.0f, 0.0f); > } > > - texture_stages = d3d_info->limits.ffp_blend_stages; > - for (texture_idx = 0; texture_idx < texture_stages; ++texture_idx) > + for (texture_idx = 0; texture_idx < d3d_info->limits.ffp_blend_stages; ++texture_idx) It doesn't matter that much, but I'm not sure that's an improvement.

March 13, 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 20:46, Sebastian Lackner wrote: > On 28.02.2017 18:10, Jacek Caban wrote: >> On 28.02.2017 16:44, Sebastian Lackner wrote: >>> Well, there are also situations where it would be relatively easy to avoid >>> the roundtrip. We could return immediately at least when no events, APCs or >>> completions are involved, right? Since this is the most common situation it >>> might still be worth the effort. >> If event is not passed, we still need to signal object itself. Thinking >> about it some more, I guess we could just signal object it before >> returning for non-overlapped operations returning immediately. It >> wouldn't be exactly right, but it can't be used for synchronization >> reliably in this case anyway. >> >> Another thing is that we currently don't know if there is user APC until >> APC_ASYNC_IO returns, but that can be fixed. >> >> Since it's just an optimization, can we process with the patch itself >> first? I'm happy to optimize that, but I find it hard to agree that an >> optimization should block inclusion of patches targeted at correctness. > I'm not saying that it should block anything, I will leave the design decision > up to Alexandre. Nevertheless, I think it certainly makes sense to discuss > about such aspects in advance. Depending on the solution used it might also > require refactoring of this patch, which is easier to do before it was accepted. I drafted an optimization discussed in this thread. It's just a proof-of-concept. A better implementation will store user APCs on server side when the async is created and I'd consider moving handling of returned data and wait handle out of fd-specific ops. I ran it on my benchmark from last year [1] and it was 2 times slower than what I called "plain wine+msgmode patchset+optimizations". It's expected, since with that we need two server calls (that one needed one). Going down to one server round-trip for simple cases is easy with that and should give about equal results. The point is, named pipe code needs just small changes for such optimization, no refactoring. I hope that addresses your concerns. Jacek [1] https://www.winehq.org/pipermail/wine-devel/2016-October/115071.html

March 13, 2017

Re: wined3d: A grammar fix in a comment.

by Henri Verbeet

On 12 March 2017 at 20:47, Francois Gouget <fgouget(a)free.fr> wrote: > @@ -3645,8 +3645,8 @@ static void wined3d_adapter_init_limits(struct wined3d_gl_info *gl_info) > gl_info->gl_ops.gl.p_glGetIntegerv(GL_MAX_COMPUTE_TEXTURE_IMAGE_UNITS, &gl_max); > gl_info->limits.compute_samplers = gl_max; > TRACE("Max compute samplers: %u.\n", gl_info->limits.compute_samplers); > - /* A majority of OpenGL implementations allow to statically partition > - * the set of texture bindings into six separate sets. */ > + /* A majority of OpenGL implementations allow applications to statically > + * partition the set of texture bindings into six separate sets. */ I think that changes the meaning somewhat.

March 13, 2017

Re: [PATCH] ole32/tests: Stop crash on XP

by Jacek Caban

Hi Alistair, On 12.03.2017 22:11, Alistair Leslie-Hughes wrote: > Fixes crash > http://test.winehq.org/data/4b1445940d47870895b7fa87b7d69d12abe701ef/xp_fg-… > > Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> > --- > dlls/ole32/tests/compobj.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/dlls/ole32/tests/compobj.c b/dlls/ole32/tests/compobj.c > index 8f6ff7107c..db68fd05c6 100644 > --- a/dlls/ole32/tests/compobj.c > +++ b/dlls/ole32/tests/compobj.c > @@ -3569,7 +3569,12 @@ static void test_GlobalOptions(void) > > hres = CoCreateInstance(&CLSID_GlobalOptions, NULL, CLSCTX_INPROC_SERVER, > &IID_IGlobalOptions, (void**)&global_options); > - ok(hres == S_OK, "CoCreateInstance(CLSID_GlobalOptions) failed: %08x\n", hres); Instead of removing ok(), please change it with '|| broken(hres == E_NOINTERFACE)'. > + if(FAILED(hres)) > + { > + win_skip("CLSID_GlobalOptions not available\n"); > + CoUninitialize(); > + return; > + } > > IGlobalOptions_Release(global_options); Jacek

March 13, 2017

Re: [v3 04/12] comctl32: Extend TaskDialog, add simple callback and tests

by Nikolay Sivov

On 12.03.2017 22:39, Fabian Maurer wrote: >> On 11.03.2017 1:14, Fabian Maurer wrote: >>>> There's no reason to introduce global variable for that, just to remove >>>> it two patches after. >>> >>> It's not split the functionality into two patches. I could merge them if >>> you want, but I thought it would be easier to understand if I separated >>> those two logical changes. First getting it to work with a global >>> variable, then change it into something thread-safe. >> >> Yes, I think it's better to do it in a clean way from the start, it >> doesn't add additional complexity anyway, only SetProp/GetProp. > I understand. I just thought it would be useful that way, but I'll change it. > >>>> For minimal working replacement of MessageBox callback support is not >>>> needed, right? >>> >>> No it's not needed. But, we certainly want the tests, don't we? Without >>> callback, there'll be no tests. >> >> We can have tests that have todo_wine's all over, until callbacks are >> supported, it's not a problem. > Well if we're going that route, why not leave the tests out alltogether and > add then once the callbacks are implemented? There isn't a real benefit to add > them now if they can't test wine yet. That works too. Just make sure to run them locally, so you don't have to fix things there were just added. > > Regards, > Fabian Maurer > >

March 13, 2017

Re: [v2] user32: Removed 16-bit windows position and size constraints

by Alex Henrie

2017-03-12 4:23 GMT-06:00 Kees Beets <kbeets68(a)gmail.com>: > Thanks. Changed and submitted again. > > > I'm new to wine. From the instructions here > https://wiki.winehq.org/Submitting_Patches it sounds like there is a > possibility to submit corrections to patches without creating new entries in > the patch list. Somehow I've not been able to get that working. Hope I don't > get on people's nerves :). Alexandre manually removes old patches from the list after a couple of days. If you suspect that there is something wrong with a patch, don't send it to wine-patches, but don't feel bad about sending a new patch to fix an honest mistake. -Alex

March 12, 2017

Re: wine/test.h: Identify the process responsible for test summary lines.

by Francois Gouget

On Thu, 9 Mar 2017, Sebastian Lackner wrote: [...] > Ugh... actually it looks like we have a summary line hardcoded in schedsvc tests: The TestBot still recognizes test summary lines that don't have a process id and will continue to do so for a good while for backward compatibility. So there is no issue here: https://testbot.winehq.org/JobDetails.pl?Key=29473&log_202=1#k202 I'm still sending a patch so the line matches modern expectations: https://testbot.winehq.org/JobDetails.pl?Key=29474 -- Francois Gouget <fgouget(a)codeweavers.com>

March 12, 2017

Re: [v3 04/12] comctl32: Extend TaskDialog, add simple callback and tests

by Fabian Maurer

> On 11.03.2017 1:14, Fabian Maurer wrote: > >> There's no reason to introduce global variable for that, just to remove > >> it two patches after. > > > > It's not split the functionality into two patches. I could merge them if > > you want, but I thought it would be easier to understand if I separated > > those two logical changes. First getting it to work with a global > > variable, then change it into something thread-safe. > > Yes, I think it's better to do it in a clean way from the start, it > doesn't add additional complexity anyway, only SetProp/GetProp. I understand. I just thought it would be useful that way, but I'll change it. > >> For minimal working replacement of MessageBox callback support is not > >> needed, right? > > > > No it's not needed. But, we certainly want the tests, don't we? Without > > callback, there'll be no tests. > > We can have tests that have todo_wine's all over, until callbacks are > supported, it's not a problem. Well if we're going that route, why not leave the tests out alltogether and add then once the callbacks are implemented? There isn't a real benefit to add them now if they can't test wine yet. Regards, Fabian Maurer

March 12, 2017

Re: [v3 03/12] comctl32: Added basic implementation for task dialogs and add tests

by Fabian Maurer

> >>> + /* Align on WORD boundary for the strings */ > >>> + template_data = align_pointer(template_data, 2); > >> > >> I don't see how it could be not aligned to WORD, could you explain? > > > > I wasn't sure about the alignment of LocalAlloc, so I added that check. > > Does it guarantee an aligned pointer? > > GlobalAlloc() is documented to align at 8 bytes, and in practice > HeapAlloc() does the same I think. I see, thanks. Then I'll remove that align_pointer call. I'll improve the other things you mentioned, too. Regards Fabian Maurer

March 12, 2017

Re: [v3 04/12] comctl32: Extend TaskDialog, add simple callback and tests

by Nikolay Sivov

On 11.03.2017 1:14, Fabian Maurer wrote: >> There's no reason to introduce global variable for that, just to remove >> it two patches after. > It's not split the functionality into two patches. I could merge them if you > want, but I thought it would be easier to understand if I separated those two > logical changes. First getting it to work with a global variable, then change > it into something thread-safe. Yes, I think it's better to do it in a clean way from the start, it doesn't add additional complexity anyway, only SetProp/GetProp. > >> For minimal working replacement of MessageBox callback support is not >> needed, right? > No it's not needed. But, we certainly want the tests, don't we? Without > callback, there'll be no tests. We can have tests that have todo_wine's all over, until callbacks are supported, it's not a problem. > > Regards, > Fabian Maurer >

March 12, 2017

Re: [v3 03/12] comctl32: Added basic implementation for task dialogs and add tests

by Nikolay Sivov

On 11.03.2017 1:26, Fabian Maurer wrote: >> This is usually done with macros in Wine I think, like >> >> ((x) + size - 1) & ~(size - 1). > Do we already have a macro like that I should use or should I just define it > like that? Not in shared headers, some dlls have their own, for example ole32/usrmarshal.c does. > >>> +LPDLGTEMPLATEW dialog_template_create(dialog_header header, struct list >>> *controls) >> Why is 'header' passed by value? > Because it doesn't really matter. Is there a benefit passing it as pointer? Looks odd to me, that's all. I think this could actually be improved by merging controls list with 'header' fields, and calling it dialog description or something like that. > >>> + /* Align on WORD boundary for the strings */ >>> + template_data = align_pointer(template_data, 2); >> >> I don't see how it could be not aligned to WORD, could you explain? > I wasn't sure about the alignment of LocalAlloc, so I added that check. Does > it guarantee an aligned pointer? GlobalAlloc() is documented to align at 8 bytes, and in practice HeapAlloc() does the same I think. > > Regarding your points I didn't comment on, I'll fix those in the next version. > > Regards > Fabian Maurer > >

March 12, 2017

Re: Interested to contribute to WINE via Outreachy

by Austin English

On Mar 5, 2017 4:51 AM, "Himadri Pandya" <himadri18.07(a)gmail.com> wrote: Hello Everyone, I'm Himadri, an undergraduate student from India pursuing my bachelors in computer engineering. I'm a newbie to the FOSS community and keen to be a part by contributing to some interesting project. And I guess starting the journey with Outreachy would be the best ! Gsoc project : "Tools - Winetest Graphical User Interface" seems quite interesting to me. I would love to be a part of it. It would be really appreciable if I can get some replies about how can I contribute to it. Regards Himadri Pandya Linked In : https://www.linkedin.com/in/himadri2117 Hi Himardi, As a first step, have you tried running winetest, on either wine or windows? We'd like to hear your ideas on how you'd like to improve it. What UI changes would you find helpful, as someone (presumably) unfamiliar with it? As Aaryaman mentioned, idling on #winehackers and asking questions would be advisable and likely get you faster answers than email ;). Good luck! And don't hesitate to reach out if you have more questions.

March 12, 2017

Re: [PATCH] oledb32: Ignore IRunnableObject in IDataSourceLocator_QueryInterface.

by Sebastian Lackner

On 12.03.2017 05:34, Alistair Leslie-Hughes wrote: > + else if (IsEqualIID(riid, &IID_IDataInitialize)) > + { > + TRACE("IID_IRunnableObject returning NULL\n"); > + } You are checking for the wrong IID. Also, the TRACE does not really prevent that the FIXME message shows up.

March 11, 2017

Re: [v2] user32: Removed 16-bit windows position and size constraints

by Alex Henrie

+ // truncate x,y to 16-bit in WM_WINDOWPOSCHANGING message (only) + // truncate x,y to 16-bit in WM_WINDOWPOSCHANGED message (only) C++-style comments are not permitted in Wine patches. See https://wiki.winehq.org/Developer_Hints#Using_only_C89-compliant_code -Alex

March 11, 2017

GSoC 2017 : CMD - implement more robust parser

by Kevin Jojo

I am Prateek Bilung , a first year undergraduate student of Mathematics & Computing at BIT Mesra ,Ranchi , India. I am interested in contributing for project "CMD - implement more robust parser" . Please guide me . Thank you !

March 11, 2017

Re: [PATCH] user32: Removed 16-bit windows position and size constraints

by Kees Beets

After some further changes does not break (win) tests anymore. Still fixes an issue in Adobe Lightroom on wine. Submitted as v2 to original patch. Kind regards, Kees Beets On 03/10/2017 11:22 PM, Alexandre Julliard wrote: > Kees Beets <kbeets68(a)gmail.com> writes: > >> Fixes https://bugs.winehq.org/show_bug.cgi?id=38231 >> >> Removed code introduced in 2006 to correctly reflect Win9x 16-bit winpos >> behaviour. Newer versions of Windows do not have the 16-bit constraint >> and this code is now actually causing a bug in Adobe Lightroom on Wine. >> I've not used iswin9x to ensure compatibility with Win9x as: >> - Applications that can run on Win9x should take care of this >> - Adding such code will over time make wine more complex >> >> Tested on Ubuntu 16.04, HP Elitebook with AMD A10. >> 13 failures before change, 10 failures after change. Test code uses >> iswin9x, so to have 0 failures means changes to the test suite or >> further introduction of 'if isinw9x' logic. >> >> Signed-off-by: Kees Beets <kbeets68(a)gmail.com> >> --- >> dlls/user32/winpos.c | 16 ---------------- >> 1 file changed, 16 deletions(-) > It breaks the tests: > > ../../../tools/runtest -q -P wine -T ../../.. -M user32.dll -p user32_test.exe.so win && touch win.ok > win.c:776: Test failed: bad winpos->x -32769 > win.c:777: Test failed: bad winpos->y -40000 > win.c:795: Test failed: bad winpos->x -32769 > win.c:796: Test failed: bad winpos->y -40000 > win.c:776: Test failed: bad winpos->x 32768 > win.c:777: Test failed: bad winpos->y 40000 > win.c:795: Test failed: bad winpos->x 32768 > win.c:796: Test failed: bad winpos->y 40000 > Makefile:649: recipe for target 'win.ok' failed >

March 11, 2017

Re: [v3 01/12] comctl32: Move TaskDialogIndirect and tests into own source files

by Nikolay Sivov

On 11.03.2017 1:07, Fabian Maurer wrote: >> Since it's move to its own file, let's call new debug channel "taskdlg" >> for instance. > Sure this is useful? Currently there aren't many debug logs anyways. > What's data that we should have in taskdialog debug logs? Traces for exported functions, dialog proc trace, maybe a trace before or after callback call. > > Regards, > Fabian Maurer >

March 11, 2017

Re: [v3 02/12] comctl32: Add notification codes and messages for TaskDialog to header

by Nikolay Sivov

On 11.03.2017 1:34, Fabian Maurer wrote: >>> +enum _TASKDIALOG_NOTIFICATIONS >>> +{ >>> + TDN_CREATED = 0, >>> + TDN_NAVIGATED = 1, >>> + TDN_BUTTON_CLICKED = 2, >>> + TDN_HYPERLINK_CLICKED = 3, >>> + TDN_TIMER = 4, >>> + TDN_DESTROYED = 5, >>> + TDN_RADIO_BUTTON_CLICKED = 6, >>> + TDN_DIALOG_CONSTRUCTED = 7, >>> + TDN_VERIFICATION_CLICKED = 8, >>> + TDN_HELP = 9, >>> + TDN_EXPANDO_BUTTON_CLICKED = 10 >>> +}; >> >> This is not how it's defined in SDK (v10). > Due to cleanroom guidelines and me not being sure, I didn't look at the SDK. It's not a problem to look at it, just don't copy from it. > >>> +/* Taskdialog messages */ >>> +#define TDM_NAVIGATE_PAGE (WM_USER + 101) >>> +#define TDM_CLICK_BUTTON (WM_USER + 102) >>> +#define TDM_SET_MARQUEE_PROGRESS_BAR (WM_USER + 103) >>> +#define TDM_SET_PROGRESS_BAR_STATE (WM_USER + 104) >>> +#define TDM_SET_PROGRESS_BAR_RANGE (WM_USER + 105) >>> +#define TDM_SET_PROGRESS_BAR_POS (WM_USER + 106) >>> +#define TDM_SET_PROGRESS_BAR_MARQUEE (WM_USER + 107) >>> +#define TDM_SET_ELEMENT_TEXT (WM_USER + 108) >>> +#define TDM_CLICK_RADIO_BUTTON (WM_USER + 110) >>> +#define TDM_ENABLE_BUTTON (WM_USER + 111) >>> +#define TDM_ENABLE_RADIO_BUTTON (WM_USER + 112) >>> +#define TDM_CLICK_VERIFICATION (WM_USER + 113) >>> +#define TDM_UPDATE_ELEMENT_TEXT (WM_USER + 114) >>> +#define TDM_SET_BUTTON_ELEVATION_REQUIRED_STATE (WM_USER + 115) >>> +#define TDM_UPDATE_ICON (WM_USER + 116) >>> + >> >> Same here, message numbers are defined as enum members. > I can turn that into an enum. > But what's wrong with the "_TASKDIALOG_NOTIFICATIONS" ? > > Regards, > Fabian Maurer > > >

March 11, 2017

Re: [PATCH 3/5] usp10: Use heap_alloc_zero() instead of HeapAlloc() with HEAP_ZERO_MEMORY.

by Francois Gouget

On Fri, 10 Mar 2017, Aric Stewart wrote: [...] > I personally have no passion either way either. Having it be void vs > BOOL is all the same to me. Francois? Do you care, if so then I think > your caring will win. :) I don't really care either way either. It would just be better for it to be the same everywhere so that later changes to that set of functions can be copy/pasted everywhere without issue. Personally I think it would be even better to move them to something like include/wine/heap.h. I don't know if Alexandre would agree though. Based on what Michael told me on IRC Alexandre would like changes to be made to these functions but I don't know if it impacts heap_free(). For instance for heap_alloc() and heap_realloc() he would probably like something like this: static inline void * __WINE_ALLOC_SIZE(1) heap_alloc(size_t size) { if (!size) return NULL; return HeapAlloc(GetProcessHeap(), 0, size); } static inline void* __WINE_ALLOC_SIZE(2) heap_realloc(void *mem, size_t size) { if (!size) { heap_free(mem); return NULL; } if (!mem) return heap_alloc(size); return HeapReAlloc(GetProcessHeap(), 0, mem, size); } Then there's the string heap functions but I did not really look at those. I think his idea is also to eliminate calls to heap_xxx() for small allocations (like a simple structure) where possible. Overall, doing this kind of change will require checking the call sites to make sure they are compatible with that and that they take advantage of them. This type of checks is probably more a Michael type of thing though ;-) -- Francois Gouget <fgouget(a)free.fr> http://fgouget.free.fr/ Vote Electronique: Les gens qui votent ne décident rien. Ceux qui comptent les votes décident de tout.

March 10, 2017

Re: [v6 PATCH 00/21] x86: Enable User-Mode Instruction Prevention

by Ricardo Neri

On Fri, 2017-03-10 at 06:17 -0800, Andy Lutomirski wrote: > On Fri, Mar 10, 2017 at 3:33 AM, Stas Sergeev <stsp(a)list.ru> wrote: > > 10.03.2017 05:39, Andy Lutomirski пишет: > > > >> On Thu, Mar 9, 2017 at 2:10 PM, Stas Sergeev <stsp(a)list.ru> wrote: > >>> > >>> 09.03.2017 04:15, Ricardo Neri пишет: > >>> > >>>> On Wed, 2017-03-08 at 08:46 -0800, Andy Lutomirski wrote: > >>>>> > >>>>> On Wed, Mar 8, 2017 at 8:29 AM, Stas Sergeev <stsp(a)list.ru> wrote: > >>>>>> > >>>>>> 08.03.2017 19:06, Andy Lutomirski пишет: > >>>>>>> > >>>>>>> On Wed, Mar 8, 2017 at 6:08 AM, Stas Sergeev <stsp(a)list.ru> wrote: > >>>>>>>> > >>>>>>>> 08.03.2017 03:32, Ricardo Neri пишет: > >>>>>>>>> > >>>>>>>>> These are the instructions covered by UMIP: > >>>>>>>>> * SGDT - Store Global Descriptor Table > >>>>>>>>> * SIDT - Store Interrupt Descriptor Table > >>>>>>>>> * SLDT - Store Local Descriptor Table > >>>>>>>>> * SMSW - Store Machine Status Word > >>>>>>>>> * STR - Store Task Register > >>>>>>>>> > >>>>>>>>> This patchset initially treated tasks running in virtual-8086 > >>>>> > >>>>> mode as a > >>>>>>>>> > >>>>>>>>> special case. However, I received clarification that DOSEMU[8] > >>>>> > >>>>> does not > >>>>>>>>> > >>>>>>>>> support applications that use these instructions. > >>>>>>> > >>>>>>> Can you remind me what was special about it? It looks like you > >>>>> > >>>>> still > >>>>>>> > >>>>>>> emulate them in v8086 mode. > >>>>>> > >>>>>> Indeed, sorry, I meant prot mode here. :) > >>>>>> So I wonder what was cited to be special about v86. > >>>> > >>>> Initially my patches disabled UMIP on virtual-8086 instructions, without > >>>> regards of protected mode (i.e., UMIP was always enabled). I didn't have > >>>> emulation at the time. Then, I added emulation code that now covers > >>>> protected and virtual-8086 modes. I guess it is not special anymore. > >>> > >>> But isn't SLDT&friends just throw UD in v86? > >>> How does UMIP affect this? How does your patch affect > >>> this? > >> > >> Er, right. Ricardo, your code may need fixing. But don't you have a > >> test case for this? > > > > Why would you need one? > > Or do you really want to allow these instructions > > in v86 by the means of emulation? If so - this wasn't > > clearly stated in the patch description, neither it was > > properly discussed, it seems. > > What I meant was: if the patches incorrectly started making these > instructions work in vm86 mode where they used to cause a vm86 exit, > then that's a bug that the selftest should have caught. Yes, this is the case. I will fix this behavior... and update the test cases.

March 10, 2017

Re: [v6 PATCH 00/21] x86: Enable User-Mode Instruction Prevention

by Ricardo Neri

On Sat, 2017-03-11 at 02:58 +0300, Stas Sergeev wrote: > 11.03.2017 02:47, Ricardo Neri пишет: > >> > >>>> It doesn't need to be a matter of this particular > >>>> patch set, i.e. this proposal should not trigger a > >>>> v7 resend of all 21 patches. :) But it would be useful > >>>> for the future development of dosemu2. > >>> Would dosemu2 use 32-bit processes in order to keep segmentation? If it > >>> could use 64-bit processes, emulation is not used in this case and the > >>> SIGSEGV is delivered to user space. > >> It does use the mix: 64bit process but some segments > >> are 32bit for DOS code. > > Do you mean that dosemu2 will start as a 64-bit process and will jump to > > 32-bit code segments? > Yes, so the offending insns are executed only in 32bit > and 16bit segments, even if the process itself is 64bit. > I guess you handle 16bit segments same as 32bit ones. I have code to handle 16-bit and 32-bit address encodings differently. Segmentation is used if !user_64bit_mode(regs). In such a case, the emulation code will check the segment descriptor D flag and the address-size overrides prefix to determine the address size and use 16-bit or 32-bit address encodings as applicable. > > > My emulation code should work in this case as it > > will use segmentation in 32-bit code descriptors. Is there anything else > > needed? > If I understand you correctly, you are saying that SLDT > executed in 64bit code segment, will inevitably segfault > to userspace. Correct. > If this is the case and it makes your code > simpler, then its perfectly fine with me as dosemu does > not do this and the 64bit DOS progs are not anticipated. But if 32-bit or 16-bit code segments are used emulation will be used. Thanks and BR, Ricardo

March 10, 2017

Re: [v6 PATCH 00/21] x86: Enable User-Mode Instruction Prevention

by Ricardo Neri

On Fri, 2017-03-10 at 14:33 +0300, Stas Sergeev wrote: > 10.03.2017 05:39, Andy Lutomirski пишет: > > On Thu, Mar 9, 2017 at 2:10 PM, Stas Sergeev <stsp(a)list.ru> wrote: > >> 09.03.2017 04:15, Ricardo Neri пишет: > >> > >>> On Wed, 2017-03-08 at 08:46 -0800, Andy Lutomirski wrote: > >>>> On Wed, Mar 8, 2017 at 8:29 AM, Stas Sergeev <stsp(a)list.ru> wrote: > >>>>> 08.03.2017 19:06, Andy Lutomirski пишет: > >>>>>> On Wed, Mar 8, 2017 at 6:08 AM, Stas Sergeev <stsp(a)list.ru> wrote: > >>>>>>> 08.03.2017 03:32, Ricardo Neri пишет: > >>>>>>>> These are the instructions covered by UMIP: > >>>>>>>> * SGDT - Store Global Descriptor Table > >>>>>>>> * SIDT - Store Interrupt Descriptor Table > >>>>>>>> * SLDT - Store Local Descriptor Table > >>>>>>>> * SMSW - Store Machine Status Word > >>>>>>>> * STR - Store Task Register > >>>>>>>> > >>>>>>>> This patchset initially treated tasks running in virtual-8086 > >>>> mode as a > >>>>>>>> special case. However, I received clarification that DOSEMU[8] > >>>> does not > >>>>>>>> support applications that use these instructions. > >>>>>> Can you remind me what was special about it? It looks like you > >>>> still > >>>>>> emulate them in v8086 mode. > >>>>> Indeed, sorry, I meant prot mode here. :) > >>>>> So I wonder what was cited to be special about v86. > >>> Initially my patches disabled UMIP on virtual-8086 instructions, without > >>> regards of protected mode (i.e., UMIP was always enabled). I didn't have > >>> emulation at the time. Then, I added emulation code that now covers > >>> protected and virtual-8086 modes. I guess it is not special anymore. > >> But isn't SLDT&friends just throw UD in v86? > >> How does UMIP affect this? How does your patch affect > >> this? > > Er, right. Ricardo, your code may need fixing. But don't you have a > > test case for this? > Why would you need one? > Or do you really want to allow these instructions > in v86 by the means of emulation? If so - this wasn't > clearly stated in the patch description, neither it was > properly discussed, it seems. It str and sldt can be emulated in vm86 but as Andy mention, the behavior sould be the same with and without emulation. Thanks and BR, Ricardo

March 10, 2017

Re: [v6 PATCH 00/21] x86: Enable User-Mode Instruction Prevention

by Stas Sergeev

11.03.2017 02:47, Ricardo Neri пишет: >> >>>> It doesn't need to be a matter of this particular >>>> patch set, i.e. this proposal should not trigger a >>>> v7 resend of all 21 patches. :) But it would be useful >>>> for the future development of dosemu2. >>> Would dosemu2 use 32-bit processes in order to keep segmentation? If it >>> could use 64-bit processes, emulation is not used in this case and the >>> SIGSEGV is delivered to user space. >> It does use the mix: 64bit process but some segments >> are 32bit for DOS code. > Do you mean that dosemu2 will start as a 64-bit process and will jump to > 32-bit code segments? Yes, so the offending insns are executed only in 32bit and 16bit segments, even if the process itself is 64bit. I guess you handle 16bit segments same as 32bit ones. > My emulation code should work in this case as it > will use segmentation in 32-bit code descriptors. Is there anything else > needed? If I understand you correctly, you are saying that SLDT executed in 64bit code segment, will inevitably segfault to userspace. If this is the case and it makes your code simpler, then its perfectly fine with me as dosemu does not do this and the 64bit DOS progs are not anticipated.

March 10, 2017

Re: [v6 PATCH 00/21] x86: Enable User-Mode Instruction Prevention

by Ricardo Neri

On Thu, 2017-03-09 at 18:39 -0800, Andy Lutomirski wrote: > On Thu, Mar 9, 2017 at 2:10 PM, Stas Sergeev <stsp(a)list.ru> wrote: > > 09.03.2017 04:15, Ricardo Neri пишет: > > > >> On Wed, 2017-03-08 at 08:46 -0800, Andy Lutomirski wrote: > >>> > >>> On Wed, Mar 8, 2017 at 8:29 AM, Stas Sergeev <stsp(a)list.ru> wrote: > >>>> > >>>> 08.03.2017 19:06, Andy Lutomirski пишет: > >>>>> > >>>>> On Wed, Mar 8, 2017 at 6:08 AM, Stas Sergeev <stsp(a)list.ru> wrote: > >>>>>> > >>>>>> 08.03.2017 03:32, Ricardo Neri пишет: > >>>>>>> > >>>>>>> These are the instructions covered by UMIP: > >>>>>>> * SGDT - Store Global Descriptor Table > >>>>>>> * SIDT - Store Interrupt Descriptor Table > >>>>>>> * SLDT - Store Local Descriptor Table > >>>>>>> * SMSW - Store Machine Status Word > >>>>>>> * STR - Store Task Register > >>>>>>> > >>>>>>> This patchset initially treated tasks running in virtual-8086 > >>> > >>> mode as a > >>>>>>> > >>>>>>> special case. However, I received clarification that DOSEMU[8] > >>> > >>> does not > >>>>>>> > >>>>>>> support applications that use these instructions. > >>>>> > >>>>> Can you remind me what was special about it? It looks like you > >>> > >>> still > >>>>> > >>>>> emulate them in v8086 mode. > >>>> > >>>> Indeed, sorry, I meant prot mode here. :) > >>>> So I wonder what was cited to be special about v86. > >> > >> Initially my patches disabled UMIP on virtual-8086 instructions, without > >> regards of protected mode (i.e., UMIP was always enabled). I didn't have > >> emulation at the time. Then, I added emulation code that now covers > >> protected and virtual-8086 modes. I guess it is not special anymore. > > > > But isn't SLDT&friends just throw UD in v86? > > How does UMIP affect this? How does your patch affect > > this? > > Er, right. Ricardo, your code may need fixing. But don't you have a > test case for this? The behavior should be the same with and without > your patches applied. The exception is #UD, not #GP, so maybe your > code just never executes in the vm86 case. Ouch! Yes, I am afraid my code will attempt to emulate sldt in vm86 mode. The test cases that I have for vm86 are only for the instructions that are valid in vm86: smsw, sidt and sgdt. I will add test cases for str and sldt and make sure that a #UD is issued. Would this trigger a v7 series? Thanks and BR, Ricardo > > --Andy

March 10, 2017

Re: [v6 PATCH 00/21] x86: Enable User-Mode Instruction Prevention

by Ricardo Neri

On Fri, 2017-03-10 at 01:01 +0300, Stas Sergeev wrote: > 09.03.2017 03:46, Ricardo Neri пишет: > > On Wed, 2017-03-08 at 17:08 +0300, Stas Sergeev wrote: > >> 08.03.2017 03:32, Ricardo Neri пишет: > >>> These are the instructions covered by UMIP: > >>> * SGDT - Store Global Descriptor Table > >>> * SIDT - Store Interrupt Descriptor Table > >>> * SLDT - Store Local Descriptor Table > >>> * SMSW - Store Machine Status Word > >>> * STR - Store Task Register > >>> > >>> This patchset initially treated tasks running in virtual-8086 mode as a > >>> special case. However, I received clarification that DOSEMU[8] does not > >>> support applications that use these instructions. > >> Yes, this is the case. > >> But at least in the past there was an attempt to > >> support SLDT as it is used by an ancient pharlap > >> DOS extender (currently unsupported by dosemu1/2). > >> So how difficult would it be to add an optional > >> possibility of delivering such SIGSEGV to userspace > >> so that the kernel's dummy emulation can be overridden? > > I suppose a umip=noemulation kernel parameter could be added in this > > case. > Why? > It doesn't need to be global: the app should be > able to change that on its own. Note that no app currently > requires this, so its just for the future, and in the > future the app can start using the new API for this, > if you provide one. Right, I missed this detail. Then, yes the API should allow only one app to relay the SIGSEGV. > > > >> It doesn't need to be a matter of this particular > >> patch set, i.e. this proposal should not trigger a > >> v7 resend of all 21 patches. :) But it would be useful > >> for the future development of dosemu2. > > Would dosemu2 use 32-bit processes in order to keep segmentation? If it > > could use 64-bit processes, emulation is not used in this case and the > > SIGSEGV is delivered to user space. > It does use the mix: 64bit process but some segments > are 32bit for DOS code. Do you mean that dosemu2 will start as a 64-bit process and will jump to 32-bit code segments? My emulation code should work in this case as it will use segmentation in 32-bit code descriptors. Is there anything else needed? Thanks and BR, Ricardo

March 10, 2017

Re: [v6 PATCH 21/21] selftests/x86: Add tests for User-Mode Instruction Prevention

by Ricardo Neri

On Wed, 2017-03-08 at 07:56 -0800, Andy Lutomirski wrote: > On Tue, Mar 7, 2017 at 4:32 PM, Ricardo Neri > <ricardo.neri-calderon(a)linux.intel.com> wrote: > > Certain user space programs that run on virtual-8086 mode may utilize > > instructions protected by the User-Mode Instruction Prevention (UMIP) > > security feature present in new Intel processors: SGDT, SIDT and SMSW. In > > such a case, a general protection fault is issued if UMIP is enabled. When > > such a fault happens, the kernel catches it and emulates the results of > > these instructions with dummy values. The purpose of this new > > test is to verify whether the impacted instructions can be executed without > > causing such #GP. If no #GP exceptions occur, we expect to exit virtual- > > 8086 mode from INT 0x80. > > > > The instructions protected by UMIP are executed in representative use > > cases: > > a) the memory address of the result is given in the form of a displacement > > from the base of the data segment > > b) the memory address of the result is given in a general purpose register > > c) the result is stored directly in a general purpose register. > > > > Unfortunately, it is not possible to check the results against a set of > > expected values because no emulation will occur in systems that do not have > > the UMIP feature. Instead, results are printed for verification. > > You could pre-initialize the result buffer to a bunch of non-matching > values (1, 2, 3, ...) and then check that all the invocations of the > same instruction gave the same value. Yes, I can do this. Alternatively, I can check in the test program if the CPU has UMIP and only run the tests in that case. > > If you do this, maybe make it a follow-up patch -- see other email. Great! Thank you! Thanks and BR, Ricardo

March 10, 2017

Re: [v3 03/12] comctl32: Added basic implementation for task dialogs and add tests

by Fabian Maurer

On Freitag, 10. März 2017 22:36:56 CET Nikolay Sivov wrote: > On 11.03.2017 0:10, Sebastian Lackner wrote: > > On 10.03.2017 21:23, Fabian Maurer wrote: > >>> Yes, you should send your patches in smaller chunks. Also, you shouldn't > >>> rely on the assumption that all of your patches are applied at once. The > >>> patchset should still make sense, even if each commit is applied > >>> separately. If it doesn't, that would mean you have to reorder or merge > >>> them. As long as you mark failing tests with todo_wine it is not a > >>> problem > >>> to have a temporary state with "less features" than the original > >>> implementation though. > >> > >> Sorry, I still don't understand what's wrong. The patches depend on each > >> other, so just applying patch 9 would result in an error. But only > >> applying > >> 1-5 should pose no problem. Except for missing functionality of course, > >> but > >> the tests should be fine. > >> But I have to send these 12 patches as single patchset, since it's > >> supposed to replace the old taskdialog-hack and I don't want to > >> introduce a regression. Only with all those patches the functionality is > >> the same. > > > > Of course they should be applied in the correct order, but introducing a > > temporary regression because of unimplemented functionality is not a > > problem, and basically expected when you are working on such a task. ;) > > I think it is a problem in this case, it's not guaranteed they will all > committed at once. One way to fix that is to incrementally add things, > still falling back to MessageBox if something is not working yet. > > Again, the usual and recommended process for upstreaming stuff is to focus > > on a set of about 5-7 patches, improve them until they fulfill all the > > requirements and have been accepted, and then proceed with next ones. > > Following these rules will make things easier for you (no need to resend > > all 12 patches when you missed some testbot errors, for example), and also > > for reviewers. As a reviewer you cannot be sure that a patch which > > previously was fine hasn't been changed, so you have to start from scratch > > and review the same patch over and over again - noone wants to do that. ;) > > If it is still not clear what I mean, maybe someone else can try and > > explain it a bit better. > > Yes, keeping is at 5 at a time is better. I really don't see how this would work. Sure, I could add a partial implementation, but it would be pretty useless until the other patches are added. Should we add code that's unused yet? I don't have a problem with that, but I thought it's against the project's rules. Falling back to the messagebox if someone is working sounds pretty impossible. You arguably need text and buttons and them working properly for the taskdialog, or you don't need to bother using it. I could delay the the cancel functionality, setting the correct window size, the default button, the callback and all the tests, but honestly I don't think that would be a good idea. Or just make the patches bigger by merging them. But I don't see how I could get a working taskdialog in just 5 patches. So, how should I go about it? Regards, Fabian Maurer

March 10, 2017

Re: [v3 02/12] comctl32: Add notification codes and messages for TaskDialog to header

by Fabian Maurer

> > +enum _TASKDIALOG_NOTIFICATIONS > > +{ > > + TDN_CREATED = 0, > > + TDN_NAVIGATED = 1, > > + TDN_BUTTON_CLICKED = 2, > > + TDN_HYPERLINK_CLICKED = 3, > > + TDN_TIMER = 4, > > + TDN_DESTROYED = 5, > > + TDN_RADIO_BUTTON_CLICKED = 6, > > + TDN_DIALOG_CONSTRUCTED = 7, > > + TDN_VERIFICATION_CLICKED = 8, > > + TDN_HELP = 9, > > + TDN_EXPANDO_BUTTON_CLICKED = 10 > > +}; > > This is not how it's defined in SDK (v10). Due to cleanroom guidelines and me not being sure, I didn't look at the SDK. > > +/* Taskdialog messages */ > > +#define TDM_NAVIGATE_PAGE (WM_USER + 101) > > +#define TDM_CLICK_BUTTON (WM_USER + 102) > > +#define TDM_SET_MARQUEE_PROGRESS_BAR (WM_USER + 103) > > +#define TDM_SET_PROGRESS_BAR_STATE (WM_USER + 104) > > +#define TDM_SET_PROGRESS_BAR_RANGE (WM_USER + 105) > > +#define TDM_SET_PROGRESS_BAR_POS (WM_USER + 106) > > +#define TDM_SET_PROGRESS_BAR_MARQUEE (WM_USER + 107) > > +#define TDM_SET_ELEMENT_TEXT (WM_USER + 108) > > +#define TDM_CLICK_RADIO_BUTTON (WM_USER + 110) > > +#define TDM_ENABLE_BUTTON (WM_USER + 111) > > +#define TDM_ENABLE_RADIO_BUTTON (WM_USER + 112) > > +#define TDM_CLICK_VERIFICATION (WM_USER + 113) > > +#define TDM_UPDATE_ELEMENT_TEXT (WM_USER + 114) > > +#define TDM_SET_BUTTON_ELEVATION_REQUIRED_STATE (WM_USER + 115) > > +#define TDM_UPDATE_ICON (WM_USER + 116) > > + > > Same here, message numbers are defined as enum members. I can turn that into an enum. But what's wrong with the "_TASKDIALOG_NOTIFICATIONS" ? Regards, Fabian Maurer

March 10, 2017

Re: [v3 03/12] comctl32: Added basic implementation for task dialogs and add tests

by Fabian Maurer

> This is usually done with macros in Wine I think, like > > ((x) + size - 1) & ~(size - 1). Do we already have a macro like that I should use or should I just define it like that? > > +LPDLGTEMPLATEW dialog_template_create(dialog_header header, struct list > > *controls) > Why is 'header' passed by value? Because it doesn't really matter. Is there a benefit passing it as pointer? > > + /* Align on WORD boundary for the strings */ > > + template_data = align_pointer(template_data, 2); > > I don't see how it could be not aligned to WORD, could you explain? I wasn't sure about the alignment of LocalAlloc, so I added that check. Does it guarantee an aligned pointer? Regarding your points I didn't comment on, I'll fix those in the next version. Regards Fabian Maurer

March 10, 2017

Re: [PATCH] user32: Removed 16-bit windows position and size constraints

by Alexandre Julliard

Kees Beets <kbeets68(a)gmail.com> writes: > Fixes https://bugs.winehq.org/show_bug.cgi?id=38231 > > Removed code introduced in 2006 to correctly reflect Win9x 16-bit winpos > behaviour. Newer versions of Windows do not have the 16-bit constraint > and this code is now actually causing a bug in Adobe Lightroom on Wine. > I've not used iswin9x to ensure compatibility with Win9x as: > - Applications that can run on Win9x should take care of this > - Adding such code will over time make wine more complex > > Tested on Ubuntu 16.04, HP Elitebook with AMD A10. > 13 failures before change, 10 failures after change. Test code uses > iswin9x, so to have 0 failures means changes to the test suite or > further introduction of 'if isinw9x' logic. > > Signed-off-by: Kees Beets <kbeets68(a)gmail.com> > --- > dlls/user32/winpos.c | 16 ---------------- > 1 file changed, 16 deletions(-) It breaks the tests: ../../../tools/runtest -q -P wine -T ../../.. -M user32.dll -p user32_test.exe.so win && touch win.ok win.c:776: Test failed: bad winpos->x -32769 win.c:777: Test failed: bad winpos->y -40000 win.c:795: Test failed: bad winpos->x -32769 win.c:796: Test failed: bad winpos->y -40000 win.c:776: Test failed: bad winpos->x 32768 win.c:777: Test failed: bad winpos->y 40000 win.c:795: Test failed: bad winpos->x 32768 win.c:796: Test failed: bad winpos->y 40000 Makefile:649: recipe for target 'win.ok' failed -- Alexandre Julliard julliard(a)winehq.org

March 10, 2017

Re: [v3 02/12] comctl32: Add notification codes and messages for TaskDialog to header

by Nikolay Sivov

On 10.03.2017 21:21, Fabian Maurer wrote: > Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> > --- > include/commctrl.h | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/include/commctrl.h b/include/commctrl.h > index d176b4fd0e..1bf9c279c5 100644 > --- a/include/commctrl.h > +++ b/include/commctrl.h > @@ -5228,6 +5228,38 @@ typedef struct _TASKDIALOG_BUTTON > PCWSTR pszButtonText; > } TASKDIALOG_BUTTON; > > +enum _TASKDIALOG_NOTIFICATIONS > +{ > + TDN_CREATED = 0, > + TDN_NAVIGATED = 1, > + TDN_BUTTON_CLICKED = 2, > + TDN_HYPERLINK_CLICKED = 3, > + TDN_TIMER = 4, > + TDN_DESTROYED = 5, > + TDN_RADIO_BUTTON_CLICKED = 6, > + TDN_DIALOG_CONSTRUCTED = 7, > + TDN_VERIFICATION_CLICKED = 8, > + TDN_HELP = 9, > + TDN_EXPANDO_BUTTON_CLICKED = 10 > +}; This is not how it's defined in SDK (v10). > + > +/* Taskdialog messages */ > +#define TDM_NAVIGATE_PAGE (WM_USER + 101) > +#define TDM_CLICK_BUTTON (WM_USER + 102) > +#define TDM_SET_MARQUEE_PROGRESS_BAR (WM_USER + 103) > +#define TDM_SET_PROGRESS_BAR_STATE (WM_USER + 104) > +#define TDM_SET_PROGRESS_BAR_RANGE (WM_USER + 105) > +#define TDM_SET_PROGRESS_BAR_POS (WM_USER + 106) > +#define TDM_SET_PROGRESS_BAR_MARQUEE (WM_USER + 107) > +#define TDM_SET_ELEMENT_TEXT (WM_USER + 108) > +#define TDM_CLICK_RADIO_BUTTON (WM_USER + 110) > +#define TDM_ENABLE_BUTTON (WM_USER + 111) > +#define TDM_ENABLE_RADIO_BUTTON (WM_USER + 112) > +#define TDM_CLICK_VERIFICATION (WM_USER + 113) > +#define TDM_UPDATE_ELEMENT_TEXT (WM_USER + 114) > +#define TDM_SET_BUTTON_ELEVATION_REQUIRED_STATE (WM_USER + 115) > +#define TDM_UPDATE_ICON (WM_USER + 116) > + Same here, message numbers are defined as enum members. > typedef HRESULT (CALLBACK *PFTASKDIALOGCALLBACK)(HWND, UINT, WPARAM, LPARAM, LONG_PTR); > > typedef struct _TASKDIALOGCONFIG >

March 10, 2017

Re: [v3 04/12] comctl32: Extend TaskDialog, add simple callback and tests

by Fabian Maurer

> There's no reason to introduce global variable for that, just to remove > it two patches after. It's not split the functionality into two patches. I could merge them if you want, but I thought it would be easier to understand if I separated those two logical changes. First getting it to work with a global variable, then change it into something thread-safe. > For minimal working replacement of MessageBox callback support is not > needed, right? No it's not needed. But, we certainly want the tests, don't we? Without callback, there'll be no tests. Regards, Fabian Maurer

March 10, 2017

Re: [v3 06/12] comctl32: TaskDialog - make thread-safe

by Fabian Maurer

On Freitag, 10. März 2017 23:06:11 CET Nikolay Sivov wrote: > On 10.03.2017 21:21, Fabian Maurer wrote: > > + > > +typedef struct > > +{ > > + const TASKDIALOGCONFIG *task_config; > > + HWND hwnd; > > +}taskdialog_info; > > It doesn't look very safe to keep original pointer here. If you only > need callback pointer and user callback data, store them directly. I certainly could do that if you think that's better. But why unsafe? The pointer should be valid until TaskDialogIndirect returns, shouldn't it? Regards, Fabian Maurer

March 10, 2017

Re: [v3 01/12] comctl32: Move TaskDialogIndirect and tests into own source files

by Fabian Maurer

> Since it's move to its own file, let's call new debug channel "taskdlg" > for instance. Sure this is useful? Currently there aren't many debug logs anyways. What's data that we should have in taskdialog debug logs? Regards, Fabian Maurer

March 10, 2017

Re: [v3 06/12] comctl32: TaskDialog - make thread-safe

by Nikolay Sivov

On 10.03.2017 21:21, Fabian Maurer wrote: > + > +typedef struct > +{ > + const TASKDIALOGCONFIG *task_config; > + HWND hwnd; > +}taskdialog_info; It doesn't look very safe to keep original pointer here. If you only need callback pointer and user callback data, store them directly.

March 10, 2017

Re: [v3 04/12] comctl32: Extend TaskDialog, add simple callback and tests

by Nikolay Sivov

On 10.03.2017 21:21, Fabian Maurer wrote: > v3: Rewrite to implement Nikolay Sivov's suggestions > > Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> > --- > dlls/comctl32/taskdialog.c | 29 +++++++++++++++++++++++-- > dlls/comctl32/tests/taskdialog.c | 46 +++++++++++++++++++++++++++++++++------- > 2 files changed, 65 insertions(+), 10 deletions(-) > > diff --git a/dlls/comctl32/taskdialog.c b/dlls/comctl32/taskdialog.c > index 2166ce1898..c5f410716e 100644 > --- a/dlls/comctl32/taskdialog.c > +++ b/dlls/comctl32/taskdialog.c > @@ -147,17 +147,41 @@ static void controls_add(struct list *controls, WORD id, const WCHAR *class, con > list_add_tail(controls, &data->entry); > } > > +/* FIXME: Make thread safe */ > +static const TASKDIALOGCONFIG *task_config = 0; > +static HRESULT callback(HWND hwnd, UINT uNotification, WPARAM wParam, LPARAM lParam) > +{ > + if(task_config->pfCallback) > + return task_config->pfCallback(hwnd, uNotification, wParam, lParam, task_config->lpCallbackData); > + return S_OK; > +} > + There's no reason to introduce global variable for that, just to remove it two patches after. > static INT_PTR CALLBACK DialogProc(HWND hwndDlg, UINT uMsg, WPARAM wParam, LPARAM lParam) > { > + HRESULT ret_callback; > + > switch (uMsg) > { > + case WM_INITDIALOG: > + callback(hwndDlg, TDN_DIALOG_CONSTRUCTED, 0, 0); > + callback(hwndDlg, TDN_CREATED, 0, 0); > + return TRUE; > case WM_COMMAND: > - if(HIWORD(wParam) == BN_CLICKED && LOWORD(wParam) == IDOK) > + if(HIWORD(wParam) == BN_CLICKED) > { > - EndDialog(hwndDlg, 0); > + WORD command_id = LOWORD(wParam); > + > + ret_callback = callback(hwndDlg, TDN_BUTTON_CLICKED, command_id, 0); > + if(ret_callback == S_OK) /* FIXME */ > + { > + EndDialog(hwndDlg, command_id); > + } > return TRUE; > } > break; > + case WM_DESTROY: > + callback(hwndDlg, TDN_DESTROYED, 0, 0); > + break; > } > return FALSE; For minimal working replacement of MessageBox callback support is not needed, right?

March 10, 2017

Re: [v3 03/12] comctl32: Added basic implementation for task dialogs and add tests

by Nikolay Sivov

On 10.03.2017 21:21, Fabian Maurer wrote: > + UINT text_size; /* Length in bytes including null-terminator */ > + UINT class_size; /* Length in bytes including null-terminator */ > + struct list entry; > +}control_info; > + > +typedef struct > +{ > + DLGTEMPLATE template; > + WORD menu; > + WORD class; > + const WCHAR *title; > + > + /* Not part of actual template */ > + UINT titleSize; /* Length in bytes including null-terminator */ > +}dialog_header; About formatting issues, for consistency please add space after bracket, same goes for 'if' and other places. "titleSize" -> "title_size". > +#define MEMCPY_MOVEPTR(target, source, size) memcpy(target, source, size); target += size; I think this one will be better as a function, also this macro assumes that target point is char*, which is impossible to guess. > +static void* align_pointer(void *ptr, unsigned int boundary) > +{ > + boundary--; > + return (void *)(((UINT_PTR)ptr + boundary) & ~boundary); > +} This is usually done with macros in Wine I think, like ((x) + size - 1) & ~(size - 1). > +LPDLGTEMPLATEW dialog_template_create(dialog_header header, struct list *controls) Why is 'header' passed by value? > + void *template_data_start; I think it's better to declare this with a type you're going to return. > + data_size += sizeof(WORD)*2 + header.titleSize; Same formatting issue - please add spaces around operator. > + /* Align on WORD boundary for the strings */ > + template_data = align_pointer(template_data, 2); I don't see how it could be not aligned to WORD, could you explain?

March 10, 2017

Re: [v3 01/12] comctl32: Move TaskDialogIndirect and tests into own source files

by Nikolay Sivov

On 10.03.2017 21:21, Fabian Maurer wrote: > diff --git a/dlls/comctl32/taskdialog.c b/dlls/comctl32/taskdialog.c > new file mode 100644 > index 0000000000..c26d53bac9 > --- /dev/null > +++ b/dlls/comctl32/taskdialog.c > @@ -0,0 +1,72 @@ > +/* > + * Task dialog control > + * > + * Copyright 2017 Fabian Maurer for the Wine project > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA > + * > + */ > + > +#include <stdarg.h> > +#include <string.h> > + > +#include "windef.h" > +#include "winbase.h" > +#include "winuser.h" > +#include "commctrl.h" > +#include "winerror.h" > +#include "comctl32.h" > +#include "wine/debug.h" > + > +WINE_DEFAULT_DEBUG_CHANNEL(commctrl); > + Since it's move to its own file, let's call new debug channel "taskdlg" for instance.

March 10, 2017

Re: [v6 PATCH 00/21] x86: Enable User-Mode Instruction Prevention

by Stas Sergeev

11.03.2017 00:04, Andy Lutomirski пишет: > On Fri, Mar 10, 2017 at 2:30 AM, Stas Sergeev <stsp(a)list.ru> wrote: >> 10.03.2017 05:41, Andy Lutomirski пишет: >> >>> On Wed, Mar 8, 2017 at 5:11 PM, Ricardo Neri >>> <ricardo.neri-calderon(a)linux.intel.com> wrote: >>>> On Wed, 2017-03-08 at 19:53 +0300, Stas Sergeev wrote: >>>>> 08.03.2017 19:46, Andy Lutomirski пишет: >>>>>>> No no, since I meant prot mode, this is not what I need. >>>>>>> I would never need to disable UMIP as to allow the >>>>>>> prot mode apps to do SLDT. Instead it would be good >>>>>>> to have an ability to provide a replacement for the dummy >>>>>>> emulation that is currently being proposed for kernel. >>>>>>> All is needed for this, is just to deliver a SIGSEGV. >>>>>> That's what I meant. Turning off FIXUP_UMIP would leave UMIP on but >>>>>> turn off the fixup, so you'd get a SIGSEGV indicating #GP (or a vm86 >>>>>> GP exit). >>>>> But then I am confused with the word "compat" in >>>>> your "COMPAT_MASK0_X86_UMIP_FIXUP" and >>>>> "sys_adjust_compat_mask(int op, int word, u32 mask);" >>>>> >>>>> Leaving UMIP on and only disabling a fixup doesn't >>>>> sound like a compat option to me. I would expect >>>>> compat to disable it completely. >>>> I guess that the _UMIP_FIXUP part makes it clear that emulation, not >>>> UMIP is disabled, allowing the SIGSEGV be delivered to the user space >>>> program. >>>> >>>> Would having a COMPAT_MASK0_X86_UMIP_FIXUP to disable emulation and a >>>> COMPAT_MASK0_X86_UMIP to disable UMIP make sense? >>>> >>>> Also, wouldn't having a COMPAT_MASK0_X86_UMIP to disable UMIP defeat its >>>> purpose? Applications could simply use this compat mask to bypass UMIP >>>> and gain access to the instructions it protects. >>>> >>> I was obviously extremely unclear. The point of the proposed syscall >>> is to let programs opt out of legacy features. >> I guess both "compat" and "legacy" are misleading >> here. Maybe these are "x86-specific" or "hypervisor-specific", >> but a mere enabling of UMIP doesn't immediately make >> the use of SLDT instruction a legacy IMHO. > Sure it is. :) Using SLDT from user mode is a legacy ability that > just happens to still work on existing CPUs and kernels. Once UMIP > goes in, it will officially be obsolete Yes, but the names you suggest, imply that "UMIP_FIXUP" is legacy or compat, which I find misleading because it have just appeared. Maybe something like "COMPAT_X86_UMIP_INSNS_EMU"?

March 10, 2017

Re: [v3 03/12] comctl32: Added basic implementation for task dialogs and add tests

by Nikolay Sivov

On 11.03.2017 0:10, Sebastian Lackner wrote: > On 10.03.2017 21:23, Fabian Maurer wrote: >>> Yes, you should send your patches in smaller chunks. Also, you shouldn't >>> rely on the assumption that all of your patches are applied at once. The >>> patchset should still make sense, even if each commit is applied >>> separately. If it doesn't, that would mean you have to reorder or merge >>> them. As long as you mark failing tests with todo_wine it is not a problem >>> to have a temporary state with "less features" than the original >>> implementation though. >> >> Sorry, I still don't understand what's wrong. The patches depend on each >> other, so just applying patch 9 would result in an error. But only applying >> 1-5 should pose no problem. Except for missing functionality of course, but >> the tests should be fine. >> But I have to send these 12 patches as single patchset, since it's supposed to >> replace the old taskdialog-hack and I don't want to introduce a regression. >> Only with all those patches the functionality is the same. > > Of course they should be applied in the correct order, but introducing a > temporary regression because of unimplemented functionality is not a > problem, and basically expected when you are working on such a task. ;) I think it is a problem in this case, it's not guaranteed they will all committed at once. One way to fix that is to incrementally add things, still falling back to MessageBox if something is not working yet. > > Again, the usual and recommended process for upstreaming stuff is to focus > on a set of about 5-7 patches, improve them until they fulfill all the > requirements and have been accepted, and then proceed with next ones. > Following these rules will make things easier for you (no need to resend > all 12 patches when you missed some testbot errors, for example), and also > for reviewers. As a reviewer you cannot be sure that a patch which > previously was fine hasn't been changed, so you have to start from scratch > and review the same patch over and over again - noone wants to do that. ;) > If it is still not clear what I mean, maybe someone else can try and > explain it a bit better. Yes, keeping is at 5 at a time is better. > > Best regards, > Sebastian > > >

March 10, 2017

Re: [PATCH] dbghelp: Make SymGetLineFromAddr64 call SymGetLineFromAddrW64

by Alistair Leslie-Hughes

On 11/03/17 03:26, Sebastian Lackner wrote: > On 10.03.2017 04:24, Alistair Leslie-Hughes wrote: >> Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> >> --- >> dlls/dbghelp/dbghelp_private.h | 3 ++ >> dlls/dbghelp/symbol.c | 96 ++++++++++++++++++++++++++++++------------ >> 2 files changed, 73 insertions(+), 26 deletions(-) >> > > Could you give some explanation for the purpose of this patch? It looks a bit odd that > we need two almost identical symt_fill_func_line_info implementations. The function stabs_finalize_function in stabs.c uses this function. However updating this as well started a snowball if changes, making the patch too large. The symt_fill_func_line_info should be able to be removed once the symt_get_func_line_next is updated to with a similar change as this one. Best Regards Alistair

March 10, 2017

Re: [v3 03/12] comctl32: Added basic implementation for task dialogs and add tests

by Sebastian Lackner

On 10.03.2017 21:23, Fabian Maurer wrote: >> Yes, you should send your patches in smaller chunks. Also, you shouldn't >> rely on the assumption that all of your patches are applied at once. The >> patchset should still make sense, even if each commit is applied >> separately. If it doesn't, that would mean you have to reorder or merge >> them. As long as you mark failing tests with todo_wine it is not a problem >> to have a temporary state with "less features" than the original >> implementation though. > > Sorry, I still don't understand what's wrong. The patches depend on each > other, so just applying patch 9 would result in an error. But only applying > 1-5 should pose no problem. Except for missing functionality of course, but > the tests should be fine. > But I have to send these 12 patches as single patchset, since it's supposed to > replace the old taskdialog-hack and I don't want to introduce a regression. > Only with all those patches the functionality is the same. Of course they should be applied in the correct order, but introducing a temporary regression because of unimplemented functionality is not a problem, and basically expected when you are working on such a task. ;) Again, the usual and recommended process for upstreaming stuff is to focus on a set of about 5-7 patches, improve them until they fulfill all the requirements and have been accepted, and then proceed with next ones. Following these rules will make things easier for you (no need to resend all 12 patches when you missed some testbot errors, for example), and also for reviewers. As a reviewer you cannot be sure that a patch which previously was fine hasn't been changed, so you have to start from scratch and review the same patch over and over again - noone wants to do that. ;) If it is still not clear what I mean, maybe someone else can try and explain it a bit better. Best regards, Sebastian

March 10, 2017

Re: [v6 PATCH 00/21] x86: Enable User-Mode Instruction Prevention

by Andy Lutomirski

On Fri, Mar 10, 2017 at 2:30 AM, Stas Sergeev <stsp(a)list.ru> wrote: > 10.03.2017 05:41, Andy Lutomirski пишет: > >> On Wed, Mar 8, 2017 at 5:11 PM, Ricardo Neri >> <ricardo.neri-calderon(a)linux.intel.com> wrote: >>> >>> On Wed, 2017-03-08 at 19:53 +0300, Stas Sergeev wrote: >>>> >>>> 08.03.2017 19:46, Andy Lutomirski пишет: >>>>>> >>>>>> No no, since I meant prot mode, this is not what I need. >>>>>> I would never need to disable UMIP as to allow the >>>>>> prot mode apps to do SLDT. Instead it would be good >>>>>> to have an ability to provide a replacement for the dummy >>>>>> emulation that is currently being proposed for kernel. >>>>>> All is needed for this, is just to deliver a SIGSEGV. >>>>> >>>>> That's what I meant. Turning off FIXUP_UMIP would leave UMIP on but >>>>> turn off the fixup, so you'd get a SIGSEGV indicating #GP (or a vm86 >>>>> GP exit). >>>> >>>> But then I am confused with the word "compat" in >>>> your "COMPAT_MASK0_X86_UMIP_FIXUP" and >>>> "sys_adjust_compat_mask(int op, int word, u32 mask);" >>>> >>>> Leaving UMIP on and only disabling a fixup doesn't >>>> sound like a compat option to me. I would expect >>>> compat to disable it completely. >>> >>> I guess that the _UMIP_FIXUP part makes it clear that emulation, not >>> UMIP is disabled, allowing the SIGSEGV be delivered to the user space >>> program. >>> >>> Would having a COMPAT_MASK0_X86_UMIP_FIXUP to disable emulation and a >>> COMPAT_MASK0_X86_UMIP to disable UMIP make sense? >>> >>> Also, wouldn't having a COMPAT_MASK0_X86_UMIP to disable UMIP defeat its >>> purpose? Applications could simply use this compat mask to bypass UMIP >>> and gain access to the instructions it protects. >>> >> I was obviously extremely unclear. The point of the proposed syscall >> is to let programs opt out of legacy features. > > I guess both "compat" and "legacy" are misleading > here. Maybe these are "x86-specific" or "hypervisor-specific", > but a mere enabling of UMIP doesn't immediately make > the use of SLDT instruction a legacy IMHO. Sure it is. :) Using SLDT from user mode is a legacy ability that just happens to still work on existing CPUs and kernels. Once UMIP goes in, it will officially be obsolete -- it will just be supported for backwards compatibility. New code should opt out and emulate in usermode if needed. (And the vast, vast majority of Linux programs don't use these instructions in the first place.) Similarly, vsyscalls were obsolete the as soon as better alternatives were fully supported and the kernel started making them slow, and the fact that new static glibc programs still used them for a little while didn't make them any less obsolete. > >> I'll ponder this a bit more. > > So if we are to invent something new, it would be nice to > also think up a clear terminology for it. Maybe something > like "X86_FEATURE_xxx_MASK" or alike. But they're misfeatures, not features. --Andy -- Andy Lutomirski AMA Capital Management, LLC

March 10, 2017

Re: [PATCH v2] user32: Always send WM_NCACTIVATE when changing the foreground window.

by Alexandre Julliard

Alex Henrie <alexhenrie24(a)gmail.com> writes: > 2017-03-10 2:02 GMT-07:00 Alexandre Julliard <julliard(a)winehq.org>: >> Alex Henrie <alexhenrie24(a)gmail.com> writes: >> >>> Fixes https://bugs.winehq.org/show_bug.cgi?id=7198 >>> >>> v2: Use the wineserver to determine if the new foreground window is >>> already active in its thread. >> >> It still looks suspicious to do it here, particularly since we already >> have code for this in set_active_window. It would need more tests to >> determine where it fits with the other messages of the sequence. > > Can you give me some details about the additional tests you want to > see, or assign the patch to someone who can help? There are already several message tests for SetActiveWindow, you'd want to do something similar for SetForegroundWindow, probably across two different threads to see the whole sequence. -- Alexandre Julliard julliard(a)winehq.org

March 10, 2017

Re: [v3 03/12] comctl32: Added basic implementation for task dialogs and add tests

by Fabian Maurer

> Nevertheless, it actually wouldn't > hurt to add a couple of interactive tests, too. Having non-interactive > tests is nice, but they don't help to notice visual bugs. That's true, I just didn't know how to do that. I'll probably add a few once this implementation is finished. > Yes, you should send your patches in smaller chunks. Also, you shouldn't > rely on the assumption that all of your patches are applied at once. The > patchset should still make sense, even if each commit is applied > separately. If it doesn't, that would mean you have to reorder or merge > them. As long as you mark failing tests with todo_wine it is not a problem > to have a temporary state with "less features" than the original > implementation though. Sorry, I still don't understand what's wrong. The patches depend on each other, so just applying patch 9 would result in an error. But only applying 1-5 should pose no problem. Except for missing functionality of course, but the tests should be fine. But I have to send these 12 patches as single patchset, since it's supposed to replace the old taskdialog-hack and I don't want to introduce a regression. Only with all those patches the functionality is the same. Regards, Fabian Maurer

March 10, 2017

Re: [PATCH v2] user32: Always send WM_NCACTIVATE when changing the foreground window.

by Alex Henrie

2017-03-10 2:02 GMT-07:00 Alexandre Julliard <julliard(a)winehq.org>: > Alex Henrie <alexhenrie24(a)gmail.com> writes: > >> Fixes https://bugs.winehq.org/show_bug.cgi?id=7198 >> >> v2: Use the wineserver to determine if the new foreground window is >> already active in its thread. > > It still looks suspicious to do it here, particularly since we already > have code for this in set_active_window. It would need more tests to > determine where it fits with the other messages of the sequence. Can you give me some details about the additional tests you want to see, or assign the patch to someone who can help? -Alex

March 10, 2017

Re: [v3 03/12] comctl32: Added basic implementation for task dialogs and add tests

by Sebastian Lackner

On 10.03.2017 20:18, Fabian Maurer wrote: >> We usually use winetest_interactive for that. > Is that necessary if I replace the test with another in the next patch > anyways? Sorry, I missed that it is only a workaround for unimplemented functionality. It should be fine then. Nevertheless, it actually wouldn't hurt to add a couple of interactive tests, too. Having non-interactive tests is nice, but they don't help to notice visual bugs. > I figured it would be enough if I don't introduce a test fail, in case > of a bisect or something along the lines. > >> Also, please try to send your changes in reasonable sized chunks (max 5-7) >> in the future. Sending more patches at once only makes sense if they do not >> depend on each other. > I don't quite understand what you mean. You mean I sent too many patches at > once? But all those 12 patches depend on each other, together only they form > an implementation that could replace the current one. Yes, you should send your patches in smaller chunks. Also, you shouldn't rely on the assumption that all of your patches are applied at once. The patchset should still make sense, even if each commit is applied separately. If it doesn't, that would mean you have to reorder or merge them. As long as you mark failing tests with todo_wine it is not a problem to have a temporary state with "less features" than the original implementation though. Best regards, Sebastian

March 10, 2017

Re: [v3 09/12] comctl32: TaskDialog - Changed tests to allow sending messages

by Fabian Maurer

> === w1064 (64 bit taskdialog) === > comctl32:taskdialog returned an error (3221226356) despite having no > failures comctl32:taskdialog has no test summary line (early exit of the > main process?) Yes, there seems to be a problem with 64bit taskdialog. I'll investigate why it crashes. Regards, Fabian Maurer

March 10, 2017

Re: [v3 12/12] comctl32: TaskDialog - Add ability to cancel dialog

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=29425 Your paranoid android. === wvistau64 (64 bit taskdialog) === comctl32:taskdialog returned an error (3221226356) despite having no failures comctl32:taskdialog has no test summary line (early exit of the main process?) === w2008s64 (64 bit taskdialog) === comctl32:taskdialog returned an error (3221226356) despite having no failures comctl32:taskdialog has no test summary line (early exit of the main process?) === w7pro64 (64 bit taskdialog) === comctl32:taskdialog returned an error (3221226356) despite having no failures comctl32:taskdialog has no test summary line (early exit of the main process?) === w864 (64 bit taskdialog) === comctl32:taskdialog returned an error (3221226356) despite having no failures comctl32:taskdialog has no test summary line (early exit of the main process?) === w1064 (64 bit taskdialog) === comctl32:taskdialog returned an error (3221226356) despite having no failures comctl32:taskdialog has no test summary line (early exit of the main process?)

March 10, 2017

Re: [v3 11/12] comctl32: TaskDialog - Set default button and add tests

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=29424 Your paranoid android. === wvistau64 (64 bit taskdialog) === comctl32:taskdialog returned an error (3221226356) despite having no failures comctl32:taskdialog has no test summary line (early exit of the main process?) === w2008s64 (64 bit taskdialog) === comctl32:taskdialog returned an error (3221226356) despite having no failures comctl32:taskdialog has no test summary line (early exit of the main process?) === w7pro64 (64 bit taskdialog) === comctl32:taskdialog returned an error (3221226356) despite having no failures comctl32:taskdialog has no test summary line (early exit of the main process?) === w864 (64 bit taskdialog) === comctl32:taskdialog returned an error (3221226356) despite having no failures comctl32:taskdialog has no test summary line (early exit of the main process?) === w1064 (64 bit taskdialog) === comctl32:taskdialog returned an error (3221226356) despite having no failures comctl32:taskdialog has no test summary line (early exit of the main process?)

March 10, 2017

Re: [PATCH 2/2] odbccp32: Implement SQLRemoveDriver/SQLRemoveDriverW

by Vincent Povirk

> + if (usage_count) > + *usage_count = 0; If we're not going to do refcounting properly, it'd make more sense to hard-code this to 1 and never actually remove the drivers. Better to leave things around than delete them while they're still needed.

March 10, 2017

Re: [v3 10/12] comctl32: TaskDialog - Add button return value and tests

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=29423 Your paranoid android. === wvistau64 (64 bit taskdialog) === comctl32:taskdialog returned an error (3221226356) despite having no failures comctl32:taskdialog has no test summary line (early exit of the main process?) === w2008s64 (64 bit taskdialog) === comctl32:taskdialog returned an error (3221226356) despite having no failures comctl32:taskdialog has no test summary line (early exit of the main process?) === w7pro64 (64 bit taskdialog) === comctl32:taskdialog returned an error (3221226356) despite having no failures comctl32:taskdialog has no test summary line (early exit of the main process?) === w864 (64 bit taskdialog) === comctl32:taskdialog returned an error (3221226356) despite having no failures comctl32:taskdialog has no test summary line (early exit of the main process?) === w1064 (64 bit taskdialog) === comctl32:taskdialog returned an error (3221226356) despite having no failures comctl32:taskdialog has no test summary line (early exit of the main process?)

March 10, 2017

Re: [v3 03/12] comctl32: Added basic implementation for task dialogs and add tests

by Fabian Maurer

> We usually use winetest_interactive for that. Is that necessary if I replace the test with another in the next patch anyways? I figured it would be enough if I don't introduce a test fail, in case of a bisect or something along the lines. > Also, please try to send your changes in reasonable sized chunks (max 5-7) > in the future. Sending more patches at once only makes sense if they do not > depend on each other. I don't quite understand what you mean. You mean I sent too many patches at once? But all those 12 patches depend on each other, together only they form an implementation that could replace the current one. Regards, Fabian Maurer

March 10, 2017

Re: [v3 09/12] comctl32: TaskDialog - Changed tests to allow sending messages

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=29422 Your paranoid android. === wvistau64 (64 bit taskdialog) === comctl32:taskdialog returned an error (3221226356) despite having no failures comctl32:taskdialog has no test summary line (early exit of the main process?) === w2008s64 (64 bit taskdialog) === comctl32:taskdialog returned an error (3221226356) despite having no failures comctl32:taskdialog has no test summary line (early exit of the main process?) === w7pro64 (64 bit taskdialog) === comctl32:taskdialog returned an error (3221226356) despite having no failures comctl32:taskdialog has no test summary line (early exit of the main process?) === w864 (64 bit taskdialog) === comctl32:taskdialog returned an error (3221226356) despite having no failures comctl32:taskdialog has no test summary line (early exit of the main process?) === w1064 (64 bit taskdialog) === comctl32:taskdialog returned an error (3221226356) despite having no failures comctl32:taskdialog has no test summary line (early exit of the main process?)

March 10, 2017

Re: [PATCH 1/2] odbccp32: Implement SQLInstallDriverEx correctly

by Vincent Povirk

> + if(RegSetValueExW(hkeydriver, driver, 0, REG_SZ, (BYTE*)installed, > + (sizeof(installed)+1)*sizeof(WCHAR)) != ERROR_SUCCESS) sizeof(installed) is already in bytes and includes the null terminator. > + /* Driver and Setup entries use the system path unless a path is specified. */ > + if(lstrcmpiW(driverW, entry) == 0 || lstrcmpiW(setupW, entry) == 0) > + { > + lstrcpyW(value, path); > + lstrcatW(value, slash); > + lstrcatW(value, divider); > + } > + else > + lstrcpyW(value, divider); Some sort of overflow check might be good here. > + ret = SQLInstallDriverEx("WINE ODBC Driver\0Driver=sample.dll\0Setup=sample.dll\0\0", NULL, path, MAX_PATH, &size, ODBC_INSTALL_COMPLETE, NULL); > + sql_ret = SQLInstallerErrorW(1, &error_code, NULL, 0, NULL); > + if (sql_ret && error_code == ODBC_ERROR_WRITING_SYSINFO_FAILED) > + { > + win_skip("not enough privileges\n"); > + return; > + } > + ok(sql_ret && error_code == SQL_SUCCESS, "SQLInstallDriverEx failed %d, %u\n", sql_ret, error_code); > + ok(!strcmp(path, syspath), "invalid path %s\n", path); This doesn't use the value of ret.

March 10, 2017

Re: [v3 03/12] comctl32: Added basic implementation for task dialogs and add tests

by Sebastian Lackner

On 10.03.2017 19:21, Fabian Maurer wrote: > + /* Skip this test on wine, because it doesn't really fail, > + * it would displays a dialog that doesn't automatically close */ > + if (strcmp(winetest_platform, "wine")) > + { > + ret = pTaskDialogIndirect(&info, NULL, NULL, NULL); > + ok(ret == S_OK, "Expected S_OK, got %x\n", ret); > + } We usually use winetest_interactive for that. There might be more comments, so please wait a bit before resending. ;) Also, please try to send your changes in reasonable sized chunks (max 5-7) in the future. Sending more patches at once only makes sense if they do not depend on each other. Best regards, Sebastian

March 10, 2017

Re: [PATCH] dbghelp: Make SymGetLineFromAddr64 call SymGetLineFromAddrW64

by Sebastian Lackner

On 10.03.2017 04:24, Alistair Leslie-Hughes wrote: > Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> > --- > dlls/dbghelp/dbghelp_private.h | 3 ++ > dlls/dbghelp/symbol.c | 96 ++++++++++++++++++++++++++++++------------ > 2 files changed, 73 insertions(+), 26 deletions(-) > Could you give some explanation for the purpose of this patch? It looks a bit odd that we need two almost identical symt_fill_func_line_info implementations. > + if (found) > + { > + char *filename = (char*)source_get(module, dli->u.source_file); > + int len = lstrlenA(filename)+1; > + > + line->FileName = fetch_buffer(module->process, len * sizeof(WCHAR)); > + > + MultiByteToWideChar(CP_ACP, 0, filename, -1, line->FileName, len); [...] > - len = MultiByteToWideChar(CP_ACP, 0, l64->FileName, -1, NULL, 0); > - if ((l64w->FileName = fetch_buffer(pcs, len * sizeof(WCHAR)))) > - MultiByteToWideChar(CP_ACP, 0, l64->FileName, -1, l64w->FileName, len); > - l64w->Address = l64->Address; Actually, the old way of converting the strings looked more reasonable. Computing the correct length and checking for errors is preferred in such cases. Best regards, Sebastian

March 10, 2017

Re: [PATCH 4/5] usp10: Use heap_alloc() instead of HeapAlloc().

by Henri Verbeet

On 10 March 2017 at 16:23, Aric Stewart <aric(a)codeweavers.com> wrote: > Feels a little like there are 2 unrelated changes here the heap_alloc work and then using the loaded_feature temporary variable to clean up the code. I am fine with both changes so I will sign off but i wonder if it makes the commit message a bit less clear. > Yeah, somewhat. It came up during the heap_alloc() work, but ended up touching a fair number of lines. In that regard it could have been split out, although I figured it wasn't too bad.

March 10, 2017

Re: [PATCH 3/5] usp10: Use heap_alloc_zero() instead of HeapAlloc() with HEAP_ZERO_MEMORY.

by Aric Stewart

On 3/9/17 9:02 AM, Henri Verbeet wrote: > On 9 March 2017 at 15:51, Sebastian Lackner <sebastian(a)fds-team.de> wrote: >> On 09.03.2017 10:37, Henri Verbeet wrote: >>> -static inline BOOL heap_free(void *mem) >>> -{ >>> - return HeapFree(GetProcessHeap(), 0, mem); >>> -} >> [...] >>> +static inline void heap_free(void *mem) >>> +{ >>> + HeapFree(GetProcessHeap(), 0, mem); >>> +} >> >> I'm not sure if Francois is happy with this change. In other dlls, he >> has made the exact opposite change in his effort to standardize those >> functions. >> > I wasn't aware of that. While I don't care particularly much if it's > there or not, I'd be somewhat surprised if there are legitimate uses > of the return value. At least inside usp10 it's always ignored. > > I personally have no passion either way either. Having it be void vs BOOL is all the same to me. Francois? Do you care, if so then I think your caring will win. :) To not slow down my review I will ignore this issue and look at the rest. Since I don't care my resulting sign-offs will reflect that. -aric

March 10, 2017

Re: [v6 PATCH 00/21] x86: Enable User-Mode Instruction Prevention

by Andy Lutomirski

On Fri, Mar 10, 2017 at 3:33 AM, Stas Sergeev <stsp(a)list.ru> wrote: > 10.03.2017 05:39, Andy Lutomirski пишет: > >> On Thu, Mar 9, 2017 at 2:10 PM, Stas Sergeev <stsp(a)list.ru> wrote: >>> >>> 09.03.2017 04:15, Ricardo Neri пишет: >>> >>>> On Wed, 2017-03-08 at 08:46 -0800, Andy Lutomirski wrote: >>>>> >>>>> On Wed, Mar 8, 2017 at 8:29 AM, Stas Sergeev <stsp(a)list.ru> wrote: >>>>>> >>>>>> 08.03.2017 19:06, Andy Lutomirski пишет: >>>>>>> >>>>>>> On Wed, Mar 8, 2017 at 6:08 AM, Stas Sergeev <stsp(a)list.ru> wrote: >>>>>>>> >>>>>>>> 08.03.2017 03:32, Ricardo Neri пишет: >>>>>>>>> >>>>>>>>> These are the instructions covered by UMIP: >>>>>>>>> * SGDT - Store Global Descriptor Table >>>>>>>>> * SIDT - Store Interrupt Descriptor Table >>>>>>>>> * SLDT - Store Local Descriptor Table >>>>>>>>> * SMSW - Store Machine Status Word >>>>>>>>> * STR - Store Task Register >>>>>>>>> >>>>>>>>> This patchset initially treated tasks running in virtual-8086 >>>>> >>>>> mode as a >>>>>>>>> >>>>>>>>> special case. However, I received clarification that DOSEMU[8] >>>>> >>>>> does not >>>>>>>>> >>>>>>>>> support applications that use these instructions. >>>>>>> >>>>>>> Can you remind me what was special about it? It looks like you >>>>> >>>>> still >>>>>>> >>>>>>> emulate them in v8086 mode. >>>>>> >>>>>> Indeed, sorry, I meant prot mode here. :) >>>>>> So I wonder what was cited to be special about v86. >>>> >>>> Initially my patches disabled UMIP on virtual-8086 instructions, without >>>> regards of protected mode (i.e., UMIP was always enabled). I didn't have >>>> emulation at the time. Then, I added emulation code that now covers >>>> protected and virtual-8086 modes. I guess it is not special anymore. >>> >>> But isn't SLDT&friends just throw UD in v86? >>> How does UMIP affect this? How does your patch affect >>> this? >> >> Er, right. Ricardo, your code may need fixing. But don't you have a >> test case for this? > > Why would you need one? > Or do you really want to allow these instructions > in v86 by the means of emulation? If so - this wasn't > clearly stated in the patch description, neither it was > properly discussed, it seems. What I meant was: if the patches incorrectly started making these instructions work in vm86 mode where they used to cause a vm86 exit, then that's a bug that the selftest should have caught.

March 10, 2017

Re: [PATCH 2/2 resend] devenum: Return S_OK in IMoniker::IsEqual() stub.

by Andrew Eikum

I'm not sure how application use these monikers, but lying about moniker equality makes me a little nervous. This doesn't look that hard to implement correctly. Andrew On Fri, Mar 10, 2017 at 06:26:55PM +0800, Jactry Zeng wrote: > Superseded patch 131295. > > Signed-off-by: Jactry Zeng <jzeng(a)codeweavers.com> > --- > dlls/devenum/mediacatenum.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > > diff --git a/dlls/devenum/mediacatenum.c b/dlls/devenum/mediacatenum.c > index 66343342c9..3543182cf4 100644 > --- a/dlls/devenum/mediacatenum.c > +++ b/dlls/devenum/mediacatenum.c > @@ -530,7 +530,10 @@ static HRESULT WINAPI DEVENUM_IMediaCatMoniker_IsEqual(IMoniker *iface, IMoniker > { > FIXME("(%p)->(%p): stub\n", iface, pmkOtherMoniker); > > - return E_NOTIMPL; > + if (!pmkOtherMoniker) > + return E_INVALIDARG; > + > + return S_OK; > } > > static HRESULT WINAPI DEVENUM_IMediaCatMoniker_Hash(IMoniker *iface, DWORD *pdwHash) > >

March 10, 2017

Re: [1/2] services/tests: Add test for database locking.

by Sebastian Lackner

On 10.03.2017 14:45, Marvin wrote: > Hi, > > While running your changed tests on Windows, I think I found new failures. > Being a bot and all I'm not very good at pattern recognition, so I might be > wrong, but could you please double-check? > Full results can be found at > https://testbot.winehq.org/JobDetails.pl?Key=29411 > > Your paranoid android. > > > === w7u (32 bit service) === > service.c:407: Test failed: LockServiceDatabase failed: 1055 > service.c:413: Test failed: UnlockServiceDatabase failed: 1071 > Hm... its probably better to ignore this series for now, while I am investigating whats going on here. This error code should not be possible. ;)

March 10, 2017

Re: [1/2] services/tests: Add test for database locking.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=29411 Your paranoid android. === w7u (32 bit service) === service.c:407: Test failed: LockServiceDatabase failed: 1055 service.c:413: Test failed: UnlockServiceDatabase failed: 1071

March 10, 2017

Re: [v6 PATCH 00/21] x86: Enable User-Mode Instruction Prevention

by Stas Sergeev

10.03.2017 05:39, Andy Lutomirski пишет: > On Thu, Mar 9, 2017 at 2:10 PM, Stas Sergeev <stsp(a)list.ru> wrote: >> 09.03.2017 04:15, Ricardo Neri пишет: >> >>> On Wed, 2017-03-08 at 08:46 -0800, Andy Lutomirski wrote: >>>> On Wed, Mar 8, 2017 at 8:29 AM, Stas Sergeev <stsp(a)list.ru> wrote: >>>>> 08.03.2017 19:06, Andy Lutomirski пишет: >>>>>> On Wed, Mar 8, 2017 at 6:08 AM, Stas Sergeev <stsp(a)list.ru> wrote: >>>>>>> 08.03.2017 03:32, Ricardo Neri пишет: >>>>>>>> These are the instructions covered by UMIP: >>>>>>>> * SGDT - Store Global Descriptor Table >>>>>>>> * SIDT - Store Interrupt Descriptor Table >>>>>>>> * SLDT - Store Local Descriptor Table >>>>>>>> * SMSW - Store Machine Status Word >>>>>>>> * STR - Store Task Register >>>>>>>> >>>>>>>> This patchset initially treated tasks running in virtual-8086 >>>> mode as a >>>>>>>> special case. However, I received clarification that DOSEMU[8] >>>> does not >>>>>>>> support applications that use these instructions. >>>>>> Can you remind me what was special about it? It looks like you >>>> still >>>>>> emulate them in v8086 mode. >>>>> Indeed, sorry, I meant prot mode here. :) >>>>> So I wonder what was cited to be special about v86. >>> Initially my patches disabled UMIP on virtual-8086 instructions, without >>> regards of protected mode (i.e., UMIP was always enabled). I didn't have >>> emulation at the time. Then, I added emulation code that now covers >>> protected and virtual-8086 modes. I guess it is not special anymore. >> But isn't SLDT&friends just throw UD in v86? >> How does UMIP affect this? How does your patch affect >> this? > Er, right. Ricardo, your code may need fixing. But don't you have a > test case for this? Why would you need one? Or do you really want to allow these instructions in v86 by the means of emulation? If so - this wasn't clearly stated in the patch description, neither it was properly discussed, it seems.

March 10, 2017

Re: [v6 PATCH 00/21] x86: Enable User-Mode Instruction Prevention

by Stas Sergeev

10.03.2017 05:41, Andy Lutomirski пишет: > On Wed, Mar 8, 2017 at 5:11 PM, Ricardo Neri > <ricardo.neri-calderon(a)linux.intel.com> wrote: >> On Wed, 2017-03-08 at 19:53 +0300, Stas Sergeev wrote: >>> 08.03.2017 19:46, Andy Lutomirski пишет: >>>>> No no, since I meant prot mode, this is not what I need. >>>>> I would never need to disable UMIP as to allow the >>>>> prot mode apps to do SLDT. Instead it would be good >>>>> to have an ability to provide a replacement for the dummy >>>>> emulation that is currently being proposed for kernel. >>>>> All is needed for this, is just to deliver a SIGSEGV. >>>> That's what I meant. Turning off FIXUP_UMIP would leave UMIP on but >>>> turn off the fixup, so you'd get a SIGSEGV indicating #GP (or a vm86 >>>> GP exit). >>> But then I am confused with the word "compat" in >>> your "COMPAT_MASK0_X86_UMIP_FIXUP" and >>> "sys_adjust_compat_mask(int op, int word, u32 mask);" >>> >>> Leaving UMIP on and only disabling a fixup doesn't >>> sound like a compat option to me. I would expect >>> compat to disable it completely. >> I guess that the _UMIP_FIXUP part makes it clear that emulation, not >> UMIP is disabled, allowing the SIGSEGV be delivered to the user space >> program. >> >> Would having a COMPAT_MASK0_X86_UMIP_FIXUP to disable emulation and a >> COMPAT_MASK0_X86_UMIP to disable UMIP make sense? >> >> Also, wouldn't having a COMPAT_MASK0_X86_UMIP to disable UMIP defeat its >> purpose? Applications could simply use this compat mask to bypass UMIP >> and gain access to the instructions it protects. >> > I was obviously extremely unclear. The point of the proposed syscall > is to let programs opt out of legacy features. I guess both "compat" and "legacy" are misleading here. Maybe these are "x86-specific" or "hypervisor-specific", but a mere enabling of UMIP doesn't immediately make the use of SLDT instruction a legacy IMHO. > I'll ponder this a bit more. So if we are to invent something new, it would be nice to also think up a clear terminology for it. Maybe something like "X86_FEATURE_xxx_MASK" or alike.

March 10, 2017