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

[Simon Marchi]

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