The 08/28/2025 15:56, Jason Merrill wrote:
> On 8/28/25 8:59 AM, Alfie Richards wrote:
> > The 08/28/2025 14:43, Jason Merrill wrote:
> > > On 8/28/25 5:49 AM, [email protected] wrote:
> > > > From: Alfie Richards <[email protected]>
> > > >
> > > > Adds the target_version and target_clones attributes to diagnostic
> > > > messages
> > > > for target_version semantics.
> > > >
> > > > This is because the target_version/target_clones attributes affect the
> > > > identity
> > > > of the decls, so need to be represented in diagnostics for them.
> > > >
> > > > This also requires making maybe_print_whitespace available to c++ code
> > > > so
> > >
> > > pp_c_maybe_whitespace
> > >
> > > But why do we need to call it in C++ when we don't in C?
> >
> > (Resending as I forgot to reply all)
> >
> > Because I use it to print the white space after the attribute which isn't
> > needed
> > in the C diagnostic.
> >
> > The difficulty is I want C++ to do:
> >
> > 'float foo [[target_version("sve")]] ()'
> > (with a space before and after)
> >
> > and
> >
> > 'float foo()'
> > with no spaces (unchanged from current formatting)
> >
> > But for C I want:
> >
> > 'foo [[target_version("sve")]]'
> >
> > Notably, without a space after the attribute.
> >
> > The slightly ugly way I achieved this is that pp_c_function_target_version
> > sets pp->set_padding (pp_before) if it prints the attribute, so then
> > pp_cpp_maybe_whitespace can be used only in the C++ code to add the extra
> > white space only if the attribute is added.
> >
> > I'm not happy with this way of achieving this, if you know a simpler
> > way to achieve this I would be happy to change it.
>
> Hmm, I guess this approach makes sense since pp_c_left_paren/dump_parameters
> don't ever call _maybe_whitespace.
>
> But the _whitespace call between _version and _clones seems wrong; if we
> didn't print _version then it won't do anything, if we did and are going to
> also print _clones it's an extra space, if we did and aren't going to print
> _clones it's redundant with the second _whitespace call.
Get agreed, I put it in on the offchance of a decl with both target_clones and
target_version, but that case is explicitly disallowed, and formatting them
without a space between would be fine anyway. I will remove it.
Is this okay with that change?
>
> > > > we can control if whitespace is needed cosistantly between c and c++
> > >
> > > "consistently"
> > >
> > > > diagnostics.
> > > >
> > > > After this change diagnostics look like:
> > > >
> > > > c:
> > > > ```
> > > > test.c:6:8: error: redefinition of ‘foo [[target_version("sve")]]’
> > > > 6 | float foo () {return 1;}
> > > > | ^~~
> > > > test.c:3:8: note: previous definition of ‘foo
> > > > [[target_version("sve")]]’ with type ‘float(void)’
> > > > 3 | float foo () {return 2;}
> > > > | ^~~
> > > > test.c:12:8: error: redefinition of ‘bar [[target_clones("sve")]]’
> > > > 12 | float bar () {return 1;}
> > > > | ^~~
> > > > test.c:9:8: note: previous definition of ‘bar [[target_clones("sve")]]’
> > > > with type ‘float(void)’
> > > > 9 | float bar () {return 2;}
> > > > | ^~~
> > > > ```
> > > >
> > > > c++:
> > > > ```
> > > > test.cpp:6:8: error: redefinition of ‘float foo
> > > > [[target_version("sve")]] ()’
> > > > 6 | float foo () {return 1;}
> > > > | ^~~
> > > > test.cpp:3:8: note: ‘float foo [[target_version("sve")]] ()’ previously
> > > > defined here
> > > > 3 | float foo () {return 2;}
> > > > | ^~~
> > > > test.cpp:12:8: error: redefinition of ‘float bar
> > > > [[target_clones("sve")]] ()’
> > > > 12 | float bar () {return 1;}
> > > > | ^~~
> > > > test.cpp:9:8: note: ‘float bar [[target_clones("sve")]] ()’ previously
> > > > defined here
> > > > 9 | float bar () {return 2;}
> > > > | ^~~
> > > > ```
> > > >
> > > > This only affects targets which use target_version (aarch64 and riscv).
> > > >
> > > > gcc/c-family/ChangeLog:
> > > >
> > > > * c-pretty-print.cc (pp_c_function_target_version): New
> > > > function.
> > > > (pp_c_function_target_clones): New function.
> > > > (pp_c_maybe_whitespace): Move to c-pretty-print.h.
> > > > * c-pretty-print.h (pp_c_function_target_version): New function.
> > > > (pp_c_function_target_clones): New function.
> > > > (pp_c_maybe_whitespace): Moved here from c-pretty-print.cc.
> > > >
> > > > gcc/c/ChangeLog:
> > > >
> > > > * c-objc-common.cc (c_tree_printer): Add printing of
> > > > target_clone and
> > > > target_version in decl diagnostics.
> > > >
> > > > gcc/cp/ChangeLog:
> > > >
> > > > * cxx-pretty-print.h (pp_cxx_function_target_version): New
> > > > macro.
> > > > (pp_cxx_function_target_clones): Ditto.
> > > > (pp_cxx_maybe_whitespace): Ditto.
> > > > * error.cc (dump_function_decl): Add printing of target_clone
> > > > and
> > > > target_version in decl diagnostics.
> > > > ---
> > > > gcc/c-family/c-pretty-print.cc | 77
> > > > +++++++++++++++++++++++++++++++---
> > > > gcc/c-family/c-pretty-print.h | 8 ++++
> > > > gcc/c/c-objc-common.cc | 6 +++
> > > > gcc/cp/cxx-pretty-print.h | 5 +++
> > > > gcc/cp/error.cc | 8 ++++
> > > > 5 files changed, 98 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/gcc/c-family/c-pretty-print.cc
> > > > b/gcc/c-family/c-pretty-print.cc
> > > > index fad6b5eb9b0..5adb13b9784 100644
> > > > --- a/gcc/c-family/c-pretty-print.cc
> > > > +++ b/gcc/c-family/c-pretty-print.cc
> > > > @@ -36,6 +36,7 @@ along with GCC; see the file COPYING3. If not see
> > > > #include "function.h"
> > > > #include "basic-block.h"
> > > > #include "gimple.h"
> > > > +#include "tm.h"
> > > > /* The pretty-printer code is primarily designed to closely follow
> > > > (GNU) C and C++ grammars. That is to be contrasted with spaghetti
> > > > @@ -45,12 +46,6 @@ along with GCC; see the file COPYING3. If not see
> > > > takes expression or declaration contexts into account. */
> > > > -#define pp_c_maybe_whitespace(PP) \
> > > > - do { \
> > > > - if ((PP)->get_padding () == pp_before) \
> > > > - pp_c_whitespace (PP); \
> > > > - } while (0)
> > > > -
> > > > /* literal */
> > > > static void pp_c_char (c_pretty_printer *, int);
> > > > @@ -3054,6 +3049,76 @@ pp_c_tree_decl_identifier (c_pretty_printer *pp,
> > > > tree t)
> > > > pp_c_identifier (pp, name);
> > > > }
> > > > +/* Prints "[version: VERSION]" for a versioned function decl.
> > > > + This will only print for targets with target_version semantics. */
> > > > +void
> > > > +pp_c_function_target_version (c_pretty_printer *pp, tree t)
> > > > +{
> > > > + if (TARGET_HAS_FMV_TARGET_ATTRIBUTE)
> > > > + return;
> > > > +
> > > > + string_slice version = get_target_version (t);
> > > > + if (!version.is_valid ())
> > > > + return;
> > > > +
> > > > + pp_c_whitespace (pp);
> > > > +
> > > > + pp_c_left_bracket (pp);
> > > > + pp_c_left_bracket (pp);
> > > > + pp_string (pp, "target_version");
> > > > + pp_c_left_paren (pp);
> > > > + pp_doublequote (pp);
> > > > + pp_string_n (pp, version.begin (), version.size ());
> > > > + pp_doublequote (pp);
> > > > + pp_c_right_paren (pp);
> > > > + pp_c_right_bracket (pp);
> > > > + pp_c_right_bracket (pp);
> > > > +
> > > > + pp->set_padding (pp_before);
> > > > +}
> > > > +
> > > > +/* Prints "[clones: VERSION, +]" for a versioned function decl.
> > > > + This only works for targets with target_version semantics. */
> > > > +void
> > > > +pp_c_function_target_clones (c_pretty_printer *pp, tree t)
> > > > +{
> > > > + /* Only print for target_version semantics.
> > > > + This is because for target FMV semantics a target_clone always
> > > > defines
> > > > + the entire FMV set. target_version semantics can mix
> > > > target_clone and
> > > > + target_version decls in the definition of a FMV set and so the
> > > > + target_clone becomes a part of the identity of the declaration.
> > > > */
> > > > + if (TARGET_HAS_FMV_TARGET_ATTRIBUTE)
> > > > + return;
> > > > +
> > > > + auto_vec<string_slice> versions = get_clone_versions (t, NULL,
> > > > false);
> > > > + if (versions.is_empty ())
> > > > + return;
> > > > +
> > > > + pp_c_whitespace (pp);
> > > > +
> > > > + string_slice final_version = versions.pop ();
> > > > + pp_c_left_bracket (pp);
> > > > + pp_c_left_bracket (pp);
> > > > + pp_string (pp, "target_clones");
> > > > + pp_c_left_paren (pp);
> > > > + for (string_slice version : versions)
> > > > + {
> > > > + pp_doublequote (pp);
> > > > + pp_string_n (pp, version.begin (), version.size ());
> > > > + pp_doublequote (pp);
> > > > + pp_string (pp, ",");
> > > > + pp_c_whitespace (pp);
> > > > + }
> > > > + pp_doublequote (pp);
> > > > + pp_string_n (pp, final_version.begin (), final_version.size ());
> > > > + pp_doublequote (pp);
> > > > + pp_c_right_paren (pp);
> > > > + pp_c_right_bracket (pp);
> > > > + pp_c_right_bracket (pp);
> > > > +
> > > > + pp->set_padding (pp_before);
> > > > +}
> > > > +
> > > > #if CHECKING_P
> > > > namespace selftest {
> > > > diff --git a/gcc/c-family/c-pretty-print.h
> > > > b/gcc/c-family/c-pretty-print.h
> > > > index c8fb6789991..5e00b952c81 100644
> > > > --- a/gcc/c-family/c-pretty-print.h
> > > > +++ b/gcc/c-family/c-pretty-print.h
> > > > @@ -102,6 +102,12 @@ public:
> > > > #define pp_ptr_operator(PP, D) (PP)->ptr_operator (PP, D)
> > > > #define pp_parameter_list(PP, T) (PP)->parameter_list (PP, T)
> > > > +#define pp_c_maybe_whitespace(PP) \
> > > > + do { \
> > > > + if ((PP)->get_padding () == pp_before) \
> > > > + pp_c_whitespace (PP); \
> > > > + } while (0)
> > > > +
> > > > void pp_c_whitespace (c_pretty_printer *);
> > > > void pp_c_left_paren (c_pretty_printer *);
> > > > void pp_c_right_paren (c_pretty_printer *);
> > > > @@ -138,6 +144,8 @@ void pp_c_ws_string (c_pretty_printer *, const char
> > > > *);
> > > > void pp_c_identifier (c_pretty_printer *, const char *);
> > > > void pp_c_string_literal (c_pretty_printer *, tree);
> > > > void pp_c_integer_constant (c_pretty_printer *, tree);
> > > > +void pp_c_function_target_version (c_pretty_printer *, tree);
> > > > +void pp_c_function_target_clones (c_pretty_printer *, tree);
> > > > void print_c_tree (FILE *file, tree t, dump_flags_t);
> > > > diff --git a/gcc/c/c-objc-common.cc b/gcc/c/c-objc-common.cc
> > > > index 5c50983544d..109a8cb33ee 100644
> > > > --- a/gcc/c/c-objc-common.cc
> > > > +++ b/gcc/c/c-objc-common.cc
> > > > @@ -23,6 +23,7 @@ along with GCC; see the file COPYING3. If not see
> > > > #include "c-tree.h"
> > > > #include "intl.h"
> > > > #include "c-family/c-pretty-print.h"
> > > > +#include "tree-core.h"
> > > > #include "tree-pretty-print.h"
> > > > #include "tree-pretty-print-markup.h"
> > > > #include "gimple-pretty-print.h"
> > > > @@ -359,6 +360,11 @@ c_tree_printer (pretty_printer *pp, text_info
> > > > *text, const char *spec,
> > > > if (DECL_NAME (t))
> > > > {
> > > > pp_identifier (cpp, lang_hooks.decl_printable_name (t, 2));
> > > > + if (TREE_CODE (t) == FUNCTION_DECL)
> > > > + {
> > > > + pp_c_function_target_version (cpp, t);
> > > > + pp_c_function_target_clones (cpp, t);
> > > > + }
> > > > return true;
> > > > }
> > > > break;
> > > > diff --git a/gcc/cp/cxx-pretty-print.h b/gcc/cp/cxx-pretty-print.h
> > > > index 86e0ce7d85f..356fd6e4d23 100644
> > > > --- a/gcc/cp/cxx-pretty-print.h
> > > > +++ b/gcc/cp/cxx-pretty-print.h
> > > > @@ -81,8 +81,13 @@ public:
> > > > #define pp_cxx_ws_string(PP, I) pp_c_ws_string (PP, I)
> > > > #define pp_cxx_identifier(PP, I) pp_c_identifier (PP, I)
> > > > +#define pp_cxx_maybe_whitespace(PP) pp_c_maybe_whitespace (PP)
> > > > #define pp_cxx_tree_identifier(PP, T) \
> > > > pp_c_tree_identifier (PP, T)
> > > > +#define pp_cxx_function_target_version(PP, T) \
> > > > + pp_c_function_target_version (PP, T)
> > > > +#define pp_cxx_function_target_clones(PP, T) \
> > > > + pp_c_function_target_clones (PP, T)
> > > > void pp_cxx_begin_template_argument_list (cxx_pretty_printer *);
> > > > void pp_cxx_end_template_argument_list (cxx_pretty_printer *);
> > > > diff --git a/gcc/cp/error.cc b/gcc/cp/error.cc
> > > > index 8ef9f9ee458..16dfeafc07a 100644
> > > > --- a/gcc/cp/error.cc
> > > > +++ b/gcc/cp/error.cc
> > > > @@ -2008,6 +2008,14 @@ dump_function_decl (cxx_pretty_printer *pp, tree
> > > > t, int flags)
> > > > dump_function_name (pp, t, dump_function_name_flags);
> > > > + /* By default we need no padding here, but if we emit target_version
> > > > or
> > > > + target_clones then we need some. */
> > > > + pp->set_padding (pp_none);
> > > > + pp_cxx_function_target_version (pp, t);
> > > > + pp_cxx_maybe_whitespace (pp);
> > > > + pp_cxx_function_target_clones (pp, t);
> > > > + pp_cxx_maybe_whitespace (pp);
> > > > +
> > > > if (!(flags & TFF_NO_FUNCTION_ARGUMENTS))
> > > > {
> > > > int const parm_flags
> > >
> >
>
--
Alfie Richards
