Wonly-top-basic-asm

Wonly-top-basic-asm

[David Wohlferd]

[-- Attachment #1: Type: text/plain, Size: 1558 bytes --]

I'm not sure which 'subsystem maintainer' to include on this as it 
affects parsers for both C and c++.  I've also cc'ed people from the 
discussion thread.

While that ~100 message thread on the gcc list about pr24414 didn't come 
to any final conclusions about clobbering registers for basic asm, there 
were a few things people agreed we could do to help users right now:

- Update the basic asm docs to describe basic asm's current (and 
historical) semantics (ie clobber nothing).
- Emphasize how that might be different from users' expectations or the 
behavior of other compilers.
- Warn that this could change in future versions of gcc.  To avoid 
impacts from this change, use extended asm.
- Implement and document -Wonly-top-basic-asm (disabled by default) as a 
way to locate affected statements.

This patch does these things.  You can review the new doc text at 
http://www.LimeGreenSocks.com/gcc/Basic-Asm.html

ChangeLog:
2016-01-24  David Wohlferd <dw@LimeGreenSocks.com>

     * doc/extend.texi: Doc basic asm behavior and new
     -Wonly-top-basic-asm option.
     * doc/invoke.texi: Doc new -Wonly-top-basic-asm option.
     * c-family/c.opt: Define -Wonly-top-basic-asm.
     * c/c-parser.c: Implement -Wonly-top-basic-asm for C.
     * cp/parser.c: Implement -Wonly-top-basic-asm for c++.
     * testsuite/c-c++-common/Wonly-top-basic-asm.c: New tests for
     -Wonly-top-basic-asm.
     * testsuite/c-c++-common/Wonly-top-basic-asm-2.c: Ditto.

Note that while I have a release on file with FSF, I don't have write 
access to SVN.

dw

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 24414N.patch --]
[-- Type: text/x-patch; name="24414N.patch", Size: 8750 bytes --]

Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 232773)
+++ 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 ObjC ObjC++ 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 232773)
+++ gcc/c/c-parser.c	(working copy)
@@ -5973,7 +5973,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)
+        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 232773)
+++ gcc/cp/parser.c	(working copy)
@@ -18003,6 +18003,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);
 
@@ -18161,6 +18163,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)
+	          warning_at(asm_loc, OPT_Wonly_top_basic_asm,
+	            "asm statement in function does not use extended syntax");
+
 	      tree temp = asm_stmt;
 	      if (TREE_CODE (temp) == CLEANUP_POINT_EXPR)
 		temp = TREE_OPERAND (temp, 0);
Index: gcc/doc/extend.texi
===================================================================
--- gcc/doc/extend.texi	(revision 232773)
+++ gcc/doc/extend.texi	(working copy)
@@ -2903,12 +2903,14 @@
 although the function call is live.  To keep such calls from being
 optimized away, put
 @smallexample
-asm ("");
+asm ("":::);
 @end smallexample
 
 @noindent
 (@pxref{Extended Asm}) in the called function, to serve as a special
 side-effect.
+Older code used @code{asm("")}, but newer code should use the extended
+@code{asm} format.
 
 @item nonnull (@var{arg-index}, @dots{})
 @cindex @code{nonnull} function attribute
@@ -7458,7 +7460,8 @@
 @end table
 
 @subsubheading Remarks
-Using extended @code{asm} typically produces smaller, safer, and more
+Using extended @code{asm} (@pxref{Extended Asm}) typically produces smaller,
+safer, and more
 efficient code, and in most cases it is a better solution than basic
 @code{asm}.  However, there are two situations where only basic @code{asm}
 can be used:
@@ -7487,6 +7490,8 @@
 consecutive in the output, put them in a single multi-instruction @code{asm}
 statement. Note that GCC's optimizers can move @code{asm} statements 
 relative to other code, including across jumps.
+Using inputs and outputs with extended @code{asm} can help correctly position
+your asm.
 
 @code{asm} statements may not perform jumps into other @code{asm} statements. 
 GCC does not know about these jumps, and therefore cannot take 
@@ -7497,6 +7502,7 @@
 assembly code when optimizing. This can lead to unexpected duplicate 
 symbol errors during compilation if your assembly code defines symbols or 
 labels.
+Extended @code{asm}'s @samp{%=} may help resolve this.
 
 Since GCC does not parse the @var{AssemblerInstructions}, it has no 
 visibility of any symbols it references. This may result in GCC discarding 
@@ -7516,11 +7522,59 @@
 Basic @code{asm} provides no
 mechanism to provide different assembler strings for different dialects.
 
-Here is an example of basic @code{asm} for i386:
+Basic @code{asm} statements within functions do not perform an implicit
+"memory" clobber (@pxref{Clobbers}).  Also, there is no implicit clobbering
+of @emph{any} registers, so (other than "naked" functions which follow the
+ABI rules) changed registers must be restored to their original value before
+exiting the @code{asm}.  While this behavior has not always been documented,
+GCC has worked this way since at least v2.95.3.  Also, lacking inputs and
+outputs means that GCC's optimizers may have difficulties consistently
+positioning the basic @code{asm} in the generated code.
 
+The concept of ``clobbering'' does not apply to basic @code{asm} statements
+outside of functions (aka top-level asm).
+
+@strong{Warning!} This "clobber nothing" behavior may be different than how
+other compilers treat basic @code{asm}, since the C standards for the
+@code{asm} statement provide no guidance regarding these semantics.  As a
+result, @code{asm} statements that work correctly on other compilers may not
+work correctly with GCC (and vice versa), even though they both compile
+without error.  Also, there is discussion underway about changing GCC to
+have basic @code{asm} clobber at least memory and perhaps some (or all)
+registers.  If implemented, this change may fix subtle problems with
+existing @code{asm} statements.  However it may break or slow down ones that
+were working correctly.
+
+If your existing code needs clobbers that GCC's basic @code{asm} is not 
+providing, or if you want to 'future-proof' your asm against possible 
+changes to basic @code{asm}'s semantics, use extended @code{asm}.
+Extended @code{asm} allows you to specify what (if anything) needs to be 
+clobbered for your code to work correctly.  You can use @ref{Warning 
+Options, @option{-Wonly-top-basic-asm}} to locate basic @code{asm} 
+statements that may need changes, and refer to 
+@uref{https://gcc.gnu.org/wiki/ConvertBasicAsmToExtended, How to convert 
+from basic asm to extended asm} for information about how to perform the 
+conversion.
+
+Here is an example of top-level basic @code{asm} for i386 that defines an
+asm macro.  That macro is then invoked from within a function using
+extended @code{asm}:
+
 @example
-/* Note that this code will not compile with -masm=intel */
-#define DebugBreak() asm("int $3")
+/* Define macro at file scope with basic asm. */
+/* Add macro parameter p to eax. */
+asm(".macro test p\n\t"
+    "addl $\\p, %eax\n\t"
+    ".endm");
+
+/* Use macro in function using extended asm.  It needs */
+/* the "cc" clobber since the flags are changed and uses */
+/* the "a" constraint since it modifies eax. */
+int DoAdd(int value)
+@{
+   asm("test 5" : "+a" (value) : : "cc");
+   return value;
+@}
 @end example
 
 @node Extended Asm
@@ -8047,7 +8101,7 @@
 for @code{d} by specifying both constraints.
 
 @anchor{FlagOutputOperands}
-@subsection Flag Output Operands
+@subsubsection Flag Output Operands
 @cindex @code{asm} flag output operands
 
 Some targets have a special register that holds the ``flags'' for the
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 232773)
+++ gcc/doc/invoke.texi	(working copy)
@@ -5693,6 +5693,21 @@
 a structure that has been marked with the @code{designated_init}
 attribute.
 
+@item -Wonly-top-basic-asm @r{(C and C++ only)}
+Warn if basic @code{asm} statements are used inside a function (i.e. not at
+top-level/file scope).
+
+When used inside of functions, basic @code{asm} can result in unexpected and
+unwanted variations in behavior between compilers due to how registers are
+handled when calling the asm (@pxref{Basic Asm}).  The lack of input and
+output constraints (@pxref{Extended Asm}) can also make it difficult for
+optimizers to correctly and consistently position the output relative to
+other code.
+
+Functions that are marked with the @option{naked} attribute (@pxref{Function
+Attributes}) and @code{asm} statements with an empty instruction string are
+excluded from this check.
+
 @item -Whsa
 Issue a warning when HSAIL cannot be emitted for the compiled function or
 OpenMP construct.

Re: Wonly-top-basic-asm

[Bernd Schmidt]

On 01/24/2016 11:23 PM, David Wohlferd wrote:
> +Wonly-top-basic-asm
> +C ObjC ObjC++ C++ Var(warn_only_top_basic_asm) Warning
> +Warn on unsafe uses of basic asm.

Maybe just -Wbasic-asm?

> +    /* Warn on basic asm used inside of functions,
> +       EXCEPT when in naked functions. Also allow asm(""). */

Two spaces after a sentence.

> +    if (warn_only_top_basic_asm && (TREE_STRING_LENGTH (str) != 1) )

Unnecessary parens, and extra space before closing paren.

> +	      if (warn_only_top_basic_asm &&
> +	          (TREE_STRING_LENGTH (string) != 1))

Extra parens, and && goes first on the next line.

> +	          warning_at(asm_loc, OPT_Wonly_top_basic_asm,

Space before "(".

> +	            "asm statement in function does not use extended syntax");

Could break that into ".." "..." over two lines so as to keep indentation.

> -asm ("");
> +asm ("":::);

Is that necessary? As far as I can tell we're treating these equally.

> @@ -7487,6 +7490,8 @@
>   consecutive in the output, put them in a single multi-instruction @code{asm}
>   statement. Note that GCC's optimizers can move @code{asm} statements
>   relative to other code, including across jumps.
> +Using inputs and outputs with extended @code{asm} can help correctly position
> +your asm.

Not sure this is needed either. Sounds a bit like advertising :) In 
general the doc changes seem much too verbose to me.

> +Extended @code{asm}'s @samp{%=} may help resolve this.

Same here. I think the block that recommends extended asm is good 
enough. I think the next part could be shrunk significantly too.

> -Here is an example of basic @code{asm} for i386:
> +Basic @code{asm} statements within functions do not perform an implicit
> +"memory" clobber (@pxref{Clobbers}).  Also, there is no implicit clobbering
> +of @emph{any} registers, so (other than "naked" functions which follow the

"other than in"? Also @code{naked} maybe. I'd place a note about 
clobbering after the existing "To access C data, it is better to use 
extended asm".

> +ABI rules) changed registers must be restored to their original value before
> +exiting the @code{asm}.  While this behavior has not always been documented,
> +GCC has worked this way since at least v2.95.3.  Also, lacking inputs and
> +outputs means that GCC's optimizers may have difficulties consistently
> +positioning the basic @code{asm} in the generated code.

The existing text already mentions ordering issues. Lose this block.

> +The concept of ``clobbering'' does not apply to basic @code{asm} statements
> +outside of functions (aka top-level asm).

Stating the obvious?

> +@strong{Warning!} This "clobber nothing" behavior may be different than how

Ok there is precedent for this, but it's spelt "@strong{Warning:}" in 
all other cases. Still, I'd probably also shrink this paragraph and put 
a note about lack of C semantics and possibly different behaviour from 
other compilers near the top, where we say that extended asm is better 
in most cases.

> +other compilers treat basic @code{asm}, since the C standards for the
> +@code{asm} statement provide no guidance regarding these semantics.  As a
> +result, @code{asm} statements that work correctly on other compilers may not
> +work correctly with GCC (and vice versa), even though they both compile
> +without error.  Also, there is discussion underway about changing GCC to
> +have basic @code{asm} clobber at least memory and perhaps some (or all)
> +registers.  If implemented, this change may fix subtle problems with
> +existing @code{asm} statements.  However it may break or slow down ones that
> +were working correctly.

How would such a change break anything? I'd also not mention discussion 
underway, just say "Future versions of GCC may treat basic @code{asm} as 
clobbering memory".

> +If your existing code needs clobbers that GCC's basic @code{asm} is not
> +providing, or if you want to 'future-proof' your asm against possible
> +changes to basic @code{asm}'s semantics, use extended @code{asm}.

Recommending it too often. Lose this.

> +Extended @code{asm} allows you to specify what (if anything) needs to be
> +clobbered for your code to work correctly.

And again.

> You can use @ref{Warning
> +Options, @option{-Wonly-top-basic-asm}} to locate basic @code{asm}

I think just plain @option is usual.

> +statements that may need changes, and refer to
> +@uref{https://gcc.gnu.org/wiki/ConvertBasicAsmToExtended, How to convert
> +from basic asm to extended asm} for information about how to perform the
> +conversion.

A link is probably good if we have such a page.

> +Here is an example of top-level basic @code{asm} for i386 that defines an
> +asm macro.  That macro is then invoked from within a function using
> +extended @code{asm}:

The updated example also looks good.

I think I'm fine with the concept but I'd like to see an updated patch 
with better docs.


Bernd

Re: Wonly-top-basic-asm

[Segher Boessenkool]

Hi David,

On Sun, Jan 24, 2016 at 02:23:53PM -0800, David Wohlferd wrote:
> - Warn that this could change in future versions of gcc.  To avoid 
> impacts from this change, use extended asm.
> - Implement and document -Wonly-top-basic-asm (disabled by default) as a 
> way to locate affected statements.

In my opinion we should not warn for any asm that means the same both
as basic and as extended asm.  The problem then becomes, what *is* the
meaning of a basic asm, what does it clobber.

Currently the only differences are:

