Re: Wonly-top-basic-asm

[David Wohlferd <dw@LimeGreenSocks.com>]

[-- 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;
+}