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.
