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

[Chad Rosier]

Hi Richard,

On Jan 7, 2013, at 3:02 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> 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?

The documentation I've been looking at can be located here:
http://faculty.petra.ac.id/indi/files/

Then click the "C++ and Assembly Language Reference.pdf" link.

Unfortunately, I don't think there's a great deal of documentation readily available.  For the most part I was planning on implementing what llvm-gcc supported.  I've also contacted Microsoft to see if they have any documentation; I didn't see any online.

I'll get back to you once I have more information.  Thanks again for the feedback, Richard.

 Chad

> 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.

Will do.

> 
> --- 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.
> 

I'll remove it.

> 
> --- 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.