From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8326 invoked by alias); 14 Feb 2019 15:26:33 -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 8308 invoked by uid 89); 14 Feb 2019 15:26:33 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy= X-HELO: mail-wm1-f68.google.com Received: from mail-wm1-f68.google.com (HELO mail-wm1-f68.google.com) (209.85.128.68) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 14 Feb 2019 15:26:30 +0000 Received: by mail-wm1-f68.google.com with SMTP id a62so6729226wmh.4 for ; Thu, 14 Feb 2019 07:26:30 -0800 (PST) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:75e6:857f:3506:a1f4? ([2001:8a0:f913:f700:75e6:857f:3506:a1f4]) by smtp.gmail.com with ESMTPSA id f17sm1315758wmh.40.2019.02.14.07.26.27 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 14 Feb 2019 07:26:27 -0800 (PST) Subject: Re: [PATCH v3] [amd64] Fix AMD64 return value ABI in expression evaluation To: Leszek Swirski , gdb-patches@sourceware.org References: <20190214151602.147300-1-leszeks@google.com> <20190214151810.149322-1-leszeks@google.com> From: Pedro Alves Message-ID: Date: Thu, 14 Feb 2019 15:26:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190214151810.149322-1-leszeks@google.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-02/txt/msg00229.txt.bz2 OK. Thanks, Pedro Alves On 02/14/2019 03:18 PM, Leszek Swirski wrote: > The AMD64 System V ABI specifies that when a function has a return type > classified as MEMORY, the caller provides space for the value and passes > the address to this space as the first argument to the function (before > even the "this" pointer). The classification of MEMORY is applied to > struct that are sufficiently large, or ones with unaligned fields. > > The expression evaluator uses call_function_by_hand to call functions, > and the hand-built frame has to push arguments in a way that matches the > ABI of the called function. call_function_by_hand supports ABI-based > struct returns, based on the value of gdbarch_return_value, however on > AMD64 the implementation of the classifier incorrectly assumed that all > non-POD types (implemented as "all types with a base class") should be > classified as MEMORY and use the struct return. > > This ABI mismatch resulted in issues when calling a function that returns > a class of size <16 bytes which has a base class, including issues such > as the "this" pointer being incorrect (as it was passed as the second > argument rather than the first). > > This is now fixed by checking for field alignment rather than POD-ness, > and a testsuite is added to test expression evaluation for AMD64. > > gdb/ChangeLog > > * amd64-tdep.c (amd64_classify_aggregate): Use cp_pass_by_reference > rather than a hand-rolled POD check when checking for forced MEMORY > classification. > > gdb/testsuite/ChangeLog > > * gdb.arch/amd64-eval.cc: New file. > * gdb.arch/amd64-eval.exp: New file. > --- > gdb/ChangeLog | 6 ++ > gdb/amd64-tdep.c | 44 +++++++--- > gdb/testsuite/ChangeLog | 5 ++ > gdb/testsuite/gdb.arch/amd64-eval.cc | 120 ++++++++++++++++++++++++++ > gdb/testsuite/gdb.arch/amd64-eval.exp | 43 +++++++++ > 5 files changed, 207 insertions(+), 11 deletions(-) > create mode 100644 gdb/testsuite/gdb.arch/amd64-eval.cc > create mode 100644 gdb/testsuite/gdb.arch/amd64-eval.exp > > diff --git a/gdb/ChangeLog b/gdb/ChangeLog > index 191863e491..ffd9f4d699 100644 > --- a/gdb/ChangeLog > +++ b/gdb/ChangeLog > @@ -1,3 +1,9 @@ > +2019-02-14 Leszek Swirski > + > + * amd64-tdep.c (amd64_classify_aggregate): Use cp_pass_by_reference > + rather than a hand-rolled POD check when checking for forced MEMORY > + classification. > + > 2019-02-12 John Baldwin > > * symfile.c (find_separate_debug_file): Use canonical path of > diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c > index 3f61997d66..d32638a20a 100644 > --- a/gdb/amd64-tdep.c > +++ b/gdb/amd64-tdep.c > @@ -541,17 +541,40 @@ amd64_merge_classes (enum amd64_reg_class class1, enum amd64_reg_class class2) > > static void amd64_classify (struct type *type, enum amd64_reg_class theclass[2]); > > -/* Return non-zero if TYPE is a non-POD structure or union type. */ > +/* Return true if TYPE is a structure or union with unaligned fields. */ > > -static int > -amd64_non_pod_p (struct type *type) > +static bool > +amd64_has_unaligned_fields (struct type *type) > { > - /* ??? A class with a base class certainly isn't POD, but does this > - catch all non-POD structure types? */ > - if (TYPE_CODE (type) == TYPE_CODE_STRUCT && TYPE_N_BASECLASSES (type) > 0) > - return 1; > + if (TYPE_CODE (type) == TYPE_CODE_STRUCT > + || TYPE_CODE (type) == TYPE_CODE_UNION) > + { > + for (int i = 0; i < TYPE_NFIELDS (type); i++) > + { > + struct type *subtype = check_typedef (TYPE_FIELD_TYPE (type, i)); > + int bitpos = TYPE_FIELD_BITPOS (type, i); > + int align = type_align(subtype); > > - return 0; > + /* Ignore static fields, or empty fields, for example nested > + empty structures. */ > + if (field_is_static (&TYPE_FIELD (type, i)) > + || (TYPE_FIELD_BITSIZE (type, i) == 0 > + && TYPE_LENGTH (subtype) == 0)) > + continue; > + > + if (bitpos % 8 != 0) > + return true; > + > + int bytepos = bitpos / 8; > + if (bytepos % align != 0) > + return true; > + > + if (amd64_has_unaligned_fields(subtype)) > + return true; > + } > + } > + > + return false; > } > > /* Classify TYPE according to the rules for aggregate (structures and > @@ -560,10 +583,9 @@ amd64_non_pod_p (struct type *type) > static void > amd64_classify_aggregate (struct type *type, enum amd64_reg_class theclass[2]) > { > - /* 1. If the size of an object is larger than two eightbytes, or in > - C++, is a non-POD structure or union type, or contains > + /* 1. If the size of an object is larger than two eightbytes, or it has > unaligned fields, it has class memory. */ > - if (TYPE_LENGTH (type) > 16 || amd64_non_pod_p (type)) > + if (TYPE_LENGTH (type) > 16 || amd64_has_unaligned_fields (type)) > { > theclass[0] = theclass[1] = AMD64_MEMORY; > return; > diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog > index 1ee4f1bd9b..5fa2b6ef7b 100644 > --- a/gdb/testsuite/ChangeLog > +++ b/gdb/testsuite/ChangeLog > @@ -1,3 +1,8 @@ > +2019-02-14 Leszek Swirski > + > + * gdb.arch/amd64-eval.cc: New file. > + * gdb.arch/amd64-eval.exp: New file. > + > 2019-02-12 Weimin Pan > > PR breakpoints/21870 > diff --git a/gdb/testsuite/gdb.arch/amd64-eval.cc b/gdb/testsuite/gdb.arch/amd64-eval.cc > new file mode 100644 > index 0000000000..a986a49db3 > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/amd64-eval.cc > @@ -0,0 +1,120 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2019 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +#include > +#include > +#include > +#include > + > +/* A simple structure with a single integer field. Should be returned in > + a register. */ > +struct SimpleBase > +{ > + SimpleBase (int32_t x) : x (x) {} > + > + int32_t x; > +}; > + > +/* A simple structure derived from the simple base. Should be returned in > + a register. */ > +struct SimpleDerived : public SimpleBase > +{ > + SimpleDerived (int32_t x) : SimpleBase (x) {} > +}; > + > +/* A structure derived from the simple base with a non-trivial destructor. > + Should be returned on the stack. */ > +struct NonTrivialDestructorDerived : public SimpleBase > +{ > + NonTrivialDestructorDerived (int32_t x) : SimpleBase (x) {} > + ~NonTrivialDestructorDerived() { x = 1; } > +}; > + > +/* A structure with unaligned fields. Should be returned on the stack. */ > +struct UnalignedFields > +{ > + UnalignedFields (int32_t x, double y) : x (x), y (y) {} > + > + int32_t x; > + double y; > +} __attribute__((packed)); > + > +/* A structure with unaligned fields in its base class. Should be > + returned on the stack. */ > +struct UnalignedFieldsInBase : public UnalignedFields > +{ > + UnalignedFieldsInBase (int32_t x, double y, int32_t x2) > + : UnalignedFields (x, y), x2 (x2) {} > + > + int32_t x2; > +}; > + > +class Foo > +{ > +public: > + SimpleBase > + return_simple_base (int32_t x) > + { > + assert (this->tag == EXPECTED_TAG); > + return SimpleBase (x); > + } > + > + SimpleDerived > + return_simple_derived (int32_t x) > + { > + assert (this->tag == EXPECTED_TAG); > + return SimpleDerived (x); > + } > + > + NonTrivialDestructorDerived > + return_non_trivial_destructor (int32_t x) > + { > + assert (this->tag == EXPECTED_TAG); > + return NonTrivialDestructorDerived (x); > + } > + > + UnalignedFields > + return_unaligned (int32_t x, double y) > + { > + assert (this->tag == EXPECTED_TAG); > + return UnalignedFields (x, y); > + } > + > + UnalignedFieldsInBase > + return_unaligned_in_base (int32_t x, double y, int32_t x2) > + { > + assert (this->tag == EXPECTED_TAG); > + return UnalignedFieldsInBase (x, y, x2); > + } > + > +private: > + /* Use a tag to detect if the "this" value is correct. */ > + static const int EXPECTED_TAG = 0xF00F00F0; > + int tag = EXPECTED_TAG; > +}; > + > +int > +main (int argc, char *argv[]) > +{ > + Foo foo; > + foo.return_simple_base(1); > + foo.return_simple_derived(2); > + foo.return_non_trivial_destructor(3); > + foo.return_unaligned(4, 5); > + foo.return_unaligned_in_base(6, 7, 8); > + return 0; // break-here > +} > diff --git a/gdb/testsuite/gdb.arch/amd64-eval.exp b/gdb/testsuite/gdb.arch/amd64-eval.exp > new file mode 100644 > index 0000000000..c33777d2b4 > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/amd64-eval.exp > @@ -0,0 +1,43 @@ > +# This testcase is part of GDB, the GNU debugger. > + > +# Copyright 2019 Free Software Foundation, Inc. > + > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see . > + > +# This testcase exercises evaluation with amd64 calling conventions. > + > +standard_testfile .cc > + > +if { [prepare_for_testing "failed to prepare" $testfile $srcfile \ > + { debug c++ }] } { > + return -1 > +} > + > +if ![runto_main] { > + return -1 > +} > + > +gdb_breakpoint [gdb_get_line_number "break-here"] > +gdb_continue_to_breakpoint "break-here" > + > +gdb_test "call foo.return_simple_base(12)" \ > + " = {x = 12}" > +gdb_test "call foo.return_simple_derived(34)" \ > + " = { = {x = 34}, }" > +gdb_test "call foo.return_non_trivial_destructor(56)" \ > + " = { = {x = 56}, }" > +gdb_test "call foo.return_unaligned(78, 9.25)" \ > + " = {x = 78, y = 9.25}" > +gdb_test "call foo.return_unaligned_in_base(23, 4.5, 67)" \ > + " = { = {x = 23, y = 4.5}, x2 = 67}" >