text: This is the mail archive of the gcc@gcc.gnu.org mailing list for the GCC project.
  On 01/12/2015 17:08, Richard Earnshaw wrote: > On 28/11/15 04:05, David Wohlferd wrote: >> On 11/24/2015 6:50 PM, David Wohlferd wrote: >>> I have solved the problem with my previous patch. Here's the update >>> (feedback welcome): >>> >>> Based on my understanding from the previous thread, this patch now >>> does what it needs to do (code-wise) to resolve this "basic asm and >>> memory clobbers" issue. As mentioned previously, this patch >>> introduces a new warning (-Wonly-top-basic-asm), which is disabled by >>> default. When enabled, it triggers a warning for any basic asm inside >>> a function, unless the function has the "naked" attribute. >>> >>> An argument can be made that the default for this warning should be >>> 'enabled.' Yes, this will break builds that use basic asm and >>> -Werror, but it can easily be disabled with -Wno-only-top-basic-asm. >>> And if we don't enable it, no one is going to know the check is even >>> available. Then hidden problems like the one Paul was just describing >>> still won't be found, and optimizations will continue to have >>> unexpected side effects. OTOH, I can also see changing this to >>> 'enabled' as more appropriate in the next phase 1. >>> >>> Now that I'm done with the code fix, I'm working on an update to the >>> docs. Obviously they should be checked in as part of the code fix. >>> I'm planning to actually use the word "deprecated" when describing the >>> use of basic asm within functions. Seems like a big step. >>> >>> But there's no point in my proceeding any further until someone in >>> authority agrees that this is the desired solution. I'm not actually >>> sure who that is, but further work is a waste of time if no one is >>> prepared to approve it. >>> >>> If you are that person, the questions to be answered are: >>> >>> 1) Is the idea of changing basic asm to "clobber everything" dead? >>> 2) Is creating a warning for the use of "basic asm inside a function" >>> the solution for this issue? >>> 3) Should the warning be enabled by default in v6? >>> 4) Should the warning be enabled by Wall or Wextra? >>> 5) Should the v6 docs explicitly describe using "basic asm inside a >>> function" as deprecated? >>> >>> If you're looking for my recommendations, I would say: Yes, Yes, >>> (reluctantly) No, No and Yes. >>> >>> With this information in hand, I'll take a shot at finishing this off. >>> >>> For something that started out as a simple 3 sentence doc patch, this >>> sure has turned into a project... >> I have incorporated the feedback from Hans-Peter Nilsson, who pointed >> out that the 'noinline' function attribute explicitly documents behavior >> related to using asm(""). The patch now includes an exception for >> this. Given how often this bit of code is used in various projects, >> allowing this exception will surely ease the transition. >> >> I'm told that some people won't review patches unless they are included >> as attachments, so this time the patch is attached. >> >> The patch includes first cut docs for the warning message, but I'm still >> hoping to hear from someone before trying to update the basic asm docs. >> >> dw >> >> 24414h.patch >> >> >> Index: gcc/c-family/c.opt >> =================================================================== >> --- gcc/c-family/c.opt (revision 230734) >> +++ gcc/c-family/c.opt (working copy) >> @@ -585,6 +585,10 @@ >> C++ ObjC++ Var(warn_namespaces) Warning >> Warn on namespace definition. >> >> +Wonly-top-basic-asm >> +C C++ Var(warn_only_top_basic_asm) Warning >> +Warn on unsafe uses of basic asm. >> + >> Wsized-deallocation >> C++ ObjC++ Var(warn_sized_deallocation) Warning EnabledBy(Wextra) >> Warn about missing sized deallocation functions. >> Index: gcc/c/c-parser.c >> =================================================================== >> --- gcc/c/c-parser.c (revision 230734) >> +++ gcc/c/c-parser.c (working copy) >> @@ -5880,7 +5880,18 @@ >> labels = NULL_TREE; >> >> if (c_parser_next_token_is (parser, CPP_CLOSE_PAREN) && !is_goto) >> + { >> + /* Warn on basic asm used inside of functions, >> + EXCEPT when in naked functions. Also allow asm(""). */ >> + if (warn_only_top_basic_asm && (TREE_STRING_LENGTH (str) != 1) ) >> + if (lookup_attribute ("naked", >> + DECL_ATTRIBUTES (current_function_decl)) >> + == NULL_TREE) > Formatting nit: the '== NULL_TREE)' should line up with the start of > 'lookup_attribute'. > > >> + warning_at(asm_loc, OPT_Wonly_top_basic_asm, >> + "asm statement in function does not use extended syntax"); >> + >> goto done_asm; >> + } >> >> /* Parse each colon-delimited section of operands. */ >> nsections = 3 + is_goto; >> Index: gcc/cp/parser.c >> =================================================================== >> --- gcc/cp/parser.c (revision 230734) >> +++ gcc/cp/parser.c (working copy) >> @@ -17594,6 +17594,8 @@ >> bool goto_p = false; >> required_token missing = RT_NONE; >> >> + location_t asm_loc = cp_lexer_peek_token (parser->lexer)->location; >> + >> /* Look for the `asm' keyword. */ >> cp_parser_require_keyword (parser, RID_ASM, RT_ASM); >> >> @@ -17752,6 +17754,16 @@ >> /* If the extended syntax was not used, mark the ASM_EXPR. */ >> if (!extended_p) >> { >> + /* Warn on basic asm used inside of functions, >> + EXCEPT when in naked functions. Also allow asm(""). */ >> + if (warn_only_top_basic_asm && >> + (TREE_STRING_LENGTH (string) != 1)) >> + if (lookup_attribute("naked", >> + DECL_ATTRIBUTES (current_function_decl)) >> + == NULL_TREE) > Same here. > > R. > And a test case is missing too. I think this warning concentrates now only on basic asm. And people will be probably fix it in the most easy way, by just adding a colon. But IMHO asm("bla":) isn't any better than asm("bla"). I think _any_ asm with non-empty assembler string, that claims to clobber _nothing_ is highly suspicious, and worth to be warned about. I don't see any exceptions from this rule. Bernd.