- asms that have a % in the string, or {|} on targets with ASSEMBLER_DIALECT;
- ia64 (for stop bits);
- mep, and this one is easily fixed.
- basic asms do not get TARGET_MD_ASM_ADJUST.


Segher

Re: Wonly-top-basic-asm

[Bernd Schmidt]

On 01/26/2016 01:29 AM, Segher Boessenkool wrote:

> In my opinion we should not warn for any asm that means the same both
> as basic and as extended asm.  The problem then becomes, what *is* the
> meaning of a basic asm, what does it clobber.

I think this may be too hard to figure out in general without parsing 
the asm string, which we don't really want to do.


Bernd

Re: Wonly-top-basic-asm

[Segher Boessenkool]

On Tue, Jan 26, 2016 at 01:11:36PM +0100, Bernd Schmidt wrote:
> On 01/26/2016 01:29 AM, Segher Boessenkool wrote:
> 
> >In my opinion we should not warn for any asm that means the same both
> >as basic and as extended asm.  The problem then becomes, what *is* the
> >meaning of a basic asm, what does it clobber.
> 
> I think this may be too hard to figure out in general without parsing 
> the asm string, which we don't really want to do.

That depends on the semantics of basic asm.  With the currently implemented
semantics, it is trivial.


Segher

Re: Wonly-top-basic-asm

[David Wohlferd]

On 1/26/2016 8:11 AM, Segher Boessenkool wrote:
> On Tue, Jan 26, 2016 at 01:11:36PM +0100, Bernd Schmidt wrote:
>> On 01/26/2016 01:29 AM, Segher Boessenkool wrote:
>>
>>> In my opinion we should not warn for any asm that means the same both
>>> as basic and as extended asm.  The problem then becomes, what *is* the
>>> meaning of a basic asm, what does it clobber.
>> I think this may be too hard to figure out in general without parsing
>> the asm string, which we don't really want to do.
> That depends on the semantics of basic asm.  With the currently implemented
> semantics, it is trivial.

Oh?

asm("cmp al, '#' # if (c == '#') {");

There's a '{', so it might look like it needs to be escaped, but it 
doesn't.  The '{' is just part of a comment.  And how do you know it's a 
comment?  Because of the comment marker (#).  And how do you know that 
it's a comment marker and not a literal?  Only by doing more assembler 
parsing that I'm prepared to write.

But more importantly, consider this MIPS statement:

    asm ("sync");

That is the same for both basic and extended.  But I absolutely want the 
warning to flag this statement.  This is exactly the kind of "broken" 
statement that people need to be able to find/fix right now.

And when Jeff makes the changes to basic asm for v7, people may want to 
be able to find the statements that are affected by that in order to 
*stop* clobbering so many registers.

I'm not clear what people would use this warning for if we made the 
change you are suggesting.

dw

Re: Wonly-top-basic-asm

[David Wohlferd]

[-- Attachment #1: Type: text/plain, Size: 8253 bytes --]

Rumors that I earn a commission for every person I switch to the 
"extended asm" plan are completely unfounded... :)

That said, I truly believe there are very few cases where using basic 
asm within a function makes sense.  What's more, either they currently 
work incorrectly and need to be found and fixed, or they are going to 
take a performance hit when the new "clobber everything" semantics are 
implemented.  So yeah, I pushed pretty hard to have the docs say "DON'T 
USE IT!"

But reading it all again, you are right:  It's too much.

On 1/25/2016 4:25 AM, Bernd Schmidt wrote:
> On 01/24/2016 11:23 PM, David Wohlferd wrote:
>> +Wonly-top-basic-asm
>> +C ObjC ObjC++ C++ Var(warn_only_top_basic_asm) Warning
>> +Warn on unsafe uses of basic asm.
>
> Maybe just -Wbasic-asm?

Hmm.  Maybe.  I've never been completely satisfied with that name. But 
wouldn't this sound like it would warn on all uses of basic asm?  The 
intent is to only flag the ones in functions.  They are the ones with 
all the problems.

What would you say to -Wbasic-asm-in-function?  A little verbose...

>> +    /* Warn on basic asm used inside of functions,
>> +       EXCEPT when in naked functions. Also allow asm(""). */
>
> Two spaces after a sentence.

Fixed.

>> +    if (warn_only_top_basic_asm && (TREE_STRING_LENGTH (str) != 1) )
>
> Unnecessary parens, and extra space before closing paren.

Fixed.

>> +          if (warn_only_top_basic_asm &&
>> +              (TREE_STRING_LENGTH (string) != 1))
>
> Extra parens, and && goes first on the next line.

Fixed.

>> +              warning_at(asm_loc, OPT_Wonly_top_basic_asm,
>
> Space before "(".

Fixed.

>> +                "asm statement in function does not use extended 
>> syntax");
>
> Could break that into ".." "..." over two lines so as to keep 
> indentation.

Fixed.

>> -asm ("");
>> +asm ("":::);
>
> Is that necessary? As far as I can tell we're treating these equally.

While you are correct that today they are treated (nearly) equally, if 
Jeff does what he plans for v7, asm("") is going to translate (on x64) to:

asm("":::"rax", "rbx", "rcx", "rdx", "r8", "r9", "r10", "r11", "r12", 
"r13", "r14", "r15", "rdi", "rsi", "rbp", "cc", "memory");

Given that, it seemed appropriate for the docs to start steering people 
away from basic.  This particular instance was just something that was 
mentioned during the discussion.

However, that's for someday.  Maybe "someday" will turn into some other 
solution.   And since I'm not prepared to go thru all the docs and 
change all the other basic asms at this time, I removed this change.

>> @@ -7487,6 +7490,8 @@
>>   consecutive in the output, put them in a single multi-instruction 
>> @code{asm}
>>   statement. Note that GCC's optimizers can move @code{asm} statements
>>   relative to other code, including across jumps.
>> +Using inputs and outputs with extended @code{asm} can help correctly 
>> position
>> +your asm.
>
> Not sure this is needed either. Sounds a bit like advertising :) In 
> general the doc changes seem much too verbose to me.

I didn't think about this until it was brought up during the 
discussion.  But once it was pointed out to me, it made sense.

However, at your suggestion I have removed this.

>> +Extended @code{asm}'s @samp{%=} may help resolve this.
>
> Same here. I think the block that recommends extended asm is good 
> enough. I think the next part could be shrunk significantly too.

Removed.

>> -Here is an example of basic @code{asm} for i386:
>> +Basic @code{asm} statements within functions do not perform an implicit
>> +"memory" clobber (@pxref{Clobbers}).  Also, there is no implicit 
>> clobbering
>> +of @emph{any} registers, so (other than "naked" functions which 
>> follow the
>
> "other than in"? Also @code{naked} maybe. 

It works for me either way.  Since it bothers you, I changed it.

> I'd place a note about clobbering after the existing "To access C 
> data, it is better to use extended asm".
>
>> +ABI rules) changed registers must be restored to their original 
>> value before
>> +exiting the @code{asm}.  While this behavior has not always been 
>> documented,
>> +GCC has worked this way since at least v2.95.3.  Also, lacking 
>> inputs and
>> +outputs means that GCC's optimizers may have difficulties consistently
>> +positioning the basic @code{asm} in the generated code.
>
> The existing text already mentions ordering issues. Lose this block.

I've removed the rest of the paragraph after "Also"

Wait, didn't you tell me to remove the other mention of 'ordering'? I 
think I've removed all of them now.  Not a huge loss, but was that what 
you intended?

>> +The concept of ``clobbering'' does not apply to basic @code{asm} 
>> statements
>> +outside of functions (aka top-level asm).
>
> Stating the obvious?

I don't actually agree with this one.  However I do agree with your 
comment re being too verbose.  Since the new sample shows how to do this 
(using extended asm for clobbers), I have removed this.

>> +@strong{Warning!} This "clobber nothing" behavior may be different 
>> than how
>
> Ok there is precedent for this, but it's spelt "@strong{Warning:}" in 
> all other cases. 

Fixed.

> Still, I'd probably also shrink this paragraph and put a note about 
> lack of C semantics and possibly different behaviour from other 
> compilers near the top, where we say that extended asm is better in 
> most cases.
>
>> +other compilers treat basic @code{asm}, since the C standards for the
>> +@code{asm} statement provide no guidance regarding these semantics.  
>> As a
>> +result, @code{asm} statements that work correctly on other compilers 
>> may not
>> +work correctly with GCC (and vice versa), even though they both compile
>> +without error.  Also, there is discussion underway about changing 
>> GCC to
>> +have basic @code{asm} clobber at least memory and perhaps some (or all)
>> +registers.  If implemented, this change may fix subtle problems with
>> +existing @code{asm} statements.  However it may break or slow down 
>> ones that
>> +were working correctly.
>
> How would such a change break anything? I'd also not mention 
> discussion underway, just say "Future versions of GCC may treat basic 
> @code{asm} as clobbering memory".

Removed 'discussion underway', but kept references to clobbering 
registers.  That's Jeff's expressed intent, and part of why I believe 
people will want to find these statements.

>> +If your existing code needs clobbers that GCC's basic @code{asm} is not
>> +providing, or if you want to 'future-proof' your asm against possible
>> +changes to basic @code{asm}'s semantics, use extended @code{asm}.
>
> Recommending it too often. Lose this.
>
>> +Extended @code{asm} allows you to specify what (if anything) needs 
>> to be
>> +clobbered for your code to work correctly.
>
> And again.

I believe I have removed and replaced all the recurring redundant 
repetitiveness.

>> You can use @ref{Warning
>> +Options, @option{-Wonly-top-basic-asm}} to locate basic @code{asm}
>
> I think just plain @option is usual.

You are recommending I remove the link to the warning options? While I'm 
normally a big fan of links, I defer to your judgement. Removed.

>> +statements that may need changes, and refer to
>> +@uref{https://gcc.gnu.org/wiki/ConvertBasicAsmToExtended, How to 
>> convert
>> +from basic asm to extended asm} for information about how to perform 
>> the
>> +conversion.
>
> A link is probably good if we have such a page.

We do have such a page, but it was written by the same "too verbose" guy 
that wrote this patch.  Might be worth a review if you have a minute.

>> +Here is an example of top-level basic @code{asm} for i386 that 
>> defines an
>> +asm macro.  That macro is then invoked from within a function using
>> +extended @code{asm}:
>
> The updated example also looks good.

There are legitimate uses for basic asm.  While this sample is still 
trivial, it shows a much more important concept than the old one.

> I think I'm fine with the concept 

Thanks for the detailed review.

> but I'd like to see an updated patch with better docs.

Updated patch attached.  Now includes testsuites.  The updated web page 
is at http://www.limegreensocks.com/gcc/Basic-Asm.html

dw

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 24414P.patch --]
[-- Type: text/x-patch; name="24414P.patch", Size: 8472 bytes --]

Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 232773)
+++ 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 ObjC ObjC++ 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 232773)
+++ gcc/c/c-parser.c	(working copy)
@@ -5973,7 +5973,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)
+        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 232773)
+++ gcc/cp/parser.c	(working copy)
@@ -18003,6 +18003,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);
 
@@ -18161,6 +18163,17 @@
 	  /* 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)
+	          warning_at (asm_loc, OPT_Wonly_top_basic_asm,
+	                      "asm statement in function does not use extended"
+                              " syntax");
+
 	      tree temp = asm_stmt;
 	      if (TREE_CODE (temp) == CLEANUP_POINT_EXPR)
 		temp = TREE_OPERAND (temp, 0);
Index: gcc/doc/extend.texi
===================================================================
--- gcc/doc/extend.texi	(revision 232773)
+++ gcc/doc/extend.texi	(working copy)
@@ -7458,7 +7458,8 @@
 @end table
 
 @subsubheading Remarks
-Using extended @code{asm} typically produces smaller, safer, and more
+Using extended @code{asm} (@pxref{Extended Asm}) typically produces smaller,
+safer, and more
 efficient code, and in most cases it is a better solution than basic
 @code{asm}.  However, there are two situations where only basic @code{asm}
 can be used:
@@ -7516,11 +7517,51 @@
 Basic @code{asm} provides no
 mechanism to provide different assembler strings for different dialects.
 
-Here is an example of basic @code{asm} for i386:
+Basic @code{asm} statements do not perform an implicit "memory" clobber
+(@pxref{Clobbers}).  Also, there is no implicit clobbering of @emph{any}
+registers, so (other than in @code{naked} functions which follow the ABI
+rules) changed registers must be restored to their original value before
+exiting the @code{asm}.  While this behavior has not always been
+documented, GCC has worked this way since at least v2.95.3.
 
+@strong{Warning:} This "clobber nothing" behavior may be different than how
+other compilers treat basic @code{asm}, since the C standards for the
+@code{asm} statement provide no guidance regarding these semantics.  As a
+result, @code{asm} statements that work correctly on other compilers may not
+work correctly with GCC (and vice versa), even though they both compile
+without error.
+
+Future versions of GCC may change basic @code{asm} to clobber memory and
+perhaps some (or all) registers.  This change may fix subtle problems with
+existing @code{asm} statements.  However it may break or slow down ones
+that were working correctly.  To ``future-proof'' your asm against possible
+changes to basic @code{asm}'s semantics, use extended @code{asm}.
+
+You can use @option{-Wonly-top-basic-asm} to locate basic @code{asm}
+statements that may need changes, and refer to
+@uref{https://gcc.gnu.org/wiki/ConvertBasicAsmToExtended, How to convert
+from basic asm to extended asm} for information about how to perform the
+conversion.
+
+Here is an example of top-level basic @code{asm} for i386 that defines an
+asm macro.  That macro is then invoked from within a function using
+extended @code{asm}:
+
 @example
-/* Note that this code will not compile with -masm=intel */
-#define DebugBreak() asm("int $3")
+/* Define macro at file scope with basic asm. */
+/* Add macro parameter p to eax. */
+asm(".macro test p\n\t"
+    "addl $\\p, %eax\n\t"
+    ".endm");
+
+/* Use macro in function using extended asm.  It needs */
+/* the "cc" clobber since the flags are changed and uses */
+/* the "a" constraint since it modifies eax. */
+int DoAdd(int value)
+@{
+   asm("test 5" : "+a" (value) : : "cc");
+   return value;
+@}
 @end example
 
 @node Extended Asm
