From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 86726 invoked by alias); 4 May 2018 10:39:47 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 86717 invoked by uid 89); 4 May 2018 10:39:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-6.6 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Nice, indication, restarts X-HELO: mx1.redhat.com Received: from mx3-rdu2.redhat.com (HELO mx1.redhat.com) (66.187.233.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 04 May 2018 10:39:45 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2A2C7427097F; Fri, 4 May 2018 10:39:44 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 933B0111DCFE; Fri, 4 May 2018 10:39:43 +0000 (UTC) Subject: Re: [PATCHv2] gdb/x86: Handle kernels using compact xsave format To: Andrew Burgess References: <20180503170624.20847-1-andrew.burgess@embecosm.com> <6131c1b9-21c1-6a38-f466-34bfef144a26@redhat.com> <20180503173712.GI3375@embecosm.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <3c303c56-2315-fb00-dff1-805ec84a115c@redhat.com> Date: Fri, 04 May 2018 10:39:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180503173712.GI3375@embecosm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-05/txt/msg00083.txt.bz2 On 05/03/2018 06:37 PM, Andrew Burgess wrote: > Hope that helps, It does, thank you. The patch looks good to me, with the following nits addressed. Thanks much for doing all this. Nice work. Bonus points for the testcase. In the commit log: > had not yet enableld the x87 feature then within GDB we would see "enableld" -> "enabled" > @@ -844,39 +908,67 @@ i387_xsave_to_cache (struct regcache *regcache, const void *buf) > } > } > > - supply_register_by_name (regcache, "fioff", &fp->fioff); > - supply_register_by_name (regcache, "fooff", &fp->fooff); > - supply_register_by_name (regcache, "mxcsr", &fp->mxcsr); > + if ((clear_bv & (X86_XSTATE_SSE | X86_XSTATE_AVX)) == > + (X86_XSTATE_SSE | X86_XSTATE_AVX)) > + { == sign goes on next line. > +ULONGEST > +i387_xsave_get_clear_bv (struct gdbarch *gdbarch, const void *xsave) > +{ > + enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); > + const gdb_byte *regs = (const gdb_byte *) xsave; > + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > + ULONGEST clear_bv, xstate_bv = 0; I see this is factored out from elsewhere, but note we can skip initializing to zero when ... > + > + /* Get `xstat_bv'. The supported bits in `xstat_bv' are 8 bytes. */ > + xstate_bv = extract_unsigned_integer (XSAVE_XSTATE_BV_ADDR (regs), > + 8, byte_order); ... we're immediately overriding with some value. While at it, we can declare the variables at their initialization to avoid potential for confusion: /* Get `xstat_bv'. The supported bits in `xstat_bv' are 8 bytes. */ ULONGEST xstate_bv = extract_unsigned_integer (XSAVE_XSTATE_BV_ADDR (regs), 8, byte_order); /* Clear part in vector registers if its bit in xstat_bv is zero. */ ULONGEST clear_bv = (~(xstate_bv)) & tdep->xcr0; > + > + /* Clear part in vector registers if its bit in xstat_bv is zero. */ > + clear_bv = (~(xstate_bv)) & tdep->xcr0; > + > + return clear_bv; > +} > + > + /* Initial value for fctrl register, as defined in the X86 > + manual, and confirmed in the (Linux) kernel source. */ > + store_unsigned_integer (buf, 4, byte_order, 0x037f); This constant appears in more than one place. How about giving it a name, like I387_FCTRL_INIT_VAL or some such. Then the describing comment can be moved/centralized there too. Likewise 0x1f80 for mxcsr. > + regcache_raw_supply (regcache, i, buf); > + /* The mxcsr register is written into the xsave buffer if either AVX > + or SSE is enabled, so only clear it if both of those features > + require clearing. */ > + if ((clear_bv & (X86_XSTATE_AVX | X86_XSTATE_SSE)) > + == (X86_XSTATE_AVX | X86_XSTATE_SSE)) > + { > + gdb_byte buf[2]; > > - if ((clear_bv & X86_XSTATE_AVX)) > - for (i = I387_YMM0H_REGNUM (tdep); > - i < I387_YMMENDH_REGNUM (tdep); i++) > - memset (XSAVE_AVXH_ADDR (tdep, regs, i), 0, 16); > + store_unsigned_integer (buf, 2, byte_order, 0x1f80); > + memcpy (FXSAVE_ADDR (tdep, regs, i), buf, 2); Do we need to extract to a buffer and then memcpy the buffer to the destination? Why not extract to the destination directly, like: store_unsigned_integer (FXSAVE_ADDR (tdep, regs, i), 2, byte_order, 0x1f80); Ditto here: > + if (i == I387_FCTRL_REGNUM (tdep)) > + { > + gdb_byte buf[2]; > + > + store_unsigned_integer (buf, 2, byte_order, 0x037f); > + memcpy (FXSAVE_ADDR (tdep, regs, i), buf, 2); > + } > + /* Check if any X87 registers are changed. Only the non-control > + registers are handled here, the control registers are all */ > + if ((tdep->xcr0 & X86_XSTATE_X87)) Incomplete sentence. > + /* We're only setting MXCSR, so check the initial state > + to see if either of AVX or SSE are already enabled. > + If they are then we'll attribute this changed MXCSR to > + that feature. If neither feature is enabled, then > + we'll attribute this change to the SSE feature. */ > + xstate_bv |= > + (initial_xstate_bv & (X86_XSTATE_AVX | X86_XSTATE_SSE)); '!=' goes on the next line. > + if ((xstate_bv & (X86_XSTATE_AVX | X86_XSTATE_SSE)) == 0) > + xstate_bv |= X86_XSTATE_SSE; > + memcpy (p, raw, 2); > +++ b/gdb/gdbserver/regcache.c > @@ -346,6 +346,17 @@ supply_register_zeroed (struct regcache *regcache, int n) > #endif > } > > +#ifndef IN_PROCESS_AGENT > + > +void > +supply_register_by_name_zeroed (struct regcache *regcache, > + const char *name) > +{ Missing intro comment. On the testcase: > +# Test initial values of x87 control registers, before any x87 > +# instructions have been executed in the inferior. > + > +if { ![istarget x86_64-*-* ] || ![is_lp64_target] } { > + return > +} Should probably be is_amd64_regs_target, so that both "i386-* -m64" and "x32" exercise it too. > + gdb_test "p/x \$${regname}" " = ${regvalue}" "Check initial value of \$${regname}" Please use lowercase test names throughout: "Check" -> "check", "Step" -> "step", etc. > +# Grab the address of this instruction, it will appear in later > +# results. > +set addr [capture_command_output "p/x \$pc" "\\\$$decimal = "] > + Could be: set addr [get_hexadecimal_valueof "\$pc" "0"] > +# =========================================================== > + > +clean_restart ${binfile} > + > +if ![runto_main] then { > + fail "can't run to main" > + return 0 > +} > + > +gdb_test_no_output "set \$mxcsr=0x9f80" "Set a new value for MXCSR" > +gdb_test "stepi" "fwait" "Step forward one instruction for mxcsr test" > +gdb_test "p/x \$mxcsr" " = 0x9f80" "Check new value of MXCSR is still in place" > + Please wrap these test sequences with e.g., with_test_prefix, like: with_test_prefix "something here" { clean_restart ${binfile} if ![runto_main] then { fail "can't run to main" return 0 } gdb_test_no_output "set \$mxcsr=0x9f80" "set new value for MXCSR" gdb_test "stepi" "fwait" "step forward one instruction for mxcsr test" gdb_test "p/x \$mxcsr" " = 0x9f80" "check new value of MXCSR still in place" } This way, the runto_main failure gets a distinct FAIL message. Plus, it gives a nice visual indication of the separate gdb restarts/testing, without the need for the ======= comment lines. A short leading sentence indicating what each ======= region is about to test would be nice too. If instead of with_test_prefix you put the subtests in procedures, like proc_with_prefix something-here {} { ... } then an early FAIL will still continue testing other tests that don't rely on the same gdb invocation. Thanks, Pedro Alves