August 2011 - Wine-Devel - list.winehq.org

Re: [PATCH] ntdll: provide a timestamp debug channel, which prints a millisecond resolution timestamp in front of debug output, very useful to debug deadlocks (resend)

by Bernhard Loos

On Wed, Aug 31, 2011 at 12:20 PM, Alexandre Julliard <julliard(a)winehq.org> wrote: > Bernhard Loos <bernhardloos(a)googlemail.com> writes: > >> @@ -164,8 +165,16 @@ static int NTDLL_dbg_vlog( enum __wine_debug_class cls, struct __wine_debug_chan >> /* only print header if we are at the beginning of the line */ >> if (info->out_pos == info->output || info->out_pos[-1] == '\n') >> { >> + if (TRACE_ON(timestamp)) >> + { >> + LARGE_INTEGER now; >> + extern timeout_t server_start_time; > > Don't add extern statements all over the place, there are functions to > return tick counts. Found NtGetTickCount, after some looking around which does what I need. In my defence, I the kernel32 version of GetTickCount also calls NtQuerySystemTime, so it never occured to me, that there might be one in ntdll. > Also you probably want a more readable formatting. I'm not exactly sure, how to make it more readable. I could add a . after seconds and ms at the end, but this makes it more likely to create line wraps, which also hurts readability and it's not that hard to figure out. > -- > Alexandre Julliard > julliard(a)winehq.org >

Aug. 31, 2011

Re: [PATCH 1/5] cmd/tests: add more tests for command line parsing

by Frédéric Delanoy

2011/8/31 Frédéric Delanoy <frederic.delanoy(a)gmail.com>: > 2011/8/31 Martin Wilck <mwilck(a)arcor.de>: >>> Just ask if you don't understand the @keywords(a). Doc is a bit lacking; >>> I might add a quick wiki help page if desired. >> >> Yes, that'd be helpful (IMO just a README file in the tests/ directory >> would be even better). It took me a while to get started with the cmd >> testing code. Submitted in http://source.winehq.org/patches/data/78195 but Alexandre prefers a wiki page so I'll convert it. Frédéric

Aug. 31, 2011

Re: [PATCH 1/5] ddraw: Make the OpenGL renderer the default one.

by Henri Verbeet

On 31 August 2011 10:58, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote: > This will break 2D-only apps in situations where OpenGL is not available. It Well, DirectDraw 2D-only applications, but yes. All three people in that situation should either install a software GL renderer or explicitly set the "DirectDrawRenderer" registry key to "gdi". > It will make it impossible to run some apps out of the box(e.g. the ones > currently affected by bug 2082, and old QuickTime versions). That's fine with I'm not sure how this will make it more impossible to fix that bug.

Aug. 31, 2011

Re: [PATCH 1/4] msi: speed up WHERE statement evaluation by evaluating the condition as early as possible

by Alexandre Julliard

Bernhard Loos <bernhardloos(a)googlemail.com> writes: > @@ -639,6 +715,8 @@ static UINT WHERE_execute( struct tagMSIVIEW *view, MSIRECORD *record ) > while ((table = table->next)); > > rows = msi_alloc( wv->table_count * sizeof(*rows) ); > + memset( rows, 0xFF, sizeof( rows ) ); > + If the goal is to set them to -1, please do that explicitly. Also defining some appropriate constant for invalid rows would be even better. -- Alexandre Julliard julliard(a)winehq.org

Aug. 31, 2011

Re: cmd/tests: Test del /s recursion in subdirectories with colons.

by Frédéric Delanoy

2011/8/30 Octavian Voicu <octavian.voicu(a)gmail.com>: > 2011/8/30 Frédéric Delanoy <frederic.delanoy(a)gmail.com>: >> You don't test the result of your 'mkdir "foo:"'. You can't know from >> the test whether the dir was created and removed, or never existed. > > That's the whole point! I tested in cmd prompt on a Windows XP machine > (and I assume all Windows versions act the same in this regard), and > mkdir refuses to create a directory with colons in its name (same goes > with explorer). Even if it might be possible to create a directory > with a colon in its name by using some API, it's not relevant. OK > So this test is strictly for Wine -- but it does have to pass on > Windows too. The test I wrote is designed to fail (with a stack > overflow) if you revert my earlier patch. It's not like any (sane) > Windows application will be trying to create directories with colons > in their names, but if it happens that the users has such a directory, > you don't want Wine to go crashing because of it. > >> You should use something like "if exist ... echo blablabla", or a 'dir >> /b' in a directory to verify it's not created. >> Be aware that error messages are generally lost/discarded, so you have >> to test differently > > I don't see the point of actually testing if the directory was > created, because it will always fail on Windows (and you can't really > mark this failure as broken, because it's standard Windows behavior). My point was simply to make sure that subsequent wine patches, maybe a year or two from now, won't make it "work" again (think regression) I never talked about marking it as broken (it obviously doesn't work on any windows), just that running a plain "mkdir foo:" and not checking the expected outcome is pointless IMHO (besides detecting crash/overflows don't occur again in wine) Maybe a simple "if exist foo: (echo wine is broken)" would be sufficient. This way you would detect quickly it wine somehow makes it "work" again. Frédéric

Aug. 31, 2011

Re: [PATCH] ntdll: provide a timestamp debug channel, which prints a millisecond resolution timestamp in front of debug output, very useful to debug deadlocks (resend)

by Alexandre Julliard

Bernhard Loos <bernhardloos(a)googlemail.com> writes: > @@ -164,8 +165,16 @@ static int NTDLL_dbg_vlog( enum __wine_debug_class cls, struct __wine_debug_chan > /* only print header if we are at the beginning of the line */ > if (info->out_pos == info->output || info->out_pos[-1] == '\n') > { > + if (TRACE_ON(timestamp)) > + { > + LARGE_INTEGER now; > + extern timeout_t server_start_time; Don't add extern statements all over the place, there are functions to return tick counts. Also you probably want a more readable formatting. -- Alexandre Julliard julliard(a)winehq.org

Aug. 31, 2011

Re: cmd/tests: Add README doc file

by Alexandre Julliard

Frédéric Delanoy <frederic.delanoy(a)gmail.com> writes: > --- > programs/cmd/tests/README | 120 +++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 120 insertions(+), 0 deletions(-) > create mode 100644 programs/cmd/tests/README That sort of thing should be on the wiki. READMEs in the source are never maintained properly. -- Alexandre Julliard julliard(a)winehq.org

Aug. 31, 2011

Re: [PATCH 1/5] ddraw: Make the OpenGL renderer the default one.

by Stefan Dösinger

On Tuesday 30 August 2011 20:12:31 Henri Verbeet wrote: > This imlicitly gets rid of the "auto" option as well, since there's no way > to explicitly specify it. This will no doubt cause its share of > regressions, but I prefer actually fixing those bugs to running into the > surface implementation switching code every time I try to fix something in > ddraw. This will break 2D-only apps in situations where OpenGL is not available. It would be a good idea to use SURFACE_GDI automatically in those situations. (I'm not sure if ddraw without GL works at all right now, we have no systems that run the tests with this setup) It will make it impossible to run some apps out of the box(e.g. the ones currently affected by bug 2082, and old QuickTime versions). That's fine with me, it's not like those apps are working at the moment, and I guess the switching breaks more than it fixes.

Aug. 31, 2011

Re: [PATCH 1/5] cmd/tests: add more tests for command line parsing

by Frédéric Delanoy

2011/8/31 Martin Wilck <mwilck(a)arcor.de>: > >> IMHO we should probably split it only when it's reasonably complete, >> where we'd have a broader view. > > Well, it's up to you. I just made an attempt to merge my stuff into > test_builtins and gave up after 2 hours :-( > >> Also, test_parsing name is probably a bit too generic (not that >> test_builtins isn't ;) ) >> >> But you can still submit it in the meantime, maybe AJ will accept it. >> Just make sure there's no duplication > > AFAICT there is no real duplication. 4 TODOs in the current > test_builtins are removed by the last of my patches, so there is some > overlap, but I wouldn't call it a duplicate. Fair enough. >> Also, when you have to make tests work on NT4->2k8 *and* wine at *all* >> times, you sometimes have to make your hands dirty. > > I tested my stuff only on XP. Perhaps someone can help me out testing > other native versions. You should subscribe to testbot. It has a shitload of available VMs to tests your patches against. Actually, my current test procedure is the following: 1. Write a passing test/expected result on a reasonably old win version (e.g. 2k or XP) 2. Submit to testbot (http://wiki.winehq.org/WineTestBot) to check it works on win versions (sometimes it fails on NT4 or other win version due to random bugs) If it fails, iterate until it passes on windows 3. Check if it works on wine (make testclean && make test) If it works, you're done. Else, make changes and goto 2 >> Just ask if you don't understand the @keywords(a). Doc is a bit lacking; >> I might add a quick wiki help page if desired. > > Yes, that'd be helpful (IMO just a README file in the tests/ directory > would be even better). It took me a while to get started with the cmd > testing code. Yeah me too. Wine's learning curve is steep. Did you read http://www.winehq.org/docs/winedev-guide/index ? It's not entirely relevant to cmd's tests but is a good starting point. I'll write a patch adding a README. There's also IRC #winehackers channel on Freenode. >> Did you correctly register via >> http://www.winehq.org/mailman/listinfo/wine-patches? > > I was hoping this was an open ML (and last time tried it was). OK, I'll > subscribe and re-submit. Think "spam". Also you might want to submit smaller patches, it helps with regression testing (and eases code review by the committer) >> Nice job by the way. > > Thanks a lot, it seems that you took on a big task with cmd, good luck! > > Martin > Frédéric

Aug. 31, 2011

Re: ieframe: Fix a few incorrect pointer/integer casts on x64.

by Michael Stefaniuc

Hello Octavian, in general please use the ULongToHandle*() / HandleToULong() macro family instead of the explicit casting. Octavian Voicu wrote: > --- > dlls/ieframe/iexplore.c | 6 +++--- > 1 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/dlls/ieframe/iexplore.c b/dlls/ieframe/iexplore.c > index 99e3003..82c2e79 100644 > --- a/dlls/ieframe/iexplore.c > +++ b/dlls/ieframe/iexplore.c > @@ -937,7 +937,7 @@ static HDDEDATA WINAPI dde_proc(UINT type, UINT uFmt, HCONV hConv, HSZ hsz1, HSZ > switch(type) { > case XTYP_CONNECT: > TRACE("XTYP_CONNECT %p\n", hsz1); > - return (HDDEDATA)!DdeCmpStringHandles(hsz1, ddestr_openurl); > + return (HDDEDATA)(INT_PTR)!DdeCmpStringHandles(hsz1, ddestr_openurl); > > case XTYP_EXECUTE: { > WCHAR *url; > @@ -961,7 +961,7 @@ static HDDEDATA WINAPI dde_proc(UINT type, UINT uFmt, HCONV hConv, HSZ hsz1, HSZ > break; > } > > - ret = (HDDEDATA)open_dde_url(url); > + ret = (HDDEDATA)(ULONG_PTR)open_dde_url(url); This is the only user of open_dde_url() so that helper might as well return a HDDEDATA instead of an ULONG. But that's something for Jacek to decide. > > heap_free(url); > return ret; > @@ -999,7 +999,7 @@ static void init_dde(void) > if(!ddestr_openurl) > WARN("Failed to create string handle: %u\n", DdeGetLastError(dde_inst)); > > - res = (ULONG)DdeNameService(dde_inst, ddestr_iexplore, 0, DNS_REGISTER); > + res = (UINT)(UINT_PTR)DdeNameService(dde_inst, ddestr_iexplore, 0, DNS_REGISTER); > if(res != DMLERR_NO_ERROR) > WARN("DdeNameService failed: %u\n", res); > } thanks bye michael

Aug. 31, 2011