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

Re: [7/7] wsdapi/tests: Add tests for Get/SetSockaddr.

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=31269 Your paranoid android. === build (build) === Patch failed to apply

May 30, 2017

Re: [6/7] wsdapi/tests: Add tests for Get/SetMessageType.

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=31268 Your paranoid android. === build (build) === Patch failed to apply

May 30, 2017

Re: [5/7] wsdapi/tests: Add tests for Get/SetPort.

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=31267 Your paranoid android. === build (build) === Patch failed to apply

May 30, 2017

Re: [4/7] wsdapi/tests: Add tests for Get/SetTransportAddress.

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=31266 Your paranoid android. === build (build) === Patch failed to apply

May 30, 2017

Re: [3/7] wsdapi/tests: Add WSDCreateUdpAddress 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=31265 Your paranoid android. === build (build) === Patch failed to apply

May 30, 2017

Re: [PATCH 7/7] regedit: Do not get text for items with an index less than one

by Alexandre Julliard

Hugh McMaster <hugh.mcmaster(a)outlook.com> writes: > @@ -59,7 +59,7 @@ LPWSTR GetItemText(HWND hwndLV, UINT item) > { > LPWSTR newStr, curStr; > unsigned int maxLen = 128; > - if (item == 0) return NULL; /* first item is ALWAYS a default */ > + if (item < 1) return NULL; /* first item is ALWAYS a default */ How is this different? -- Alexandre Julliard julliard(a)winehq.org

May 30, 2017

Re: [PATCH 2/7] msvcrt: Clean up registered C++ object in handler.

by Piotr Caban

On 05/29/17 21:56, Piotr Caban wrote: > On 05/24/17 02:55, Daniel Lehman wrote: >> +static DWORD cxx_catch_cleanup(EXCEPTION_RECORD *rec, >> EXCEPTION_REGISTRATION_RECORD *frame, >> + CONTEXT *context, >> EXCEPTION_REGISTRATION_RECORD **pdispatcher) >> +{ >> + if (rec->ExceptionFlags & (EH_UNWINDING | EH_EXIT_UNWIND)) >> + { >> + thread_data_t *data = msvcrt_get_thread_data(); >> + frame_info *cur; >> + >> + if (cxx_is_consolidate(rec)) > Is this condition really needed? Shouldn't we clean the object no matter > what's the reason of unwind? > >> + { >> + rec = (void*)rec->ExceptionInformation[4]; >> + >> + for (cur = data->frame_info_head; cur; cur = cur->next) >> + { >> + if ((ULONG64)cur <= (ULONG64)frame) > This condition is not working. It's making assumption about order of > catch_frame and frame_info variables on stack while they are declared > this way: >> + EXCEPTION_REGISTRATION_RECORD catch_frame; >> cxx_frame_info frame_info; > Shouldn't the cxx_catch_cleanup just unregister the object that was > registered in call_catch_block? Here's a test case that demonstrate the problem with cur <= frame comparison: try { try { int *p = NULL; *p = 0x42; } catch (klass x) { throw 1; } } catch (int i) { } try { throw 1; } catch(...) {} Cheers, Piotr

May 30, 2017

Re: [PATCH v7 10/26] x86/insn-eval: Add utility functions to get segment selector

by Borislav Petkov

On Fri, May 05, 2017 at 11:17:08AM -0700, Ricardo Neri wrote: > When computing a linear address and segmentation is used, we need to know > the base address of the segment involved in the computation. In most of > the cases, the segment base address will be zero as in USER_DS/USER32_DS. > However, it may be possible that a user space program defines its own > segments via a local descriptor table. In such a case, the segment base > address may not be zero .Thus, the segment base address is needed to > calculate correctly the linear address. > > The segment selector to be used when computing a linear address is > determined by either any of segment override prefixes in the > instruction or inferred from the registers involved in the computation of > the effective address; in that order. Also, there are cases when the > overrides shall be ignored (code segments are always selected by the CS > segment register; string instructions always use the ES segment register > along with the EDI register). > > For clarity, this process can be split into two steps: resolving the > relevant segment register to use and, once known, read its value to > obtain the segment selector. > > The method to obtain the segment selector depends on several factors. In > 32-bit builds, segment selectors are saved into the pt_regs structure > when switching to kernel mode. The same is also true for virtual-8086 > mode. In 64-bit builds, segmentation is mostly ignored, except when > running a program in 32-bit legacy mode. In this case, CS and SS can be > obtained from pt_regs. DS, ES, FS and GS can be read directly from > the respective segment registers. > > Lastly, the only two segment registers that are not ignored in long mode > are FS and GS. In these two cases, base addresses are obtained from the > respective MSRs. > > Cc: Dave Hansen <dave.hansen(a)linux.intel.com> > Cc: Adam Buchbinder <adam.buchbinder(a)gmail.com> > Cc: Colin Ian King <colin.king(a)canonical.com> > Cc: Lorenzo Stoakes <lstoakes(a)gmail.com> > Cc: Qiaowei Ren <qiaowei.ren(a)intel.com> > Cc: Arnaldo Carvalho de Melo <acme(a)redhat.com> > Cc: Masami Hiramatsu <mhiramat(a)kernel.org> > Cc: Adrian Hunter <adrian.hunter(a)intel.com> > Cc: Kees Cook <keescook(a)chromium.org> > Cc: Thomas Garnier <thgarnie(a)google.com> > Cc: Peter Zijlstra <peterz(a)infradead.org> > Cc: Borislav Petkov <bp(a)suse.de> > Cc: Dmitry Vyukov <dvyukov(a)google.com> > Cc: Ravi V. Shankar <ravi.v.shankar(a)intel.com> > Cc: x86(a)kernel.org > Signed-off-by: Ricardo Neri <ricardo.neri-calderon(a)linux.intel.com> > --- > arch/x86/lib/insn-eval.c | 256 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 256 insertions(+) > > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c > index 1634762..0a496f4 100644 > --- a/arch/x86/lib/insn-eval.c > +++ b/arch/x86/lib/insn-eval.c > @@ -9,6 +9,7 @@ > #include <asm/inat.h> > #include <asm/insn.h> > #include <asm/insn-eval.h> > +#include <asm/vm86.h> > > enum reg_type { > REG_TYPE_RM = 0, > @@ -33,6 +34,17 @@ enum string_instruction { > SCASW_SCASD = 0xaf, > }; > > +enum segment_register { > + SEG_REG_INVAL = -1, > + SEG_REG_IGNORE = 0, > + SEG_REG_CS = 0x23, > + SEG_REG_SS = 0x36, > + SEG_REG_DS = 0x3e, > + SEG_REG_ES = 0x26, > + SEG_REG_FS = 0x64, > + SEG_REG_GS = 0x65, > +}; Yuck, didn't we talk about this already? Those are segment override prefixes so call them as such. #define SEG_OVR_PFX_CS 0x23 #define SEG_OVR_PFX_SS 0x36 ... and we already have those! arch/x86/include/asm/inat.h: ... #define INAT_PFX_CS 5 /* 0x2E */ #define INAT_PFX_DS 6 /* 0x3E */ #define INAT_PFX_ES 7 /* 0x26 */ #define INAT_PFX_FS 8 /* 0x64 */ #define INAT_PFX_GS 9 /* 0x65 */ #define INAT_PFX_SS 10 /* 0x36 */ well, kinda, they're numbers there and not the actual prefix values. And then there's: arch/x86/kernel/uprobes.c::is_prefix_bad() which looks at some of those. Please add your defines to inat.h and make that function is_prefix_bad() use them instead of naked numbers. We need to pay attention to all those different things needing to look at insn opcodes and not let them go unwieldy by each defining and duplicating stuff. > /** > * is_string_instruction - Determine if instruction is a string instruction > * @insn: Instruction structure containing the opcode > @@ -83,6 +95,250 @@ static bool is_string_instruction(struct insn *insn) > } > } > > +/** > + * resolve_seg_register() - obtain segment register That function is still returning the segment override prefix and we use *that* to determine the segment register. > + * @insn: Instruction structure with segment override prefixes > + * @regs: Structure with register values as seen when entering kernel mode > + * @regoff: Operand offset, in pt_regs, used to deterimine segment register > + * > + * The segment register to which an effective address refers depends on > + * a) whether segment override prefixes must be ignored: always use CS when > + * the register is (R|E)IP; always use ES when operand register is (E)DI with > + * string instructions as defined in the Intel documentation. b) If segment > + * overrides prefixes are used in the instruction instruction prefixes. C) Use > + * the default segment register associated with the operand register. > + * > + * The operand register, regoff, is represented as the offset from the base of > + * pt_regs. Also, regoff can be -EDOM for cases in which registers are not > + * used as operands (e.g., displacement-only memory addressing). > + * > + * This function returns the segment register as value from an enumeration > + * as per the conditions described above. Please note that this function > + * does not return the value in the segment register (i.e., the segment > + * selector). The segment selector needs to be obtained using > + * get_segment_selector() and passing the segment register resolved by > + * this function. > + * > + * Return: Enumerated segment register to use, among CS, SS, DS, ES, FS, GS, > + * ignore (in 64-bit mode as applicable), or -EINVAL in case of error. > + */ > +static enum segment_register resolve_seg_register(struct insn *insn, > + struct pt_regs *regs, > + int regoff) > +{ > + int i; > + int sel_overrides = 0; > + int seg_register = SEG_REG_IGNORE; > + > + if (!insn) > + return SEG_REG_INVAL; > + > + /* First handle cases when segment override prefixes must be ignored */ > + if (regoff == offsetof(struct pt_regs, ip)) { > + if (user_64bit_mode(regs)) > + return SEG_REG_IGNORE; > + else > + return SEG_REG_CS; > + return SEG_REG_CS; Simplify: if (user_64bit_mode(regs)) return SEG_REG_IGNORE; return SEG_REG_CS; > + } > + > + /* > + * If the (E)DI register is used with string instructions, the ES > + * segment register is always used. > + */ > + if ((regoff == offsetof(struct pt_regs, di)) && > + is_string_instruction(insn)) { > + if (user_64bit_mode(regs)) > + return SEG_REG_IGNORE; > + else > + return SEG_REG_ES; > + return SEG_REG_CS; What is that second return actually supposed to do? > + } > + > + /* Then check if we have segment overrides prefixes*/ Missing space and fullstop: "... overrides prefixes. */" > + for (i = 0; i < insn->prefixes.nbytes; i++) { > + switch (insn->prefixes.bytes[i]) { > + case SEG_REG_CS: > + seg_register = SEG_REG_CS; > + sel_overrides++; > + break; > + case SEG_REG_SS: > + seg_register = SEG_REG_SS; > + sel_overrides++; > + break; > + case SEG_REG_DS: > + seg_register = SEG_REG_DS; > + sel_overrides++; > + break; > + case SEG_REG_ES: > + seg_register = SEG_REG_ES; > + sel_overrides++; > + break; > + case SEG_REG_FS: > + seg_register = SEG_REG_FS; > + sel_overrides++; > + break; > + case SEG_REG_GS: > + seg_register = SEG_REG_GS; > + sel_overrides++; > + break; > + default: > + return SEG_REG_INVAL; So SEG_REG_NONE or so? It is not invalid if it is not a segment override prefix. > + /* > + * Having more than one segment override prefix leads to undefined > + * behavior. If this is the case, return with error. > + */ > + if (sel_overrides > 1) > + return SEG_REG_INVAL; Yuck, wrapping of -E value in a SEG_REG enum. Just return -EINVAL here and make the function return an int, not that ugly enum. And the return convention should be straight-forward: default segment if no prefix or ignored, -EINVAL if error and the actual override prefix if present. Also, that test should be *after* the user_64bit_mode() because in long mode, segment overrides get ignored. IOW, those three if-tests around here should be combined into a single one, i.e., something like this: if (64-bit) { if (!FS || !GS) ignore else return seg_override_pfx; <--- Yes, that variable should be called seg_override_pfx to denote what it is. } else if (sel_overrides > 1) -EINVAL else if (sel_overrides) return seg_override_pfx; > + > + if (sel_overrides == 1) { > + /* > + * If in long mode all segment registers but FS and GS are > + * ignored. > + */ > + if (user_64bit_mode(regs) && !(seg_register == SEG_REG_FS || > + seg_register == SEG_REG_GS)) > + return SEG_REG_IGNORE; > + > + return seg_register; > + } > + > + /* In long mode, all segment registers except FS and GS are ignored */ > + if (user_64bit_mode(regs)) > + return SEG_REG_IGNORE; > + > + /* > + * Lastly, if no segment overrides were found, determine the default > + * segment register as described in the Intel documentation: SS for > + * (E)SP or (E)BP. DS for all data references, AX, CX and DX are not > + * valid register operands in 16-bit address encodings. > + * -EDOM is reserved to identify for cases in which no register is used > + * the default segment register (displacement-only addressing). The > + * default segment register used in these cases is DS. > + */ > + > + switch (regoff) { > + case offsetof(struct pt_regs, ax): > + /* fall through */ > + case offsetof(struct pt_regs, cx): > + /* fall through */ > + case offsetof(struct pt_regs, dx): > + if (insn && insn->addr_bytes == 2) > + return SEG_REG_INVAL; > + case offsetof(struct pt_regs, di): > + /* fall through */ > + case -EDOM: > + /* fall through */ > + case offsetof(struct pt_regs, bx): > + /* fall through */ > + case offsetof(struct pt_regs, si): > + return SEG_REG_DS; > + case offsetof(struct pt_regs, bp): > + /* fall through */ > + case offsetof(struct pt_regs, sp): > + return SEG_REG_SS; > + case offsetof(struct pt_regs, ip): > + return SEG_REG_CS; > + default: > + return SEG_REG_INVAL; > + } So group all the fall through cases together so that you don't have this dense block of code with "/* fall through */" on every other line. -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --

May 30, 2017

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

by Paul Gofman

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. > > The "Merging" one is fine, I'm just a tiny bit annoyed by the "Merged > 0 child" case. BTW merging "0" technically never happens, merging "1" means that it actually merging nothing. I will add another TRACE message for this case. Ironically I had that before some last edits, but considered a single unified TRACE message nicer than more code branching and different TRACEs.

May 30, 2017

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

by Paul Gofman

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.

May 30, 2017