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

Message ID 20241023103600.500910-4-matthieu.longo@arm.com
State New
Headers
Series aarch64: add minimal support of AEABI build attributes for GCS |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 fail Patch failed to apply

Commit Message

Matthieu Longo Oct. 23, 2024, 10:35 a.m. UTC
  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
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.

gcc/ChangeLog:

	* config/aarch64/aarch64.cc
	(aarch64_emit_aeabi_attribute): Delete.
	(aarch64_emit_aeabi_subsection): Delete.
	(aarch64_start_file): Use aeabi_subsection.
	* config/aarch64/aarch64-dwarf-metadata.h: New file.

gcc/testsuite/ChangeLog:

	* gcc.target/aarch64/build-attributes/build-attribute-gcs.c:
	Improve test to match debugging comments in assembly.
	* gcc.target/aarch64/build-attributes/build-attribute-standard.c:
	Likewise.
---
 gcc/config/aarch64/aarch64-dwarf-metadata.h   | 226 ++++++++++++++++++
 gcc/config/aarch64/aarch64.cc                 |  48 +---
 .../build-attributes/build-attribute-gcs.c    |   4 +-
 .../build-attribute-standard.c                |   4 +-
 4 files changed, 243 insertions(+), 39 deletions(-)
 create mode 100644 gcc/config/aarch64/aarch64-dwarf-metadata.h
  

Comments

Richard Sandiford Oct. 23, 2024, 4:40 p.m. UTC | #1
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
  

Patch

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"
+#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)))
+{
+  return "\t.aeabi_attribute %u, %u";
+}
+
+constexpr const char *
+aeabi_attr_str_fmt (const char *phantom __attribute__((unused)))
+{
+  return "\t.aeabi_attribute %u, \"%s\"";
+}
+
+template <
+  typename T,
+  typename = typename std::enable_if<std::is_unsigned<T>::value, T>::type
+>
+constexpr uint8_t
+aeabi_attr_val_for_fmt (T value)
+{
+  return static_cast<uint8_t>(value);
+}
+
+constexpr const char *
+aeabi_attr_val_for_fmt (const char *s)
+{
+  return s;
+}
+
+template <typename T_tag, typename T_val>
+void
+write (FILE *out_file, aeabi_attribute<T_tag, T_val> const &attr)
+{
+  asm_fprintf (out_file, aeabi_attr_str_fmt (T_val{}),
+	       attr.tag, aeabi_attr_val_for_fmt (attr.value));
+  if (flag_debug_asm)
+    asm_fprintf (out_file, "\t%s %s: %s", ASM_COMMENT_START,
+		 to_c_str (attr.tag), to_c_str (attr.value));
+  asm_fprintf (out_file, "\n");
+}
+
+template <
+  typename T,
+  typename = typename std::enable_if<std::is_unsigned<T>::value, T>::type
+>
+constexpr attr_val_type
+deduce_attr_av_type (T value __attribute__((unused)))
+{
+  return attr_val_type::uleb128;
+}
+
+constexpr attr_val_type
+deduce_attr_av_type (const char *value __attribute__((unused)))
+{
+  return attr_val_type::asciz;
+}
+
+} // namespace details
+
+/* AEABI subsections can be public or private.  A subsection is public if it is
+   prefixed with "aeabi", private otherwise.  The header of an AEABI subsection
+   is composed of a name (usually a vendor name), a status whether it is
+   optional or not, and the expected type of its associated attributes (ULEB128
+   or asciz).  Note: The attributes in the same subsection have all the same
+   type.  An attribute is composed of a tag identifier (ULEB128), and its value
+   (ULEB128 or asciz).
+
+   Syntax:
+     .aeabi_subsection NameOfTheSubsection: string (=asciz),
+		       Optional: boolean (=ULEB128),
+		       AttributeValueType: enum{ULEB128, asciz} (=ULEB128)
+     [
+       .aeabi_attribute  TagIdentifier: ULEB128,
+			 TagValue: Variant[ULEB128|asciz]
+     ]*
+
+   Example:
+     .aeabi_subsection .aeabi-feature-and-bits, 1, 0
+		       // Optional: true, AttrValType: ULEB128
+     .aeabi_attribute 3, 1 // Tag_Feature_GCS: true
+
+   Note: The textual representations of the tag and its value are emitted as a
+   comment along their numerical representations to annotate the assembler
+   output when the developer flag '-dA' is provided.  */
+template <
+  typename T_tag, /* The type of a tag.  */
+  typename T_val, /* The type of a value.  */
+  size_t N = 0    /* The number of expected attributes if we know it.  */
+>
+class aeabi_subsection
+{
+ public:
+  aeabi_subsection (const char *name, bool optional)
+    : m_name (name),
+      m_optional (optional),
+      m_avtype (details::deduce_attr_av_type (T_val{}))
+  {}
+
+  /* Append an attribute to the subsection.  */
+  void append (aeabi_attribute<T_tag, T_val> &&attr)
+  {
+    m_attributes.quick_push (std::move (attr));
+  }
+
+  /* Write the data to the assembly file.  */
+  void write (FILE *out_file) const
+  {
+    asm_fprintf (out_file, "\t.aeabi_subsection %s, %u, %u",
+		 m_name, static_cast<uint8_t> (m_optional),
+		 static_cast<uint8_t> (m_avtype));
+    if (flag_debug_asm)
+      asm_fprintf (out_file, "\t%s Optional: %s, AttrValType: %s",
+		   ASM_COMMENT_START,
+		   details::to_c_str (m_optional),
+		   details::to_c_str (m_avtype));
+    asm_fprintf (out_file, "\n");
+
+    for (auto const &attr : m_attributes)
+      details::write (out_file, attr);
+  }
+
+  /* Indicate if the subsection is empty.  */
+  bool empty () const
+  {
+    return m_attributes.is_empty ();
+  }
+
+ private:
+  const char *m_name;
+  bool m_optional;
+  attr_val_type m_avtype;
+  auto_vec<aeabi_attribute<T_tag, T_val>, N> m_attributes;
+};
+
+} // namespace aarch64
+
+#endif /* GCC_AARCH64_DWARF_METADATA_H */
\ No newline at end of file
diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index 90aba0fff88..e5a94738f70 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -84,6 +84,7 @@ 
 #include "rtlanal.h"
 #include "tree-dfa.h"
 #include "asan.h"
