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

Re: Wine uses the wrong pkg-config for 32 bit builds

by Francois Gouget

On Tue, 6 Jun 2017, Alexandre Julliard wrote: > Francois Gouget <fgouget(a)codeweavers.com> writes: > > > * If one runs 'configure --enable-win64' on a 32 bit platform, one would > > want to make sure to use x86_64-${host_os}-pkg-config rather than > > plain pkg-config. > > That can be accomplished by adding another AC_CHECK_PROGS in the > > relevant 'case $host' branch but it led me to think that maybe a more > > general mechanism would be better. > > I don't think there's any 32-bit platform where --enable-win64 would > work, and we don't support it anyway. I meant one where: $ dpkg --print-architecture i386 $ dpkg --print-foreign-architectures amd64 Rather than the opposite which is more common nowadays. (You may recognize a certain old Debian 7 chroot ;-) -- Francois Gouget <fgouget(a)codeweavers.com>

June 6, 2017

Re: Wine uses the wrong pkg-config for 32 bit builds

by Alexandre Julliard

Francois Gouget <fgouget(a)codeweavers.com> writes: > * If one runs 'configure --enable-win64' on a 32 bit platform, one would > want to make sure to use x86_64-${host_os}-pkg-config rather than > plain pkg-config. > That can be accomplished by adding another AC_CHECK_PROGS in the > relevant 'case $host' branch but it led me to think that maybe a more > general mechanism would be better. I don't think there's any 32-bit platform where --enable-win64 would work, and we don't support it anyway. -- Alexandre Julliard julliard(a)winehq.org

June 6, 2017

Re: Wine uses the wrong pkg-config for 32 bit builds

by Francois Gouget

On Tue, 6 Jun 2017, Alexandre Julliard wrote: [...] > It seems to me that you'd want to do this only in the specific case that > we force 32-bit, other cases should be handled by the normal --host > mechanism. Yes, that's another possible approach. Note though that: * The modified version of WINE_CHECK_HOST_TOOL() honors the --host mechanism (now I fixed the bug). * If one runs 'configure --enable-win64' on a 32 bit platform, one would want to make sure to use x86_64-${host_os}-pkg-config rather than plain pkg-config. That can be accomplished by adding another AC_CHECK_PROGS in the relevant 'case $host' branch but it led me to think that maybe a more general mechanism would be better. As a side note WINE_CHECK_HOST_TOOL() is only used once; to check for pkg-config. I'm not sure of the implications. -- Francois Gouget <fgouget(a)codeweavers.com>

June 6, 2017

Re: Wine uses the wrong pkg-config for 32 bit builds

by Francois Gouget

On Mon, 5 Jun 2017, Michael Müller wrote: > You patch breaks the cross compilation for OSX. See: > https://builds.wine-staging.com/2017-06-05-15-57-04-job-11586/build.log > > As you can see in the log, we provide a wrapper script > i686-apple-darwin12-pkg-config which sets the pkg-config variables > correctly. So far this worked without issues when compiling Wine using > --host i686-apple-darwin12. Thanks for testing! There were two typos in the --host branch of WINE_CHECK_HOST_TOOL(). First ${ac_tool_prefix} already contains the trailing dash so I should not have added one, but that hardly mattered since I had an extra "_tool" in the variable I was setting (a copy/paste error). The result of all that is that the macro was just returning the plain pkg-config tool in the end. As far as I can tell the --host case now works as expected in the patch below. commit 745c7c5293c9163232a104cabc57dc73912b0efd Author: Francois Gouget <fgouget(a)codeweavers.com> Date: Mon Jun 5 16:59:05 2017 +0200 configure: Take the target host into account in WINE_CHECK_HOST_TOOL(). In particular this gets us the right pkg-config tool when building the 32 bit Wine. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> diff --git a/aclocal.m4 b/aclocal.m4 index 9364188e334..5892a19d146 100644 --- a/aclocal.m4 +++ b/aclocal.m4 @@ -27,13 +27,17 @@ dnl dnl Like AC_CHECK_TOOL but without the broken fallback to non-prefixed name dnl AC_DEFUN([WINE_CHECK_HOST_TOOL], -[AS_VAR_SET_IF([ac_tool_prefix], - AC_CHECK_PROG([$1],[${ac_tool_prefix}$2],[${ac_tool_prefix}$2],,[$4])) -AS_VAR_IF([ac_cv_prog_$1],[], - [AS_VAR_IF([cross_compiling],[yes],[], - [AS_UNSET([ac_cv_prog_$1]) - AC_CHECK_PROG([$1],[$2],[$2],[$3],[$4])])], -[AS_VAR_COPY([$1],[ac_cv_prog_$1])])]) +[AS_VAR_IF([ac_tool_prefix],[], + [case "$host_cpu" in + i[[3456789]]86*) + ac_host_tool_list="m4_foreach([ac_wine_cpu],[i686,i586,i486,i386],[ac_wine_cpu-${host_os}-$2 ]) $2" ;; + x86_64) + ac_host_tool_list="x86_64-${host_os}-$2 $2" ;; + *) + ac_host_tool_list="" ;; + esac], + [ac_host_tool_list="${ac_tool_prefix}$2"]) + AC_CHECK_PROGS($1,$ac_host_tool_list,[$3],[$4])]) dnl **** Initialize the programs used by other checks **** dnl -- Francois Gouget <fgouget(a)codeweavers.com>

