[cfe-commits] [Patch] CodeWarrior/MS-style inline assembly asm function

[Richard Smith]

Hi Chad,

On Mon, Jan 7, 2013 at 12:52 PM, Chad Rosier <mcrosier at apple.com> wrote:
> All,
> The attached patch add support for CodeWarrior style asm functions.
>
> static asm void t1() {
>   mov eax, 0
> }
>
> This is a WIP.  The current implementation only works when there is an additional function specifier (e.g., static, const) before the asm keyword.  The parser needs to be enhanced to differentiate between module-level assembly and asm functions.
>
> module asm "inline asm here"
>
> vs
>
> __asm void t2() {
>   mov eax, 0
> }

Can you provide a link to some documentation for this feature? I have
lots of questions about how it is supposed to work... For instance,
can you declare variables in such a function? Can you apply the 'asm'
keyword to a declaration that is not the definition? (And if so, is
the definition also implicitly 'asm', even if it omits the keyword?)
Can an 'asm' function have a function-try-block? If it's a
constructor, can it have member initializers? Can it be defaulted or
deleted?


There's lots of this sort of thing in your patch:

-                               bool isStatic,
-                               StorageClass SCAsWritten,
-                               bool isInline,
-                               bool isConstexpr,
+                               bool isStatic, StorageClass SCAsWritten,
+                               bool isAsm, bool isInline, bool isConstexpr,

Please convert the is* flags on FunctionDecl and friends into an enum
first, as a separate commit. Sorry, this really isn't your problem,
but this is the proverbial straw that broke the camel's back... This
would also remove most of the noise from the patch.


--- include/clang/AST/Decl.h	(revision 171536)
+++ include/clang/AST/Decl.h	(working copy)
@@ -1470,6 +1470,7 @@
   // NOTE: VC++ treats enums as signed, avoid using the StorageClass enum
   unsigned SClass : 2;
   unsigned SClassAsWritten : 2;
+  bool IsAsmSpecified : 1;

You'll need to add serialization code for this flag.

+  /// Set whether the "asm" keyword was specified for this function.
+  void setAsmSpecified(bool I) { IsAsmSpecified = I; }

This function is unused, and seems unnecessary.


--- lib/CodeGen/CodeGenFunction.cpp	(revision 171536)
+++ lib/CodeGen/CodeGenFunction.cpp	(working copy)
@@ -347,9 +347,21 @@
   CurFnInfo = &FnInfo;
   assert(CurFn->isDeclaration() && "Function already has body?");

+  // Mark the function as naked and noinline if this is an asm function.
+  bool IsAsmFunc = false;
+  if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(D))
+    for (FunctionDecl::redecl_iterator RI = FD->redecls_begin(),
+           RE = FD->redecls_end(); RI != RE; ++RI)
+      if (RI->isAsmSpecified()) {
+        IsAsmFunc = true;
+        Fn->addFnAttr(llvm::Attribute::Naked);
+        Fn->addFnAttr(llvm::Attribute::NoInline);
+        break;
+      }

Should this really be looking for any declaration which is 'asm', or
just looking for the definition? Why does 'asm' imply 'noinline'?


--- lib/Parse/ParseDecl.cpp	(revision 171536)
+++ lib/Parse/ParseDecl.cpp	(working copy)
@@ -2538,6 +2540,11 @@
       break;
     }

+    // Microsoft asm support.
+    case tok::kw_asm:
+      isInvalid = DS.setFunctionSpecAsm(Loc);
+      break;

Emit a diagnostic here if the extension is not enabled.


--- lib/Parse/ParseStmt.cpp	(revision 171536)
+++ lib/Parse/ParseStmt.cpp	(working copy)
-  StmtResult FnBody(ParseCompoundStatementBody());
+  StmtResult FnBody(ParseCompoundStatementBody(/*isStmtExpr*/false,
+                                               ParsingAsmFunction));

Instead of wiring a flag through ParseCompoundStatementBody and
friends, please implement separate parsing code here. Reusing
ParseCompoundStatementBody does not seem worthwhile, since the only
shared syntax is apparently the braces.