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

Re: [PATCH 1/5] d3dx9/tests: Get rid of compare_rotation.

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

April 26, 2017

Re: [v6 PATCH 06/21] x86/insn-eval: Add utility functions to get segment selector

by Ricardo Neri

On Wed, 2017-04-26 at 13:44 -0700, Ricardo Neri wrote: > > > > > + */ > > > + for (i = 0; i < insn->prefixes.nbytes; i++) { > > > + switch (insn->prefixes.bytes[i]) { > > > + case SEG_CS: > > > + return SEG_CS; > > > + case SEG_SS: > > > + return SEG_SS; > > > + case SEG_DS: > > > + return SEG_DS; > > > + case SEG_ES: > > > + return SEG_ES; > > > + case SEG_FS: > > > + return SEG_FS; > > > + case SEG_GS: > > > + return SEG_GS; > > > > So what happens if you're in 64-bit mode and you have CS, DS, ES, or > SS? > > Or is this what @get_default is supposed to do? But it doesn't look > like > > it, it still returns segments ignored in 64-bit mode. > > I regard that the role of this function is to obtain the the segment > selector from either of the prefixes or inferred from the operands. It > is the role of caller to determine if the segment selector should be > ignored. So far the only caller is insn_get_seg_base() [1]. If in long > mode, the segment base address is regarded as 0 unless the segment > selector is FS or GS. > > > > > + default: > > > + return -EINVAL; > > > + } > > > + } > > > + > > > +default_seg: > > > + /* > > > + * If no overrides, use default selectors as described in the > > > + * Intel documentation: SS for ESP or EBP. DS for all data > references, > > > + * except when relative to stack or string destination. > > > + * Also, AX, CX and DX are not valid register operands in > 16-bit > > > + * address encodings. > > > + * Callers must interpret the result correctly according to > the type > > > + * of instructions (e.g., use ES for string instructions). > > > + * Also, some values of modrm and sib might seem to indicate > the use > > > + * of EBP and ESP (e.g., modrm_mod = 0, modrm_rm = 5) but > actually > > > + * they refer to cases in which only a displacement used. > These cases > > > + * should be indentified by the caller and not with this > function. > > > + */ > > > + 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 -EINVAL; > > > + case -EDOM: /* no register involved in address computation */ > > > + case offsetof(struct pt_regs, bx): > > > + /* fall through */ > > > + case offsetof(struct pt_regs, di): > > > + /* fall through */ > > > > return SEG_ES; > > > > ? > > I double-checked the latest version of the Intel Software Development > manual [2], in the table 3-5 in section 3.7.4 mentions that DS is > default segment for all data references, except string destinations. I > tested this code with the UMIP-protected instructions and whenever I > use > %edi the default segment is %ds. I forgot my references: [1]. https://lkml.org/lkml/2017/3/7/876 [2]. https://software.intel.com/en-us/articles/intel-sdm#combined

April 26, 2017

Re: [v6 PATCH 06/21] x86/insn-eval: Add utility functions to get segment selector

by Ricardo Neri

