From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13680 invoked by alias); 14 Feb 2019 14:28:48 -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 12748 invoked by uid 89); 14 Feb 2019 14:28:47 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,KAM_SHORT,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy=gdbpatches, gdb-patches X-HELO: mail-wr1-f49.google.com Received: from mail-wr1-f49.google.com (HELO mail-wr1-f49.google.com) (209.85.221.49) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 14 Feb 2019 14:28:44 +0000 Received: by mail-wr1-f49.google.com with SMTP id x10so6678727wrs.8 for ; Thu, 14 Feb 2019 06:28:44 -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 n2sm3169109wrq.58.2019.02.14.06.28.41 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 14 Feb 2019 06:28:41 -0800 (PST) Subject: Re: [PATCH] [amd64] Fix AMD64 return value ABI in expression evaluation To: Leszek Swirski , gdb-patches@sourceware.org References: <20190214133507.38697-1-leszeks@google.com> From: Pedro Alves Message-ID: <45c95dd6-7ea2-5428-c520-974452c6db89@redhat.com> Date: Thu, 14 Feb 2019 14:28: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: <20190214133507.38697-1-leszeks@google.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-02/txt/msg00219.txt.bz2 On 02/14/2019 01:35 PM, Leszek Swirski via gdb-patches 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. Thanks! I wonder whether this fixes PR gdb/24104, . Generally looks good. Some minor comments below. > > 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. Watch out for tabs vs spaces in the ChangeLog entry above. > --- > gdb/ChangeLog | 6 ++ > gdb/amd64-tdep.c | 46 +++++++++--- > gdb/testsuite/ChangeLog | 5 ++ > gdb/testsuite/gdb.arch/amd64-eval.cc | 103 ++++++++++++++++++++++++++ > gdb/testsuite/gdb.arch/amd64-eval.exp | 43 +++++++++++ > 5 files changed, 193 insertions(+), 10 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..d2c518ed6d 100644 > --- a/gdb/amd64-tdep.c > +++ b/gdb/amd64-tdep.c > @@ -541,15 +541,42 @@ 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 non-zero if TYPE is a structure or union with unaligned fields. */ > > static int > -amd64_non_pod_p (struct type *type) Since you're basically rewriting this, please make it return bool and use true/false. Update the "non-zero" in the comment above as well. > -{ > - /* ??? 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; > +amd64_has_unaligned_fields (struct type *type) { Formatting, { does on the next line. > + > + if (TYPE_CODE (type) == TYPE_CODE_STRUCT > + || TYPE_CODE (type) == TYPE_CODE_UNION) > + { > + int i; > + > + for (i = 0; i < TYPE_NFIELDS (type); i++) Write: 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); > + int bytepos; Can declare bytepod below where it's initialized. > + > + /* Ignore static fields, or empty fields, for example nested > + empty structures.*/ Missing double space after the period. > + if (field_is_static (&TYPE_FIELD (type, i)) > + || (TYPE_FIELD_BITSIZE (type, i) == 0 > + && TYPE_LENGTH (subtype) == 0)) > + continue; > + > + if (bitpos % 8 != 0) > + return 1; > + bytepos = bitpos / 8; > + > + if (bytepos % align != 0) > + return 1; > + > + if (amd64_has_unaligned_fields(subtype)) > + return 1; > + } > + > + } > > return 0; > } > @@ -560,10 +587,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..81f9ba589c > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/amd64-eval.cc > @@ -0,0 +1,103 @@ > +/* 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 > + > +// A simple structure with a single integer field. Should be returned in > +// a register. We follow GDB's conventions in testcases as well. Use /**/ comments throughout, and double space after periods. > +struct SimpleBase { { on the next line. > + int32_t x; Missing cstdint include for int32_t. > +}; > + > +// A simple structure derived from the simple base. Should be returned in > +// a register. > +struct SimpleDerived : public SimpleBase {}; > + > +// A structure derived from the simple base with a non-trivial destructor. > +// Should be returned on the stack. > +struct NonTrivialDestructorDerived : public SimpleBase { { on the next line. > + ~NonTrivialDestructorDerived() { x = 1; } > +}; > + > +// A structure with unaligned fields. Should be returned on the stack. > +struct UnalignedFields { > + int32_t x; { on the next line. > + double y; > +} __attribute__((packed)); > + > +// A structure with unaligned fields in its base class. Should be > +// returned on the stack. > +struct UnalignedFieldsInBase : public UnalignedFields { { on the next line. > + int32_t x2; > +}; > + > +class Foo { { on the next line. > +public: > + SimpleBase > + return_simple_base () > + { > + assert(this->tag == EXPECTED_TAG); > + return SimpleBase(); Missing space before parens, both in assert call and in ctor call. Happens multiple times. > + } > + > + SimpleDerived > + return_simple_derived () > + { > + assert(this->tag == EXPECTED_TAG); > + return SimpleDerived(); > + } > + > + NonTrivialDestructorDerived > + return_non_trivial_destructor () > + { > + assert(this->tag == EXPECTED_TAG); > + return NonTrivialDestructorDerived(); > + } > + > + UnalignedFields > + return_unaligned () > + { > + assert(this->tag == EXPECTED_TAG); > + return UnalignedFields(); > + } > + > + UnalignedFieldsInBase > + return_unaligned_in_base () > + { > + assert(this->tag == EXPECTED_TAG); > + return UnalignedFieldsInBase(); > + } > + > +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(); Space before parens. > + foo.return_simple_derived(); > + foo.return_non_trivial_destructor(); > + foo.return_unaligned(); > + foo.return_unaligned_in_base(); > + 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..f3da83e4ca > --- /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()" \ > + ".* = {x = 0}" Leading ".*" is unnecessary -- it's implied. > +gdb_test "call foo.return_simple_derived()" \ > + ".* = { = {x = 0}, }" > +gdb_test "call foo.return_non_trivial_destructor()" \ > + ".* = { = {x = 0}, }" > +gdb_test "call foo.return_unaligned()" \ > + ".* = {x = 0, y = 0}" > +gdb_test "call foo.return_unaligned_in_base()" \ > + ".* = { = {x = 0, y = 0}, x2 = 0}" > Is it possible to use values other than "0" in x/y/x2 in the success cases? That tends to be more robust to changes, because it's easier to hit zeroed memory with a bug and not notice it than some other value. Thanks, Pedro Alves