+#include "aarch64-dwarf-metadata.h"
 #include "aarch64-feature-deps.h"
 #include "config/arm/aarch-common.h"
 #include "config/arm/aarch-common-protos.h"
@@ -24787,33 +24788,6 @@  aarch64_post_cfi_startproc (FILE *f, tree ignored ATTRIBUTE_UNUSED)
 	asm_fprintf (f, "\t.cfi_b_key_frame\n");
 }
 
-/* This function is used to emit an AEABI attribute with tag and its associated
-   value.  We emit the numerical value of the tag and the textual tags as
-   comment so that anyone reading the assembler output will know which tag is
-   being set.
-   example: .aeabi_attribute 3, 1 // Tag_Feature_GCS  */
-
-void
-aarch64_emit_aeabi_attribute (const char *name, uint8_t num, uint8_t val)
-{
-  asm_fprintf (asm_out_file, "\t.aeabi_attribute %u, %u", num, val);
-  if (flag_debug_asm)
-    asm_fprintf (asm_out_file, "\t%s %s", ASM_COMMENT_START, name);
-  asm_fprintf (asm_out_file, "\n");
-}
-
-/* This function is used to emit an AEABI subsection with vendor name,
-   optional status and value type.
-   example: .aeabi_subsection .aeabi-feature-and-bits, 1, 0  */
-
-void
-aarch64_emit_aeabi_subsection (const char *name, uint8_t num, uint8_t val)
-{
-  asm_fprintf (asm_out_file, "\t.aeabi_subsection %s, %u, %u\n",
-	       name, num, val);
-}
-
-
 /* Implements TARGET_ASM_FILE_START.  Output the assembly header.  */
 
 static void