On Tue, 2017-04-18 at 11:42 +0200, Borislav Petkov wrote: > On Tue, Mar 07, 2017 at 04:32:39PM -0800, 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 select 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. > > > > For clarity, this process can be split into two steps: resolving the > > relevant segment and, once known, read the applicable 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 > > registers. > > > Lastly, segmentation is possible in 64-bit mode via FS and GS. > > I'd say "Lastly, the only two segment registers which are not ignored in > long mode are FS and GS." I will make this clarification. > > > In these two cases, base addresses are obtained from the relevant MSRs. > > s/relevant/respective/ Will clarify. > > > 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 | 195 +++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 195 insertions(+) > > > > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c > > index 78df1c9..8d45df8 100644 > > --- a/arch/x86/lib/insn-eval.c > > +++ b/arch/x86/lib/insn-eval.c > > @@ -8,6 +8,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, > > @@ -15,6 +16,200 @@ enum reg_type { > > REG_TYPE_BASE, > > }; > > > > +enum segment { > > + SEG_CS = 0x23, > > + SEG_SS = 0x36, > > + SEG_DS = 0x3e, > > + SEG_ES = 0x26, > > + SEG_FS = 0x64, > > + SEG_GS = 0x65 > > +}; > > + > > +/** > > + * resolve_seg_selector() - obtain segment selector > > + * @regs: Set of registers containing the segment selector > > That arg is gone. This came from one of my initial implementations. I will remove it. > > > + * @insn: Instruction structure with selector override prefixes > > + * @regoff: Operand offset, in pt_regs, of which the selector is needed > > + * @default: Resolve default segment selector (i.e., ignore overrides) > > + * > > + * The segment selector to which an effective address refers depends on > > + * a) segment selector overrides instruction prefixes or b) the operand > > + * register indicated in the ModRM or SiB byte. > > + * > > + * For case a), the function inspects any prefixes in the insn instruction; > > s/insn // In this case I meant "any prefixes in the insn structure". Probably it will make it more clear. > > > + * insn can be null to indicate that selector override prefixes shall be > > + * ignored. > > This is not what the code does: it returns -EINVAL when insn is NULL. This was the behavior in a previous implementation. I will update it. > > > This is useful when the use of prefixes is forbidden (e.g., > > + * obtaining the code selector). For case b), the operand register shall be > > + * represented as the offset from the base address of pt_regs. Also, regoff > > + * can be -EINVAL for cases in which registers are not used as operands (e.g., > > + * when the mod and r/m parts of the ModRM byte are 0 and 5, respectively). > > + * > > + * This function returns the segment selector to utilize as per the conditions > > + * described above. Please note that this functin does not return the value > > + * of the segment selector. The value of the segment selector needs to be > > + * obtained using get_segment_selector and passing the segment selector type > > + * resolved by this function. > > + * > > + * Return: Segment selector to use, among CS, SS, DS, ES, FS or GS. > > : negative value when... I will document this behavior. > > > + */ > > +static int resolve_seg_selector(struct insn *insn, int regoff, bool get_default) > > +{ > > + int i; > > + > > + if (!insn) > > + return -EINVAL; > > + > > + if (get_default) > > + goto default_seg; > > + /* > > + * Check first if we have selector overrides. Having more than > > + * one selector override leads to undefined behavior. We > > + * only use the first one and return > > Well, I'd return -EINVAL to catch that undefined behavior. Note in a > local var that I've already seen a seg reg and then if I see another > one, return -EINVAL. Sure. Will do. > > > + */ > > + for (i = 0; i < insn->prefixes.nbytes; i++) { > > + switch (insn->prefixes.bytes[i]) { > > + case SEG_CS: > > + return SEG_CS; > > + case SEG_SS: > > + return SEG_SS; > > + case SEG_DS: > > + return SEG_DS; > > + case SEG_ES: > > + return SEG_ES; > > + case SEG_FS: > > + return SEG_FS; > > + case SEG_GS: > > + return SEG_GS; > > So what happens if you're in 64-bit mode and you have CS, DS, ES, or SS? > Or is this what @get_default is supposed to do? But it doesn't look like > it, it still returns segments ignored in 64-bit mode. I regard that the role of this function is to obtain the the segment selector from either of the prefixes or inferred from the operands. It is the role of caller to determine if the segment selector should be ignored. So far the only caller is insn_get_seg_base() [1]. If in long mode, the segment base address is regarded as 0 unless the segment selector is FS or GS. > > > + default: > > + return -EINVAL; > > + } > > + } > > + > > +default_seg: > > + /* > > + * If no overrides, use default selectors as described in the > > + * Intel documentation: SS for ESP or EBP. DS for all data references, > > + * except when relative to stack or string destination. > > + * Also, AX, CX and DX are not valid register operands in 16-bit > > + * address encodings. > > + * Callers must interpret the result correctly according to the type > > + * of instructions (e.g., use ES for string instructions). > > + * Also, some values of modrm and sib might seem to indicate the use > > + * of EBP and ESP (e.g., modrm_mod = 0, modrm_rm = 5) but actually > > + * they refer to cases in which only a displacement used. These cases > > + * should be indentified by the caller and not with this function. > > + */ > > + 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 -EINVAL; > > + case -EDOM: /* no register involved in address computation */ > > + case offsetof(struct pt_regs, bx): > > + /* fall through */ > > + case offsetof(struct pt_regs, di): > > + /* fall through */ > > return SEG_ES; > > ? I double-checked the latest version of the Intel Software Development manual [2], in the table 3-5 in section 3.7.4 mentions that DS is default segment for all data references, except string destinations. I tested this code with the UMIP-protected instructions and whenever I use %edi the default segment is %ds. > > It is even in the comment above. This function does not decode instructions but only the segment selectors. This is the reason I added a comment about callers using the segment carefully when string instructions. Perhaps I can move the comment to the function documentation. Given that string instructions seem to be the only exception, the function could take a boolean parameter if the segment is to be obtained for a destination string operand. How does this sound? > I'm looking at MOVS %es:%rdi, %ds:%rsi, > for example. Is this example valid? The documentation of MOVS specifies that it always moves DS:(E)SI to ES:(E)DI. > > > + case offsetof(struct pt_regs, si): > > + return SEG_DS; > > + case offsetof(struct pt_regs, bp): > > + /* fall through */ > > + case offsetof(struct pt_regs, sp): > > + return SEG_SS; > > + case offsetof(struct pt_regs, ip): > > + return SEG_CS; > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +/** > > + * get_segment_selector() - obtain segment selector > > + * @regs: Set of registers containing the segment selector > > + * @seg_type: Type of segment selector to obtain > > + * @regoff: Operand offset, in pt_regs, of which the selector is needed > > That's gone. I will remove it. > > > + * > > + * Obtain the segment selector for any of CS, SS, DS, ES, FS, GS. In > > + * CONFIG_X86_32, the segment is obtained from either pt_regs or > > + * kernel_vm86_regs as applicable. In CONFIG_X86_64, CS and SS are obtained > > + * from pt_regs. DS, ES, FS and GS are obtained by reading the ds and es, fs > > + * and gs, respectively. > > ... and DS and ES are ignored in long mode. I will clarify that callers need to ignore DS and ES if in long mode. > > > + * > > + * Return: Value of the segment selector > > ... or negative... I will complement documentation on this specific case. Thanks and BR, Ricardo

April 26, 2017

Re: [PATCH] d3dx9: Support parameters sharing in effect.

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

April 26, 2017

Re: [PATCH] d3dx9/tests: Cleanup expect_vec3.

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

April 26, 2017

Re: [v6 PATCH 05/21] x86/insn-eval: Add utility functions to get register offsets

by Ricardo Neri

On Wed, 2017-04-12 at 18:28 +0200, Borislav Petkov wrote: > On Tue, Mar 07, 2017 at 04:32:38PM -0800, Ricardo Neri wrote: > > The function insn_get_reg_offset takes as argument an enumeration that > > Please end function names with parentheses. Will do! > > And do you mean get_reg_offset(), per chance? Yes, I meant that. This was a copy/paste error. > > > indicates the type of offset that is returned: the R/M part of the ModRM > > byte, the index of the SIB byte or the base of the SIB byte. > > Err, you mean, it returns the offset to the register the argument > specifies. Yes. I will reword. > > > Callers of > > this function would need the definition of such enumeration. This is not > > needed. Instead, helper functions can be defined for this purpose can be > > added. > > "Instead, add helpers... " I will reword. > > > These functions are useful in cases when, for instance, the caller > > needs to decide whether the operand is a register or a memory location by > > looking at the mod part of the ModRM byte. > > > > 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/include/asm/insn-eval.h | 3 +++ > > arch/x86/lib/insn-eval.c | 51 ++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 54 insertions(+) > > > > diff --git a/arch/x86/include/asm/insn-eval.h b/arch/x86/include/asm/insn-eval.h > > index 5cab1b1..754211b 100644 > > --- a/arch/x86/include/asm/insn-eval.h > > +++ b/arch/x86/include/asm/insn-eval.h > > @@ -12,5 +12,8 @@ > > #include <asm/ptrace.h> > > > > void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs); > > +int insn_get_reg_offset_modrm_rm(struct insn *insn, struct pt_regs *regs); > > +int insn_get_reg_offset_sib_base(struct insn *insn, struct pt_regs *regs); > > +int insn_get_reg_offset_sib_base(struct insn *insn, struct pt_regs *regs); > > Forgotten to edit the copy-paste? > > Which means, nothing really needs insn_get_reg_offset_sib_index() and > you can get rid of it? Yes, I can get rid of it. > > > #endif /* _ASM_X86_INSN_EVAL_H */ > > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c > > index 23cf010..78df1c9 100644 > > --- a/arch/x86/lib/insn-eval.c > > +++ b/arch/x86/lib/insn-eval.c > > @@ -98,6 +98,57 @@ static int get_reg_offset(struct insn *insn, struct pt_regs *regs, > > return regoff[regno]; > > } > > > > +/** > > + * insn_get_reg_offset_modrm_rm - Obtain register in r/m part of ModRM byte > > + * @insn: Instruction structure containing the ModRM byte > > + * @regs: Set of registers indicated by the ModRM byte > > That's simply struct pt_regs - not a set of registers indicated by > ModRM?!? I will reword it to say "A struct pt_regs containing register values indicated by the ModRM byte". > > > + * Obtain the register indicated by the r/m part of the ModRM byte. The > > + * register is obtained as an offset from the base of pt_regs. In specific > > + * cases, the returned value can be -EDOM to indicate that the particular value > > + * of ModRM does not refer to a register. > > Put that sentence under the "Return: " paragraph below so that it is > immediately obvious what the retvals are. Will do. > > > + * > > + * Return: Register indicated by r/m, as an offset within struct pt_regs > > + */ > > +int insn_get_reg_offset_modrm_rm(struct insn *insn, struct pt_regs *regs) > > That name is too long: insn_get_modrm_rm_off() should be enough. > > > +{ > > + return get_reg_offset(insn, regs, REG_TYPE_RM); > > +} > > + > > +/** > > + * insn_get_reg_offset_sib_base - Obtain register in base part of SiB byte > > + * @insn: Instruction structure containing the SiB byte > > + * @regs: Set of registers indicated by the SiB byte > > + * > > + * Obtain the register indicated by the base part of the SiB byte. The > > + * register is obtained as an offset from the base of pt_regs. In specific > > + * cases, the returned value can be -EDOM to indicate that the particular value > > + * of SiB does not refer to a register. > > + * > > + * Return: Register indicated by SiB's base, as an offset within struct pt_regs Will make the spelling consistent. > > Let's stick to a single spelling: SIB, all caps. > > > + */ > > +int insn_get_reg_offset_sib_base(struct insn *insn, struct pt_regs *regs) > > insn_get_sib_base_off() > > Ditto for the rest of the comments on insn_get_reg_offset_modrm_rm() above. > > > +{ > > + return get_reg_offset(insn, regs, REG_TYPE_BASE); > > +} > > + > > +/** > > + * insn_get_reg_offset_sib_index - Obtain register in index part of SiB byte > > + * @insn: Instruction structure containing the SiB byte > > + * @regs: Set of registers indicated by the SiB byte > > + * > > + * Obtain the register indicated by the index part of the SiB byte. The > > + * register is obtained as an offset from the index of pt_regs. In specific > > + * cases, the returned value can be -EDOM to indicate that the particular value > > + * of SiB does not refer to a register. > > + * > > + * Return: Register indicated by SiB's base, as an offset within struct pt_regs > > + */ > > +int insn_get_reg_offset_sib_index(struct insn *insn, struct pt_regs *regs) > > insn_get_sib_idx_off() > > And again, if this function is unused, don't add it. Masami Hiramatsu had originally requested to add the two functions. I suppose the unneeded functions could be added if/when needed. Thanks and BR, Ricardo