June 6, 2017

Re: Wine uses the wrong pkg-config for 32 bit builds

by Alexandre Julliard

Francois Gouget <fgouget(a)codeweavers.com> writes: > We already have a WINE_CHECK_HOST_TOOL() macro that looks like it should > do the trick. But it only takes ac_tool_prefix into account. So I > modified it to take the host target into account if ac_tool_prefix is > not set. This is modeled after the WINE_CHECK_MINGW_PROG macro and you > can see the result in the patch below. It seems to me that you'd want to do this only in the specific case that we force 32-bit, other cases should be handled by the normal --host mechanism. Maybe something like this: diff --git a/configure.ac b/configure.ac index 129a8f62c7e2..1d84e6d17d10 100644 --- a/configure.ac +++ b/configure.ac @@ -151,6 +151,7 @@ case $host in host_cpu="i386" notice_platform="32-bit " AC_SUBST(TARGETFLAGS,"-m32") + AC_CHECK_PROGS([PKG_CONFIG],[i686-${host_os}-pkg-config],[]) enable_win16=${enable_win16:-yes} else if test "x${GCC}" = "xyes" -- Alexandre Julliard julliard(a)winehq.org

June 6, 2017

Re: [PATCH v7 05/26] x86/mpx: Do not use SIB.base if its value is 101b and ModRM.mod = 0

by Ricardo Neri

On Mon, 2017-05-29 at 15:07 +0200, Borislav Petkov wrote: > On Fri, May 05, 2017 at 11:17:03AM -0700, Ricardo Neri wrote: > > Section 2.2.1.2 of the Intel 64 and IA-32 Architectures Software > > Developer's Manual volume 2A states that when a SIB byte is used and the > > base of the SIB byte points is base = 101b and the mod part > > of the ModRM byte is zero, the base port on the effective address > > computation is null. In this case, a 32-bit displacement follows the SIB > > byte. This is obtained when the instruction decoder parses the operands. > > > > To signal this scenario, a -EDOM error is returned to indicate callers that > > they should ignore the base. > > > > Cc: Borislav Petkov <bp(a)suse.de> > > Cc: Andy Lutomirski <luto(a)kernel.org> > > 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: Peter Zijlstra <peterz(a)infradead.org> > > Cc: Nathan Howard <liverlint(a)gmail.com> > > Cc: Adan Hawthorn <adanhawthorn(a)gmail.com> > > Cc: Joe Perches <joe(a)perches.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/mm/mpx.c | 27 ++++++++++++++++++++------- > > 1 file changed, 20 insertions(+), 7 deletions(-) > > > > diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c > > index 7397b81..30aef92 100644 > > --- a/arch/x86/mm/mpx.c > > +++ b/arch/x86/mm/mpx.c > > @@ -122,6 +122,15 @@ static int get_reg_offset(struct insn *insn, struct pt_regs *regs, > > > > case REG_TYPE_BASE: > > regno = X86_SIB_BASE(insn->sib.value); > > + /* > > + * If ModRM.mod is 0 and SIB.base == 5, the base of the > > + * register-indirect addressing is 0. In this case, a > > + * 32-bit displacement is expected in this case; the > > + * instruction decoder finds such displacement for us. > > That last sentence reads funny. Just say: > > "In this case, a 32-bit displacement follows the SIB byte." Agreed. I will update the comment to make more sense. Thanks and BR, Ricardo

