From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 104718 invoked by alias); 7 Feb 2016 06:29:18 -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 104702 invoked by uid 89); 7 Feb 2016 06:29:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.8 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_NONE autolearn=no version=3.3.2 spammy=classification, 693,6, 6936, Calculate X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Sun, 07 Feb 2016 06:29:15 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id B1C89116635; Sun, 7 Feb 2016 01:29:13 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id Ty3rfVC5Yp5T; Sun, 7 Feb 2016 01:29:13 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 3F37911662D; Sun, 7 Feb 2016 01:29:12 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 4CCFD406C6; Sun, 7 Feb 2016 10:29:03 +0400 (RET) Date: Sun, 07 Feb 2016 06:29:00 -0000 From: Joel Brobecker To: Walfred Tedeschi Cc: eliz@gnu.org, gdb-patches@sourceware.org Subject: Re: [PATCH V3 1/2] ABI changes for MPX. Message-ID: <20160207062903.GA15342@adacore.com> References: <1452688799-15633-1-git-send-email-walfred.tedeschi@intel.com> <1452688799-15633-2-git-send-email-walfred.tedeschi@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1452688799-15633-2-git-send-email-walfred.tedeschi@intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-SW-Source: 2016-02/txt/msg00170.txt.bz2 Walfred, On Wed, Jan 13, 2016 at 01:39:58PM +0100, Walfred Tedeschi wrote: > Code reflects what is presented in the ABI document: > https://github.com/hjl-tools/x86-psABI/wiki/X86-psABI > Here new class POINTER was added. GDB code is modified to mirror > this new class. (page 134) > > In addition to that set bound registers to INIT state (access to > all memory) before performing the inferior function call. > > 2016-01-13 Walfred Tedeschi > > * amd64-tdep.c (amd64_reg_class): Add new class AMD64_POINTER. > (amd64_merge_classes): Add AMD64_POINTER to merging classes rules. > (amd64_classify): Add AMD64_POINTER. > (amd64_return_value): Add AMD64_POINTER. > (amd64_push_arguments): Add new AMD64_POINTER class. > (amd64_push_dummy_call): Set bound registers to INIT state before > performing the call. I think you need to update the following piece of code as well (in amd64_push_arguments): /* Calculate the number of integer and SSE registers needed for this argument. */ for (j = 0; j < 2; j++) { if (theclass[j] == AMD64_INTEGER) needed_integer_regs++; Otherwise, you'll have a discrepancy between needed_integer_regs and integer_reg. As far as I understand, POINTER and INTEGER arguments are passed exactly the same way. The distinction appears to only be useful for bound register handling. And since this patch unilaterally sets the bound registers to zero, we could have done it without adding the extra register classification (and thus reducing the maintenance cost of having to handle both). I'm really tempted to suggest we drop the new class altogether, avoiding the risk of having missed another spot where we needd to add POINTER class handling, and just keep the change which sets the bound registers to zero. If, one day, someone decides to enhance the debugger to handle the bound registers more closely to what the ABI suggests, I think we could have a separate function that would tell us if a given type is of class POINTER or not, rather than adding a class. Am I understanding the situation correctly? > gdb/doc/ChangeLog: > > * gdb.texinfo (Intel Memory Protection Extension): Add entry for > inferior function calls for MPX. > @@ -693,6 +701,7 @@ amd64_classify (struct type *type, enum amd64_reg_class theclass[2]) > amd64_classify_aggregate (type, theclass); > } > > + > static enum return_value_convention > amd64_return_value (struct gdbarch *gdbarch, struct value *function, > struct type *type, struct regcache *regcache, There is no reason to add a new line, here, so let's avoid this unnecessary change. -- Joel