@@ -8047,7 +8088,7 @@
 for @code{d} by specifying both constraints.
 
 @anchor{FlagOutputOperands}
-@subsection Flag Output Operands
+@subsubsection Flag Output Operands
 @cindex @code{asm} flag output operands
 
 Some targets have a special register that holds the ``flags'' for the
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 232773)
+++ gcc/doc/invoke.texi	(working copy)
@@ -5693,6 +5693,21 @@
 a structure that has been marked with the @code{designated_init}
 attribute.
 
+@item -Wonly-top-basic-asm @r{(C and C++ only)}
+Warn if basic @code{asm} statements are used inside a function (i.e. not at
+top-level/file scope).
+
+When used inside of functions, basic @code{asm} can result in unexpected and
+unwanted variations in behavior between compilers due to how registers are
+handled when calling the asm (@pxref{Basic Asm}).  The lack of input and
+output constraints (@pxref{Extended Asm}) can also make it difficult for
+optimizers to correctly and consistently position the output relative to
+other code.
+
+Functions that are marked with the @option{naked} attribute (@pxref{Function
+Attributes}) and @code{asm} statements with an empty instruction string are
+excluded from this check.
+
 @item -Whsa
 Issue a warning when HSAIL cannot be emitted for the compiled function or
 OpenMP construct.
Index: gcc/testsuite/c-c++-common/Wonly-top-basic-asm-2.c
===================================================================
--- gcc/testsuite/c-c++-common/Wonly-top-basic-asm-2.c	(revision 0)
+++ gcc/testsuite/c-c++-common/Wonly-top-basic-asm-2.c	(working copy)
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target naked_functions } */
+/* { dg-options "-Wonly-top-basic-asm" } */
+
+int __attribute__((naked))
+func (int x, int y)
+{
+  /* basic asm should not warn in naked functions. */
+   asm(" "); /* no warning */
+}
+
+int main(int argc, char *argv[])
+{
+   return func(argc, argc);
+}
Index: gcc/testsuite/c-c++-common/Wonly-top-basic-asm.c
===================================================================
--- gcc/testsuite/c-c++-common/Wonly-top-basic-asm.c	(revision 0)
+++ gcc/testsuite/c-c++-common/Wonly-top-basic-asm.c	(working copy)
@@ -0,0 +1,35 @@
+/* { dg-do compile } */
+/* { dg-options "-Wonly-top-basic-asm" } */
+
+/* acceptable */
+register int b asm("esi");
+
+/* acceptable */
+int foo asm ("myfoo") = 2;
+
+/* acceptable */
+asm ("# top level");
+
+/* acceptable */
+int func (int x, int y) asm ("MYFUNC");
+
+int main(int argc, char *argv[])
+{
+   /* acceptable */
+   register int a asm("edi");
+
+   /* acceptable */
+   asm("#"::"r"(a), "r" (b));
+
+   /* acceptable */
+   asm goto (""::::done);
+
+   /* acceptable */
+   asm("");
+
+   /* warning */
+   asm(" "); /* { dg-warning "does not use extended syntax" } */
+
+  done:
+   return 0;
+}

Re: Wonly-top-basic-asm

[David Wohlferd]

[-- Attachment #1: Type: text/plain, Size: 8952 bytes --]

Hey Bernd.

I replied with a patch that includes most of the changes you asked for 
(see inline below).  Were you waiting on me for something more?

I have cleaned up the testcases so they aren't so i386-specific, but 
otherwise this patch (attached) is the same.  Let me know if there is 
something more I need to do here.

Thanks,
dw

On 1/27/2016 11:20 PM, David Wohlferd wrote:
> Rumors that I earn a commission for every person I switch to the 
> "extended asm" plan are completely unfounded... :)
>
> That said, I truly believe there are very few cases where using basic 
> asm within a function makes sense.  What's more, either they currently 
> work incorrectly and need to be found and fixed, or they are going to 
> take a performance hit when the new "clobber everything" semantics are 
> implemented.  So yeah, I pushed pretty hard to have the docs say 
> "DON'T USE IT!"
>
> But reading it all again, you are right:  It's too much.
>
> On 1/25/2016 4:25 AM, Bernd Schmidt wrote:
>> On 01/24/2016 11:23 PM, David Wohlferd wrote:
>>> +Wonly-top-basic-asm
>>> +C ObjC ObjC++ C++ Var(warn_only_top_basic_asm) Warning
>>> +Warn on unsafe uses of basic asm.
>>
>> Maybe just -Wbasic-asm?
>
> Hmm.  Maybe.  I've never been completely satisfied with that name. But 
> wouldn't this sound like it would warn on all uses of basic asm?  The 
> intent is to only flag the ones in functions.  They are the ones with 
> all the problems.
>
> What would you say to -Wbasic-asm-in-function?  A little verbose...
>
>>> +    /* Warn on basic asm used inside of functions,
>>> +       EXCEPT when in naked functions. Also allow asm(""). */
>>
>> Two spaces after a sentence.
>
> Fixed.
>
>>> +    if (warn_only_top_basic_asm && (TREE_STRING_LENGTH (str) != 1) )
>>
>> Unnecessary parens, and extra space before closing paren.
>
> Fixed.
>
>>> +          if (warn_only_top_basic_asm &&
>>> +              (TREE_STRING_LENGTH (string) != 1))
>>
>> Extra parens, and && goes first on the next line.
>
> Fixed.
>
>>> +              warning_at(asm_loc, OPT_Wonly_top_basic_asm,
>>
>> Space before "(".
>
> Fixed.
>
>>> +                "asm statement in function does not use extended 
>>> syntax");
>>
>> Could break that into ".." "..." over two lines so as to keep 
>> indentation.
>
> Fixed.
>
>>> -asm ("");
>>> +asm ("":::);
>>
>> Is that necessary? As far as I can tell we're treating these equally.
>
> While you are correct that today they are treated (nearly) equally, if 
> Jeff does what he plans for v7, asm("") is going to translate (on x64) 
> to:
>
> asm("":::"rax", "rbx", "rcx", "rdx", "r8", "r9", "r10", "r11", "r12", 
> "r13", "r14", "r15", "rdi", "rsi", "rbp", "cc", "memory");
>
> Given that, it seemed appropriate for the docs to start steering 
> people away from basic.  This particular instance was just something 
> that was mentioned during the discussion.
>
> However, that's for someday.  Maybe "someday" will turn into some 
> other solution.   And since I'm not prepared to go thru all the docs 
> and change all the other basic asms at this time, I removed this change.
>
>>> @@ -7487,6 +7490,8 @@
>>>   consecutive in the output, put them in a single multi-instruction 
>>> @code{asm}
>>>   statement. Note that GCC's optimizers can move @code{asm} statements
>>>   relative to other code, including across jumps.
>>> +Using inputs and outputs with extended @code{asm} can help 
>>> correctly position
>>> +your asm.
>>
>> Not sure this is needed either. Sounds a bit like advertising :) In 
>> general the doc changes seem much too verbose to me.
>
> I didn't think about this until it was brought up during the 
> discussion.  But once it was pointed out to me, it made sense.
>
> However, at your suggestion I have removed this.
>
>>> +Extended @code{asm}'s @samp{%=} may help resolve this.
>>
>> Same here. I think the block that recommends extended asm is good 
>> enough. I think the next part could be shrunk significantly too.
>
> Removed.
>
>>> -Here is an example of basic @code{asm} for i386:
>>> +Basic @code{asm} statements within functions do not perform an 
>>> implicit
>>> +"memory" clobber (@pxref{Clobbers}).  Also, there is no implicit 
>>> clobbering
>>> +of @emph{any} registers, so (other than "naked" functions which 
>>> follow the
>>
>> "other than in"? Also @code{naked} maybe. 
>
> It works for me either way.  Since it bothers you, I changed it.
>
>> I'd place a note about clobbering after the existing "To access C 
>> data, it is better to use extended asm".
>>
>>> +ABI rules) changed registers must be restored to their original 
>>> value before
>>> +exiting the @code{asm}.  While this behavior has not always been 
>>> documented,
>>> +GCC has worked this way since at least v2.95.3.  Also, lacking 
>>> inputs and
>>> +outputs means that GCC's optimizers may have difficulties consistently
>>> +positioning the basic @code{asm} in the generated code.
>>
>> The existing text already mentions ordering issues. Lose this block.
>
> I've removed the rest of the paragraph after "Also"
>
> Wait, didn't you tell me to remove the other mention of 'ordering'? I 
> think I've removed all of them now.  Not a huge loss, but was that 
> what you intended?
>
>>> +The concept of ``clobbering'' does not apply to basic @code{asm} 
>>> statements
>>> +outside of functions (aka top-level asm).
>>
>> Stating the obvious?
>
> I don't actually agree with this one.  However I do agree with your 
> comment re being too verbose.  Since the new sample shows how to do 
> this (using extended asm for clobbers), I have removed this.
>
>>> +@strong{Warning!} This "clobber nothing" behavior may be different 
>>> than how
>>
>> Ok there is precedent for this, but it's spelt "@strong{Warning:}" in 
>> all other cases. 
>
> Fixed.
>
>> Still, I'd probably also shrink this paragraph and put a note about 
>> lack of C semantics and possibly different behaviour from other 
>> compilers near the top, where we say that extended asm is better in 
>> most cases.
>>
>>> +other compilers treat basic @code{asm}, since the C standards for the
>>> +@code{asm} statement provide no guidance regarding these 
>>> semantics.  As a
>>> +result, @code{asm} statements that work correctly on other 
>>> compilers may not
>>> +work correctly with GCC (and vice versa), even though they both 
>>> compile
>>> +without error.  Also, there is discussion underway about changing 
>>> GCC to
>>> +have basic @code{asm} clobber at least memory and perhaps some (or 
>>> all)
>>> +registers.  If implemented, this change may fix subtle problems with
>>> +existing @code{asm} statements.  However it may break or slow down 
>>> ones that
>>> +were working correctly.
>>
>> How would such a change break anything? I'd also not mention 
>> discussion underway, just say "Future versions of GCC may treat basic 
>> @code{asm} as clobbering memory".
>
> Removed 'discussion underway', but kept references to clobbering 
> registers.  That's Jeff's expressed intent, and part of why I believe 
> people will want to find these statements.
>
>>> +If your existing code needs clobbers that GCC's basic @code{asm} is 
>>> not
>>> +providing, or if you want to 'future-proof' your asm against possible
>>> +changes to basic @code{asm}'s semantics, use extended @code{asm}.
>>
>> Recommending it too often. Lose this.
>>
>>> +Extended @code{asm} allows you to specify what (if anything) needs 
>>> to be
>>> +clobbered for your code to work correctly.
>>
>> And again.
>
> I believe I have removed and replaced all the recurring redundant 
> repetitiveness.
>
>>> You can use @ref{Warning
>>> +Options, @option{-Wonly-top-basic-asm}} to locate basic @code{asm}
>>
>> I think just plain @option is usual.
>
> You are recommending I remove the link to the warning options? While 
> I'm normally a big fan of links, I defer to your judgement. Removed.
>
>>> +statements that may need changes, and refer to
>>> +@uref{https://gcc.gnu.org/wiki/ConvertBasicAsmToExtended, How to 
>>> convert
>>> +from basic asm to extended asm} for information about how to 
>>> perform the
>>> +conversion.
>>
>> A link is probably good if we have such a page.
>
> We do have such a page, but it was written by the same "too verbose" 
> guy that wrote this patch.  Might be worth a review if you have a minute.
>
>>> +Here is an example of top-level basic @code{asm} for i386 that 
>>> defines an
>>> +asm macro.  That macro is then invoked from within a function using
>>> +extended @code{asm}:
>>
>> The updated example also looks good.
>
> There are legitimate uses for basic asm.  While this sample is still 
> trivial, it shows a much more important concept than the old one.
>
>> I think I'm fine with the concept 
>
> Thanks for the detailed review.
>
>> but I'd like to see an updated patch with better docs.
>
> Updated patch attached.  Now includes testsuites.  The updated web 
> page is at http://www.limegreensocks.com/gcc/Basic-Asm.html
>
> dw


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 24414Q.patch --]
[-- Type: text/x-patch; name="24414Q.patch", Size: 8617 bytes --]

Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 233206)
+++ 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 ObjC ObjC++ 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 233206)
+++ gcc/c/c-parser.c	(working copy)
@@ -5972,7 +5972,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)
+        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 233206)
+++ gcc/cp/parser.c	(working copy)
@@ -18021,6 +18021,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);
 
