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.
