From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9459 invoked by alias); 11 May 2018 19:56:36 -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 9433 invoked by uid 89); 11 May 2018 19:56:35 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.0 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=story X-HELO: mail-wr0-f170.google.com Received: from mail-wr0-f170.google.com (HELO mail-wr0-f170.google.com) (209.85.128.170) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 11 May 2018 19:56:33 +0000 Received: by mail-wr0-f170.google.com with SMTP id 94-v6so6405805wrf.5 for ; Fri, 11 May 2018 12:56:33 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=cxcPkHbMWRO/qArE2zHhnXMsr0/zM751Q+qTyQ9eIKY=; b=uebpgWzpuAZ3L8XoszbOQh4B6Ts8phzig7zq2AGzmksT2aE+krc//Ce+Ed4wsBsBjE I1lLm3u5rSlDSdHltHGpYLbcgc4d7etca5dLqmELw1nhTww9Bw3c2dDMlhQXS+bcGBeD 16+EoU13hdn2dCv8azpCfGxGYkp1tHzLMZBr9sCf92u7jrzTTOswWjA2C8nQZ5Or8peS NKg3k0TDDzIkXFzMSJXtC4euoywjUTpmS0FX7bq87n3EI6vumprUR+ARDpOrN9EFcphn PMEw6HlaZtq4dxwzItJgcXheZpCg/ZbjzFi+ImZ6+hMJT9nHqiSKkbMQ3mvHo5Srm0Yg hvAw== X-Gm-Message-State: ALKqPwfC/CpvEouXVLIV5qBg43xmp2uY4Gs6MyKLZFu0q18Rna3Q2iDn S1hPx1AityKQbfJg/q62S4nMZg== X-Google-Smtp-Source: AB8JxZo8pqpUxIcEhgVtAENW31JVFXZ0oSJ6PtVe0Hd2cPKuowjQyklJl26qBVgi1huzIIdevxaG2w== X-Received: by 2002:adf:8e27:: with SMTP id n36-v6mr254252wrb.27.1526068591437; Fri, 11 May 2018 12:56:31 -0700 (PDT) Received: from localhost (host81-147-175-127.range81-147.btcentralplus.com. [81.147.175.127]) by smtp.gmail.com with ESMTPSA id o9-v6sm4010178wrn.74.2018.05.11.12.56.30 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 11 May 2018 12:56:30 -0700 (PDT) Date: Fri, 11 May 2018 21:55:00 -0000 From: Andrew Burgess To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] gdb/x86: Fix write out of mxcsr register for xsave targets Message-ID: <20180511195628.GD3797@embecosm.com> References: <20180511115228.22098-1-andrew.burgess@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2018-05/txt/msg00312.txt.bz2 * Pedro Alves [2018-05-11 18:14:20 +0100]: > On 05/11/2018 12:52 PM, Andrew Burgess wrote: > > In commit: > > > > commit 8ee22052f690c007556b97eed59f49350ece5ca9 > > Author: Andrew Burgess > > Date: Thu May 3 17:46:14 2018 +0100 > > > > gdb/x86: Handle kernels using compact xsave format > > > > in two places FXSAVE_ADDR was used instead of FXSAVE_MXCSR_ADDR to get > > the address of the mxcsr register within the xsave buffer. This will > > mean we are potentially accessing the wrong location within the xsave > > buffer. > > > > There are no tests included with this patch. The first mistake would > > only trigger an issue if/when the user tries to manually set the mxcsr > > register to a value that matches the random (value off stack) value > > that is in the xsave buffer, in this case the change by the user will > > go unnoticed by GDB, and the default value of mxcsr will be preserved. > > > > The second mistake only happens on the code path where all x87 > > registers are being written out of the register cache. I'm not sure > > how to trigger that code path. > > > > OK as is. > > How did you notice this? Valgrind? That's a funny story... I had reason to compile HEAD on a machine with GCC 5.4, and the build failed with a warning that 'i' was used uninitialised in this block: /* 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)) store_unsigned_integer (FXSAVE_ADDR (tdep, regs, i), 2, byte_order, I387_MXCSR_INIT_VAL); Now the warning from GCC is bogus, 'i' will be initialised, but it's not going to be initialised correctly, and when I looked into how to set 'i' to the correct value I realised the mistake I made. After that I double checked all the accesses to mxcsr that I changed, and found the second bug. Just a lucky find at the end of the day :) Thanks, Andrew