@@ -18179,6 +18181,17 @@
 	  /* 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)
+	          warning_at (asm_loc, OPT_Wonly_top_basic_asm,
+	                      "asm statement in function does not use extended"
+                              " syntax");
+
 	      tree temp = asm_stmt;
 	      if (TREE_CODE (temp) == CLEANUP_POINT_EXPR)
 		temp = TREE_OPERAND (temp, 0);
Index: gcc/doc/extend.texi
===================================================================
--- gcc/doc/extend.texi	(revision 233206)
+++ gcc/doc/extend.texi	(working copy)
@@ -7458,7 +7458,8 @@
 @end table
 
 @subsubheading Remarks
-Using extended @code{asm} typically produces smaller, safer, and more
+Using extended @code{asm} (@pxref{Extended Asm}) typically produces smaller,
+safer, and more
 efficient code, and in most cases it is a better solution than basic
 @code{asm}.  However, there are two situations where only basic @code{asm}
 can be used:
@@ -7516,11 +7517,51 @@
 Basic @code{asm} provides no
 mechanism to provide different assembler strings for different dialects.
 
-Here is an example of basic @code{asm} for i386:
+Basic @code{asm} statements do not perform an implicit "memory" clobber
+(@pxref{Clobbers}).  Also, there is no implicit clobbering of @emph{any}
+registers, so (other than in @code{naked} functions which follow the ABI
+rules) changed registers must be restored to their original value before
+exiting the @code{asm}.  While this behavior has not always been
+documented, GCC has worked this way since at least v2.95.3.
 
+@strong{Warning:} This "clobber nothing" behavior may be different than how
+other compilers treat basic @code{asm}, since the C standards for the
+@code{asm} statement provide no guidance regarding these semantics.  As a
+result, @code{asm} statements that work correctly on other compilers may not
+work correctly with GCC (and vice versa), even though they both compile
+without error.
+
+Future versions of GCC may change basic @code{asm} to clobber memory and
+perhaps some (or all) registers.  This change may fix subtle problems with
+existing @code{asm} statements.  However it may break or slow down ones
+that were working correctly.  To ``future-proof'' your asm against possible
+changes to basic @code{asm}'s semantics, use extended @code{asm}.
+
+You can use @option{-Wonly-top-basic-asm} to locate basic @code{asm}
+statements that may need changes, and refer to
+@uref{https://gcc.gnu.org/wiki/ConvertBasicAsmToExtended, How to convert
+from basic asm to extended asm} for information about how to perform the
+conversion.
+
+Here is an example of top-level basic @code{asm} for i386 that defines an
+asm macro.  That macro is then invoked from within a function using
+extended @code{asm}:
+
 @example
-/* Note that this code will not compile with -masm=intel */
-#define DebugBreak() asm("int $3")
+/* Define macro at file scope with basic asm. */
+/* Add macro parameter p to eax. */
+asm(".macro test p\n\t"
+    "addl $\\p, %eax\n\t"
+    ".endm");
+
+/* Use macro in function using extended asm.  It needs */
+/* the "cc" clobber since the flags are changed and uses */
+/* the "a" constraint since it modifies eax. */
+int DoAdd(int value)
+@{
+   asm("test 5" : "+a" (value) : : "cc");
+   return value;
+@}
 @end example
 
 @node Extended Asm
@@ -8047,7 +8088,7 @@
 for @code{d} by specifying both constraints.
 
 @anchor{FlagOutputOperands}
-@subsection Flag Output Operands
+@subsubsection Flag Output Operands
 @cindex @code{asm} flag output operands
 
 Some targets have a special register that holds the ``flags'' for the
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 233206)
+++ gcc/doc/invoke.texi	(working copy)
@@ -5728,6 +5728,21 @@
 a structure that has been marked with the @code{designated_init}
 attribute.
 
+@item -Wonly-top-basic-asm @r{(C and C++ only)}
+Warn if basic @code{asm} statements are used inside a function (i.e. not at
+top-level/file scope).
+
+When used inside of functions, basic @code{asm} can result in unexpected and
+unwanted variations in behavior between compilers due to how registers are
+handled when calling the asm (@pxref{Basic Asm}).  The lack of input and
+output constraints (@pxref{Extended Asm}) can also make it difficult for
+optimizers to correctly and consistently position the output relative to
+other code.
+
+Functions that are marked with the @option{naked} attribute (@pxref{Function
+Attributes}) and @code{asm} statements with an empty instruction string are
+excluded from this check.
+
 @item -Whsa
 Issue a warning when HSAIL cannot be emitted for the compiled function or
 OpenMP construct.
Index: gcc/testsuite/c-c++-common/Wonly-top-basic-asm-2.c
===================================================================
--- gcc/testsuite/c-c++-common/Wonly-top-basic-asm-2.c	(revision 0)
+++ gcc/testsuite/c-c++-common/Wonly-top-basic-asm-2.c	(working copy)
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target naked_functions } */
+/* { dg-options "-Wonly-top-basic-asm" } */
+
+int __attribute__((naked))
+func (int x, int y)
+{
+  /* basic asm should not warn in naked functions. */
+   asm(" "); /* no warning */
+}
+
+int main(int argc, char *argv[])
+{
+   return func(argc, argc);
+}
Index: gcc/testsuite/c-c++-common/Wonly-top-basic-asm.c
===================================================================
--- gcc/testsuite/c-c++-common/Wonly-top-basic-asm.c	(revision 0)
+++ gcc/testsuite/c-c++-common/Wonly-top-basic-asm.c	(working copy)
@@ -0,0 +1,43 @@
+/* { dg-do compile } */
+/* { dg-options "-Wonly-top-basic-asm" } */
+
+#if defined(__i386__) || defined(__x86_64__)
+/* acceptable */
+register int b asm("esi");
+#else
+int b = 3;
+#endif
+
+/* acceptable */
+int foo asm ("myfoo") = 2;
+
+/* acceptable */
+asm (" ");
+
+/* acceptable */
+int func (int x, int y) asm ("MYFUNC");
+
+int main(int argc, char *argv[])
+{
+#if defined(__i386__) || defined(__x86_64__)
+   /* acceptable */
+   register int a asm("edi");
+#else
+   int a = 2;
+#endif
+
+   /* acceptable */
+   asm(" "::"r"(a), "r" (b));
+
+   /* acceptable */
+   asm goto (""::::done);
+
+   /* acceptable */
+   asm("");
+
+   /* warning */
+   asm(" "); /* { dg-warning "does not use extended syntax" } */
+
+  done:
+   return 0;
+}

AW: Wonly-top-basic-asm

[Bernd Edlinger]

On 8. 2. 2016 04:45, David Wohlferd wrote:
> Hey Bernd.
> 
> I replied with a patch that includes most of the changes you asked for
> (see inline below).  Were you waiting on me for something more?
> 

ChangeLog entries are still missing.

> I have cleaned up the testcases so they aren't so i386-specific, but
> otherwise this patch (attached) is the same.  Let me know if there is
> something more I need to do here.
> 
> Thanks,
> dw

David, there is a tool that you can use to check the patch
for some style-nits before submission.

I used it and it complains about these things:
contrib/check_GNU_style.sh 24414Q.patch

Blocks of 8 spaces should be replaced with tabs.
29:+                            DECL_ATTRIBUTES (current_function_decl)) 
30:+          == NULL_TREE)
31:+        warning_at (asm_loc, OPT_Wonly_top_basic_asm, 
32:+                    "asm statement in function does not use extended syntax");
57:+	         EXCEPT when in naked functions.  Also allow asm(""). */
59:+	          && TREE_STRING_LENGTH (string) != 1)
60:+	        if (lookup_attribute("naked",
61:+	                             DECL_ATTRIBUTES (current_function_decl))
62:+	            == NULL_TREE)
63:+	          warning_at (asm_loc, OPT_Wonly_top_basic_asm,
64:+	                      "asm statement in function does not use extended"
65:+                              " syntax");

Trailing whitespace.
25:+    /* Warn on basic asm used inside of functions, 
28:+      if (lookup_attribute ("naked", 
29:+                            DECL_ATTRIBUTES (current_function_decl)) 
31:+        warning_at (asm_loc, OPT_Wonly_top_basic_asm, 

Dot, space, space, end of comment.
122:+/* Define macro at file scope with basic asm. */
123:+/* Add macro parameter p to eax. */
130:+/* the "a" constraint since it modifies eax. */
186:+  /* basic asm should not warn in naked functions. */

Sentences should end with a dot.  Dot, space, space, end of the comment.
128:+/* Use macro in function using extended asm.  It needs */
129:+/* the "cc" clobber since the flags are changed and uses */
187:+   asm(" "); /* no warning */
203:+/* acceptable */
209:+/* acceptable */
212:+/* acceptable */
215:+/* acceptable */
221:+   /* acceptable */
227:+   /* acceptable */
230:+   /* acceptable */
233:+   /* acceptable */
236:+   /* warning */

There should be exactly one space between function name and parentheses.
10:+C ObjC ObjC++ C++ Var(warn_only_top_basic_asm) Warning
26:+       EXCEPT when in naked functions.  Also allow asm(""). */
57:+	         EXCEPT when in naked functions.  Also allow asm(""). */
60:+	        if (lookup_attribute("naked",
124:+asm(".macro test p\n\t"
131:+int DoAdd(int value)
133:+   asm("test 5" : "+a" (value) : : "cc");
187:+   asm(" "); /* no warning */
190:+int main(int argc, char *argv[])
192:+   return func(argc, argc);
202:+#if defined(__i386__) || defined(__x86_64__)
204:+register int b asm("esi");
218:+int main(int argc, char *argv[])
220:+#if defined(__i386__) || defined(__x86_64__)
222:+   register int a asm("edi");
228:+   asm(" "::"r"(a), "r" (b));
234:+   asm("");
237:+   asm(" "); /* { dg-warning "does not use extended syntax" } */

Well regarding line 10 that is of course correct syntax.
And I looked with vi at gcc/testsuite/c-c++-common/Wonly-top-basic-asm.c
it seems to be using mixed windows and unix style line endings.
I would use unix line endings wherever possible, for consistency.


Thanks
Bernd.

Re: AW: Wonly-top-basic-asm

[David Wohlferd]

On 2/7/2016 10:45 PM, Bernd Edlinger wrote:
> On 8. 2. 2016 04:45, David Wohlferd wrote:
>> I replied with a patch that includes most of the changes you asked for
>> (see inline below).  Were you waiting on me for something more?
> ChangeLog entries are still missing.

I'll add them back in the next post.

> David, there is a tool that you can use to check the patch
> for some style-nits before submission.

I was not aware of this tool.  I'll fix these before the next post.

At one point you proposed renaming the option -Wbasic-asm.  This seemed 
a little terse (and possibly misleading) so I counter-proposed 
-Wbasic-asm-in-function (a little verbose, but clearer).  I have no 
strong preferences here, and you haven't said one way or the other.  Are 
we just going to stick with -Wonly-top-basic-asm?

Hopefully one more try and this will be done.

Thanks,
dw

Re: AW: Wonly-top-basic-asm

[David Wohlferd]

[-- Attachment #1: Type: text/plain, Size: 946 bytes --]

Since no one expressed any objections, I have renamed the option from 
-Wonly-top-basic-asm to -Wbasic-asm-in-function.  This more clearly 
conveys what the option does (give a warning if you find basic asm in a 
function).

I believe the attached patch addresses all the other outstanding comments.

ChangeLog:
2016-02-10  David Wohlferd  <dw@LimeGreenSocks.com>

     * doc/extend.texi: Doc basic asm behavior and new
     -Wbasic-asm-in-function option.
     * doc/invoke.texi: Doc new -Wbasic-asm-in-function option.
     * c-family/c.opt: Define -Wbasic-asm-in-function.
     * c/c-parser.c: Implement -Wbasic-asm-in-function for C.
     * cp/parser.c: Implement -Wbasic-asm-in-function for c++.
     * testsuite/c-c++-common/Wbasic-asm-in-function.c: New tests for
     -Wbasic-asm-in-function.
     * testsuite/c-c++-common/Wbasic-asm-in-function-2.c: Ditto.

While I have a release on file with FSF, I don't have write access to SVN.

dw

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 24414R.patch --]
[-- Type: text/x-patch; name="24414R.patch", Size: 8237 bytes --]

Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 233308)
+++ gcc/c-family/c.opt	(working copy)
@@ -585,6 +585,10 @@
 C++ ObjC++ Var(warn_namespaces) Warning
 Warn on namespace definition.
 
+Wbasic-asm-in-function
+C ObjC ObjC++ C++ Var(warn_basic_asm_in_function) 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 233308)
+++ gcc/c/c-parser.c	(working copy)
@@ -5972,7 +5972,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_basic_asm_in_function && TREE_STRING_LENGTH (str) != 1)
+      if (lookup_attribute ("naked",
+			    DECL_ATTRIBUTES (current_function_decl))
+	  == NULL_TREE)
+	warning_at (asm_loc, OPT_Wbasic_asm_in_function,
+		    "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 233308)
+++ gcc/cp/parser.c	(working copy)
@@ -18041,6 +18041,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);
 
