[PATCH] AArch64: Prevent infinite recursion when checking AAPCS types
Alan Hayward
Alan.Hayward@arm.com
Tue Jan 15 10:30:00 GMT 2019
> On 10 Jan 2019, at 16:07, Pedro Alves <palves@redhat.com> wrote:
>
> On 01/10/2019 01:44 PM, Alan Hayward wrote:
>>
>>
>>> On 9 Jan 2019, at 19:01, Pedro Alves <palves@redhat.com> wrote:
>>>
>>> On 12/12/2018 09:59 AM, Alan Hayward wrote:
>>>> gdb.dwarf2/dw2-cp-infcall-ref-static.exp includes a struct which
>>>> contains itself. Add support to aapcs_is_vfp_call_or_return_candidate to
>>>> prevent infinite recursion.
>>>
>>> But the contained struct is a static field, no? How does that affect
>>> the calling convention (or whatever is being computed) ?
>>>
>>> Without looking at any of this in detail, I'd off hand think that
>>> this code should be skipping/ignoring static fields?
>>>
>>>
>>
>> Hmmm... yes, good spot. Tried this in GCC, and the static members are not
>> passed as args.
>
> Right, C++ static data members aren't passed anywhere, they're just globals
> with fixed addresses. You can think of:
>
> struct A
> {
> static A a;
> };
>
> Basically the same as:
>
> struct A
> {
> };
> A a;
>
> The difference is mainly that in the former, the global is
> called 'A::a', while in the latter it's called 'a'. Not that
> much different from, say:
>
> struct A
> {
> };
> namespace NS
> {
> A a;
> };
>
Just an fyi,
By creating a c++ test case, I’ve also found some issues with empty structs
inside structs (c++ classes have a minimum size of 0, causing g++/clang to
reject passing it in registers because it’s not aligned properly).
Also, looks like it gdb_asserts when running the test on x86 too.
This patch will still be needed for infinite recursive structs, but I’ll throw
it in as part of a new patch series.
Alan.
> Thanks,
> Pedro Alves
>
>> GCC doesn’t have any explicit code to cover this in the
>> backend - I suspect that by the time it gets there the static members are
>> effectivley already in a special global var.
>
>
>>
>> I’ll write up some new test cases to cover the cases with normal structs with
>> static members.
>> And then I’ll see if I generate some tests with non-static infinite structs.
>> Maybe it’ll squash the need for this patch.
>>
>>
>> Thanks,
>> Alan.
>>
>>
>>>>
>>>> The simple solution is to pass a counter through the functions and abort once
>>>> the max limit is reached. However, this does not catch the case where a
>>>> struct only contains itself and no other members. That can be solved by
>>>> marking a type as invalid before checking all of it's children.
>>>>
>>>> Add a count for the call or ret candidate to main_type, restricted to 3
>>>> bits - the value 5 (or above) is used as invalid.
>>>>
>>>> This code is testsed by both gdb.base/infcall-nested-structs.exp
>>>> and gdb.dwarf2/dw2-cp-infcall-ref-static.exp.
>>>>
>>>> gdb/ChangeLog:
>>>>
>>>> 2018-12-12 Alan Hayward <alan.hayward@arm.com>
>>>>
>>>> * aarch64-tdep.c (HA_MAX_NUM_FLDS): Remove define.
>>>> (aapcs_is_vfp_call_or_return_candidate_1): Remember type.
>>>> (aapcs_is_vfp_call_or_return_candidate): Likewise.
>>>> * gdbtypes.h (AAPCS_VFP_CALL_OR_RET_CAND_MAX): Add define.
>>>> (AAPCS_VFP_CALL_OR_RET_CAND_INVALID): Likewise.
>>>> (TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE): Likewise.
>>>> (TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE): Likewise.
>>>> (struct main_type): Add aapcs_candidate_count.
>>>> ---
>>>> gdb/aarch64-tdep.c | 87 ++++++++++++++++++++++++++--------------------
>>>> gdb/gdbtypes.h | 23 ++++++++++++
>>>> 2 files changed, 72 insertions(+), 38 deletions(-)
>>>>
>>>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>>>> index ae56c9ca34..3fc58308d6 100644
>>>> --- a/gdb/aarch64-tdep.c
>>>> +++ b/gdb/aarch64-tdep.c
>>>> @@ -64,10 +64,6 @@
>>>> #define bit(obj,st) (((obj) >> (st)) & 1)
>>>> #define bits(obj,st,fn) (((obj) >> (st)) & submask ((fn) - (st)))
>>>>
>>>> -/* A Homogeneous Floating-Point or Short-Vector Aggregate may have at most
>>>> - four members. */
>>>> -#define HA_MAX_NUM_FLDS 4
>>>> -
>>>> /* All possible aarch64 target descriptors. */
>>>> struct target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1];
>>>>
>>>> @@ -1146,46 +1142,58 @@ aarch64_type_align (struct type *t)
>>>>
>>>> /* Worker function for aapcs_is_vfp_call_or_return_candidate.
>>>>
>>>> - Return the number of register required, or -1 on failure.
>>>> + Set TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE to the number of registers required,
>>>> + or AAPCS_VFP_CALL_OR_RET_CAND_INVALID on failure.
>>>>
>>>> When encountering a base element, if FUNDAMENTAL_TYPE is not set then set it
>>>> to the element, else fail if the type of this element does not match the
>>>> existing value. */
>>>>
>>>> -static int
>>>> +static void
>>>> aapcs_is_vfp_call_or_return_candidate_1 (struct type *type,
>>>> struct type **fundamental_type)
>>>> {
>>>> if (type == nullptr)
>>>> - return -1;
>>>> + return;
>>>> +
>>>> + /* Check if we have already evaluated this TYPE. */
>>>> + if (TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type)
>>>> + >= AAPCS_VFP_CALL_OR_RET_CAND_INVALID)
>>>> + return;
>>>> +
>>>> + /* Set TYPE to invalid to prevent infinite recursion. */
>>>> + TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type,
>>>> + AAPCS_VFP_CALL_OR_RET_CAND_INVALID);
>>>>
>>>> switch (TYPE_CODE (type))
>>>> {
>>>> case TYPE_CODE_FLT:
>>>> if (TYPE_LENGTH (type) > 16)
>>>> - return -1;
>>>> + return;
>>>>
>>>> if (*fundamental_type == nullptr)
>>>> *fundamental_type = type;
>>>> else if (TYPE_LENGTH (type) != TYPE_LENGTH (*fundamental_type)
>>>> || TYPE_CODE (type) != TYPE_CODE (*fundamental_type))
>>>> - return -1;
>>>> + return;
>>>>
>>>> - return 1;
>>>> + TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, 1);
>>>> + return;
>>>>
>>>> case TYPE_CODE_COMPLEX:
>>>> {
>>>> struct type *target_type = check_typedef (TYPE_TARGET_TYPE (type));
>>>> if (TYPE_LENGTH (target_type) > 16)
>>>> - return -1;
>>>> + return;
>>>>
>>>> if (*fundamental_type == nullptr)
>>>> *fundamental_type = target_type;
>>>> else if (TYPE_LENGTH (target_type) != TYPE_LENGTH (*fundamental_type)
>>>> || TYPE_CODE (target_type) != TYPE_CODE (*fundamental_type))
>>>> - return -1;
>>>> + return;
>>>>
>>>> - return 2;
>>>> + TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, 2);
>>>> + return;
>>>> }
>>>>
>>>> case TYPE_CODE_ARRAY:
>>>> @@ -1193,28 +1201,33 @@ aapcs_is_vfp_call_or_return_candidate_1 (struct type *type,
>>>> if (TYPE_VECTOR (type))
>>>> {
>>>> if (TYPE_LENGTH (type) != 8 && TYPE_LENGTH (type) != 16)
>>>> - return -1;
>>>> + return;
>>>>
>>>> if (*fundamental_type == nullptr)
>>>> *fundamental_type = type;
>>>> else if (TYPE_LENGTH (type) != TYPE_LENGTH (*fundamental_type)
>>>> || TYPE_CODE (type) != TYPE_CODE (*fundamental_type))
>>>> - return -1;
>>>> + return;
>>>>
>>>> - return 1;
>>>> + TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, 1);
>>>> }
>>>> else
>>>> {
>>>> + /* Calculate if type of an element in the array is valid. */
>>>> struct type *target_type = TYPE_TARGET_TYPE (type);
>>>> - int count = aapcs_is_vfp_call_or_return_candidate_1
>>>> - (target_type, fundamental_type);
>>>> + aapcs_is_vfp_call_or_return_candidate_1 (target_type,
>>>> + fundamental_type);
>>>>
>>>> - if (count == -1)
>>>> - return count;
>>>> + int count = TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (target_type);
>>>> + if (count >= AAPCS_VFP_CALL_OR_RET_CAND_INVALID)
>>>> + return;
>>>> +
>>>> + /* Expand to cover the whole array. The set handles overflow. */
>>>>
>>>> count *= (TYPE_LENGTH (type) / TYPE_LENGTH (target_type));
>>>> - return count;
>>>> + TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, count);
>>>> }
>>>> + return;
>>>> }
>>>>
>>>> case TYPE_CODE_STRUCT:
>>>> @@ -1226,20 +1239,23 @@ aapcs_is_vfp_call_or_return_candidate_1 (struct type *type,
>>>> {
>>>> struct type *member = check_typedef (TYPE_FIELD_TYPE (type, i));
>>>>
>>>> - int sub_count = aapcs_is_vfp_call_or_return_candidate_1
>>>> - (member, fundamental_type);
>>>> - if (sub_count == -1)
>>>> - return -1;
>>>> + aapcs_is_vfp_call_or_return_candidate_1 (member, fundamental_type);
>>>> +
>>>> + int sub_count = TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (member);
>>>> +
>>>> + if (sub_count + count > AAPCS_VFP_CALL_OR_RET_CAND_MAX)
>>>> + return;
>>>> +
>>>> count += sub_count;
>>>> }
>>>> - return count;
>>>> +
>>>> + TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type, count);
>>>> + return;
>>>> }
>>>>
>>>> default:
>>>> - break;
>>>> + return;
>>>> }
>>>> -
>>>> - return -1;
>>>> }
>>>>
>>>> /* Return true if an argument, whose type is described by TYPE, can be passed or
>>>> @@ -1269,16 +1285,11 @@ aapcs_is_vfp_call_or_return_candidate (struct type *type, int *count,
>>>>
>>>> *fundamental_type = nullptr;
>>>>
>>>> - int ag_count = aapcs_is_vfp_call_or_return_candidate_1 (type,
>>>> - fundamental_type);
>>>> + aapcs_is_vfp_call_or_return_candidate_1 (type, fundamental_type);
>>>>
>>>> - if (ag_count > 0 && ag_count <= HA_MAX_NUM_FLDS)
>>>> - {
>>>> - *count = ag_count;
>>>> - return true;
>>>> - }
>>>> - else
>>>> - return false;
>>>> + *count = TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (type);
>>>> +
>>>> + return (*count < AAPCS_VFP_CALL_OR_RET_CAND_INVALID);
>>>> }
>>>>
>>>> /* AArch64 function call information structure. */
>>>> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
>>>> index f0adec7a20..600112abf5 100644
>>>> --- a/gdb/gdbtypes.h
>>>> +++ b/gdb/gdbtypes.h
>>>> @@ -391,6 +391,23 @@ DEF_ENUM_FLAGS_TYPE (enum type_instance_flag_value, type_instance_flags);
>>>> #define TYPE_ADDRESS_CLASS_ALL(t) (TYPE_INSTANCE_FLAGS(t) \
>>>> & TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL)
>>>>
>>>> +
>>>> +/* * Is the given TYPE able to be placed in an Aarch64 VFP when used as a call
>>>> + parameter or return parameter, as per the AAPCS64 5.4.2.C. Value can be:
>>>> + 0: the type has not been checked.
>>>> + 1 to AAPCS_VFP_CALL_OR_RET_CAND_MAX: Valid, and has that many members.
>>>> + AAPCS_VFP_CALL_OR_RET_CAND_MAX: Invalid. */
>>>> +
>>>> +#define AAPCS_VFP_CALL_OR_RET_CAND_MAX 4
>>>> +#define AAPCS_VFP_CALL_OR_RET_CAND_INVALID (AAPCS_VFP_CALL_OR_RET_CAND_MAX + 1)
>>>> +
>>>> +#define TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE(thistype) \
>>>> + (TYPE_MAIN_TYPE (thistype)->aapcs_candidate_count)
>>>> +
>>>> +#define TYPE_SET_AAPCS_VFP_CALL_OR_RET_CANDIDATE(thistype, val) \
>>>> + TYPE_AAPCS_VFP_CALL_OR_RET_CANDIDATE (thistype) = \
>>>> + (std::min (val, AAPCS_VFP_CALL_OR_RET_CAND_MAX + 1))
>>>> +
>>>> /* * Information needed for a discriminated union. A discriminated
>>>> union is handled somewhat differently from an ordinary union.
>>>>
>>>> @@ -716,6 +733,12 @@ struct main_type
>>>>
>>>> ENUM_BITFIELD(type_specific_kind) type_specific_field : 3;
>>>>
>>>> + /* * Is the given TYPE able to be placed in an Aarch64 VFP when used as a call
>>>> + parameter or return parameter, as per the AAPCS64 5.4.2.C. Required to
>>>> + prevent recursive checking. */
>>>> +
>>>> + unsigned int aapcs_candidate_count : 3;
>>>> +
>>>> /* * Number of fields described for this type. This field appears
>>>> at this location because it packs nicely here. */
More information about the Gdb-patches
mailing list