Sorry for the slow review.
Matthieu Longo <matthieu.lo...@arm.com> writes:
> GNU properties are emitted to provide some information about the features
> used in the generated code like PAC, BTI, or GCS. However, no debug
> comment are emitted in the generated assembly even if -dA is provided.
> This makes understanding the information stored in the .note.gnu.property
> section more difficult than needed.
>
> This patch adds assembly comments (if -dA is provided) next to the GNU
> properties. For instance, if PAC and BTI are enabled, it will emit:
> .word 3 // GNU_PROPERTY_AARCH64_FEATURE_1_AND (BTI, PAC)
>
> gcc/ChangeLog:
>
> * config/aarch64/aarch64.cc
> (aarch64_file_end_indicate_exec_stack): Emit assembly comments.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/aarch64/bti-1.c: Emit assembly comments, and update
> test assertion.
> ---
> gcc/config/aarch64/aarch64.cc | 41 +++++++++++++++++++++++-
> gcc/testsuite/gcc.target/aarch64/bti-1.c | 5 +--
> 2 files changed, 43 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index 4b2e7a690c6..6d9075011ec 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -98,6 +98,8 @@
> #include "ipa-fnsummary.h"
> #include "hash-map.h"
>
> +#include <numeric>
> +
If we do keep this, it needs to be done via a new INCLUDE_NUMERIC
in system.h, which aarch64.cc would then define before including
any files. This avoids clashes between GCC and system headers
on some hosts. But see below.
> /* This file should be included last. */
> #include "target-def.h"
>
> @@ -29129,8 +29131,45 @@ aarch64_file_end_indicate_exec_stack ()
> data = feature_1_and. */
> assemble_integer (GEN_INT (GNU_PROPERTY_AARCH64_FEATURE_1_AND), 4, 32,
> 1);
> assemble_integer (GEN_INT (4), 4, 32, 1);
> - assemble_integer (GEN_INT (feature_1_and), 4, 32, 1);
>
> + if (!flag_debug_asm)
> + assemble_integer (GEN_INT (feature_1_and), 4, 32, 1);
> + else
> + {
> + asm_fprintf (asm_out_file, "\t.word\t%u", feature_1_and);
It's probably better to use integer_asm_op (4, 1) rather than
hard-coding .word.
> +
> + auto join_s = [] (std::string s1,
> + const std::string &s2,
> + const std::string &separator = ", ") -> std::string
> + {
> + return std::move (s1)
> + .append (separator)
> + .append (s2);
> + };
> +
> + auto features_to_string
> + = [&join_s] (unsigned feature_1_and) -> std::string
> + {
> + std::vector<std::string> feature_bits;
> + if (feature_1_and & GNU_PROPERTY_AARCH64_FEATURE_1_BTI)
> + feature_bits.push_back ("BTI");
> + if (feature_1_and & GNU_PROPERTY_AARCH64_FEATURE_1_PAC)
> + feature_bits.push_back ("PAC");
> + if (feature_1_and & GNU_PROPERTY_AARCH64_FEATURE_1_GCS)
> + feature_bits.push_back ("GCS");
> +
> + if (feature_bits.empty ())
> + return {};
> + return std::accumulate(std::next(feature_bits.cbegin()),
> + feature_bits.cend(),
> + feature_bits[0],
> + join_s);
> + };
> + auto const& s_features = features_to_string (feature_1_and);
I do like this! But I wonder whether it would be simpler to go for
the more prosaic:
struct flag_name { unsigned int mask; const char *name; };
static const flag_name flags[] =
{
{ GNU_PROPERTY_AARCH64_FEATURE_1_BTI, "BTI" },
{ GNU_PROPERTY_AARCH64_FEATURE_1_PAC, "PAC" }
};
const char *separator = "";
std::string s_features;
for (auto &flag : flags)
if (feature_1_and & flag.mask)
{
s_features.append (separator).append (flag.name);
separator = ", ";
}
It's slightly shorter, but also means that there's a bit less
cut-&-paste for each flag. (In principle, the table could even
be generated from the same source as the definitions of the
GNU_PROPERTY_*s, but that's probaby overkill.)
> + asm_fprintf (asm_out_file,
> + "\t%s GNU_PROPERTY_AARCH64_FEATURE_1_AND (%s)\n",
> + ASM_COMMENT_START, s_features.c_str ());
> + }
Formatting:
asm_fprintf (asm_out_file,
"\t%s GNU_PROPERTY_AARCH64_FEATURE_1_AND (%s)\n",
ASM_COMMENT_START, s_features.c_str ());
Thanks,
Richard
> /* Pad the size of the note to the required alignment. */
> assemble_align (POINTER_SIZE);
> }
> diff --git a/gcc/testsuite/gcc.target/aarch64/bti-1.c
> b/gcc/testsuite/gcc.target/aarch64/bti-1.c
> index 5a556b08ed1..e48017abc35 100644
> --- a/gcc/testsuite/gcc.target/aarch64/bti-1.c
> +++ b/gcc/testsuite/gcc.target/aarch64/bti-1.c
> @@ -1,6 +1,6 @@
> /* { dg-do compile } */
> /* -Os to create jump table. */
> -/* { dg-options "-Os" } */
> +/* { dg-options "-Os -dA" } */
> /* { dg-require-effective-target lp64 } */
> /* If configured with --enable-standard-branch-protection, don't use
> command line option. */
> @@ -61,4 +61,5 @@ lab2:
> }
> /* { dg-final { scan-assembler-times "hint\t34" 1 } } */
> /* { dg-final { scan-assembler-times "hint\t36" 12 } } */
> -/* { dg-final { scan-assembler ".note.gnu.property" { target *-*-linux* } }
> } */
> +/* { dg-final { scan-assembler "\.section\t\.note\.gnu\.property" { target
> *-*-linux* } } } */
> +/* { dg-final { scan-assembler "\.word\t7\t\/\/
> GNU_PROPERTY_AARCH64_FEATURE_1_AND \\(BTI, PAC, GCS\\)" { target *-*-linux* }
> } } */
> \ No newline at end of file
