On 2024-10-08 18:45, Richard Sandiford wrote:
Matthieu Longo <matthieu.lo...@arm.com> writes:The previous implementation to emit 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.Generally this looks really nice! Just some minor comments below. Most of them are formatting nits; sorry that we don't have automatic formatting to care of this stuff.
All the formatting comments are addressed in the next revision.
--- gcc/config/aarch64/aarch64-dwarf-metadata.h | 247 ++++++++++++++++++ gcc/config/aarch64/aarch64.cc | 43 +-- .../build-attributes/build-attribute-gcs.c | 4 +- .../build-attribute-standard.c | 4 +- 4 files changed, 261 insertions(+), 37 deletions(-) create mode 100644 gcc/config/aarch64/aarch64-dwarf-metadata.h diff --git a/gcc/config/aarch64/aarch64-dwarf-metadata.h b/gcc/config/aarch64/aarch64-dwarf-metadata.h new file mode 100644 index 00000000000..9638bc7702f --- /dev/null +++ b/gcc/config/aarch64/aarch64-dwarf-metadata.h @@ -0,0 +1,247 @@ +/* Machine description for AArch64 architecture. + Copyright (C) 2009-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 <type_traits> +#include <cstdint>As mentioned in the part 1 review, we need to include system headers via system.h. These two should be included by default, though, so I think they can just be removed.
Fixed.
+#include <vec.h>"vec.h"
Fixed.
+ +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*Formatting nit, but: space before "*", even here. Same below.
Fixed.
+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) +{ + const char *s = nullptr; + switch (t) { + case uleb128: + s = "ULEB128"; + break; + case asciz: + s = "asciz"; + break; + }Formatting nit: the opening brace should be on its own line, indented like so: switch (t) { case uleb128: s = "ULEB128"; break; case asciz: s = "asciz"; break; } However...+ return s;...we are unfortunately limited to C++11 constexprs, so I think this needs to be: return (t == uleb128 ? "ULEB128" : t == asciz ? "asciz" : nullptr); if we want it to be treated as a constant for all builds.
Fixed in the next revision.FYI we might have C++14 soon :) => https://gcc.gnu.org/pipermail/gcc-patches/2024-October/665644.html
+} + +constexpr const char* +to_c_str (BA_TagFeature_t feature) +{ + const char *s = nullptr; + switch (feature) { + case Tag_Feature_BTI: + s = "Tag_Feature_BTI"; + break; + case Tag_Feature_PAC: + s = "Tag_Feature_PAC"; + break; + case Tag_Feature_GCS: + s = "Tag_Feature_GCS"; + break; + } + return s;Similarly here, I think we need to use ?: to get constexprs for C++11.
Fixed.
+} + +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";Formatting nit: should be indented by 2 spaces only. Similarly below.
Fixed.
+} + +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{}),Formatting nit, but: space before "(" for each call in this function. Same for the functions below.
Fixed.
+ 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): + name_(name), + optional_(optional), + avtype_(details::deduce_attr_av_type (T_val{}))Formatting nit, should be indented as: class aeabi_subsection { public: aeabi_subsection (const char *name, bool optional) : name_ (name) ... But the usual GCC style is to use an "m_" prefix for private members, rather than a "_" suffix.
Fixed.For "public:", Richard Earnshaw recommended not to respect the recommended GNU style as it breaks the mklog script, and keep 1 space before.
+ {} + + /* Append an attribute to the subsection. */ + void + append (aeabi_attribute<T_tag, T_val> &&attr) + { + 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", + name_, static_cast<uint8_t>(optional_), + static_cast<uint8_t>(avtype_)); + if (flag_debug_asm) + asm_fprintf (out_file, "\t%s Optional: %s, AttrValType: %s", + ASM_COMMENT_START, + details::to_c_str(optional_), + details::to_c_str(avtype_)); + asm_fprintf (out_file, "\n"); + + for (auto const& attr: attributes_)"auto const &attr : attributes_"
Fixed.
+ { + details::write (out_file, attr); + }Formatting nit, but: no braces for single statements.+ } + + /* Indicate if the subsection is empty. */ + bool empty() constSpace before "(" even for empty arguments. (I agree it looks silly.)
Fixed.
Thanks, Richard+ { + return attributes_.is_empty (); + } + + private: + const char *name_; + bool optional_; + attr_val_type avtype_; + auto_vec<aeabi_attribute<T_tag, T_val>, N> 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 61e0248817f..0cc8a5ee9ad 100644 --- a/gcc/config/aarch64/aarch64.cc +++ b/gcc/config/aarch64/aarch64.cc @@ -83,6 +83,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" @@ -24677,33 +24678,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@@ -24727,18 +24701,21 @@ aarch64_start_file (void) /* Emit gcs build attributes only when building a native AArch64-hosted compiler. */ #if defined(__aarch64__) + using namespace aarch64; + aeabi_subsection<BA_TagFeature_t, bool, 3> + aeabi_subsec (".aeabi-feature-and-bits", true); + /* Check the current assembly supports gcs build attributes, if not fallback to .note.gnu.property section. */ #ifdef HAVE_AS_BUILD_ATTRIBUTES_GCS - if (aarch64_gcs_enabled ()) - { - aarch64_emit_aeabi_subsection (".aeabi-feature-and-bits", 1, 0); - aarch64_emit_aeabi_attribute ("Tag_Feature_GCS", 3, 1); - } + aeabi_subsec.append ( + make_aeabi_attribute (Tag_Feature_GCS, aarch64_gcs_enabled ())); #endif + 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 ce43a79c1d0..7b241a2c889 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\t4\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 fd175f8137f..5c3c8738040 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\t7\t\/\/ GNU_PROPERTY_AARCH64_FEATURE_1_AND \\(BTI, PAC, GCS\\)" } } */ \ No newline at end of file
