diff mbox series

[v1,1/4] aarch64: add debug comments to feature properties in .note.gnu.property

Message ID 20240927144755.632627-2-matthieu.longo@arm.com
State New
Headers
Series aarch64: add minimal support for GCS build attributes |

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 Sept. 27, 2024, 2:47 p.m. UTC 
  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(-)

Comments

Richard Sandiford Oct. 8, 2024, 5:39 p.m. UTC | #1 
Sorry for the slow review.

Matthieu Longo <matthieu.longo@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
diff mbox series

Patch

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>
+
 /* 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);
+
+	  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);
+	  asm_fprintf (asm_out_file,
+	    "\t%s GNU_PROPERTY_AARCH64_FEATURE_1_AND (%s)\n",
+	    ASM_COMMENT_START, s_features.c_str ());
+	}
       /* 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