This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v3] [amd64] Fix AMD64 return value ABI in expression evaluation
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 <leszeks@google.com>
> +
> + * 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 <jhb@FreeBSD.org>
>
> * 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 <leszeks@google.com>
> +
> + * gdb.arch/amd64-eval.cc: New file.
> + * gdb.arch/amd64-eval.exp: New file.
> +
> 2019-02-12 Weimin Pan <weimin.pan@oracle.com>
>
> 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 <http://www.gnu.org/licenses/>. */
> +
> +#include <cstdint>
> +#include <cstdio>
> +#include <cstdlib>
> +#include <cassert>
> +
> +/* 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 <http://www.gnu.org/licenses/>.
> +
> +# 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)" \
> + " = {<SimpleBase> = {x = 34}, <No data fields>}"
> +gdb_test "call foo.return_non_trivial_destructor(56)" \
> + " = {<SimpleBase> = {x = 56}, <No data fields>}"
> +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)" \
> + " = {<UnalignedFields> = {x = 23, y = 4.5}, x2 = 67}"
>