@@ -18199,6 +18201,17 @@
 	  /* 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_basic_asm_in_function
+		  && TREE_STRING_LENGTH (string) != 1)
+		if (lookup_attribute ("naked",
+				     DECL_ATTRIBUTES (current_function_decl))
+		    == NULL_TREE)
+		  warning_at (asm_loc, OPT_Wbasic_asm_in_function,
+			      "asm statement in function does not use extended"
+			      " syntax");
+
 	      tree temp = asm_stmt;
 	      if (TREE_CODE (temp) == CLEANUP_POINT_EXPR)
 		temp = TREE_OPERAND (temp, 0);
Index: gcc/doc/extend.texi
===================================================================
--- gcc/doc/extend.texi	(revision 233308)
+++ gcc/doc/extend.texi	(working copy)
@@ -7458,7 +7458,8 @@
 @end table
 
 @subsubheading Remarks
-Using extended @code{asm} typically produces smaller, safer, and more
+Using extended @code{asm} (@pxref{Extended Asm}) typically produces smaller,
+safer, and more
 efficient code, and in most cases it is a better solution than basic
 @code{asm}.  However, there are two situations where only basic @code{asm}
 can be used:
@@ -7516,11 +7517,51 @@
 Basic @code{asm} provides no
 mechanism to provide different assembler strings for different dialects.
 
-Here is an example of basic @code{asm} for i386:
+Basic @code{asm} statements do not perform an implicit "memory" clobber
+(@pxref{Clobbers}).  Also, there is no implicit clobbering of @emph{any}
+registers, so (other than in @code{naked} functions which follow the ABI
+rules) changed registers must be restored to their original value before
+exiting the @code{asm}.  While this behavior has not always been
+documented, GCC has worked this way since at least v2.95.3.
 
+@strong{Warning:} This "clobber nothing" behavior may be different than how
+other compilers treat basic @code{asm}, since the C standards for the
+@code{asm} statement provide no guidance regarding these semantics.  As a
+result, @code{asm} statements that work correctly on other compilers may not
+work correctly with GCC (and vice versa), even though they both compile
+without error.
+
+Future versions of GCC may change basic @code{asm} to clobber memory and
+perhaps some (or all) registers.  This change may fix subtle problems with
+existing @code{asm} statements.  However it may break or slow down ones
+that were working correctly.  To ``future-proof'' your asm against possible
+changes to basic @code{asm}'s semantics, use extended @code{asm}.
+
+You can use @option{-Wbasic-asm-in-function} to locate basic @code{asm}
+statements that may need changes, and refer to
+@uref{https://gcc.gnu.org/wiki/ConvertBasicAsmToExtended, How to convert
+from basic asm to extended asm} for information about how to perform the
+conversion.
+
+Here is an example of top-level basic @code{asm} for i386 that defines an
+asm macro.  That macro is then invoked from within a function using
+extended @code{asm}:
+
 @example
-/* Note that this code will not compile with -masm=intel */
-#define DebugBreak() asm("int $3")
+/* Define macro at file scope with basic asm.  */
+/* Add macro parameter p to eax.  */
+asm (".macro test p\n\t"
+    "addl $\\p, %eax\n\t"
+    ".endm");
+
+/* Use macro in function using extended asm.  It needs
+   the "cc" clobber since the flags are changed and uses
+   the "a" constraint since it modifies eax.  */
+int DoAdd (int value)
+@{
+   asm ("test 5" : "+a" (value) : : "cc");
+   return value;
+@}
 @end example
 
 @node Extended Asm
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 233308)
+++ gcc/doc/invoke.texi	(working copy)
@@ -5727,6 +5727,21 @@
 a structure that has been marked with the @code{designated_init}
 attribute.
 
+@item -Wbasic-asm-in-function @r{(C and C++ only)}
+Warn if basic @code{asm} statements are used inside a function (i.e. not at
+top-level/file scope).
+
+When used inside of functions, basic @code{asm} can result in unexpected and
+unwanted variations in behavior between compilers due to how registers are
+handled when calling the asm (@pxref{Basic Asm}).  The lack of input and
+output constraints (@pxref{Extended Asm}) can also make it difficult for
+optimizers to correctly and consistently position the output relative to
+other code.
+
+Functions that are marked with the @option{naked} attribute (@pxref{Function
+Attributes}) and @code{asm} statements with an empty instruction string are
+excluded from this check.
+
 @item -Whsa
 Issue a warning when HSAIL cannot be emitted for the compiled function or
 OpenMP construct.
Index: gcc/testsuite/c-c++-common/Wbasic-asm-in-function-2.c
===================================================================
--- gcc/testsuite/c-c++-common/Wbasic-asm-in-function-2.c	(revision 0)
+++ gcc/testsuite/c-c++-common/Wbasic-asm-in-function-2.c	(working copy)
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target naked_functions } */
+/* { dg-options "-Wbasic-asm-in-function" } */
+
+int __attribute__((naked))
+func (int x, int y)
+{
+  /* Basic asm should not warn in naked functions.  */
+   asm (" "); /* No warning.  */
+}
+
+int main (int argc, char *argv[])
+{
+   return func (argc, argc);
+}
Index: gcc/testsuite/c-c++-common/Wbasic-asm-in-function.c
===================================================================
--- gcc/testsuite/c-c++-common/Wbasic-asm-in-function.c	(revision 0)
+++ gcc/testsuite/c-c++-common/Wbasic-asm-in-function.c	(working copy)
@@ -0,0 +1,43 @@
+/* { dg-do compile } */
+/* { dg-options "-Wbasic-asm-in-function" } */
+
+#if defined (__i386__) || defined (__x86_64__)
+/* Acceptable.  */
+register int b asm ("esi");
+#else
+int b = 3;
+#endif
+
+/* Acceptable.  */
+int foo asm ("myfoo") = 2;
+
+/* Acceptable.  */
+asm (" ");
+
+/* Acceptable.  */
+int func (int x, int y) asm ("MYFUNC");
+
+int main (int argc, char *argv[])
+{
+#if defined (__i386__) || defined (__x86_64__)
+   /* Acceptable.  */
+   register int a asm ("edi");
+#else
+   int a = 2;
+#endif
+
+   /* Acceptable.  */
+   asm (" "::"r"(a), "r" (b));
+
+   /* Acceptable.  */
+   asm goto (""::::done);
+
+   /* Acceptable.  */
+   asm ("");
+
+   /* Acceptable.  */
+   asm (" "); /* { dg-warning "does not use extended syntax" } */
+
+  done:
+   return 0;
+}

AW: AW: Wonly-top-basic-asm

[Bernd Edlinger]

On 11.2.2016, David Wohlferd wrote:
>
> Since no one expressed any objections, I have renamed the option from
> -Wonly-top-basic-asm to -Wbasic-asm-in-function.  This more clearly
> conveys what the option does (give a warning if you find basic asm in a
> function).
> 

why not simply -Wbasic-asm ?

> I believe the attached patch addresses all the other outstanding comments.
> 

Indentation wrong here. The whole block must be indented by 2 spaces.

>   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 (""). */

Comments should end with dot space space */

>+    if (warn_basic_asm_in_function && TREE_STRING_LENGTH (str) != 1)
>+      if (lookup_attribute ("naked",
>+			    DECL_ATTRIBUTES (current_function_decl))

the DECL_ATTRIBUTES should be at the same column as the "naked".

>+	  == NULL_TREE)
>+	warning_at (asm_loc, OPT_Wbasic_asm_in_function,
>+		    "asm statement in function does not use extended syntax");
>+
>     goto done_asm;
>+  }

>@@ -18199,6 +18201,17 @@
> 	  /* 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 (""). */

Comments should end with dot space space */

>+	      if (warn_basic_asm_in_function
>+		  && TREE_STRING_LENGTH (string) != 1)
>+		if (lookup_attribute ("naked",
>+				     DECL_ATTRIBUTES (current_function_decl))

the DECL_ATTRIBUTES should be at the same column as the "naked".

> ChangeLog:
> 2016-02-10  David Wohlferd  <dw@LimeGreenSocks.com>
> 
>     * doc/extend.texi: Doc basic asm behavior and new
>     -Wbasic-asm-in-function option.
>      * doc/invoke.texi: Doc new -Wbasic-asm-in-function option.
>      * c-family/c.opt: Define -Wbasic-asm-in-function.
>      * c/c-parser.c: Implement -Wbasic-asm-in-function for C.
>      * cp/parser.c: Implement -Wbasic-asm-in-function for c++.

C++, isn't it always upper case?

>      * testsuite/c-c++-common/Wbasic-asm-in-function.c: New tests for
>      -Wbasic-asm-in-function.
>      * testsuite/c-c++-common/Wbasic-asm-in-function-2.c: Ditto.
> 

ChangeLog lines begin with TAB.

Please split the ChangeLog, there are separate ChangeLogs
at gcc/ChangeLog (doc changes go in there)
gcc/c/ChangeLog, gcc/cp/ChangeLog, gcc/c-family/ChangLog
and gcc/testsuite/ChangeLog, the respective ChangeLog entries
use relative file names.

Please add the function name where you changed in brackets.

For instance:
    * c-parser.c (cp_parser_asm_definition): Implement -Wbasic-asm-in-function.


Thanks
Bernd.

> While I have a release on file with FSF, I don't have write access to SVN.
> 
> dw

Re: AW: Wonly-top-basic-asm

[Bernd Schmidt]

On 02/11/2016 12:49 AM, David Wohlferd wrote:
> I believe the attached patch addresses all the other outstanding comments.

Bernd Edlinger made some thorough comments; I'll just add a few more. I 
don't think this is a patch we're considering for gcc-6, at least not 
for the initial release - I imagine it could be backported from gcc-7 at 
some point.

Like the other Bernd I have a preference for just -Wbasic-asm. I'd make 
the analogy with -Wparentheses - we don't warn about every parenthesis, 
but the name of an option is not the place for detailed explanations of 
how it works. Less typing and less to remember is preferrable IMO.

> +	      /* Warn on basic asm used inside of functions,
> +		 EXCEPT when in naked functions.  Also allow asm (""). */

No all-caps.

>   @subsubheading Remarks
> -Using extended @code{asm} typically produces smaller, safer, and more
> +Using extended @code{asm} (@pxref{Extended Asm}) typically produces smaller,
> +safer, and more
>   efficient code, and in most cases it is a better solution than basic

Rewrap the paragraph?

> -Here is an example of basic @code{asm} for i386:
> +Basic @code{asm} statements do not perform an implicit "memory" clobber
> +(@pxref{Clobbers}).  Also, there is no implicit clobbering of @emph{any}
> +registers, so (other than in @code{naked} functions which follow the ABI
> +rules) changed registers must be restored to their original value before
> +exiting the @code{asm}.  While this behavior has not always been
> +documented, GCC has worked this way since at least v2.95.3.
>
> +@strong{Warning:} This "clobber nothing" behavior may be different than how
> +other compilers treat basic @code{asm}, since the C standards for the
> +@code{asm} statement provide no guidance regarding these semantics.  As a
> +result, @code{asm} statements that work correctly on other compilers may not
> +work correctly with GCC (and vice versa), even though they both compile
> +without error.
> +
> +Future versions of GCC may change basic @code{asm} to clobber memory and
> +perhaps some (or all) registers.  This change may fix subtle problems with
> +existing @code{asm} statements.  However it may break or slow down ones
> +that were working correctly.  To ``future-proof'' your asm against possible
> +changes to basic @code{asm}'s semantics, use extended @code{asm}.
> +
> +You can use @option{-Wbasic-asm-in-function} to locate basic @code{asm}
> +statements that may need changes, and refer to
> +@uref{https://gcc.gnu.org/wiki/ConvertBasicAsmToExtended, How to convert
> +from basic asm to extended asm} for information about how to perform the
> +conversion.

I still think this is too verbose and would serve to confuse rather than 
enlighten in practice. (If anyone feels otherwise, feel free to overrule 
me.) I'm also no longer sure about references to the wiki.

Let's look at this existing paragraph from the manual:

   Since GCC does not parse the @var{AssemblerInstructions}, it has
   no visibility of any symbols it references. This may result in GCC
   discarding those symbols as unreferenced.

I think extending this would be better. Something like

"Since the C standards does not specify semantics for @code{asm}, it is 
a potential source of incompatibilities between compilers.  GCC does not 
parse the @var{AssemblerInstructions}, which means there is no way to 
communicate to the compiler what is happening inside them.  GCC has no 
visibility of any symbols referenced in the @code{asm} and may discard 
them as unreferenced. It also does not know about side effects that may 
occur, such as modifications of memory locations or registers. GCC 
assumes that no such side effects occur, which may not be what the user 
expected if code was written for other compilers.

Since basic @code{asm} cannot easily be used in a reliable way, 
@option{-Wbasic-asm} should be used to warn about the use of basic asm 
inside a function. See 
@uref{https://gcc.gnu.org/wiki/ConvertBasicAsmToExtended, How to convert 
from basic asm to extended asm} for information about how to convert 
code to use extended @code{asm}."

But again, if someone feels the docs patch as posted is preferrable, go 
ahead and approve it (for stage1 I assume).


Bernd

Re: AW: Wonly-top-basic-asm

[Sandra Loosemore]

On 02/11/2016 08:40 AM, Bernd Schmidt wrote:

> But again, if someone feels the docs patch as posted is preferrable, go
> ahead and approve it (for stage1 I assume).

TBH, I haven't looked at the documentation patch at all; I've been 
ignoring this issue because (a) I thought the technical details were 
still under discussion and (b) adding a new command-line option is stage 
1 material not suitable to commit right now anyway.

I'll take a look at the docs once the name and behavior of the new 
option have been settled upon.

-Sandra

Re: AW: AW: Wonly-top-basic-asm

[David Wohlferd]

[-- Attachment #1: Type: text/plain, Size: 1639 bytes --]

> why not simply -Wbasic-asm ?

Since both you and Bernd favor this shorter name, I have changed it.

> Indentation wrong here. The whole block must be indented by 2 spaces.

Fixed.

> Comments should end with dot space space */

Fixed.

> the DECL_ATTRIBUTES should be at the same column as the "naked".

Fixed.

> Comments should end with dot space space */

Fixed.

> the DECL_ATTRIBUTES should be at the same column as the "naked".

Fixed.

> C++, isn't it always upper case?

Fixed.

> ChangeLog lines begin with TAB.

Hmm.  Thunderbird changed them to spaces.  I've tried something 
different this time.  Hopefully fixed.

> Please split the ChangeLog
> use relative file names.

Fixed (I think).

> Please add the function name where you changed in brackets.

Fixed. ================================================== ChangeLog: 
2016-02-11 David Wohlferd <dw@LimeGreenSocks.com> * doc/extend.texi: Doc 
basic asm behavior and new -Wbasic-asm option. * doc/invoke.texi: Doc 
new -Wbasic-asm option. ChangeLog: 2016-02-11 David Wohlferd 
<dw@LimeGreenSocks.com> * c.opt: Define -Wbasic-asm. ChangeLog: 
2016-02-11 David Wohlferd <dw@LimeGreenSocks.com> * c-parser.c 
(c_parser_asm_statement): Implement -Wbasic-asm for C. ChangeLog: 
2016-02-11 David Wohlferd <dw@LimeGreenSocks.com> * parser.c 
(cp_parser_asm_definition): Implement -Wbasic-asm for C++. ChangeLog: 
2016-02-11 David Wohlferd <dw@LimeGreenSocks.com> * 
c-c++-common/Wbasic-asm.c: New tests for -Wbasic-asm. * 
c-c++-common/Wbasic-asm-2.c: Ditto. New patch is attached. Note that 
Bernd Schmidt and I are still discussing changes to the docs (see next 
message). dw


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 24414S.patch --]
[-- Type: text/x-patch; name="24414S.patch", Size: 8129 bytes --]

Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 233367)
+++ gcc/c-family/c.opt	(working copy)
@@ -585,6 +585,10 @@
 C++ ObjC++ Var(warn_namespaces) Warning
 Warn on namespace definition.
 
