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

[David Wohlferd <dw at LimeGreenSocks dot com>]

> 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

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