April 26, 2017

Re: [PATCH 1/13] msidb: Add stub tool for manipulating MSI databases.

by Hans Leidekker

On Sun, 2017-04-09 at 13:12 -0600, Erich E. Hoover wrote: > +static int open_database( struct msidb_state *state ) > +{ > + LPCWSTR db_mode = state->create_database ? MSIDBOPEN_CREATEDIRECT : MSIDBOPEN_DIRECT; > + UINT ret; > + > + ret = MsiOpenDatabaseW( state->database_file, db_mode, &state->database_handle ); Databases opened in direct mode should always be committed. You should either do that in following patches, or open them in transacted mode.

April 26, 2017

Re: [PATCH 1/13] msidb: Add stub tool for manipulating MSI databases.

by Alexandre Julliard

Hans Leidekker <hans(a)codeweavers.com> writes: > On Sun, 2017-04-09 at 13:12 -0600, Erich E. Hoover wrote: >> + /* process and validate all the command line flags */ >> + for (i = 1; n && i < argc; i += n) >> + n = process_argument( &state, i, argc, argv ); >> + if (!valid_state( &state )) >> + return 1; >> + >> + /* perform the requested operations */ >> + if (!open_database( &state )) >> + { >> + ERR( "Failed to open database '%s'.\n", wine_dbgstr_w(state.database_file) ); > > For programs it should be WINE_ERR, etc. Actually we build programs with __WINESRC__ these days, so it no longer matters. -- Alexandre Julliard julliard(a)winehq.org

April 26, 2017

Re: po: Update Simplified Chinese translation.

by Jactry Zeng

Hi, 2017-04-26 15:50 GMT+08:00 Alexandre Julliard <julliard(a)winehq.org>: > > Is it really necessary to change all the ellipsis characters? They were > changed the other way around not so long ago. I change it again because I notice that translation checker reported these lines as error: http://fgouget.free.fr/wine-po/zh_CN.check.html I check the style guide again, it show that Windows use English ellipses for Chinese: """ English (...) is used in both software and document localization. """ ( p26 of http://download.microsoft.com/download/4/c/a/4ca95933-3496-4793-9d77-a89b60… ) Thanks. -- Regards, Jactry Zeng

April 26, 2017

Re: [PATCH 1/13] msidb: Add stub tool for manipulating MSI databases.

by Hans Leidekker

On Sun, 2017-04-09 at 13:12 -0600, Erich E. Hoover wrote: > + /* process and validate all the command line flags */ > + for (i = 1; n && i < argc; i += n) > + n = process_argument( &state, i, argc, argv ); > + if (!valid_state( &state )) > + return 1; > + > + /* perform the requested operations */ > + if (!open_database( &state )) > + { > + ERR( "Failed to open database '%s'.\n", wine_dbgstr_w(state.database_file) ); For programs it should be WINE_ERR, etc.

April 26, 2017