+Wbasic-asm
+C ObjC ObjC++ C++ Var(warn_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 233367)
+++ gcc/c/c-parser.c	(working copy)
@@ -5978,8 +5978,19 @@
   labels = NULL_TREE;
 
   if (c_parser_next_token_is (parser, CPP_CLOSE_PAREN) && !is_goto)
-    goto done_asm;
+    {
+      /* Warn on basic asm used inside of functions,
+	 except when in naked functions.  Also allow asm ("").  */
+      if (warn_basic_asm && TREE_STRING_LENGTH (str) != 1)
+	if (lookup_attribute ("naked",
+			      DECL_ATTRIBUTES (current_function_decl))
+	    == NULL_TREE)
+	  warning_at (asm_loc, OPT_Wbasic_asm,
+		      "asm statement in function does not use extended syntax");
 
+      goto done_asm;
+    }
+
   /* Parse each colon-delimited section of operands.  */
   nsections = 3 + is_goto;
   for (section = 0; section < nsections; ++section)
Index: gcc/cp/parser.c
===================================================================
--- gcc/cp/parser.c	(revision 233367)
+++ gcc/cp/parser.c	(working copy)
@@ -18041,6 +18041,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);
 
@@ -18199,6 +18201,17 @@
 	  /* 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_basic_asm
+		  && TREE_STRING_LENGTH (string) != 1)
+		if (lookup_attribute ("naked",
+				      DECL_ATTRIBUTES (current_function_decl))
+		    == NULL_TREE)
+		  warning_at (asm_loc, OPT_Wbasic_asm,
+			      "asm statement in function does not use extended"
+			      " syntax");
+
 	      tree temp = asm_stmt;
 	      if (TREE_CODE (temp) == CLEANUP_POINT_EXPR)
 		temp = TREE_OPERAND (temp, 0);
Index: gcc/doc/extend.texi
===================================================================
--- gcc/doc/extend.texi	(revision 233367)
+++ gcc/doc/extend.texi	(working copy)
@@ -7458,7 +7458,8 @@
 @end table
 
 @subsubheading Remarks
-Using extended @code{asm} typically produces smaller, safer, and more
+Using extended @code{asm} (@pxref{Extended Asm}) typically produces smaller,
+safer, and more
 efficient code, and in most cases it is a better solution than basic
 @code{asm}.  However, there are two situations where only basic @code{asm}
 can be used:
@@ -7516,11 +7517,51 @@
 Basic @code{asm} provides no
 mechanism to provide different assembler strings for different dialects.
 
-Here is an example of basic @code{asm} for i386:
+Basic @code{asm} statements do not perform an implicit "memory" clobber
+(@pxref{Clobbers}).  Also, there is no implicit clobbering of @emph{any}
+registers, so (other than in @code{naked} functions which follow the ABI
+rules) changed registers must be restored to their original value before
+exiting the @code{asm}.  While this behavior has not always been
+documented, GCC has worked this way since at least v2.95.3.
 
+@strong{Warning:} This "clobber nothing" behavior may be different than how
+other compilers treat basic @code{asm}, since the C standards for the
+@code{asm} statement provide no guidance regarding these semantics.  As a
+result, @code{asm} statements that work correctly on other compilers may not
+work correctly with GCC (and vice versa), even though they both compile
+without error.
+
+Future versions of GCC may change basic @code{asm} to clobber memory and
+perhaps some (or all) registers.  This change may fix subtle problems with
+existing @code{asm} statements.  However it may break or slow down ones
+that were working correctly.  To ``future-proof'' your asm against possible
+changes to basic @code{asm}'s semantics, use extended @code{asm}.
+
+You can use @option{-Wbasic-asm} to locate basic @code{asm}
+statements that may need changes, and refer to
+@uref{https://gcc.gnu.org/wiki/ConvertBasicAsmToExtended, How to convert
+from basic asm to extended asm} for information about how to perform the
+conversion.
+
+Here is an example of top-level basic @code{asm} for i386 that defines an
+asm macro.  That macro is then invoked from within a function using
+extended @code{asm}:
+
 @example
-/* Note that this code will not compile with -masm=intel */
-#define DebugBreak() asm("int $3")
+/* Define macro at file scope with basic asm.  */
+/* Add macro parameter p to eax.  */
+asm (".macro test p\n\t"
+    "addl $\\p, %eax\n\t"
+    ".endm");
+
+/* Use macro in function using extended asm.  It needs
+   the "cc" clobber since the flags are changed and uses
+   the "a" constraint since it modifies eax.  */
+int DoAdd (int value)
+@{
+   asm ("test 5" : "+a" (value) : : "cc");
+   return value;
+@}
 @end example
 
 @node Extended Asm
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 233367)
+++ gcc/doc/invoke.texi	(working copy)
@@ -5727,6 +5727,21 @@
 a structure that has been marked with the @code{designated_init}
 attribute.
 
+@item -Wbasic-asm @r{(C and C++ only)}
+Warn if basic @code{asm} statements are used inside a function (i.e. not at
+top-level/file scope).
+
+When used inside of functions, basic @code{asm} can result in unexpected and
+unwanted variations in behavior between compilers due to how registers are
+handled when calling the asm (@pxref{Basic Asm}).  The lack of input and
+output constraints (@pxref{Extended Asm}) can also make it difficult for
+optimizers to correctly and consistently position the output relative to
+other code.
+
+Functions that are marked with the @option{naked} attribute (@pxref{Function
+Attributes}) and @code{asm} statements with an empty instruction string are
+excluded from this check.
+
 @item -Whsa
 Issue a warning when HSAIL cannot be emitted for the compiled function or
 OpenMP construct.
Index: gcc/testsuite/c-c++-common/Wbasic-asm-2.c
===================================================================
--- gcc/testsuite/c-c++-common/Wbasic-asm-2.c	(revision 0)
+++ gcc/testsuite/c-c++-common/Wbasic-asm-2.c	(working copy)
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target naked_functions } */
+/* { dg-options "-Wbasic-asm" } */
+
+int __attribute__((naked))
+func (int x, int y)
+{
+  /* Basic asm should not warn in naked functions.  */
+   asm (" "); /* No warning.  */
+}
+
+int main (int argc, char *argv[])
+{
+   return func (argc, argc);
+}
Index: gcc/testsuite/c-c++-common/Wbasic-asm.c
===================================================================
--- gcc/testsuite/c-c++-common/Wbasic-asm.c	(revision 0)
+++ gcc/testsuite/c-c++-common/Wbasic-asm.c	(working copy)
@@ -0,0 +1,43 @@
+/* { dg-do compile } */
+/* { dg-options "-Wbasic-asm" } */
+
+#if defined (__i386__) || defined (__x86_64__)
+/* Acceptable.  */
+register int b asm ("esi");
+#else
+int b = 3;
+#endif
+
+/* Acceptable.  */
+int foo asm ("myfoo") = 2;
+
+/* Acceptable.  */
+asm (" ");
+
+/* Acceptable.  */
+int func (int x, int y) asm ("MYFUNC");
+
+int main (int argc, char *argv[])
+{
+#if defined (__i386__) || defined (__x86_64__)
+   /* Acceptable.  */
+   register int a asm ("edi");
+#else
+   int a = 2;
+#endif
+
+   /* Acceptable.  */
+   asm (" "::"r"(a), "r" (b));
+
+   /* Acceptable.  */
+   asm goto (""::::done);
+
+   /* Acceptable.  */
+   asm ("");
+
+   /* Acceptable.  */
+   asm (" "); /* { dg-warning "does not use extended syntax" } */
+
+  done:
+   return 0;
+}

Re: AW: Wonly-top-basic-asm

[David Wohlferd]

> I don't think this is a patch we're considering for gcc-6, at least 
> not for the initial release - I imagine it could be backported from 
> gcc-7 at some point.

Actually, it was my intent that this apply to v6.  It's not like there 
is a significant change here.  We're documenting long-time behavior, and 
adding a (disabled) warning.

The reasons I think this is needed for v6 are:

1) We have become aware of places where basic asm's existing behavior is 
a problem.  This patch provides a way for users to locate these issues 
with a minimal, non-intrusive change.
2) There is a significant change to this behavior being proposed for 
v7.  When this happens, having a way to locate affected statements with 
features from a stable release seems desirable.

> Like the other Bernd I have a preference for just -Wbasic-asm. I'd 
> make the analogy with -Wparentheses - we don't warn about every 
> parenthesis, but the name of an option is not the place for detailed 
> explanations of how it works. Less typing and less to remember is 
> preferrable IMO.

While I still prefer Wbasic-asm-in-function, I really don't have strong 
feelings here.  Since both you and Bernd prefer this, I have changed it.

>> +          /* Warn on basic asm used inside of functions,
>> +         EXCEPT when in naked functions.  Also allow asm (""). */
>
> No all-caps.

Fixed.

>>   @subsubheading Remarks
>> -Using extended @code{asm} typically produces smaller, safer, and more
>> +Using extended @code{asm} (@pxref{Extended Asm}) typically produces 
>> smaller,
>> +safer, and more
>>   efficient code, and in most cases it is a better solution than basic
>
> Rewrap the paragraph?

I could, but then people get cranky about how hard the patch is to review.

>> -Here is an example of basic @code{asm} for i386:
>> +Basic @code{asm} statements do not perform an implicit "memory" clobber
>> +(@pxref{Clobbers}).  Also, there is no implicit clobbering of 
>> @emph{any}
>> +registers, so (other than in @code{naked} functions which follow the 
>> ABI
>> +rules) changed registers must be restored to their original value 
>> before
>> +exiting the @code{asm}.  While this behavior has not always been
>> +documented, GCC has worked this way since at least v2.95.3.
>>
>> +@strong{Warning:} This "clobber nothing" behavior may be different 
>> than how
>> +other compilers treat basic @code{asm}, since the C standards for the
>> +@code{asm} statement provide no guidance regarding these semantics.  
>> As a
>> +result, @code{asm} statements that work correctly on other compilers 
>> may not
>> +work correctly with GCC (and vice versa), even though they both compile
>> +without error.
>> +
>> +Future versions of GCC may change basic @code{asm} to clobber memory 
>> and
>> +perhaps some (or all) registers.  This change may fix subtle 
>> problems with
>> +existing @code{asm} statements.  However it may break or slow down ones
>> +that were working correctly.  To ``future-proof'' your asm against 
>> possible
>> +changes to basic @code{asm}'s semantics, use extended @code{asm}.
>> +
>> +You can use @option{-Wbasic-asm-in-function} to locate basic @code{asm}
>> +statements that may need changes, and refer to
>> +@uref{https://gcc.gnu.org/wiki/ConvertBasicAsmToExtended, How to 
>> convert
>> +from basic asm to extended asm} for information about how to perform 
>> the
>> +conversion.
>
> I still think this is too verbose and would serve to confuse rather 
> than enlighten in practice. (If anyone feels otherwise, feel free to 
> overrule me.) 

I assume you mean someone besides me...

> I'm also no longer sure about references to the wiki.

This is not the first reference to the gcc wiki in the docs (looks like 
the 6th for gcc, plus 10 for other wikis).

> Let's look at this existing paragraph from the manual:
>
>   Since GCC does not parse the @var{AssemblerInstructions}, it has
>   no visibility of any symbols it references. This may result in GCC
>   discarding those symbols as unreferenced.
>
> I think extending this would be better. Something like
>
> "Since the C standards does not specify semantics for @code{asm}, it 
> is a potential source of incompatibilities between compilers. GCC does 
> not parse the @var{AssemblerInstructions}, which means there is no way 
> to communicate to the compiler what is happening inside them.  GCC has 
> no visibility of any symbols referenced in the @code{asm} and may 
> discard them as unreferenced. It also does not know about side effects 
> that may occur, such as modifications of memory locations or 
> registers. GCC assumes that no such side effects occur, which may not 
> be what the user expected if code was written for other compilers.
>
> Since basic @code{asm} cannot easily be used in a reliable way, 
> @option{-Wbasic-asm} should be used to warn about the use of basic asm 
> inside a function. See 
> @uref{https://gcc.gnu.org/wiki/ConvertBasicAsmToExtended, How to 
> convert from basic asm to extended asm} for information about how to 
> convert code to use extended @code{asm}."

