From patchwork Thu Feb 14 15:18:10 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Terekhov, Mikhail via Gdb-patches" X-Patchwork-Id: 31470 Received: (qmail 113633 invoked by alias); 14 Feb 2019 15:18:28 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 113624 invoked by uid 89); 14 Feb 2019 15:18:28 -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, SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-vk1-f201.google.com Received: from mail-vk1-f201.google.com (HELO mail-vk1-f201.google.com) (209.85.221.201) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 14 Feb 2019 15:18:25 +0000 Received: by mail-vk1-f201.google.com with SMTP id a124so2604745vka.11 for ; Thu, 14 Feb 2019 07:18:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:in-reply-to:message-id:mime-version:references:subject:from:to :cc; bh=le/SzRzTrV8k7601gzv+sRCUoiUpyboIMgTr8MOATNs=; b=P3Wo/+UelzLixM8R6ypdFhYJzAnXM6P6cxTePSZbF/EzFyd/HcXKfTzLJ19pknqlLQ rcfGO1RnDKsmXC9YBVs+iJdrBVQVtb8tcDYWB35ioZ/mznObo1ijQ6SQQ9cQjDZSzFBF iES0BKNkBW734K6QQX+i1/ppENJSLvuSwmpynG2KwKqxRxj8ExpmYyySalQJi3DpBbDp KU4sOM87Uy5lIP4QfWEsjvEC1GNig/MAhcHH4RSZAZjQbvAnHTcXZ3Ak6CrSYyYxVo53 ngd+sLmd56b/wjbXHbHS99/X6Na1FOAeMPa4PRvymv7h7eVPLmzU8gTugXHhnbDyk+DK B0Xw== Date: Thu, 14 Feb 2019 16:18:10 +0100 In-Reply-To: <20190214151602.147300-1-leszeks@google.com> Message-Id: <20190214151810.149322-1-leszeks@google.com> Mime-Version: 1.0 References: <20190214151602.147300-1-leszeks@google.com> Subject: [PATCH v3] [amd64] Fix AMD64 return value ABI in expression evaluation X-Patchwork-Original-From: "Leszek Swirski via gdb-patches" From: "Terekhov, Mikhail via Gdb-patches" Reply-To: Leszek Swirski To: gdb-patches@sourceware.org Cc: palves@redhat.com, Leszek Swirski 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}"