Re: [PATCH v4 2/3] arc: Add hardware loop detection

Simon Marchi
2020-08-01T14:53:46Z

This patch is also OK with these nits fixed.

On 2020-07-23 3:35 p.m., Shahab Vahedi wrote:
> diff --git a/gdb/arc-tdep.c b/gdb/arc-tdep.c
> index 0c7e045e54b..59e698bb18b 100644
> --- a/gdb/arc-tdep.c
> +++ b/gdb/arc-tdep.c
> @@ -2006,6 +2006,35 @@ arc_check_tdesc_feature (struct tdesc_arch_data *tdesc_data,
> return true;
> }
>
> +/* Check for the existance of "lp_start" and "lp_end" in target description.
> + If both are present, assume there is hardware loop support in the target.
> + This can be improved by looking into "lpc_size" field of "isa_config"
> + auxiliary register. */
> +
> +static bool
> +arc_check_for_hw_loops (const struct target_desc *tdesc,
> + struct tdesc_arch_data *data)
> +{
> + const auto feature_aux = tdesc_find_feature (tdesc, ARC_AUX_FEATURE_NAME);
> + const auto *aux_regset = determine_aux_reg_feature_set ();

To be consistent, I'd suggest using the star in both places or not use it in both places.

> +
> + if (feature_aux == nullptr || aux_regset == nullptr)
> + return false;

If a function (I'm thinking of determine_aux_reg_feature_set) can't (according to its contract)
return nullptr, don't check for nullptr, that's:

(1) unnecessary
(2) confusing, since by reading this code my mind goes searching for a reason why
determine_aux_reg_feature_set might return nullptr

I suppose that tdesc_find_feature can return nullptr, if the feature is not present.

> diff --git a/gdb/arc-tdep.h b/gdb/arc-tdep.h
> index 6ca759a661d..e752348a262 100644
> --- a/gdb/arc-tdep.h
> +++ b/gdb/arc-tdep.h
> @@ -23,6 +23,7 @@
>
> /* Need disassemble_info. */
> #include "dis-asm.h"
> +#include "gdbarch.h"

I suppose that adding this is correct, as this file uses some things from gdbarch.h.

It's a change unrelated to the topic of this patch, so it's preferable to send it as
a separate patch, with its own explanation.

Simon