Hmm.  Yes, that's better.  But there are some things that got lost here 
that I think are important.  How about:

------------
@strong{Warning:} The C standards do not specify semantics for 
@code{asm}, making it a potential source of incompatibilities between 
compilers.  @code{asm} statements that work correctly on other compilers 
may not work correctly with GCC (and vice versa), even though both 
compile without error.

GCC does not parse basic @code{asm}'s @var{AssemblerInstructions}, which 
means there is no way to communicate to the compiler what is happening 
inside them.  GCC has no visibility of symbols in the @code{asm} and may 
discard them as unreferenced.  It also does not know about side effects 
of the assembler code, such as modifications to memory or registers.  
Unlike some compilers, GCC assumes that no changes to either memory or 
registers occur.  This assumption may change in a future release.

To avoid complications from future changes to the semantics and the 
compatibility issues between compilers, use @option{-Wbasic-asm} to warn 
about the use of basic asm inside a function.  See 
@uref{https://gcc.gnu.org/wiki/ConvertBasicAsmToExtended, How to convert 
from basic asm to extended asm} for information about how to convert 
code to use extended @code{asm}.
------------

dw

Re: AW: Wonly-top-basic-asm

[David Wohlferd]

On 2/11/2016 8:03 AM, Sandra Loosemore wrote:
> On 02/11/2016 08:40 AM, Bernd Schmidt wrote:
>
>> But again, if someone feels the docs patch as posted is preferrable, go
>> ahead and approve it (for stage1 I assume).
>
> TBH, I haven't looked at the documentation patch at all; I've been 
> ignoring this issue because (a) I thought the technical details were 
> still under discussion and (b) adding a new command-line option is 
> stage 1 material not suitable to commit right now anyway.
>
> I'll take a look at the docs once the name and behavior of the new 
> option have been settled upon.

So far, no one has suggested any changes to the behavior.  And we seem 
to have settled on a name.

dw

Re: AW: Wonly-top-basic-asm

[Bernd Schmidt]

On 02/12/2016 08:05 AM, David Wohlferd wrote:
> Actually, it was my intent that this apply to v6.  It's not like there
> is a significant change here.  We're documenting long-time behavior, and
> adding a (disabled) warning.

The doc patch (minus mentioning the warning) could go in now, but for 
gcc-6 we're at a stage where we're only accepting regression fixes with 
very few exceptions. If you can convince a RM that this is important 
enough then it could still go in.

> 2) There is a significant change to this behavior being proposed for
> v7.  When this happens, having a way to locate affected statements with
> features from a stable release seems desirable.

I'm actually not convinced that we'll want to change much in asm 
behaviour. Clobbering memory, maybe, but I can't see much beyond that - 
there's just no gain and some risk. So I'm a little more relaxed about 
the whole thing.

>> "Since the C standards does not specify semantics for @code{asm}, it
>> is a potential source of incompatibilities between compilers. GCC does
>> not parse the @var{AssemblerInstructions}, which means there is no way
>> to communicate to the compiler what is happening inside them.  GCC has
>> no visibility of any symbols referenced in the @code{asm} and may
>> discard them as unreferenced. It also does not know about side effects
>> that may occur, such as modifications of memory locations or
>> registers. GCC assumes that no such side effects occur, which may not
>> be what the user expected if code was written for other compilers.
>>
>> Since basic @code{asm} cannot easily be used in a reliable way,
>> @option{-Wbasic-asm} should be used to warn about the use of basic asm
>> inside a function. See
>> @uref{https://gcc.gnu.org/wiki/ConvertBasicAsmToExtended, How to
>> convert from basic asm to extended asm} for information about how to
>> convert code to use extended @code{asm}."
>
> Hmm.  Yes, that's better.  But there are some things that got lost here
> that I think are important.  How about:
>
> ------------
> @strong{Warning:} The C standards do not specify semantics for
> @code{asm}, making it a potential source of incompatibilities between
> compilers.  @code{asm} statements that work correctly on other compilers
> may not work correctly with GCC (and vice versa), even though both
> compile without error.

This is what I mean when I say "too verbose" - the second sentence 
essentially says exactly the same thing as the first. The repetition is 
unnecessary, and I'd drop it.

> GCC does not parse basic @code{asm}'s @var{AssemblerInstructions}, which
> means there is no way to communicate to the compiler what is happening
> inside them.  GCC has no visibility of symbols in the @code{asm} and may
> discard them as unreferenced.  It also does not know about side effects
> of the assembler code, such as modifications to memory or registers.
> Unlike some compilers, GCC assumes that no changes to either memory or
> registers occur.  This assumption may change in a future release.
>
> To avoid complications from future changes to the semantics and the
> compatibility issues between compilers, use @option{-Wbasic-asm} to warn
> about the use of basic asm inside a function.  See
> @uref{https://gcc.gnu.org/wiki/ConvertBasicAsmToExtended, How to convert
> from basic asm to extended asm} for information about how to convert
> code to use extended @code{asm}.

Other than that they look quite similar, and I think your new suggestion 
is good too. Let's let Sandra have the last word.


Bernd

Re: AW: Wonly-top-basic-asm

[Sandra Loosemore]

On 02/12/2016 05:51 AM, Bernd Schmidt wrote:
> On 02/12/2016 08:05 AM, David Wohlferd wrote:
>> Actually, it was my intent that this apply to v6.  It's not like there
>> is a significant change here.  We're documenting long-time behavior, and
>> adding a (disabled) warning.
>
> The doc patch (minus mentioning the warning) could go in now, but for
> gcc-6 we're at a stage where we're only accepting regression fixes with
> very few exceptions. If you can convince a RM that this is important
> enough then it could still go in.

I looked at the last version of the patch I saw and this is my 
conclusion as well.  If you would like me to commit just the doc change 
(minus the references to the new warning) now, please split the patch 
and I will do that.  But, I cannot commit the change to add the new 
warning during Stage 4 without approval from a RM.

-Sandra

Re: AW: Wonly-top-basic-asm

[David Wohlferd]

On 2/12/2016 4:51 AM, Bernd Schmidt wrote:
> On 02/12/2016 08:05 AM, David Wohlferd wrote:
>> Actually, it was my intent that this apply to v6.  It's not like there
>> is a significant change here.  We're documenting long-time behavior, and
>> adding a (disabled) warning.
>
> The doc patch (minus mentioning the warning) could go in now, but for 
> gcc-6 we're at a stage where we're only accepting regression fixes 
> with very few exceptions. If you can convince a RM that this is 
> important enough then it could still go in.

Understood.  While I think the patch is small enough to be safe, whether 
it's important enough for this stage is a different question.

>> 2) There is a significant change to this behavior being proposed for
>> v7.  When this happens, having a way to locate affected statements with
>> features from a stable release seems desirable.
>
> I'm actually not convinced that we'll want to change much in asm 
> behaviour. Clobbering memory, maybe, but I can't see much beyond that 
> - there's just no gain and some risk. So I'm a little more relaxed 
> about the whole thing.

And that's the question.  If you are correct that we won't be changing 
this, then yeah, update the docs for v6, push the code change to v7.  Done.

But Jeff sounded quite serious when he said "the only rational behaviour 
for a traditional asm is that it has to be considered a use/clobber of 
memory and hard registers."  If indeed that is the plan for v7, then 
having this warning available in v6 to allow people to prepare becomes 
important.

Jeff Law: If you are listening, care to share your plans here?

> Let's let Sandra have the last word [about the docs].

Good plan.

dw

Re: AW: Wonly-top-basic-asm

[David Wohlferd]

On 2/12/2016 5:03 PM, Sandra Loosemore wrote:
> On 02/12/2016 05:51 AM, Bernd Schmidt wrote:
>> On 02/12/2016 08:05 AM, David Wohlferd wrote:
>>> Actually, it was my intent that this apply to v6.  It's not like there
>>> is a significant change here.  We're documenting long-time behavior, 
>>> and
>>> adding a (disabled) warning.
>>
>> The doc patch (minus mentioning the warning) could go in now, but for
>> gcc-6 we're at a stage where we're only accepting regression fixes with
>> very few exceptions. If you can convince a RM that this is important
>> enough then it could still go in.
>
> I looked at the last version of the patch I saw and this is my 
> conclusion as well.  If you would like me to commit just the doc 
> change (minus the references to the new warning) now, please split the 
> patch and I will do that.  But, I cannot commit the change to add the 
> new warning during Stage 4 without approval from a RM.

Fair enough.  Committing what we can right now sounds like a good plan.

