Re: basic asm and memory clobbers - Proposed solution

[Bernd Edlinger <bernd dot edlinger at hotmail dot de>]

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): 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.
>

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.