Re: basic asm and memory clobbers - Proposed solution

[Richard Earnshaw <Richard dot Earnshaw at foss dot arm dot com>]

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): http://www.LimeGreenSocks.com/gcc/24414g.zip
>>
>> 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.