June 6, 2017

Re: [PATCH v7 07/26] x86/insn-eval: Do not BUG on invalid register type

by Ricardo Neri

On Mon, 2017-05-29 at 18:37 +0200, Borislav Petkov wrote: > On Fri, May 05, 2017 at 11:17:05AM -0700, Ricardo Neri wrote: > > We are not in a critical failure path. The invalid register type is caused > > when trying to decode invalid instruction bytes from a user-space program. > > Thus, simply print an error message. To prevent this warning from being > > abused from user space programs, use the rate-limited variant of printk. > > > > Cc: Borislav Petkov <bp(a)suse.de> > > Cc: Andy Lutomirski <luto(a)kernel.org> > > 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: 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 | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c > > index e746a6f..182e2ae 100644 > > --- a/arch/x86/lib/insn-eval.c > > +++ b/arch/x86/lib/insn-eval.c > > @@ -5,6 +5,7 @@ > > */ > > #include <linux/kernel.h> > > #include <linux/string.h> > > +#include <linux/ratelimit.h> > > #include <asm/inat.h> > > #include <asm/insn.h> > > #include <asm/insn-eval.h> > > @@ -85,9 +86,8 @@ static int get_reg_offset(struct insn *insn, struct pt_regs *regs, > > break; > > > > default: > > - pr_err("invalid register type"); > > - BUG(); > > - break; > > + printk_ratelimited(KERN_ERR "insn-eval: x86: invalid register type"); > > You can use pr_err_ratelimited() and define "insn-eval" with pr_fmt. > Look for examples in the tree. Will do. I have looked at the examples. > > Btw, "insn-eval" is perhaps not the right name - since we're building > an instruction decoder, maybe it should be called "insn-dec" or so. I'm > looking at those other arch/x86/lib/insn.c, arch/x86/include/asm/inat.h > things and how they're starting to morph into one decoding facility, > AFAICT. I agree that insn-eval reads somewhat funny. I did not want to go with insn-dec.c as insn.c, in my opinion, already decodes the instruction (i.e., it finds prefixes, opcodes, ModRM, SIB and displacement bytes). In insn-eval.c I simply take those decoded parameters and evaluate them to obtain the values they contain (e.g., a specific memory location). Perhaps, insn-resolve.c could be a better name? Or maybe isnn-operands? Thanks and BR, Ricardo

June 6, 2017

Re: [PATCH v7 08/26] x86/insn-eval: Add a utility function to get register offsets

by Ricardo Neri

On Mon, 2017-05-29 at 19:16 +0200, Borislav Petkov wrote: > On Fri, May 05, 2017 at 11:17:06AM -0700, Ricardo Neri wrote: > > The function get_reg_offset() returns the offset to the register the > > argument specifies as indicated in an enumeration of type offset. Callers > > of this function would need the definition of such enumeration. This is > > not needed. Instead, add helper functions for this purpose. 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 rm part > > of the ModRM byte. As of now, this is the only helper function that is > > needed. > > > > 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 | 1 + > > arch/x86/lib/insn-eval.c | 15 +++++++++++++++ > > 2 files changed, 16 insertions(+) > > > > diff --git a/arch/x86/include/asm/insn-eval.h b/arch/x86/include/asm/insn-eval.h > > index 5cab1b1..7e8c963 100644 > > --- a/arch/x86/include/asm/insn-eval.h > > +++ b/arch/x86/include/asm/insn-eval.h > > @@ -12,5 +12,6 @@ > > #include <asm/ptrace.h> > > > > void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs); > > +int insn_get_modrm_rm_off(struct insn *insn, struct pt_regs *regs); > > > > #endif /* _ASM_X86_INSN_EVAL_H */ > > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c > > index 182e2ae..8b16761 100644 > > --- a/arch/x86/lib/insn-eval.c > > +++ b/arch/x86/lib/insn-eval.c > > @@ -97,6 +97,21 @@ 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 > > That name needs to be synced with the function name below. Ugh! I missed this. I will update accordingly. Thanks for the detailed review. BR, Ricardo

June 6, 2017

Re: [PATCH v7 09/26] x86/insn-eval: Add utility function to identify string instructions

