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

Re: [PATCH 6/6] d3dx9: Merge constant setting for child parameters when possible.

by Matteo Bruni

2017-05-30 10:01 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > On 05/30/2017 02:03 AM, Matteo Bruni wrote: >> >> 2017-05-29 20:44 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: >>> >>> On 05/29/2017 09:24 PM, Matteo Bruni wrote: >>>> >>>> 2017-05-24 11:46 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: >>>> >>>>> + if (!(const_set->table == current_table && >>>>> current_start_offset == start_offset >>>>> + && const_set->direct_copy == >>>>> first_const->direct_copy >>>>> + && current_data == const_set->param->data >>>>> + && (const_set->direct_copy || >>>>> (first_const->param->type == const_set->param->type >>>>> + && first_const->param->class == >>>>> const_set->param->class >>>>> + && first_const->param->columns == >>>>> const_set->param->columns >>>>> + && first_const->param->rows == >>>>> const_set->param->rows >>>>> + && first_const->register_count == >>>>> const_set->register_count >>>>> + && (i == const_tab->const_set_count - 1 >>>>> + || first_const->param->element_count == >>>>> const_set->param->element_count))))) >>>>> + { >>>>> + TRACE("direct_copy %u, i %u, index %u, param %s.%s, >>>>> current_data %p, const_set->param->data %p, "\ >>>>> + "current_start_offset %u, start_offset %u, >>>>> const_set->table %u, current_table %u.\n", >>>>> + const_set->direct_copy, i, index, >>>>> debugstr_a(param->name), >>>>> + debugstr_a(const_set->param->name), >>>>> current_data, const_set->param->data, >>>>> + current_start_offset, start_offset, >>>>> const_set->table, current_table); >>>>> + break; >>>>> + } >>>> >>>> This looks like a debug trace. >>>> >>>>> + TRACE("Merging %u child parameters for %s, not merging %u, >>>>> direct_copy %u.\n", i - index, >>>>> + debugstr_a(param->name), const_tab->const_set_count - >>>>> i, >>>>> first_const->direct_copy); >>>> >>>> I guess it's intentional that you're printing this even when not >>>> merging anything, in that case the TRACE sounds a bit awkward though. >>>> Maybe have a separate TRACE for that case? >>> >>> Both traces are to have a possibility to check from trace log what is >>> merged >>> and not "merged" and why, for performance analysis. If you think it is >>> more >>> of debug I can remove it, though since it is in initialization only I was >>> thinking it does not add too much noise. >> >> The first trace feels pretty noisy to me for upstream Wine, do you >> think it would be useful in a trace attached on a generic bug to >> figure out potential performance improvements? > > I will remove it. Anyway, I can recover the same information from the other > present TRACE data, just harder. I don't think this particular message alone > worth putting as a "performance debugging" patch. If to do some performance > analysis remotely, I would start from time measurements dumping in FIXMEs, > though I don't have any more or less universal patch for that now apart from > timing BeginPass / CommitChanges. I did inner functions timing at some > points but adding that for all possible functions of interest together is > not feasible as it does biases the outer functions measurements. If to think > of a general patch it probably should have switches of what functions to > profile, or just rotate that switches on the fly. Yeah, my point (which admittedly didn't quite come out clearly in my previous email) is that there is no easy and generic patch which would help with remote performance debugging. There is always going to be some back and forth with debug patches specific to the case at hand and generally it's going to be a PITA to work on...

May 31, 2017

Re: [v2 2/6] d3dx9: Factor out get_const_upload_info() function.

by Matteo Bruni

2017-05-30 11:38 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> > --- > v2: > - renamed 'n' to 'major_count'; major_count is still a bit meh, since "major" is really already a count and there is an unspecified "count" too. I don't have any good suggestion and this is an improvement over the current naming though, so...

May 31, 2017

Re: [PATCH 1/6] d3dx9: Get rid of table lookup for converting between register indexes and offsets.

by Matteo Bruni

2017-05-30 9:51 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > On 05/30/2017 01:59 AM, Matteo Bruni wrote: >> >> >>>>> @@ -1033,9 +1035,9 @@ static void set_constants(struct d3dx_regstore >>>>> *rs, >>>>> struct d3dx_const_tab *const >>>>> unsigned int offset; >>>>> >>>>> offset = start_offset + i * major_stride + j; >>>>> - if (offset / table_info[table].reg_component_count >= >>>>> rs->table_sizes[table]) >>>>> + if (get_reg_offset(table, offset) >= >>>>> rs->table_sizes[table]) >>>>> { >>>>> - if (table_info[table].reg_component_count != 1) >>>>> + if (table != PRES_REGTAB_OBCONST) >>>>> FIXME("Output offset exceeds table size, >>>>> name >>>>> %s, component %u.\n", >>>>> debugstr_a(param->name), i); >>>>> break; >>>> >>>> Not new and probably I just forgot the details, but, why would the >>>> register component count matter for the FIXME? >>> >>> The length of the data to be copied to registers can be limited by >>> RegisterCount in (pre)shader constant description. But when register is >>> 4-value, the major count for output loop should already mind that (since >>> we >>> always have row count and column count <= 4). The only legitimate case we >>> may need to break from the inner loop is boolean constant matrix setting. >>> So >>> actually fixme condition should never happen, though break from the inner >>> loop on boolean constant setting may be required. I am changing this >>> place >>> in the next patches (I didn't send yet) when optimizing this place. I was >>> actually going to add a bit more tests for this case along with the >>> forthcoming changes, and I suspect a bug here (I actually need to >>> effectively compare with constant register count and not with the table >>> size). We have a test for full update with boolean matrices (which will >>> not >>> trigger the error here) but commit of individual matrices test probably >>> will. >> >> Right, that makes sense, including the likely bug you mention. >> >> In theory it should be possible to do the FIXME check at init time. >> Actually, the whole constant upload info could be precomputed. Just >> putting it out there... >> > My plan was actually to get rid of this FIXME at all (it is actually more of > a debug assertion rather than true FIXME which seems redundant now), and > besides introduce more code paths for matrix settings, like e. g. the cases > when output stride is the same as input. I strictly want to avoid any 'if' > in the inner loops at least. Ideally I would maybe like to end up with > strides values specialized loops for possible matrix sizes, but not sure yet > if it worth it. Regarding precomputing matrix strides / counts, I tried to > store them all in const_set structure (after the optimizations I mentioned), > but did not see any improvement. It takes quite a lot of fields and probably > getting them on the fly takes not more time than adding much more data to > const_set. Makes sense.

May 31, 2017

Re: [PATCH] user32: Keep tracking if we just created the menu.

by Zebediah Figura

On 05/31/2017 03:30 PM, Huw Davies wrote: > On Wed, May 31, 2017 at 02:34:06PM -0500, Zebediah Figura wrote: >> Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com> >> --- >> dlls/user32/menu.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/dlls/user32/menu.c b/dlls/user32/menu.c >> index a65f01eaa5..c52b723f76 100644 >> --- a/dlls/user32/menu.c >> +++ b/dlls/user32/menu.c >> @@ -2597,7 +2597,10 @@ static BOOL MENU_ButtonDown( MTRACKER* pmt, HMENU hPtMenu, UINT wFlags ) >> >> /* If the popup menu is not already "popped" */ >> if (!(ptmenu->items[pos].fState & MF_MOUSESELECT)) >> + { >> pmt->hCurrentMenu = MENU_ShowSubPopup( pmt->hOwnerWnd, hPtMenu, FALSE, wFlags ); >> + return TRUE; >> + } >> } >> >> /* A click on an item or anywhere on a popup keeps tracking going */ > > So under what conditions does the if statement directly below this > hunk not get triggered? > > Huw. > When we clicked outside the menu (hPtMenu == NULL), or when we clicked on the menu bar outside of a menu (hPtMenu != NULL, but ht == border && not a popup). I might be missing something, so if anything in particular looks problematic, please let me know.

May 31, 2017

Re: [PATCH] user32: Keep tracking if we just created the menu.

by Huw Davies

On Wed, May 31, 2017 at 02:34:06PM -0500, Zebediah Figura wrote: > Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com> > --- > dlls/user32/menu.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/dlls/user32/menu.c b/dlls/user32/menu.c > index a65f01eaa5..c52b723f76 100644 > --- a/dlls/user32/menu.c > +++ b/dlls/user32/menu.c > @@ -2597,7 +2597,10 @@ static BOOL MENU_ButtonDown( MTRACKER* pmt, HMENU hPtMenu, UINT wFlags ) > > /* If the popup menu is not already "popped" */ > if (!(ptmenu->items[pos].fState & MF_MOUSESELECT)) > + { > pmt->hCurrentMenu = MENU_ShowSubPopup( pmt->hOwnerWnd, hPtMenu, FALSE, wFlags ); > + return TRUE; > + } > } > > /* A click on an item or anywhere on a popup keeps tracking going */ So under what conditions does the if statement directly below this hunk not get triggered? Huw.

May 31, 2017

Re: [PATCH v7 12/26] x86/insn-eval: Add utility functions to get segment descriptor base address and limit

by Borislav Petkov

On Fri, May 05, 2017 at 11:17:10AM -0700, Ricardo Neri wrote: > With segmentation, the base address of the segment descriptor is needed > to compute a linear address. The segment descriptor used in the address > computation depends on either any segment override prefixes in the > instruction or the default segment determined by the registers involved > in the address computation. Thus, both the instruction as well as the > register (specified as the offset from the base of pt_regs) are given as > inputs, along with a boolean variable to select between override and > default. ... > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c > index f46cb31..c77ed80 100644 > --- a/arch/x86/lib/insn-eval.c > +++ b/arch/x86/lib/insn-eval.c > @@ -476,6 +476,133 @@ static struct desc_struct *get_desc(unsigned short sel) > } > > /** > + * insn_get_seg_base() - Obtain base address of segment descriptor. > + * @regs: Structure with register values as seen when entering kernel mode > + * @insn: Instruction structure with selector override prefixes > + * @regoff: Operand offset, in pt_regs, of which the selector is needed > + * > + * Obtain the base address of the segment descriptor as indicated by either > + * any segment override prefixes contained in insn or the default segment > + * applicable to the register indicated by regoff. regoff is specified as the > + * offset in bytes from the base of pt_regs. > + * > + * Return: In protected mode, base address of the segment. Zero in for long > + * mode, except when FS or GS are used. In virtual-8086 mode, the segment > + * selector shifted 4 positions to the right. -1L in case of > + * error. > + */ > +unsigned long insn_get_seg_base(struct pt_regs *regs, struct insn *insn, > + int regoff) > +{ > + struct desc_struct *desc; > + unsigned short sel; > + enum segment_register seg_reg; > + > + seg_reg = resolve_seg_register(insn, regs, regoff); > + if (seg_reg == SEG_REG_INVAL) > + return -1L; > + > + sel = get_segment_selector(regs, seg_reg); > + if ((short)sel < 0) I guess it would be better if that function returned a signed short so you don't have to cast it here. (You're casting it to an unsigned long below anyway.) > + return -1L; > + > + if (v8086_mode(regs)) > + /* > + * Base is simply the segment selector shifted 4 > + * positions to the right. > + */ > + return (unsigned long)(sel << 4); > + ... > +static unsigned long get_seg_limit(struct pt_regs *regs, struct insn *insn, > + int regoff) > +{ > + struct desc_struct *desc; > + unsigned short sel; > + unsigned long limit; > + enum segment_register seg_reg; > + > + seg_reg = resolve_seg_register(insn, regs, regoff); > + if (seg_reg == SEG_REG_INVAL) > + return 0; > + > + sel = get_segment_selector(regs, seg_reg); > + if ((short)sel < 0) Ditto. > + return 0; > + > + if (user_64bit_mode(regs) || v8086_mode(regs)) > + return -1L; > + > + if (!sel) > + return 0; > + > + desc = get_desc(sel); > + if (!desc) > + return 0; > + > + /* > + * If the granularity bit is set, the limit is given in multiples > + * of 4096. When the granularity bit is set, the least 12 significant the 12 least significant bits > + * bits are not tested when checking the segment limits. In practice, > + * this means that the segment ends in (limit << 12) + 0xfff. > + */ > + limit = get_desc_limit(desc); > + if (desc->g) > + limit <<= 12 | 0x7; That 0x7 doesn't look like 0xfff - it shifts limit by 15 instead. You can simply write it like you mean it: limit = (limit << 12) + 0xfff; -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --

May 31, 2017

Re: [PATCH 3/4] quartz: fix crash in FilterGraph2_Render

by Andrew Eikum

Can you add a test for this? Andrew On Thu, May 25, 2017 at 04:54:49PM +0200, Miklós Máté wrote: > Note: there are a lot of other places as well where the null pointer handling > is present in the msdn docs but missing in wine. > > Signed-off-by: Miklós Máté <mtmkls(a)gmail.com> > --- > dlls/quartz/filtergraph.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/dlls/quartz/filtergraph.c b/dlls/quartz/filtergraph.c > index a143b05d83..e26eb199c8 100644 > --- a/dlls/quartz/filtergraph.c > +++ b/dlls/quartz/filtergraph.c > @@ -1279,6 +1279,9 @@ static HRESULT WINAPI FilterGraph2_Render(IFilterGraph2 *iface, IPin *ppinOut) > > TRACE("(%p/%p)->(%p)\n", This, iface, ppinOut); > > + if (!ppinOut) > + return E_POINTER; > + > if (TRACE_ON(quartz)) > { > PIN_INFO PinInfo; > -- > 2.11.0 > > >

May 31, 2017

Re: [PATCH 1/4] quartz: AsyncReader is always expected to have output pin

by Andrew Eikum

Thanks for working on this. I tested your patch sequence against a number of games as well as embedding videos in MSOffice 2007 and 2010 and found no regressions. I do have some comments, though, in-line below. Is this something you could write a test for? E.g. create the filter and then query for its output pin immediately after creation. On Thu, May 25, 2017 at 04:54:47PM +0200, Miklós Máté wrote: > Even between create() and Load(). This fixes a crash in DSPlayer. > > Signed-off-by: Miklós Máté <mtmkls(a)gmail.com> > --- > dlls/quartz/filesource.c | 31 ++++++++++++++++++++----------- > 1 file changed, 20 insertions(+), 11 deletions(-) > > diff --git a/dlls/quartz/filesource.c b/dlls/quartz/filesource.c > index ed7e5ee201..200463ff5b 100644 > --- a/dlls/quartz/filesource.c > +++ b/dlls/quartz/filesource.c > @@ -73,7 +73,8 @@ static const IFileSourceFilterVtbl FileSource_Vtbl; > static const IAsyncReaderVtbl FileAsyncReader_Vtbl; > static const IAMFilterMiscFlagsVtbl IAMFilterMiscFlags_Vtbl; > > -static HRESULT FileAsyncReader_Construct(HANDLE hFile, IBaseFilter * pBaseFilter, LPCRITICAL_SECTION pCritSec, IPin ** ppPin); > +static HRESULT FileAsyncReader_Construct(IBaseFilter * pBaseFilter, LPCRITICAL_SECTION pCritSec, IPin ** ppPin); > +static HRESULT FileAsyncReader_SetFile(IAsyncReader * iface, HANDLE hFile); > > static const WCHAR mediatype_name[] = { > 'M', 'e', 'd', 'i', 'a', ' ', 'T', 'y', 'p', 'e', 0 }; > @@ -416,6 +417,7 @@ static const BaseFilterFuncTable BaseFuncTable = { > HRESULT AsyncReader_create(IUnknown * pUnkOuter, LPVOID * ppv) > { > AsyncReader *pAsyncRead; > + HRESULT hr; > > if( pUnkOuter ) > return CLASS_E_NOAGGREGATION; > @@ -429,7 +431,8 @@ HRESULT AsyncReader_create(IUnknown * pUnkOuter, LPVOID * ppv) > > pAsyncRead->IFileSourceFilter_iface.lpVtbl = &FileSource_Vtbl; > pAsyncRead->IAMFilterMiscFlags_iface.lpVtbl = &IAMFilterMiscFlags_Vtbl; > - pAsyncRead->pOutputPin = NULL; > + hr = FileAsyncReader_Construct(&pAsyncRead->filter.IBaseFilter_iface, &pAsyncRead->filter.csFilter, &pAsyncRead->pOutputPin); > + BaseFilterImpl_IncrementPinVersion(&pAsyncRead->filter); If this fails, I think we should free resources and return the error immediately. > > pAsyncRead->pszFileName = NULL; > pAsyncRead->pmt = NULL; > @@ -438,7 +441,7 @@ HRESULT AsyncReader_create(IUnknown * pUnkOuter, LPVOID * ppv) > > TRACE("-- created at %p\n", pAsyncRead); > > - return S_OK; > + return hr; > } > > /** IUnknown methods **/ > @@ -606,7 +609,7 @@ static ULONG WINAPI FileSource_Release(IFileSourceFilter * iface) > > static HRESULT WINAPI FileSource_Load(IFileSourceFilter * iface, LPCOLESTR pszFileName, const AM_MEDIA_TYPE * pmt) > { > - HRESULT hr; > + HRESULT hr = E_FAIL; > HANDLE hFile; > IAsyncReader * pReader = NULL; > AsyncReader *This = impl_from_IFileSourceFilter(iface); > @@ -625,16 +628,15 @@ static HRESULT WINAPI FileSource_Load(IFileSourceFilter * iface, LPCOLESTR pszFi > return HRESULT_FROM_WIN32(GetLastError()); > } > > - /* create pin */ > - hr = FileAsyncReader_Construct(hFile, &This->filter.IBaseFilter_iface, &This->filter.csFilter, &This->pOutputPin); > - BaseFilterImpl_IncrementPinVersion(&This->filter); > > - if (SUCCEEDED(hr)) > + if (This->pOutputPin) Since we now create the output pin during filter creation, this should never be non-NULL, right? > hr = IPin_QueryInterface(This->pOutputPin, &IID_IAsyncReader, (LPVOID *)&pReader); > > /* store file name & media type */ > if (SUCCEEDED(hr)) > { > + FileAsyncReader_SetFile(pReader, hFile); > + > CoTaskMemFree(This->pszFileName); > if (This->pmt) > FreeMediaType(This->pmt); > @@ -939,7 +941,7 @@ static const BaseOutputPinFuncTable output_BaseOutputFuncTable = { > BaseOutputPinImpl_BreakConnect > }; > > -static HRESULT FileAsyncReader_Construct(HANDLE hFile, IBaseFilter * pBaseFilter, LPCRITICAL_SECTION pCritSec, IPin ** ppPin) > +static HRESULT FileAsyncReader_Construct(IBaseFilter * pBaseFilter, LPCRITICAL_SECTION pCritSec, IPin ** ppPin) > { > PIN_INFO piOutput; > HRESULT hr; > @@ -952,9 +954,8 @@ static HRESULT FileAsyncReader_Construct(HANDLE hFile, IBaseFilter * pBaseFilter > > if (SUCCEEDED(hr)) > { > - FileAsyncReader *pPinImpl = (FileAsyncReader *)*ppPin; > + FileAsyncReader *pPinImpl = impl_from_IPin(*ppPin); > pPinImpl->IAsyncReader_iface.lpVtbl = &FileAsyncReader_Vtbl; > - pPinImpl->hFile = hFile; > pPinImpl->bFlushing = FALSE; > pPinImpl->sample_list = NULL; > pPinImpl->handle_list = NULL; > @@ -965,6 +966,14 @@ static HRESULT FileAsyncReader_Construct(HANDLE hFile, IBaseFilter * pBaseFilter > return hr; > } > > +static HRESULT FileAsyncReader_SetFile(IAsyncReader * iface, HANDLE hFile) > +{ > + FileAsyncReader *This = impl_from_IAsyncReader(iface); > + > + This->hFile = hFile; > + return S_OK; > +} > + Why is this a separate function instead of just setting hFile in FileSource_Load? > /* IAsyncReader */ > > static HRESULT WINAPI FileAsyncReader_QueryInterface(IAsyncReader * iface, REFIID riid, LPVOID * ppv) > -- > 2.11.0 > > >

May 31, 2017

Re: PATCH 1/7] msvcrt: Translate SEH exception on each call to __CxxFrameHandler.

by Piotr Caban

On 05/24/17 02:54, Daniel Lehman wrote: > +extern void __wine_rtl_unwind( EXCEPTION_REGISTRATION_RECORD* frame, EXCEPTION_RECORD *record, > + void (*target)(void) ); It's already defined in wine/exception.h. > +extern void CDECL __DestructExceptionObject(EXCEPTION_RECORD *rec); This is also already defined (in msvcrt.h) > + > +static void DECLSPEC_NORETURN seh_unwind_target(void) > +{ > + seh_frame_info *seh_frame = (seh_frame_info *)__wine_get_frame(); > + __wine_pop_frame( &seh_frame->frame ); There's no need to call __wine_pop_frame here, it will be done after the jump anyway. I think that the whole series will be cleaner/easier to read if we add the __EXCEPT_CTX/__FINALLY_CTX macros and use them here. I'm attaching a patch with proposition of changes for this functions. With such changes your patch will look like this (this is not tested): typedef struct { ULONG64 dest_frame; ULONG64 orig_frame; DISPATCHER_CONTEXT *dispatch; const cxx_function_descr *descr; } se_translator_ctx; static DWORD se_translation_filter(EXCEPTION_POINTERS *ep, void *c) { se_translator_ctx *ctx = (se_translator_ctx*)c; /* this is copied from original patch but I think that */ /* terminate should be called on non C++ exceptions here */ if (ep->rec->ExceptionCode != CXX_EXCEPTION) return ExceptionContinueSearch; find_catch_block(...); __DestructExceptionObject(ep->rec); return ExceptionExecuteHandler; } in cxx_frame_handler: if (data->se_translator) { se_translator_ctx ctx; //initialize ctx __TRY { except_ptrs.ExceptionRecord = rec; except_ptrs.ContextRecord = context; data->se_translator(rec->ExceptionCode, &except_ptrs); } __EXCEPT_CTX(se_translation_filter, &ctx) { } __END_TRY } With such changes second patch can be also changed to use __TRY/__FINALLY_CTX around "ret_addr = handler(0, frame);" file. What do you think about it? Thanks, Piotr

May 31, 2017

Re: [PATCH 3/6] d3d9: Validate adapter format in CheckDeviceFormat().

by Józef Kucia

On Wed, May 31, 2017 at 11:55 AM, Marvin <testbot(a)winehq.org> 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=31280 > > Your paranoid android. > > > === wvistau64 (32 bit device) === > device.c:11206: Test failed: Got unexpected hr 0, device type 0x4. > device.c:11209: Test failed: Got unexpected hr 0, device type 0x4. > device.c:11212: Test failed: Got unexpected hr 0, device type 0x4. > device.c:11215: Test failed: Got unexpected hr 0, device type 0x4. The test is wrong for NULLREF device. The rest of the series should be fine.

May 31, 2017