@@ -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)
-  if (aarch64_gcs_enabled ())
-    {
-      aarch64_emit_aeabi_subsection (".aeabi-feature-and-bits", 1, 0);
-      aarch64_emit_aeabi_attribute ("Tag_Feature_GCS", 3, 1);
-    }
+  using namespace aarch64;
+  aeabi_subsection<BA_TagFeature_t, bool, 3>
+    aeabi_subsec (".aeabi-feature-and-bits", true);
+
+  aeabi_subsec.append (
+    make_aeabi_attribute (Tag_Feature_GCS, aarch64_gcs_enabled ()));
+
+  if (!aeabi_subsec.empty ())
+    aeabi_subsec.write (asm_out_file);
 #endif
 
-   default_file_start ();
+  default_file_start ();
 }
 
 /* Emit load exclusive.  */
diff --git a/gcc/testsuite/gcc.target/aarch64/build-attributes/build-attribute-gcs.c b/gcc/testsuite/gcc.target/aarch64/build-attributes/build-attribute-gcs.c
index 26938d84dab..bafd3261b22 100644
--- a/gcc/testsuite/gcc.target/aarch64/build-attributes/build-attribute-gcs.c
+++ b/gcc/testsuite/gcc.target/aarch64/build-attributes/build-attribute-gcs.c
@@ -6,7 +6,7 @@  int main()
   return 0;
 }
 
-/* { dg-final { scan-assembler "\.aeabi_subsection \.aeabi-feature-and-bits, 1, 0" } } */
-/* { dg-final { scan-assembler "\.aeabi_attribute 3, 1\t\/\/ Tag_Feature_GCS" } } */
+/* { dg-final { scan-assembler "\.aeabi_subsection \.aeabi-feature-and-bits, 1, 0\t\/\/ Optional: true, AttrValType: ULEB128" } } */
+/* { dg-final { scan-assembler "\.aeabi_attribute 3, 1\t\/\/ Tag_Feature_GCS: true" } } */
 /* { dg-final { scan-assembler "\.section\t\.note\.gnu\.property" } } */
 /* { dg-final { scan-assembler "\.word\t0x4\t\/\/ GNU_PROPERTY_AARCH64_FEATURE_1_AND \\(GCS\\)" } } */
\ No newline at end of file
diff --git a/gcc/testsuite/gcc.target/aarch64/build-attributes/build-attribute-standard.c b/gcc/testsuite/gcc.target/aarch64/build-attributes/build-attribute-standard.c
index 6a41b52d298..1ca7166ad29 100644
--- a/gcc/testsuite/gcc.target/aarch64/build-attributes/build-attribute-standard.c
+++ b/gcc/testsuite/gcc.target/aarch64/build-attributes/build-attribute-standard.c
@@ -6,7 +6,7 @@  int main()
   return 0;
 }
 
-/* { dg-final { scan-assembler "\.aeabi_subsection \.aeabi-feature-and-bits, 1, 0" } } */
-/* { dg-final { scan-assembler "\.aeabi_attribute 3, 1\t\/\/ Tag_Feature_GCS" } } */
+/* { dg-final { scan-assembler "\.aeabi_subsection \.aeabi-feature-and-bits, 1, 0\t\/\/ Optional: true, AttrValType: ULEB128" } } */
+/* { dg-final { scan-assembler "\.aeabi_attribute 3, 1\t\/\/ Tag_Feature_GCS: true" } } */
 /* { dg-final { scan-assembler "\.section\t\.note\.gnu\.property" } } */
 /* { dg-final { scan-assembler "\.word\t0x7\t\/\/ GNU_PROPERTY_AARCH64_FEATURE_1_AND \\(BTI, PAC, GCS\\)" } } */
\ No newline at end of file