Bernd and I have both posted alternate text to what was in the last 
patch (see https://gcc.gnu.org/ml/gcc-patches/2016-02/msg00861.html).  
He proposed (and I agreed) that having you make the call about which was 
better might be reasonable way to finalize this.

If you want to pick one, I'll remove the Wbasic-asm and turn it into a 
doc-only patch.  Or maybe you'd rather scrap them both and propose your own?

I'm flexible here.  There are important concepts that need to be 
conveyed.  Doesn't much matter to me who writes them.

dw

Re: Wonly-top-basic-asm

[Jan Hubicka]

>  @example
> -/* Note that this code will not compile with -masm=intel */
> -#define DebugBreak() asm("int $3")
> +/* Define macro at file scope with basic asm. */
> +/* Add macro parameter p to eax. */
> +asm(".macro test p\n\t"
> +    "addl $\\p, %eax\n\t"
> +    ".endm");
> +
> +/* Use macro in function using extended asm.  It needs */
> +/* the "cc" clobber since the flags are changed and uses */
> +/* the "a" constraint since it modifies eax. */
> +int DoAdd(int value)
> +@{
> +   asm("test 5" : "+a" (value) : : "cc");
> +   return value;
> +@}

To make this work you need the .macro appear before all uses in the asm files.  I do not think
we really promise that wihtout -fno-toplevel-reorder -fno-lto

Honza

Re: Wonly-top-basic-asm

[Bernd Edlinger]

On 16.02.2016 15:03, Jan Hubicka wrote:
>>   @example
>> -/* Note that this code will not compile with -masm=intel */
>> -#define DebugBreak() asm("int $3")
>> +/* Define macro at file scope with basic asm. */
>> +/* Add macro parameter p to eax. */
>> +asm(".macro test p\n\t"
>> +    "addl $\\p, %eax\n\t"
>> +    ".endm");
>> +
>> +/* Use macro in function using extended asm.  It needs */
>> +/* the "cc" clobber since the flags are changed and uses */
>> +/* the "a" constraint since it modifies eax. */
>> +int DoAdd(int value)
>> +@{
>> +   asm("test 5" : "+a" (value) : : "cc");
>> +   return value;
>> +@}
>
> To make this work you need the .macro appear before all uses in the asm files.  I do not think
> we really promise that wihtout -fno-toplevel-reorder -fno-lto
>
> Honza
>

And, isn't "test" an assembler instruction name?
What if the compiler emits something like test %eax,1
somewhere in the same translation unit?

Bernd.

Re: AW: Wonly-top-basic-asm

[David Wohlferd]

[-- Attachment #1: Type: text/plain, Size: 351 bytes --]

On 2/13/2016 8:00 PM, David Wohlferd wrote:
> Fair enough.  Committing what we can right now sounds like a good plan.

Attached is the doc patch, minus the proposed warning.

ChangeLog:

2016-02-19  David Wohlferd  <dw@LimeGreenSocks.com>
         Bernd Schmidt  <bschmidt@redhat.com>

     * doc/extend.texi: Doc basic asm behavior re clobbers.

dw


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 24414T.patch --]
[-- Type: text/x-patch; name="24414T.patch", Size: 3015 bytes --]

Index: extend.texi
===================================================================
--- extend.texi	(revision 233367)
+++ extend.texi	(working copy)
@@ -7458,7 +7458,8 @@
 @end table
 
 @subsubheading Remarks
-Using extended @code{asm} typically produces smaller, safer, and more
+Using extended @code{asm} (@pxref{Extended Asm}) typically produces smaller,
+safer, and more
 efficient code, and in most cases it is a better solution than basic
 @code{asm}.  However, there are two situations where only basic @code{asm}
 can be used:
@@ -7498,10 +7499,25 @@
 symbol errors during compilation if your assembly code defines symbols or 
 labels.
 
-Since GCC does not parse the @var{AssemblerInstructions}, it has no 
-visibility of any symbols it references. This may result in GCC discarding 
-those symbols as unreferenced.
+@strong{Warning:} The C standards do not specify semantics for @code{asm},
+making it a potential source of incompatibilities between compilers.  These
+incompatibilities may not produce compiler warnings/errors.
 
+GCC does not parse basic @code{asm}'s @var{AssemblerInstructions}, which
+means there is no way to communicate to the compiler what is happening
+inside them.  GCC has no visibility of symbols in the @code{asm} and may
+discard them as unreferenced.  It also does not know about side effects of
+the assembler code, such as modifications to memory or registers.  Unlike
+some compilers, GCC assumes that no changes to either memory or registers
+occur.  This assumption may change in a future release.
+
+To avoid complications from future changes to the semantics and the
+compatibility issues between compilers, consider replacing basic @code{asm}
+with extended @code{asm}.  See
+@uref{https://gcc.gnu.org/wiki/ConvertBasicAsmToExtended, How to convert
+from basic asm to extended asm} for information about how to perform this
+conversion.
+
 The compiler copies the assembler instructions in a basic @code{asm} 
 verbatim to the assembly language output file, without 
 processing dialects or any of the @samp{%} operators that are available with
@@ -7516,11 +7532,25 @@
 Basic @code{asm} provides no
 mechanism to provide different assembler strings for different dialects.
 
-Here is an example of basic @code{asm} for i386:
+Here is an example of top-level basic @code{asm} for i386 that defines an
+asm macro.  That macro is then invoked from within a function using
+extended @code{asm}:
 
 @example
-/* Note that this code will not compile with -masm=intel */
-#define DebugBreak() asm("int $3")
+/* Define macro at file scope with basic asm.  */
+/* Add macro parameter p to eax.  */
+asm (".macro testme p\n\t"
+    "addl $\\p, %eax\n\t"
+    ".endm");
+
+/* Use macro in function using extended asm.  It needs
+   the "cc" clobber since the flags are changed and uses
+   the "a" constraint since it modifies eax.  */
+int DoAdd (int value)
+@{
+   asm ("testme 5" : "+a" (value) : : "cc");
+   return value;
+@}
 @end example
 
 @node Extended Asm

Re: Wonly-top-basic-asm

[Bernd Edlinger]

On 20.02.2016 02:03, David Wohlferd wrote:
>   @example
> -/* Note that this code will not compile with -masm=intel */
> -#define DebugBreak() asm("int $3")
> +/* Define macro at file scope with basic asm.  */
> +/* Add macro parameter p to eax.  */
> +asm (".macro testme p\n\t"
> +    "addl $\\p, %eax\n\t"
> +    ".endm");
> +
> +/* Use macro in function using extended asm.  It needs
> +   the "cc" clobber since the flags are changed and uses
> +   the "a" constraint since it modifies eax.  */
> +int DoAdd (int value)
> +@{
> +   asm ("testme 5" : "+a" (value) : : "cc");
> +   return value;
> +@}
>   @end example

Sorry, but I don't like this example at all.

First the new example is essentially academic and useless,
while the previous example could well be used
in real world code, except we could note here
that we also have a __builtin_trap () now.

Second if you don't mention that "cc" is already implicitly
clobbered by default, and if it is written here it is ignored
on i386 targets, then everybody will hurry to modify their
asm code when there is no real reason for that.


Bernd.

Re: Wonly-top-basic-asm

[David Wohlferd]

On 2/20/2016 4:08 AM, Bernd Edlinger wrote:
> Sorry, but I don't like this example at all.
>
> First the new example is essentially academic and useless,

When used within a function, basic asm:

- causes difficulties for optimizers
- produces incompatibilities with other compilers
- has semantics that are the opposite of what users expect
- may soon produce incompatibilities with other versions of gcc

Given all this, why would we want our sample to show using basic asm 
within a function (as you are suggesting) instead of working to 
discourage it?

Contrawise, even people who are advocating for the deprecation/removal 
of basic asm (like me) acknowledge that it is needed for top level.  
That is why I use it for the sample.  I suppose I could grab some actual 
top level code from the linux source (like ".symver 
__netf2_compat,__netf2@GCC_3.0").  But while it's real-world instead of 
"academic," it's also not as clear as I'd like for a sample.

Obviously this testme code isn't going to be placed verbatim in 
someone's project.  However, if someone wants to use asm macros (a 
plausible if uncommon case), this sample shows them how, using a simple, 
easy-to-understand (if not particularly useful) method.

In contrast, the "int $3" you seem to favor doesn't really show anything 
(even how to do multiple lines).  And worse, Jeff's plan is to change 
basic asm statements so they clobber ALL registers plus memory.  Even a 
trivial example shows how this results in gcc generating completely 
different code around an "int $3."  Which makes it rather problematical 
for debugging (can you say heisenbug?), which is the primary use for int 3.

> while the previous example could well be used
> in real world code, except we could note here
> that we also have a __builtin_trap () now.

After reading the documentation for __builtin_trap, I still have 
absolutely no idea what it does.  Worse, after NOT saying precisely what 
it does on any given platform, the docs make it clear that whatever it 
does isn't fixed and can change at any time.

I'm a big believer in using builtins instead of inline asm.  But based 
on these docs, I can't ever see using __builtin_trap myself.

> Second if you don't mention that "cc" is already implicitly
> clobbered by default, and if it is written here it is ignored
> on i386 targets, then everybody will hurry to modify their
> asm code when there is no real reason for that.

The meaning of the "cc" clobber is defined under extended asm.  The 
usage in this sample is consistent with that definition.  In the 
(unlikely) event that "everybody" decides to change their code, they 
will all have (slightly) better documented code which is written in 
accordance with the docs.  And which behaves precisely the same as what 
they have now.

So now what?  I have one Bernd who likes the sample, and one who 
doesn't.  Obviously I think what I'm proposing is better than what's 
there now and I've done my best to say why.  But me believing it to be 
better doesn't get anything checked in.

What will?

dw

Re: Wonly-top-basic-asm

[Bernd Schmidt]

On 02/21/2016 11:27 AM, David Wohlferd wrote:
> So now what?  I have one Bernd who likes the sample, and one who
> doesn't.  Obviously I think what I'm proposing is better than what's
> there now and I've done my best to say why.  But me believing it to be
> better doesn't get anything checked in.

I hadn't thought it through well enough. Jan's objection (order isn't 
guaranteed) is relevant. I'd drop the example.


Bernd

Re: Wonly-top-basic-asm

[David Wohlferd]

On 2/26/2016 7:09 AM, Bernd Schmidt wrote:
> On 02/21/2016 11:27 AM, David Wohlferd wrote:
>> So now what?  I have one Bernd who likes the sample, and one who
>> doesn't.  Obviously I think what I'm proposing is better than what's
>> there now and I've done my best to say why.  But me believing it to be
>> better doesn't get anything checked in.
>
> I hadn't thought it through well enough. Jan's objection (order isn't 
> guaranteed) is relevant. I'd drop the example.

To be clear: Are you suggesting that we delete the sample that is there 
and have no example at all for basic asm?

I'm not sure I agree.  Looking at the linux kernel source, there are 
times and places where basic asm is appropriate, even necessary.  I 
realize that macros are an uncommon usage.  But it makes for a more 
interesting sample than simply outputting a section name.

If ordering is your concern, would adding a reference to 
-fno-toplevel-reorder make you feel better about this?  It seems 
unnecessary in this particular context, but mentioning this option on 
the basic asm page is certainly appropriate.

dw

Re: Wonly-top-basic-asm

[David Wohlferd]

So, we have been discussing this issue for 4 months now.  Over that 
time, I have tried to incorporate everyone's feedback.

As a result we have gone from a tiny doc patch (just describe the 
current semantics), to a big doc patch (completely deprecate basic asm 
when used in a function) to a medium doc patch + code fix (warning when 
using basic asm in a function) and now back to a 
slightly-bigger-than-tiny doc patch.

I have made no changes since the last patch I posted 
(https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01406.html) for the 
reasons discussed below.

I assert that this patch both contains important information users need 
and is better than the current text.  I expect that Sandra is prepared 
to check this in as soon as someone signs off on its technical accuracy.

dw

On 2/28/2016 11:02 PM, David Wohlferd wrote:
> On 2/26/2016 7:09 AM, Bernd Schmidt wrote:
>> On 02/21/2016 11:27 AM, David Wohlferd wrote:
>>> So now what?  I have one Bernd who likes the sample, and one who
>>> doesn't.  Obviously I think what I'm proposing is better than what's
>>> there now and I've done my best to say why.  But me believing it to be
>>> better doesn't get anything checked in.
>>
>> I hadn't thought it through well enough. Jan's objection (order isn't 
>> guaranteed) is relevant. I'd drop the example.
>
> To be clear: Are you suggesting that we delete the sample that is 
> there and have no example at all for basic asm?
>
> I'm not sure I agree.  Looking at the linux kernel source, there are 
> times and places where basic asm is appropriate, even necessary.  I 
> realize that macros are an uncommon usage.  But it makes for a more 
> interesting sample than simply outputting a section name.
>
> If ordering is your concern, would adding a reference to 
> -fno-toplevel-reorder make you feel better about this?  It seems 
> unnecessary in this particular context, but mentioning this option on 
> the basic asm page is certainly appropriate.
>
> dw
>

Re: Wonly-top-basic-asm

[Bernd Schmidt]

On 03/11/2016 01:55 AM, David Wohlferd wrote:
> So, we have been discussing this issue for 4 months now.  Over that
> time, I have tried to incorporate everyone's feedback.
>
> As a result we have gone from a tiny doc patch (just describe the
> current semantics), to a big doc patch (completely deprecate basic asm
> when used in a function) to a medium doc patch + code fix (warning when
> using basic asm in a function) and now back to a
> slightly-bigger-than-tiny doc patch.
>
> I have made no changes since the last patch I posted
> (https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01406.html) for the
> reasons discussed below.
>
> I assert that this patch both contains important information users need
> and is better than the current text.  I expect that Sandra is prepared
> to check this in as soon as someone signs off on its technical accuracy.

The example is not good, as discussed previously, and IMO the best 
option is to remove it. Otherwise I have no objections to the latest 
variant.


Bernd

Re: Wonly-top-basic-asm

[David Wohlferd]

[-- Attachment #1: Type: text/plain, Size: 642 bytes --]

On 3/14/2016 8:28 AM, Bernd Schmidt wrote:
> The example is not good, as discussed previously, and IMO the best 
> option is to remove it. Otherwise I have no objections to the latest 
> variant.

Despite the problems I have with the existing sample, adding the 
information/warnings is more important to me, and more valuable to 
users.  Perhaps we can revisit the sample when pr24414 gets addressed.

I have removed my changes to the sample in the attached patch.

ChangeLog:

2016-03-16  David Wohlferd  <dw@LimeGreenSocks.com>
         Bernd Schmidt  <bschmidt@redhat.com>

     * doc/extend.texi: Doc basic asm behavior re clobbers.

dw

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 24414V.patch --]
[-- Type: text/x-patch; name="24414V.patch", Size: 2103 bytes --]

Index: extend.texi
===================================================================
--- extend.texi	(revision 234245)
+++ extend.texi	(working copy)
@@ -7441,7 +7441,8 @@
 @end table
 
 @subsubheading Remarks
-Using extended @code{asm} typically produces smaller, safer, and more
+Using extended @code{asm} (@pxref{Extended Asm}) typically produces smaller,
+safer, and more
 efficient code, and in most cases it is a better solution than basic
 @code{asm}.  However, there are two situations where only basic @code{asm}
 can be used:
@@ -7481,10 +7482,25 @@
 symbol errors during compilation if your assembly code defines symbols or 
 labels.
 
-Since GCC does not parse the @var{AssemblerInstructions}, it has no 
-visibility of any symbols it references. This may result in GCC discarding 
-those symbols as unreferenced.
+@strong{Warning:} The C standards do not specify semantics for @code{asm},
+making it a potential source of incompatibilities between compilers.  These
+incompatibilities may not produce compiler warnings/errors.
 
+GCC does not parse basic @code{asm}'s @var{AssemblerInstructions}, which
+means there is no way to communicate to the compiler what is happening
+inside them.  GCC has no visibility of symbols in the @code{asm} and may
+discard them as unreferenced.  It also does not know about side effects of
+the assembler code, such as modifications to memory or registers.  Unlike
+some compilers, GCC assumes that no changes to either memory or registers
+occur.  This assumption may change in a future release.
+
+To avoid complications from future changes to the semantics and the
+compatibility issues between compilers, consider replacing basic @code{asm}
+with extended @code{asm}.  See
+@uref{https://gcc.gnu.org/wiki/ConvertBasicAsmToExtended, How to convert
+from basic asm to extended asm} for information about how to perform this
+conversion.
+
 The compiler copies the assembler instructions in a basic @code{asm} 
 verbatim to the assembly language output file, without 
 processing dialects or any of the @samp{%} operators that are available with

Re: Wonly-top-basic-asm

[Bernd Schmidt]

On 03/17/2016 06:23 AM, David Wohlferd wrote:
> 2016-03-16  David Wohlferd  <dw@LimeGreenSocks.com>
>          Bernd Schmidt  <bschmidt@redhat.com>
>
>      * doc/extend.texi: Doc basic asm behavior re clobbers.
>

Any objections from the release managers if I install this for David at 
this stage?


Bernd

Re: Wonly-top-basic-asm

[Richard Biener]

On Fri, Mar 18, 2016 at 1:46 PM, Bernd Schmidt <bschmidt@redhat.com> wrote:
> On 03/17/2016 06:23 AM, David Wohlferd wrote:
>>
>> 2016-03-16  David Wohlferd  <dw@LimeGreenSocks.com>
>>          Bernd Schmidt  <bschmidt@redhat.com>
>>
>>      * doc/extend.texi: Doc basic asm behavior re clobbers.
>>
>
> Any objections from the release managers if I install this for David at this
> stage?

No.

Richard.

>
> Bernd
>

Re: Wonly-top-basic-asm

[Bernd Schmidt]

On 03/17/2016 06:23 AM, David Wohlferd wrote:
> On 3/14/2016 8:28 AM, Bernd Schmidt wrote:
>> The example is not good, as discussed previously, and IMO the best
>> option is to remove it. Otherwise I have no objections to the latest
>> variant.
>
> Despite the problems I have with the existing sample, adding the
> information/warnings is more important to me, and more valuable to
> users.  Perhaps we can revisit the sample when pr24414 gets addressed.

My thought was to remove the example altogether, which we might want to 
do later on. But for now I've committed your change since it can be seen 
as an improvement in itself.


Bernd