by Ricardo Neri

On Mon, 2017-05-29 at 23:48 +0200, Borislav Petkov wrote: > On Fri, May 05, 2017 at 11:17:07AM -0700, Ricardo Neri wrote: > > String instructions are special because in protected mode, the linear > > address is always obtained via the ES segment register in operands that > > use the (E)DI register. > > ... and DS for rSI. Right, I omitted this in the commit message. > > If we're going to account for both operands of string instructions with > two operands. > > Btw, LODS and OUTS use only DS:rSI as a source operand. So we have to be > careful with the generalization here. So if ES:rDI is the only seg. reg > we want, then we don't need to look at those insns... (we assume DS by > default). My intention with this function is to write a function that does only one thing: identify string instructions, irrespective of the operands they use. A separate function, resolve_seg_register, will have the logic to decide what to segment register to use based on the registers used as operands, whether we are looking at a string instruction, whether we have segment override prefixes and whether such overrides should be ignored. If I was to leave out string instructions from this function it should be renamed as is_string_instruction_non_lods_outs. In my opinion this separation makes the code more clear and I would end up having logic to decide which segment register to use in two places. Does it makes sense to you? > > ... > > > +/** > > + * is_string_instruction - Determine if instruction is a string instruction > > + * @insn: Instruction structure containing the opcode > > + * > > + * Return: true if the instruction, determined by the opcode, is any of the > > + * string instructions as defined in the Intel Software Development manual. > > + * False otherwise. > > + */ > > +static bool is_string_instruction(struct insn *insn) > > +{ > > + insn_get_opcode(insn); > > + > > + /* all string instructions have a 1-byte opcode */ > > + if (insn->opcode.nbytes != 1) > > + return false; > > + > > + switch (insn->opcode.bytes[0]) { > > + case INSB: > > + /* fall through */ > > + case INSW_INSD: > > + /* fall through */ > > + case OUTSB: > > + /* fall through */ > > + case OUTSW_OUTSD: > > + /* fall through */ > > + case MOVSB: > > + /* fall through */ > > + case MOVSW_MOVSD: > > + /* fall through */ > > + case CMPSB: > > + /* fall through */ > > + case CMPSW_CMPSD: > > + /* fall through */ > > + case STOSB: > > + /* fall through */ > > + case STOSW_STOSD: > > + /* fall through */ > > + case LODSB: > > + /* fall through */ > > + case LODSW_LODSD: > > + /* fall through */ > > + case SCASB: > > + /* fall through */ > > That "fall through" for every opcode is just too much. Also, you can use > the regularity of the x86 opcode space and do: > > case 0x6c ... 0x6f: /* INS/OUTS */ > case 0xa4 ... 0xa7: /* MOVS/CMPS */ > case 0xaa ... 0xaf: /* STOS/LODS/SCAS */ > return true; > default: > return false; > } > > And voila, there's your compact is_string_insn() function! :^) Thanks for the suggestion! It looks really nice. I will implement accordingly. Thanks and BR, Ricardo

June 6, 2017

Re: [PATCH 2/4] ntdll: Provide more accurate virtual memory counters.

by Matteo Bruni

2017-06-05 16:15 GMT+02:00 Akihiro Sagawa <sagawa.aki(a)gmail.com>: > On Sun, 4 Jun 2017 22:51:10 -0600, Alex Henrie wrote: >> Out of curiosity, is there a particular application that depends on >> this behavior? It could be argued that reporting the value from the >> native operating system is better because it tells what's really >> happening in the process. >> >> -Alex > > Good question. I'm trying to fix Sorcery Jokers (trial) issue, bug 43119. > The application repeatedly trys to allocate virtual memory as much as > possible, and measures available memory size by GlobalMemoryStatusEx(). > > This patch is better than nothing. The application stops memory > allocation about 384MB (0x17ffffff) free. After the allocation, it > succeeds to load libGL.so. However, it shows driver pointer missing > error when loading DRI driver, radeonsi_dri.so. I'm still in trouble > with this point. > > By the way, I have another idea about this issue. That is reserving > virtual memory area when loading wined3d.dll and release it just before > loading OpenGL libraries. Being hackish idea, I chose the forementioned > method in the beginning. As far as hacks go, does adding some arbitrary "reserved" amount to pvmi.VirtualSize make any difference?

June 5, 2017