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. > --- > 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. > +#include <vec.h> "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* Formatting nit, but: space before "*", even here. Same below. > +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. > +} > + > +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. > +} > + > +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. > +} > + > +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. > + 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. > + {} > + > + /* 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_" > + { > + details::write (out_file, attr); > + } Formatting nit, but: no braces for single statements. > + } > + > + /* Indicate if the subsection is empty. */ > + bool empty() const Space before "(" even for empty arguments. (I agree it looks silly.) 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
