| Message ID | 1505759191.2286.72.camel@cavium.com |
|---|---|
| State | New |
| Headers | show |
| Series | [aarch64] Fix error calls in aarch64 code so they can be tranlated | expand |
On 09/18/2017 12:26 PM, Steve Ellcey wrote: > This patch is for PR target/79868, where some aarch64 diagnostics are > said to be not translatable due to how they are implemented. See the > bug report for more details on why the current setup of passing > the string 'pragma' or 'attribute' doesn't work. > > This patch fixes it, unfortunately by increasing the number of calls we > have to 'error' (16 calls become 32 calls), but that seems to be the > most straight forward way to get translatable strings. I haven't looked at all of them but from the few I have seen it seems that rephrasing the messages along the following lines would be a way to get around the translation issue and without increasing the number of calls (though not without the conditional): error (is_pragma ? G_("missing name in %<#pragma target\(\"%s=\")%>") : G_("missing name in %<target(\"%s=\")%> attribute"), "arch"); The additional benefit of this approach is that it would also make the quoting consistent with what seems to be the prevailing style of these sorts of messages. (It would be nice to eventually converge on the same style/quoting and phrasing across all back and front ends.) Martin > > This patch is an update to the one I originally attached to the bug > report and I have fixed the issue that Frederic Marchal found in my > original patch. > > OK to checkin? > > Steve Ellcey > sellcey@cavium.com > > > > 2017-09-18 Steve Ellcey <sellcey@cavium.com> > > PR target/79868 > * config/aarch64/aarch64-c.c (aarch64_pragma_target_parse): > Change argument type on aarch64_process_target_attr call. > * config/aarch64/aarch64-protos.h (aarch64_process_target_attr): > Change argument type. > * config/aarch64/aarch64.c (aarch64_attribute_info): Change > field type. > (aarch64_handle_attr_arch): Change argument type, use boolean > argument to make different error calls. > (aarch64_handle_attr_cpu): Ditto. > (aarch64_handle_attr_tune): Ditto. > (aarch64_handle_attr_isa_flags): Ditto. > (aarch64_process_one_target_attr): Ditto. > (aarch64_process_target_attr): Ditto. > (aarch64_option_valid_attribute_p): Change argument type on > aarch64_process_target_attr call. > >
On lundi 18 septembre 2017 13 h 20 min 25 s CEST Martin Sebor wrote: > On 09/18/2017 12:26 PM, Steve Ellcey wrote: > > This patch is for PR target/79868, where some aarch64 diagnostics are > > said to be not translatable due to how they are implemented. See the > > bug report for more details on why the current setup of passing > > the string 'pragma' or 'attribute' doesn't work. > > > > This patch fixes it, unfortunately by increasing the number of calls we > > have to 'error' (16 calls become 32 calls), but that seems to be the > > most straight forward way to get translatable strings. > > I haven't looked at all of them but from the few I have seen it > seems that rephrasing the messages along the following lines would > be a way to get around the translation issue and without increasing > the number of calls (though not without the conditional): > > error (is_pragma > ? G_("missing name in %<#pragma target\(\"%s=\")%>") > : G_("missing name in %<target(\"%s=\")%> attribute"), > "arch"); > > The additional benefit of this approach is that it would also make > the quoting consistent with what seems to be the prevailing style > of these sorts of messages. (It would be nice to eventually > converge on the same style/quoting and phrasing across all back > and front ends.) Indeed! That's even better as the message uses words the user sees in the source code whatever his/her locale language is. With your proposal, I know I must not translate "target" because it clearly is part of the programming language. With the former message, I would have translated "target" as part of the human language message. Your approach is clearly better. Frederic
On Tue, 2017-09-19 at 09:50 +0200, Frédéric Marchal wrote: > On lundi 18 septembre 2017 13 h 20 min 25 s CEST Martin Sebor wrote: > > > > I haven't looked at all of them but from the few I have seen it > > seems that rephrasing the messages along the following lines would > > be a way to get around the translation issue and without increasing > > the number of calls (though not without the conditional): > > > > error (is_pragma > > ? G_("missing name in %<#pragma target\(\"%s=\")%>") > > : G_("missing name in %<target(\"%s=\")%> attribute"), > > "arch"); > > > > The additional benefit of this approach is that it would also make > > the quoting consistent with what seems to be the prevailing style > > of these sorts of messages. (It would be nice to eventually > > converge on the same style/quoting and phrasing across all back > > and front ends.) > Indeed! That's even better as the message uses words the user sees in the > source code whatever his/her locale language is. > > With your proposal, I know I must not translate "target" because it clearly is > part of the programming language. With the former message, I would have > translated "target" as part of the human language message. Your approach is > clearly better. > > Frederic Are '%<...%>' described somewhere? These aren't normal printf options are they? I don't think I have ever used them before. I am also not sure why you have '%s=' means vs. '%s'. Is it even worth breaking the word 'arch' out of the string vs. having something like: error (is_pragma ? G_("missing name in %<#pragma target \"arch\"%>) : G_("missing name in %<target arch%> attribute")); Steve Ellcey sellcey@cavium.com
On 09/19/2017 09:54 AM, Steve Ellcey wrote: > On Tue, 2017-09-19 at 09:50 +0200, Frédéric Marchal wrote: >> On lundi 18 septembre 2017 13 h 20 min 25 s CEST Martin Sebor wrote: >>> >>> I haven't looked at all of them but from the few I have seen it >>> seems that rephrasing the messages along the following lines would >>> be a way to get around the translation issue and without increasing >>> the number of calls (though not without the conditional): >>> >>> error (is_pragma >>> ? G_("missing name in %<#pragma target\(\"%s=\")%>") >>> : G_("missing name in %<target(\"%s=\")%> attribute"), >>> "arch"); >>> >>> The additional benefit of this approach is that it would also make >>> the quoting consistent with what seems to be the prevailing style >>> of these sorts of messages. (It would be nice to eventually >>> converge on the same style/quoting and phrasing across all back >>> and front ends.) >> Indeed! That's even better as the message uses words the user sees in the >> source code whatever his/her locale language is. >> >> With your proposal, I know I must not translate "target" because it clearly is >> part of the programming language. With the former message, I would have >> translated "target" as part of the human language message. Your approach is >> clearly better. >> >> Frederic > > Are '%<...%>' described somewhere? These aren't normal printf options > are they? I don't think I have ever used them before. I am also not > sure why you have '%s=' means vs. '%s'. Is it even worth breaking the > word 'arch' out of the string vs. having something like: > > error (is_pragma > ? G_("missing name in %<#pragma target \"arch\"%>) > : G_("missing name in %<target arch%> attribute")); %< and %> are (briefly) mentioned in pretty-print.c, just above pp_format. They should be used in preference to the apostrophe to introduce quoting and color highlighting in diagnostics. (There's also logic in the pretty printer to help detect mismatches between these and other related directives, but none for the apostrophe.) I used %s= because it appears with some consistency in other similar messages in gcc.pot, e.g.: %<generic%> CPU can be used only for %<target(\"tune=\")%> attribute" (I ran 'grep target gcc.pot | grep attr' to find them, along with some of the inconsistencies among them.) It doesn't look to me like parameterizing the message on the name of the target attribute reduces the number of messages that need to be translated so it doesn't gain much (if anything) here. It does have help in a few other places where I've been working recently that deal with attributes and I used mostly out of habit. Martin
On Tue, 2017-09-19 at 10:49 -0600, Martin Sebor wrote: > On 09/19/2017 09:54 AM, Steve Ellcey wrote: > > On Tue, 2017-09-19 at 09:50 +0200, Frédéric Marchal wrote: > > > > > > > > > > > error (is_pragma > > > > ? G_("missing name in %<#pragma target\(\"%s=\")%>") > > > > : G_("missing name in %<target(\"%s=\")%> > > > > attribute"), > > > > "arch"); > > > > > > > > The additional benefit of this approach is that it would also make > > > > the quoting consistent with what seems to be the prevailing style > > > > of these sorts of messages. (It would be nice to eventually > > > > converge on the same style/quoting and phrasing across all back > > > > and front ends.) > > > Indeed! That's even better as the message uses words the user sees in the > > > source code whatever his/her locale language is. So I am looking at redoing the error messages and I am trying to decide between two forms. Make pragma/attribute part of the non-translatable string by showing the pragma and/or attribute syntax ast it would be in the C/C++ code (what about Fortran or Ada?). error (is_pragma ? G_("missing architecture name in %<#pragma GCC target (\"arch=string\")%>") : G_("missing architecture name in %<__attribute__ ((target(\"arch=string\"))) %>")); Or include them in the part that does get translated and include less of what is actually in the text of the file being compiled. error (is_pragma ? G_("missing architecture name in %<target (\"arch=string\")%> pragma") : G_("missing architecture name in %<target(\"arch=string\")%> attribute")); Opinions? Steve Ellcey sellcey@cavium.com
On 09/20/2017 05:36 PM, Steve Ellcey wrote: > On Tue, 2017-09-19 at 10:49 -0600, Martin Sebor wrote: >> On 09/19/2017 09:54 AM, Steve Ellcey wrote: >>> On Tue, 2017-09-19 at 09:50 +0200, Frédéric Marchal wrote: >>>> >>>>> >>>>> error (is_pragma >>>>> ? G_("missing name in %<#pragma target\(\"%s=\")%>") >>>>> : G_("missing name in %<target(\"%s=\")%> >>>>> attribute"), >>>>> "arch"); >>>>> >>>>> The additional benefit of this approach is that it would also make >>>>> the quoting consistent with what seems to be the prevailing style >>>>> of these sorts of messages. (It would be nice to eventually >>>>> converge on the same style/quoting and phrasing across all back >>>>> and front ends.) >>>> Indeed! That's even better as the message uses words the user sees in the >>>> source code whatever his/her locale language is. > > > So I am looking at redoing the error messages and I am trying to decide > between two forms. > > Make pragma/attribute part of the non-translatable string by showing the > pragma and/or attribute syntax ast it would be in the C/C++ code (what about > Fortran or Ada?). > > error (is_pragma > ? G_("missing architecture name in %<#pragma GCC target (\"arch=string\")%>") > : G_("missing architecture name in %<__attribute__ ((target(\"arch=string\"))) > %>")); > > Or include them in the part that does get translated and include less of > what is actually in the text of the file being compiled. > > error (is_pragma > ? G_("missing architecture name in %<target (\"arch=string\")%> pragma") > : G_("missing architecture name in %<target(\"arch=string\")%> attribute")); > > Opinions? There are two distinctly different spellings of the target (and every other) attribute: besides the one above, the C+++ standard recommended syntax is [[gnu::target("arch=...")]], so I think hardcoding one into the message wouldn't be optimal. (The C standard hasn't adopted the C++ syntax but a proposal to do so is being considered. There are some questions about the exact spelling, and some people have proposed adding _Attribute() instead, analogous to _Pragma, or possibly in addition to the C++ syntax.) There are, of course, also two ways to spell a pragma: #pragma and _Pragma, so the same argument against hardcoding either one could be made there. In my view, the ideal message would use the same form as in the source code. But I suspect that's not easy to determine at all sites where the message is being formatted, so the next best thing is to avoid using the keyword and fall back on the general term. Martin PS If possible, I would also suggest adopting a style that is already in use elsewhere, for consistency. (That would suggest using %<target(\"arch=\")%> with no spaces and no "string" after arch=.)
> In my view, the ideal message would use the same form as in > the source code. But I suspect that's not easy to determine at > all sites where the message is being formatted, so the next best > thing is to avoid using the keyword and fall back on the general > term. > > Martin > > PS If possible, I would also suggest adopting a style that is > already in use elsewhere, for consistency. (That would suggest > using %<target(\"arch=\")%> with no spaces and no "string" after > arch=.) Here is a new version of my patch. I think it addresses these issues and is consistent. It does cause some regressions in tests that check for certain error messages, specifically: gcc.target/aarch64/spellcheck_1.c gcc.target/aarch64/spellcheck_2.c gcc.target/aarch64/spellcheck_3.c gcc.target/aarch64/target_attr_11.c gcc.target/aarch64/target_attr_12.c gcc.target/aarch64/target_attr_17.c But I thought we should try and get some agreement on the format of the messages before updating the tests. If we think these changes look good I can submit a final patch that includes the testsuite changes. Steve Ellcey sellcey@cavium.com 2017-09-22 Steve Ellcey <sellcey@cavium.com> PR target/79868 * config/aarch64/aarch64-c.c (aarch64_pragma_target_parse): Change argument type on aarch64_process_target_attr call. * config/aarch64/aarch64-protos.h (aarch64_process_target_attr): Change argument type. * config/aarch64/aarch64.c (aarch64_attribute_info): Change field type. (aarch64_handle_attr_arch): Change argument type, use boolean argument to use different strings in error calls. (aarch64_handle_attr_cpu): Ditto. (aarch64_handle_attr_tune): Ditto. (aarch64_handle_attr_isa_flags): Ditto. (aarch64_process_one_target_attr): Ditto. (aarch64_process_target_attr): Ditto. (aarch64_option_valid_attribute_p): Change argument type on aarch64_process_target_attr call. diff --git a/gcc/config/aarch64/aarch64-c.c b/gcc/config/aarch64/aarch64-c.c index 177e638..c9945db 100644 --- a/gcc/config/aarch64/aarch64-c.c +++ b/gcc/config/aarch64/aarch64-c.c @@ -165,7 +165,7 @@ aarch64_pragma_target_parse (tree args, tree pop_target) information that it specifies. */ if (args) { - if (!aarch64_process_target_attr (args, "pragma")) + if (!aarch64_process_target_attr (args, true)) return false; aarch64_override_options_internal (&global_options); diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h index e67c2ed..4323e9e 100644 --- a/gcc/config/aarch64/aarch64-protos.h +++ b/gcc/config/aarch64/aarch64-protos.h @@ -445,7 +445,7 @@ bool aarch64_gen_adjusted_ldpstp (rtx *, bool, scalar_mode, RTX_CODE); void aarch64_init_builtins (void); -bool aarch64_process_target_attr (tree, const char*); +bool aarch64_process_target_attr (tree, bool); void aarch64_override_options_internal (struct gcc_options *); rtx aarch64_expand_builtin (tree exp, diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index 1c14008..3bf91b3 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -67,6 +67,7 @@ #include "common/common-target.h" #include "selftest.h" #include "selftest-rtl.h" +#include "intl.h" /* This file should be included last. */ #include "target-def.h" @@ -9554,15 +9555,15 @@ struct aarch64_attribute_info const char *name; enum aarch64_attr_opt_type attr_type; bool allow_neg; - bool (*handler) (const char *, const char *); + bool (*handler) (const char *, bool); enum opt_code opt_num; }; /* Handle the ARCH_STR argument to the arch= target attribute. - PRAGMA_OR_ATTR is used in potential error messages. */ + IS_PRAGMA is used in potential error messages. */ static bool -aarch64_handle_attr_arch (const char *str, const char *pragma_or_attr) +aarch64_handle_attr_arch (const char *str, bool is_pragma) { const struct processor *tmp_arch = NULL; enum aarch64_parse_opt_result parse_res @@ -9579,15 +9580,22 @@ aarch64_handle_attr_arch (const char *str, const char *pragma_or_attr) switch (parse_res) { case AARCH64_PARSE_MISSING_ARG: - error ("missing architecture name in 'arch' target %s", pragma_or_attr); + error (is_pragma + ? G_("missing name in %<target(\"arch=\")%> pragma") + : G_("missing name in %<target(\"arch=\")%> attribute")); break; case AARCH64_PARSE_INVALID_ARG: - error ("unknown value %qs for 'arch' target %s", str, pragma_or_attr); + error (is_pragma + ? G_("invalid name (%qs) in %<target(\"arch=\")%> pragma") + : G_("invalid name (%qs) in %<target(\"arch=\")%> attribute"), + str); aarch64_print_hint_for_arch (str); break; case AARCH64_PARSE_INVALID_FEATURE: - error ("invalid feature modifier %qs for 'arch' target %s", - str, pragma_or_attr); + error (is_pragma + ? G_("invalid value (%qs) in %<target()%> pragma") + : G_("invalid value (%qs) in %<target()%> attribute"), + str); break; default: gcc_unreachable (); @@ -9597,10 +9605,10 @@ aarch64_handle_attr_arch (const char *str, const char *pragma_or_attr) } /* Handle the argument CPU_STR to the cpu= target attribute. - PRAGMA_OR_ATTR is used in potential error messages. */ + IS_PRAGMA is used in potential error messages. */ static bool -aarch64_handle_attr_cpu (const char *str, const char *pragma_or_attr) +aarch64_handle_attr_cpu (const char *str, bool is_pragma) { const struct processor *tmp_cpu = NULL; enum aarch64_parse_opt_result parse_res @@ -9620,15 +9628,22 @@ aarch64_handle_attr_cpu (const char *str, const char *pragma_or_attr) switch (parse_res) { case AARCH64_PARSE_MISSING_ARG: - error ("missing cpu name in 'cpu' target %s", pragma_or_attr); + error (is_pragma + ? G_("missing name in %<target(\"cpu=\")%> pragma") + : G_("missing name in %<target(\"cpu=\")%> attribute")); break; case AARCH64_PARSE_INVALID_ARG: - error ("unknown value %qs for 'cpu' target %s", str, pragma_or_attr); + error (is_pragma + ? G_("invalid name (%qs) in %<target(\"cpu=\")%> pragma") + : G_("invalid name (%qs) in %<target(\"cpu=\")%> attribute"), + str); aarch64_print_hint_for_core (str); break; case AARCH64_PARSE_INVALID_FEATURE: - error ("invalid feature modifier %qs for 'cpu' target %s", - str, pragma_or_attr); + error (is_pragma + ? G_("invalid value (%qs) in %<target()%> pragma") + : G_("invalid value (%qs) in %<target()%> attribute"), + str); break; default: gcc_unreachable (); @@ -9638,10 +9653,10 @@ aarch64_handle_attr_cpu (const char *str, const char *pragma_or_attr) } /* Handle the argument STR to the tune= target attribute. - PRAGMA_OR_ATTR is used in potential error messages. */ + IS_PRAGMA is used in potential error messages. */ static bool -aarch64_handle_attr_tune (const char *str, const char *pragma_or_attr) +aarch64_handle_attr_tune (const char *str, bool is_pragma) { const struct processor *tmp_tune = NULL; enum aarch64_parse_opt_result parse_res @@ -9658,7 +9673,10 @@ aarch64_handle_attr_tune (const char *str, const char *pragma_or_attr) switch (parse_res) { case AARCH64_PARSE_INVALID_ARG: - error ("unknown value %qs for 'tune' target %s", str, pragma_or_attr); + error (is_pragma + ? G_("invalid name (%qs) in %<target(\"tune=\")%> pragma") + : G_("invalid name (%qs) in %<target(\"tune=\")%> attribute"), + str); aarch64_print_hint_for_core (str); break; default: @@ -9672,10 +9690,10 @@ aarch64_handle_attr_tune (const char *str, const char *pragma_or_attr) For example "+fp+nosimd". Show any errors if needed. Return TRUE if successful. Update aarch64_isa_flags to reflect the ISA features modified. - PRAGMA_OR_ATTR is used in potential error messages. */ + IS_PRAGMA is used in potential error messages. */ static bool -aarch64_handle_attr_isa_flags (char *str, const char *pragma_or_attr) +aarch64_handle_attr_isa_flags (char *str, bool is_pragma) { enum aarch64_parse_opt_result parse_res; unsigned long isa_flags = aarch64_isa_flags; @@ -9699,13 +9717,16 @@ aarch64_handle_attr_isa_flags (char *str, const char *pragma_or_attr) switch (parse_res) { case AARCH64_PARSE_MISSING_ARG: - error ("missing feature modifier in target %s %qs", - pragma_or_attr, str); + error (is_pragma + ? G_("missing value in %<target()%> pragma") + : G_("missing value in %<target()%> attribute")); break; case AARCH64_PARSE_INVALID_FEATURE: - error ("invalid feature modifier in target %s %qs", - pragma_or_attr, str); + error (is_pragma + ? G_("invalid value (%qs) in %<target()%> pragma") + : G_("invalid value (%qs) in %<target()%> attribute"), + str); break; default: @@ -9744,11 +9765,11 @@ static const struct aarch64_attribute_info aarch64_attributes[] = /* Parse ARG_STR which contains the definition of one target attribute. Show appropriate errors if any or return true if the attribute is valid. - PRAGMA_OR_ATTR holds the string to use in error messages about whether + IS_PRAGMA holds the boolean to tell error messages about whether we're processing a target attribute or pragma. */ static bool -aarch64_process_one_target_attr (char *arg_str, const char* pragma_or_attr) +aarch64_process_one_target_attr (char *arg_str, bool is_pragma) { bool invert = false; @@ -9756,7 +9777,9 @@ aarch64_process_one_target_attr (char *arg_str, const char* pragma_or_attr) if (len == 0) { - error ("malformed target %s", pragma_or_attr); + error (is_pragma + ? G_("malformed %<target()%> pragma") + : G_("malformed %<target()%> attribute")); return false; } @@ -9772,7 +9795,7 @@ aarch64_process_one_target_attr (char *arg_str, const char* pragma_or_attr) through the machinery for the rest of the target attributes in this function. */ if (*str_to_check == '+') - return aarch64_handle_attr_isa_flags (str_to_check, pragma_or_attr); + return aarch64_handle_attr_isa_flags (str_to_check, is_pragma); if (len > 3 && strncmp (str_to_check, "no-", 3) == 0) { @@ -9804,8 +9827,10 @@ aarch64_process_one_target_attr (char *arg_str, const char* pragma_or_attr) if (attr_need_arg_p ^ (arg != NULL)) { - error ("target %s %qs does not accept an argument", - pragma_or_attr, str_to_check); + error (is_pragma + ? G_("pragma %<target(%qs)%> does not accept an argument") + : G_("attribute %<target(%qs)%> does not accept an argument"), + str_to_check); return false; } @@ -9813,8 +9838,10 @@ aarch64_process_one_target_attr (char *arg_str, const char* pragma_or_attr) then we can't match. */ if (invert && !p_attr->allow_neg) { - error ("target %s %qs does not allow a negated form", - pragma_or_attr, str_to_check); + error (is_pragma + ? G_("pragma %<target(%qs)%> does not allow a negated form") + : G_("attribute %<target(%qs)%> does not allow a negated form"), + str_to_check); return false; } @@ -9824,7 +9851,7 @@ aarch64_process_one_target_attr (char *arg_str, const char* pragma_or_attr) For example, cpu=, arch=, tune=. */ case aarch64_attr_custom: gcc_assert (p_attr->handler); - if (!p_attr->handler (arg, pragma_or_attr)) + if (!p_attr->handler (arg, is_pragma)) return false; break; @@ -9868,8 +9895,10 @@ aarch64_process_one_target_attr (char *arg_str, const char* pragma_or_attr) } else { - error ("target %s %s=%s is not valid", - pragma_or_attr, str_to_check, arg); + error (is_pragma + ? G_("pragma %<target(\"%s=%s\")%> is not valid") + : G_("attribute %<target(\"%s=%s\")%> is not valid"), + str_to_check, arg); } break; } @@ -9903,12 +9932,12 @@ num_occurences_in_str (char c, char *str) } /* Parse the tree in ARGS that contains the target attribute information - and update the global target options space. PRAGMA_OR_ATTR is a string - to be used in error messages, specifying whether this is processing + and update the global target options space. IS_PRAGMA is a boolean + to be used by error messages, specifying whether this is processing a target attribute or a target pragma. */ bool -aarch64_process_target_attr (tree args, const char* pragma_or_attr) +aarch64_process_target_attr (tree args, bool is_pragma) { if (TREE_CODE (args) == TREE_LIST) { @@ -9917,7 +9946,7 @@ aarch64_process_target_attr (tree args, const char* pragma_or_attr) tree head = TREE_VALUE (args); if (head) { - if (!aarch64_process_target_attr (head, pragma_or_attr)) + if (!aarch64_process_target_attr (head, is_pragma)) return false; } args = TREE_CHAIN (args); @@ -9938,7 +9967,9 @@ aarch64_process_target_attr (tree args, const char* pragma_or_attr) if (len == 0) { - error ("malformed target %s value", pragma_or_attr); + error (is_pragma + ? G_("malformed %<target()%> pragma") + : G_("malformed %<target()%> attribute")); return false; } @@ -9953,9 +9984,12 @@ aarch64_process_target_attr (tree args, const char* pragma_or_attr) while (token) { num_attrs++; - if (!aarch64_process_one_target_attr (token, pragma_or_attr)) + if (!aarch64_process_one_target_attr (token, is_pragma)) { - error ("target %s %qs is invalid", pragma_or_attr, token); + error (is_pragma + ? G_("pragma %<target(%qs)%> is not valid") + : G_("attribute %<target(%qs)%> is not valid"), + token); return false; } @@ -9964,8 +9998,10 @@ aarch64_process_target_attr (tree args, const char* pragma_or_attr) if (num_attrs != num_commas + 1) { - error ("malformed target %s list %qs", - pragma_or_attr, TREE_STRING_POINTER (args)); + error (is_pragma + ? G_("malformed %<target(%qs)%> pragma") + : G_("malformed %<target(%qs)%> attribute"), + TREE_STRING_POINTER (args)); return false; } @@ -10025,7 +10061,7 @@ aarch64_option_valid_attribute_p (tree fndecl, tree, tree args, int) TREE_TARGET_OPTION (target_option_current_node)); - ret = aarch64_process_target_attr (args, "attribute"); + ret = aarch64_process_target_attr (args, false); /* Set up any additional state. */ if (ret)
On 09/22/2017 11:54 AM, Steve Ellcey wrote: > >> In my view, the ideal message would use the same form as in >> the source code. But I suspect that's not easy to determine at >> all sites where the message is being formatted, so the next best >> thing is to avoid using the keyword and fall back on the general >> term. >> >> Martin >> >> PS If possible, I would also suggest adopting a style that is >> already in use elsewhere, for consistency. (That would suggest >> using %<target(\"arch=\")%> with no spaces and no "string" after >> arch=.) > > Here is a new version of my patch. I think it addresses these issues > and is consistent. It does cause some regressions in tests that check > for certain error messages, specifically: > > gcc.target/aarch64/spellcheck_1.c > gcc.target/aarch64/spellcheck_2.c > gcc.target/aarch64/spellcheck_3.c > gcc.target/aarch64/target_attr_11.c > gcc.target/aarch64/target_attr_12.c > gcc.target/aarch64/target_attr_17.c > > But I thought we should try and get some agreement on the format of the > messages before updating the tests. If we think these changes look > good I can submit a final patch that includes the testsuite changes. Thank you for taking my suggestions! The changes look good to me. Just one minor nit. The following won't produce the kind of output we'd like it to: error (is_pragma ? G_("pragma %<target(%qs)%> does not accept an argument") : G_("attribute %<target(%qs)%> does not accept an argument"), str_to_check); It will end up printing: error: pragma ‘target(‘...’)’ does not accept an argument (note the single quotes around the ... where the syntax otherwise calls for a string). The %s directive should not use the 'q' flag when nested between %< and %>. -Wformat normally warns about this but in this case it doesn't see the format string because of the conditional (I think -Wformat can be extended to handle this case). warning: 'q' flag used within a quoted sequence [-Wformat=] To get the right output I think the format string should like something like this: "pragma %<target(\"%s\")%> does not accept an argument" Martin
diff --git a/gcc/config/aarch64/aarch64-c.c b/gcc/config/aarch64/aarch64-c.c index 177e638..c9945db 100644 --- a/gcc/config/aarch64/aarch64-c.c +++ b/gcc/config/aarch64/aarch64-c.c @@ -165,7 +165,7 @@ aarch64_pragma_target_parse (tree args, tree pop_target) information that it specifies. */ if (args) { - if (!aarch64_process_target_attr (args, "pragma")) + if (!aarch64_process_target_attr (args, true)) return false; aarch64_override_options_internal (&global_options); diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h index e67c2ed..4323e9e 100644 --- a/gcc/config/aarch64/aarch64-protos.h +++ b/gcc/config/aarch64/aarch64-protos.h @@ -445,7 +445,7 @@ bool aarch64_gen_adjusted_ldpstp (rtx *, bool, scalar_mode, RTX_CODE); void aarch64_init_builtins (void); -bool aarch64_process_target_attr (tree, const char*); +bool aarch64_process_target_attr (tree, bool); void aarch64_override_options_internal (struct gcc_options *); rtx aarch64_expand_builtin (tree exp, diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index 1c14008..054b1d2 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -9554,7 +9554,7 @@ struct aarch64_attribute_info const char *name; enum aarch64_attr_opt_type attr_type; bool allow_neg; - bool (*handler) (const char *, const char *); + bool (*handler) (const char *, bool); enum opt_code opt_num; }; @@ -9562,7 +9562,7 @@ struct aarch64_attribute_info PRAGMA_OR_ATTR is used in potential error messages. */ static bool -aarch64_handle_attr_arch (const char *str, const char *pragma_or_attr) +aarch64_handle_attr_arch (const char *str, bool is_pragma) { const struct processor *tmp_arch = NULL; enum aarch64_parse_opt_result parse_res @@ -9579,15 +9579,24 @@ aarch64_handle_attr_arch (const char *str, const char *pragma_or_attr) switch (parse_res) { case AARCH64_PARSE_MISSING_ARG: - error ("missing architecture name in 'arch' target %s", pragma_or_attr); + if (is_pragma) + error ("missing architecture name in 'arch' target pragma"); + else + error ("missing architecture name in 'arch' target attribute"); break; case AARCH64_PARSE_INVALID_ARG: - error ("unknown value %qs for 'arch' target %s", str, pragma_or_attr); + if (is_pragma) + error ("unknown value %qs for 'arch' target pragma", str); + else + error ("unknown value %qs for 'arch' target attribute", str); aarch64_print_hint_for_arch (str); break; case AARCH64_PARSE_INVALID_FEATURE: - error ("invalid feature modifier %qs for 'arch' target %s", - str, pragma_or_attr); + if (is_pragma) + error ("invalid feature modifier %qs for 'arch' target pragma", str); + else + error ("invalid feature modifier %qs for 'arch' target attribute", + str); break; default: gcc_unreachable (); @@ -9600,7 +9609,7 @@ aarch64_handle_attr_arch (const char *str, const char *pragma_or_attr) PRAGMA_OR_ATTR is used in potential error messages. */ static bool -aarch64_handle_attr_cpu (const char *str, const char *pragma_or_attr) +aarch64_handle_attr_cpu (const char *str, bool is_pragma) { const struct processor *tmp_cpu = NULL; enum aarch64_parse_opt_result parse_res @@ -9620,15 +9629,24 @@ aarch64_handle_attr_cpu (const char *str, const char *pragma_or_attr) switch (parse_res) { case AARCH64_PARSE_MISSING_ARG: - error ("missing cpu name in 'cpu' target %s", pragma_or_attr); + if (is_pragma) + error ("missing cpu name in 'cpu' target pragma"); + else + error ("missing cpu name in 'cpu' target attribute"); break; case AARCH64_PARSE_INVALID_ARG: - error ("unknown value %qs for 'cpu' target %s", str, pragma_or_attr); + if (is_pragma) + error ("unknown value %qs for 'cpu' target pragma", str); + else + error ("unknown value %qs for 'cpu' target attribute", str); aarch64_print_hint_for_core (str); break; case AARCH64_PARSE_INVALID_FEATURE: - error ("invalid feature modifier %qs for 'cpu' target %s", - str, pragma_or_attr); + if (is_pragma) + error ("invalid feature modifier %qs for 'cpu' target pragma", str); + else + error ("invalid feature modifier %qs for 'cpu' target attribute", + str); break; default: gcc_unreachable (); @@ -9641,7 +9659,7 @@ aarch64_handle_attr_cpu (const char *str, const char *pragma_or_attr) PRAGMA_OR_ATTR is used in potential error messages. */ static bool -aarch64_handle_attr_tune (const char *str, const char *pragma_or_attr) +aarch64_handle_attr_tune (const char *str, bool is_pragma) { const struct processor *tmp_tune = NULL; enum aarch64_parse_opt_result parse_res @@ -9658,7 +9676,10 @@ aarch64_handle_attr_tune (const char *str, const char *pragma_or_attr) switch (parse_res) { case AARCH64_PARSE_INVALID_ARG: - error ("unknown value %qs for 'tune' target %s", str, pragma_or_attr); + if (is_pragma) + error ("unknown value %qs for 'tune' target pragma", str); + else + error ("unknown value %qs for 'tune' target attribute", str); aarch64_print_hint_for_core (str); break; default: @@ -9675,7 +9696,7 @@ aarch64_handle_attr_tune (const char *str, const char *pragma_or_attr) PRAGMA_OR_ATTR is used in potential error messages. */ static bool -aarch64_handle_attr_isa_flags (char *str, const char *pragma_or_attr) +aarch64_handle_attr_isa_flags (char *str, bool is_pragma) { enum aarch64_parse_opt_result parse_res; unsigned long isa_flags = aarch64_isa_flags; @@ -9699,13 +9720,17 @@ aarch64_handle_attr_isa_flags (char *str, const char *pragma_or_attr) switch (parse_res) { case AARCH64_PARSE_MISSING_ARG: - error ("missing feature modifier in target %s %qs", - pragma_or_attr, str); + if (is_pragma) + error ("missing feature modifier in target pragma %qs", str); + else + error ("missing feature modifier in target attribute %qs", str); break; case AARCH64_PARSE_INVALID_FEATURE: - error ("invalid feature modifier in target %s %qs", - pragma_or_attr, str); + if (is_pragma) + error ("invalid feature modifier in target pragma %qs", str); + else + error ("invalid feature modifier in target attribute %qs", str); break; default: @@ -9748,7 +9773,7 @@ static const struct aarch64_attribute_info aarch64_attributes[] = we're processing a target attribute or pragma. */ static bool -aarch64_process_one_target_attr (char *arg_str, const char* pragma_or_attr) +aarch64_process_one_target_attr (char *arg_str, bool is_pragma) { bool invert = false; @@ -9756,7 +9781,10 @@ aarch64_process_one_target_attr (char *arg_str, const char* pragma_or_attr) if (len == 0) { - error ("malformed target %s", pragma_or_attr); + if (is_pragma) + error ("malformed target pragma"); + else + error ("malformed target attribute"); return false; } @@ -9772,7 +9800,7 @@ aarch64_process_one_target_attr (char *arg_str, const char* pragma_or_attr) through the machinery for the rest of the target attributes in this function. */ if (*str_to_check == '+') - return aarch64_handle_attr_isa_flags (str_to_check, pragma_or_attr); + return aarch64_handle_attr_isa_flags (str_to_check, is_pragma); if (len > 3 && strncmp (str_to_check, "no-", 3) == 0) { @@ -9804,8 +9832,12 @@ aarch64_process_one_target_attr (char *arg_str, const char* pragma_or_attr) if (attr_need_arg_p ^ (arg != NULL)) { - error ("target %s %qs does not accept an argument", - pragma_or_attr, str_to_check); + if (is_pragma) + error ("target pragma %qs does not accept an argument", + str_to_check); + else + error ("target attribute %qs does not accept an argument", + str_to_check); return false; } @@ -9813,8 +9845,12 @@ aarch64_process_one_target_attr (char *arg_str, const char* pragma_or_attr) then we can't match. */ if (invert && !p_attr->allow_neg) { - error ("target %s %qs does not allow a negated form", - pragma_or_attr, str_to_check); + if (is_pragma) + error ("target pragma %qs does not allow a negated form", + str_to_check); + else + error ("target attribute %qs does not allow a negated form", + str_to_check); return false; } @@ -9824,7 +9860,7 @@ aarch64_process_one_target_attr (char *arg_str, const char* pragma_or_attr) For example, cpu=, arch=, tune=. */ case aarch64_attr_custom: gcc_assert (p_attr->handler); - if (!p_attr->handler (arg, pragma_or_attr)) + if (!p_attr->handler (arg, is_pragma)) return false; break; @@ -9868,8 +9904,12 @@ aarch64_process_one_target_attr (char *arg_str, const char* pragma_or_attr) } else { - error ("target %s %s=%s is not valid", - pragma_or_attr, str_to_check, arg); + if (is_pragma) + error ("target pragma %s=%s is not valid", + str_to_check, arg); + else + error ("target attribute %s=%s is not valid", + str_to_check, arg); } break; } @@ -9908,7 +9948,7 @@ num_occurences_in_str (char c, char *str) a target attribute or a target pragma. */ bool -aarch64_process_target_attr (tree args, const char* pragma_or_attr) +aarch64_process_target_attr (tree args, bool is_pragma) { if (TREE_CODE (args) == TREE_LIST) { @@ -9917,7 +9957,7 @@ aarch64_process_target_attr (tree args, const char* pragma_or_attr) tree head = TREE_VALUE (args); if (head) { - if (!aarch64_process_target_attr (head, pragma_or_attr)) + if (!aarch64_process_target_attr (head, is_pragma)) return false; } args = TREE_CHAIN (args); @@ -9938,7 +9978,10 @@ aarch64_process_target_attr (tree args, const char* pragma_or_attr) if (len == 0) { - error ("malformed target %s value", pragma_or_attr); + if (is_pragma) + error ("malformed target pragma value"); + else + error ("malformed target attribute value"); return false; } @@ -9953,9 +9996,12 @@ aarch64_process_target_attr (tree args, const char* pragma_or_attr) while (token) { num_attrs++; - if (!aarch64_process_one_target_attr (token, pragma_or_attr)) + if (!aarch64_process_one_target_attr (token, is_pragma)) { - error ("target %s %qs is invalid", pragma_or_attr, token); + if (is_pragma) + error ("target pragma %qs is invalid", token); + else + error ("target attribute %qs is invalid", token); return false; } @@ -9964,8 +10010,11 @@ aarch64_process_target_attr (tree args, const char* pragma_or_attr) if (num_attrs != num_commas + 1) { - error ("malformed target %s list %qs", - pragma_or_attr, TREE_STRING_POINTER (args)); + if (is_pragma) + error ("malformed target pragma list %qs", TREE_STRING_POINTER (args)); + else + error ("malformed target attribute list %qs", + TREE_STRING_POINTER (args)); return false; } @@ -10025,7 +10074,7 @@ aarch64_option_valid_attribute_p (tree fndecl, tree, tree args, int) TREE_TARGET_OPTION (target_option_current_node)); - ret = aarch64_process_target_attr (args, "attribute"); + ret = aarch64_process_target_attr (args, false); /* Set up any additional state. */ if (ret)