[PATCH v2 3/4] aarch64: improve assembly debug comments for AEABI build attributes

[Richard Sandiford]

Matthieu Longo <matthieu.longo@arm.com> writes:
> The previous implementation to emit AEABI build attributes did not
> support string values (asciz) in aeabi_subsection, and was not
> emitting values associated to tags in the assembly comments.
>
> This new approach provides a more user-friendly interface relying on
> typing, and improves the emitted assembly comments:
>   * aeabi_attribute:
>     ** Adds the interpreted value next to the tag in the assembly
>     comment.
>     ** Supports asciz values.
>   * aeabi_subsection:
>     ** Adds debug information for its parameters.
>     ** Auto-detects the attribute types when declaring the subsection.
>
> Additionally, it is also interesting to note that the code was moved
> to a separate file to improve modularity and "releave" the 1000-lines

I think you dropped a 0.  I wish it was only 1000 :-)

> long aarch64.cc file from a few lines. Finally, it introduces a new
> namespace "aarch64::" for AArch64 backend which reduce the length of
> function names by not prepending 'aarch64_' to each of them.
> [...]
> diff --git a/gcc/config/aarch64/aarch64-dwarf-metadata.h b/gcc/config/aarch64/aarch64-dwarf-metadata.h
> new file mode 100644
> index 00000000000..01f08ad073e
> --- /dev/null
> +++ b/gcc/config/aarch64/aarch64-dwarf-metadata.h
> @@ -0,0 +1,226 @@
> +/* DWARF metadata for AArch64 architecture.
> +   Copyright (C) 2024 Free Software Foundation, Inc.
> +   Contributed by ARM Ltd.
> +
> +   This file is part of GCC.
> +
> +   GCC 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, or (at your option)
> +   any later version.
> +
> +   GCC 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 GCC; see the file COPYING3.  If not see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef GCC_AARCH64_DWARF_METADATA_H
> +#define GCC_AARCH64_DWARF_METADATA_H
> +
> +#include "system.h"

We should drop this line.  It's the .cc file's responsibility to
include system.h.

> +#include "vec.h"
> +
> +namespace aarch64 {
> +
> +enum attr_val_type : uint8_t
> +{
> +  uleb128 = 0x0,
> +  asciz = 0x1,
> +};
> +
> +enum BA_TagFeature_t : uint8_t
> +{
> +  Tag_Feature_BTI = 1,
> +  Tag_Feature_PAC = 2,
> +  Tag_Feature_GCS = 3,
> +};
> +
> +template <typename T_tag, typename T_val>
> +struct aeabi_attribute
> +{
> +  T_tag tag;
> +  T_val value;
> +};
> +
> +template <typename T_tag, typename T_val>
> +aeabi_attribute<T_tag, T_val>
> +make_aeabi_attribute (T_tag tag, T_val val)
> +{
> +  return aeabi_attribute<T_tag, T_val>{tag, val};
> +}
> +
> +namespace details {
> +
> +constexpr const char *
> +to_c_str (bool b)
> +{
> +  return b ? "true" : "false";
> +}
> +
> +constexpr const char *
> +to_c_str (const char *s)
> +{
> +  return s;
> +}
> +
> +constexpr const char *
> +to_c_str (attr_val_type t)
> +{
> +  return (t == uleb128 ? "ULEB128"
> +	  : t == asciz ? "asciz"
> +	  : nullptr);
> +}
> +
> +constexpr const char *
> +to_c_str (BA_TagFeature_t feature)
> +{
> +  return (feature == Tag_Feature_BTI ? "Tag_Feature_BTI"
> +	  : feature == Tag_Feature_PAC ? "Tag_Feature_PAC"
> +	  : feature == Tag_Feature_GCS ? "Tag_Feature_GCS"
> +	  : nullptr);
> +}
> +
> +template <
> +  typename T,
> +  typename = typename std::enable_if<std::is_unsigned<T>::value, T>::type
> +>
> +constexpr const char *
> +aeabi_attr_str_fmt (T phantom __attribute__((unused)))

FWIW, it would be ok to drop the parameter name and the attribute.
But it's ok as-is too, if you think it makes the intention clearer.

> +{
> +  return "\t.aeabi_attribute %u, %u";
> +}
> +
> +constexpr const char *
> +aeabi_attr_str_fmt (const char *phantom __attribute__((unused)))
> +{
> +  return "\t.aeabi_attribute %u, \"%s\"";
> +}
> [...]
> @@ -24834,17 +24808,21 @@ aarch64_start_file (void)
>     asm_fprintf (asm_out_file, "\t.arch %s\n",
>  		aarch64_last_printed_arch_string.c_str ());
>  
> -  /* Check whether the current assembly supports gcs build attributes, if not
> -     fallback to .note.gnu.property section.  */
> +  /* Check whether the current assembler supports AEABI build attributes, if
> +     not fallback to .note.gnu.property section.  */
>  #if (HAVE_AS_AEABI_BUILD_ATTRIBUTES)

Just to note that, as with patch 2, I hope this could be:

  if (HAVE_AS_AEABI_BUILD_ATTRIBUTES)
    {
      ...
    }

instead.

OK with those changes, thanks.

Richard