This is the mail archive of the gcc@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: basic asm and memory clobbers - Proposed solution


On 11/24/2015 6:50 PM, David Wohlferd wrote:
I have solved the problem with my previous patch. Here's the update (feedback welcome): http://www.LimeGreenSocks.com/gcc/24414g.zip

Based on my understanding from the previous thread, this patch now does what it needs to do (code-wise) to resolve this "basic asm and memory clobbers" issue. As mentioned previously, this patch introduces a new warning (-Wonly-top-basic-asm), which is disabled by default. When enabled, it triggers a warning for any basic asm inside a function, unless the function has the "naked" attribute.

An argument can be made that the default for this warning should be 'enabled.' Yes, this will break builds that use basic asm and -Werror, but it can easily be disabled with -Wno-only-top-basic-asm. And if we don't enable it, no one is going to know the check is even available. Then hidden problems like the one Paul was just describing still won't be found, and optimizations will continue to have unexpected side effects. OTOH, I can also see changing this to 'enabled' as more appropriate in the next phase 1.

Now that I'm done with the code fix, I'm working on an update to the docs. Obviously they should be checked in as part of the code fix. I'm planning to actually use the word "deprecated" when describing the use of basic asm within functions. Seems like a big step.

But there's no point in my proceeding any further until someone in authority agrees that this is the desired solution. I'm not actually sure who that is, but further work is a waste of time if no one is prepared to approve it.

If you are that person, the questions to be answered are:

1) Is the idea of changing basic asm to "clobber everything" dead?
2) Is creating a warning for the use of "basic asm inside a function" the solution for this issue?
3) Should the warning be enabled by default in v6?
4) Should the warning be enabled by Wall or Wextra?
5) Should the v6 docs explicitly describe using "basic asm inside a function" as deprecated?

If you're looking for my recommendations, I would say: Yes, Yes, (reluctantly) No, No and Yes.

With this information in hand, I'll take a shot at finishing this off.

For something that started out as a simple 3 sentence doc patch, this sure has turned into a project...

I have incorporated the feedback from Hans-Peter Nilsson, who pointed out that the 'noinline' function attribute explicitly documents behavior related to using asm(""). The patch now includes an exception for this. Given how often this bit of code is used in various projects, allowing this exception will surely ease the transition.

I'm told that some people won't review patches unless they are included as attachments, so this time the patch is attached.

The patch includes first cut docs for the warning message, but I'm still hoping to hear from someone before trying to update the basic asm docs.

dw
Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 230734)
+++ gcc/c-family/c.opt	(working copy)
@@ -585,6 +585,10 @@
 C++ ObjC++ Var(warn_namespaces) Warning
 Warn on namespace definition.
 
+Wonly-top-basic-asm
+C C++ Var(warn_only_top_basic_asm) Warning
+Warn on unsafe uses of basic asm.
+
 Wsized-deallocation
 C++ ObjC++ Var(warn_sized_deallocation) Warning EnabledBy(Wextra)
 Warn about missing sized deallocation functions.
Index: gcc/c/c-parser.c
===================================================================
--- gcc/c/c-parser.c	(revision 230734)
+++ gcc/c/c-parser.c	(working copy)
@@ -5880,7 +5880,18 @@
   labels = NULL_TREE;
 
   if (c_parser_next_token_is (parser, CPP_CLOSE_PAREN) && !is_goto)
+  {
+    /* Warn on basic asm used inside of functions, 
+       EXCEPT when in naked functions. Also allow asm(""). */
+    if (warn_only_top_basic_asm && (TREE_STRING_LENGTH (str) != 1) )
+      if (lookup_attribute ("naked", 
+                            DECL_ATTRIBUTES (current_function_decl)) 
+                            == NULL_TREE)
+        warning_at(asm_loc, OPT_Wonly_top_basic_asm, 
+                   "asm statement in function does not use extended syntax");
+
     goto done_asm;
+  }
 
   /* Parse each colon-delimited section of operands.  */
   nsections = 3 + is_goto;
Index: gcc/cp/parser.c
===================================================================
--- gcc/cp/parser.c	(revision 230734)
+++ gcc/cp/parser.c	(working copy)
@@ -17594,6 +17594,8 @@
   bool goto_p = false;
   required_token missing = RT_NONE;
 
+  location_t asm_loc = cp_lexer_peek_token (parser->lexer)->location;
+
   /* Look for the `asm' keyword.  */
   cp_parser_require_keyword (parser, RID_ASM, RT_ASM);
 
@@ -17752,6 +17754,16 @@
 	  /* If the extended syntax was not used, mark the ASM_EXPR.  */
 	  if (!extended_p)
 	    {
+	      /* Warn on basic asm used inside of functions,
+	         EXCEPT when in naked functions. Also allow asm(""). */
+	      if (warn_only_top_basic_asm && 
+	          (TREE_STRING_LENGTH (string) != 1))
+	        if (lookup_attribute("naked",
+	                             DECL_ATTRIBUTES (current_function_decl))
+	                               == NULL_TREE)
+	          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 230734)
+++ gcc/doc/extend.texi	(working copy)
@@ -2899,12 +2899,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
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 230734)
+++ gcc/doc/invoke.texi	(working copy)
@@ -5687,6 +5687,14 @@
 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 (ie not
+at file scope/top level).  Due to the potential for unsafe optimizations,
+always use extended instead of basic @code{asm} inside functions.  Functions
+that are marked with the @option{naked} attribute are excluded from this
+check. @code{asm} statements with an empty instruction string are also
+excluded.
+
 @end table
 
 @node Debugging Options

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]