Adding option -fno-inline-asm to disallow inline asm

[steven_wu]

Summary

This patch add a new option to dis-allow all inline asm.
Any GCC style inline asm will be reported as an error.

[rsmith]

The flag name doesn't match its behavior here: it disables GNU C inline asm only, not CodeView/MSVC-style inline asm. What's the intent here? Why should we allow this particular GNU extension to be disabled? Would this make more sense as part of a (much-discussed but never implemented) -fno-gnu-extensions flag?

[steven_wu]

The intention of this patch is to provide a way to control the usage of inline assembly since they are harder to get correct. The reason why it is not issuing an error on MSVC style assembly is because we already have a way to control its usage (-fno-asm-block).
I can imply -fno-asm-block with -fno-inline-asm but that will require some refactoring of my patch since these two flags triggers different error with this patch. -fno-inline-asm is currently a sema check and -fno-asm-block will disable a path in the parser.
It is a good point that we can use this patch to get a one-step closer to -fno-gnu-extensions. In that case, I think it should be better to make a new flag (-fno-gnu-asm) which disable gnu-style asm parser, and -fno-inline-asm should implies both -fno-asm-block and -fno-gnu-asm.
I will try to come up with a new patch tomorrow.

[echristo]

Yes please, the confusing option wasn't really good.

[steven_wu]

• Another attempt to implement -fno-inline-asm according to the feedback. Now implemented a new flag -fno-gnu-inline-asm which return error when parsing GNU-style inline assembly. -fno-inline-asm will just implies both -fno-gnu-inline-asm and -fno-asm-blocks

[steven_wu]

Hi Richard and Eric

Let me know what you think about this approach! Thanks for the feedback!

Steven

[rnk]

lgtm

Unlike -fno-ms-extensions, this doesn't make us blind to __asm__ like we are for __fastcall & co. That seems OK.

Maybe this should this also affect asm register variables like this?

int foo1 asm ("bar1");

include/clang/Basic/LangOptions.def

117 ↗

(On Diff #18276)

Let's invert this to GNUAsm and make it on by default.

[steven_wu]

• Update patch that AsmLabels are now disabled with -fno-gnu-inline-asm. This also means functions with asm-labels will also be reported as an error with -fno-gnu-inline-asm.

[steven_wu]

• Fix the code format and fix the comments. Sorry for the spam

[rnk]

In D6870#109616, @steven_wu wrote:

• Update patch that AsmLabels are now disabled with -fno-gnu-inline-asm. This also means functions with asm-labels will also be reported as an error with -fno-gnu-inline-asm.

I wasn't suggesting that we reject function asm labels, I was suggesting that we reject variables marked as living in specific registers. Consider something like:

register void *sp asm("esp");
int main () {
  printf("sp: %p\n", sp);
}

This code compiles down to read the esp register in main.

I think using asm labels to rename something is not inherently architecture specific and could be accepted even in the presence of -fno-inline-asm. What do you think?

[bob.wilson]

In D6870#109832, @rnk wrote:

In D6870#109616, @steven_wu wrote:

• Update patch that AsmLabels are now disabled with -fno-gnu-inline-asm. This also means functions with asm-labels will also be reported as an error with -fno-gnu-inline-asm.

I wasn't suggesting that we reject function asm labels, I was suggesting that we reject variables marked as living in specific registers. Consider something like:

register void *sp asm("esp");
int main () {
  printf("sp: %p\n", sp);
}

This code compiles down to read the esp register in main.

I think using asm labels to rename something is not inherently architecture specific and could be accepted even in the presence of -fno-inline-asm. What do you think?

I agree that function asm labels are a separate thing and should not be affected by -fno-gnu-inline-asm. It would still be good to disallow register asm with that flag, though.

I like the suggestion to call this option "-fno-gnu-inline-asm", but if we do that, I don't see a need to keep "-fno-inline-asm". If someone really wants to prevent both GNU and Microsoft-style inline asm, they can just specify -fno-gnu-inline-asm and -fno-asm-blocks.

[steven_wu]

• Remove -fno-inline-asm option and do not disallow asm label
  
  I agree that it is not a good idea disallow all asm labels with the new option. It is probably a Sema check should be implemented later to distinguish different asm labels and the type of occurrence (like in a system header).
  
  I don't like the part of code using -fno-inline-asm to imply the other two very much as well. I have no problem removing that flag.

[echristo]

Couple small inline nits, otherwise LGTM.

-eric

lib/Driver/Tools.cpp

4323 ↗

(On Diff #18318)

Period at the end of the sentence.

lib/Parse/ParseStmtAsm.cpp

619 ↗

(On Diff #18318)

Periods at the end of sentences.

test/Parser/ms-inline-asm.c

3 ↗

(On Diff #18318)

"Disabling gnu inline assembly should have no effect on this testcase."

[steven_wu]

Thanks. committed in r226340.

Steven

cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td

 
 let CategoryName = "Inline Assembly Issue" in {
 def err_asm_empty : Error<"__asm used with no assembly instructions">;
 def err_inline_ms_asm_parsing : Error<"%0">;
 def err_msasm_unsupported_arch : Error<
   "Unsupported architecture '%0' for MS-style inline assembly">;
 def err_msasm_unable_to_create_target : Error<
   "MS-style inline assembly is not available: %0">;
+def err_gnu_inline_asm_disabled : Error<
+  "GNU-style inline assembly is disabled">;
 }
 
 let CategoryName = "Parse Issue" in {
 
 def ext_empty_translation_unit : Extension<
   "ISO C requires a translation unit to contain at least one declaration">,
   InGroup<DiagGroup<"empty-translation-unit">>;
 def warn_cxx98_compat_top_level_semi : Warning<

cfe/trunk/include/clang/Basic/LangOptions.def

 LANGOPT(SjLjExceptions    , 1, 0, "setjmp-longjump exception handling")
 LANGOPT(TraditionalCPP    , 1, 0, "traditional CPP emulation")
 LANGOPT(RTTI              , 1, 1, "run-time type information")
 LANGOPT(RTTIData          , 1, 1, "emit run-time type information data")
 LANGOPT(MSBitfields       , 1, 0, "Microsoft-compatible structure layout")
 LANGOPT(Freestanding, 1, 0, "freestanding implementation")
 LANGOPT(NoBuiltin         , 1, 0, "disable builtin functions")
 LANGOPT(NoMathBuiltin     , 1, 0, "disable math builtin functions")
+LANGOPT(GNUAsm            , 1, 1, "GNU-style inline assembly")
 
 BENIGN_LANGOPT(ThreadsafeStatics , 1, 1, "thread-safe static initializers")
 LANGOPT(POSIXThreads      , 1, 0, "POSIX thread support")
 LANGOPT(Blocks            , 1, 0, "blocks extension to C")
 BENIGN_LANGOPT(EmitAllDecls      , 1, 0, "support for emitting all declarations")
 LANGOPT(MathErrno         , 1, 1, "errno support for math functions")
 BENIGN_LANGOPT(HeinousExtensions , 1, 0, "Extensions that we really don't like and may be ripped out at any time")
 LANGOPT(Modules           , 1, 0, "modules extension to C")

cfe/trunk/include/clang/Driver/Options.td

 def fast : Flag<["-"], "fast">, Group<f_Group>;
 def fasynchronous_unwind_tables : Flag<["-"], "fasynchronous-unwind-tables">, Group<f_Group>;
 
 def fautolink : Flag <["-"], "fautolink">, Group<f_Group>;
 def fno_autolink : Flag <["-"], "fno-autolink">, Group<f_Group>,
   Flags<[DriverOption, CC1Option]>,
   HelpText<"Disable generation of linker directives for automatic library linking">;
 
+def fgnu_inline_asm : Flag<["-"], "fgnu-inline-asm">, Group<f_Group>, Flags<[DriverOption]>;
+def fno_gnu_inline_asm : Flag<["-"], "fno-gnu-inline-asm">, Group<f_Group>,
+  Flags<[DriverOption, CC1Option]>,
+  HelpText<"Disable GNU style inline asm">;
+
 def fprofile_sample_use_EQ : Joined<["-"], "fprofile-sample-use=">,
     Group<f_Group>, Flags<[DriverOption, CC1Option]>,
     HelpText<"Enable sample-based profile guided optimizations">;
 def fauto_profile_EQ : Joined<["-"], "fauto-profile=">,
     Alias<fprofile_sample_use_EQ>;
 def fprofile_instr_generate : Flag<["-"], "fprofile-instr-generate">,
     Group<f_Group>, Flags<[CC1Option]>,
     HelpText<"Generate instrumented code to collect execution counts">;

cfe/trunk/lib/Driver/Tools.cpp

if (!Args.hasFlag(options::OPT_fspell_checking,
     CmdArgs.push_back("-fno-spell-checking");
 
 
   // -fno-asm-blocks is default.
   if (Args.hasFlag(options::OPT_fasm_blocks, options::OPT_fno_asm_blocks,
                    false))
     CmdArgs.push_back("-fasm-blocks");
 
+  // -fgnu-inline-asm is default.
+  if (!Args.hasFlag(options::OPT_fgnu_inline_asm,
+                    options::OPT_fno_gnu_inline_asm, true))
+    CmdArgs.push_back("-fno-gnu-inline-asm");
+
   // Enable vectorization per default according to the optimization level
   // selected. For optimization levels that want vectorization we use the alias
   // option to simplify the hasFlag logic.
   bool EnableVec = shouldEnableVectorizerAtOLevel(Args, false);
   OptSpecifier VectorizeAliasOption = EnableVec ? options::OPT_O_Group :
     options::OPT_fvectorize;
   if (Args.hasFlag(options::OPT_fvectorize, VectorizeAliasOption,
                    options::OPT_fno_vectorize, EnableVec))

cfe/trunk/lib/Frontend/CompilerInvocation.cpp

#include "clang/Frontend/LangStandards.def"
   Opts.DebuggerCastResultToId = Args.hasArg(OPT_fdebugger_cast_result_to_id);
   Opts.DebuggerObjCLiteral = Args.hasArg(OPT_fdebugger_objc_literal);
   Opts.ApplePragmaPack = Args.hasArg(OPT_fapple_pragma_pack);
   Opts.CurrentModule = Args.getLastArgValue(OPT_fmodule_name);
   Opts.ImplementationOfModule =
       Args.getLastArgValue(OPT_fmodule_implementation_of);
   Opts.NativeHalfType = Opts.NativeHalfType;
   Opts.HalfArgsAndReturns = Args.hasArg(OPT_fallow_half_arguments_and_returns);
+  Opts.GNUAsm = !Args.hasArg(OPT_fno_gnu_inline_asm);
 
   if (!Opts.CurrentModule.empty() && !Opts.ImplementationOfModule.empty() &&
       Opts.CurrentModule != Opts.ImplementationOfModule) {
     Diags.Report(diag::err_conflicting_module_names)
         << Opts.CurrentModule << Opts.ImplementationOfModule;
   }
 
   if (Arg *A = Args.getLastArg(OPT_faddress_space_map_mangling_EQ)) {

cfe/trunk/lib/Parse/ParseStmtAsm.cpp

StmtResult Parser::ParseAsmStatement(bool &msAsm) {
   assert(Tok.is(tok::kw_asm) && "Not an asm stmt");
   SourceLocation AsmLoc = ConsumeToken();
 
   if (getLangOpts().AsmBlocks && Tok.isNot(tok::l_paren) &&
       !isTypeQualifier()) {
     msAsm = true;
     return ParseMicrosoftAsmStatement(AsmLoc);
   }
+
+  // Check if GNU-style inline Asm is disabled.
+  if (!getLangOpts().GNUAsm)
+    Diag(AsmLoc, diag::err_gnu_inline_asm_disabled);
+
   DeclSpec DS(AttrFactory);
   SourceLocation Loc = Tok.getLocation();
   ParseTypeQualifierListOpt(DS, AR_VendorAttributesParsed);
 
   // GNU asms accept, but warn, about type-qualifiers other than volatile.
   if (DS.getTypeQualifiers() & DeclSpec::TQ_const)
     Diag(Loc, diag::w_asm_qualifier_ignored) << "const";
   if (DS.getTypeQualifiers() & DeclSpec::TQ_restrict)

cfe/trunk/test/Driver/inline-asm.c

+// RUN: %clang -target x86_64-apple-darwin10 \
+// RUN:   -### -fsyntax-only -fasm-blocks %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-BLOCKS < %t %s
+
+// RUN: %clang -target x86_64-apple-darwin10 \
+// RUN:   -### -fsyntax-only -fno-asm-blocks -fasm-blocks %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-BLOCKS < %t %s
+
+// CHECK-BLOCKS: "-fasm-blocks"
+
+// RUN: %clang -target x86_64-apple-darwin10 \
+// RUN:   -### -fsyntax-only -fasm-blocks -fno-asm-blocks %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-NO-BLOCKS < %t %s
+
+// CHECK-NO-BLOCKS-NOT: "-fasm-blocks"
+
+// RUN: %clang -target x86_64-apple-darwin10 \
+// RUN:   -### -fsyntax-only -fno-gnu-inline-asm %s 2>&1 | \
+// RUN: FileCheck --check-prefix=CHECK-NO-GNU-INLINE-ASM %s
+// RUN: %clang -target x86_64-apple-darwin10 \
+// RUN:   -### -fsyntax-only -fgnu-inline-asm -fno-gnu-inline-asm %s 2>&1 | \
+// RUN: FileCheck --check-prefix=CHECK-NO-GNU-INLINE-ASM %s
+
+// CHECK-NO-GNU-INLINE-ASM: "-fno-gnu-inline-asm"

cfe/trunk/test/Driver/ms-inline-asm.c

-// RUN: %clang -target x86_64-apple-darwin10 \
-// RUN:   -### -fsyntax-only -fasm-blocks %s 2> %t
-// RUN: FileCheck --check-prefix=CHECK-BLOCKS < %t %s
-
-// RUN: %clang -target x86_64-apple-darwin10 \
-// RUN:   -### -fsyntax-only -fno-asm-blocks -fasm-blocks %s 2> %t
-// RUN: FileCheck --check-prefix=CHECK-BLOCKS < %t %s
-
-// CHECK-BLOCKS: "-fasm-blocks"
-
-// RUN: %clang -target x86_64-apple-darwin10 \
-// RUN:   -### -fsyntax-only -fasm-blocks -fno-asm-blocks %s 2> %t
-// RUN: FileCheck --check-prefix=CHECK-NO-BLOCKS < %t %s
-
-// CHECK-NO-BLOCKS-NOT: "-fasm-blocks"

cfe/trunk/test/Parser/ms-inline-asm.c

 // REQUIRES: x86-registered-target
 // RUN: %clang_cc1 %s -triple i386-apple-darwin10 -verify -fasm-blocks
+// Disabling gnu inline assembly should have no effect on this testcase
+// RUN: %clang_cc1 %s -triple i386-apple-darwin10 -verify -fasm-blocks -fno-gnu-inline-asm
 
 #define M __asm int 0x2c
 #define M2 int
 
 void t1(void) { M }
 void t2(void) { __asm int 0x2c }
 void t3(void) { __asm M2 0x2c }
 void t4(void) { __asm mov eax, fs:[0x10] }

cfe/trunk/test/Parser/no-gnu-inline-asm.c

+// RUN: %clang_cc1 %s -triple i686-apple-darwin -verify -fsyntax-only -fno-gnu-inline-asm
+
+void f (void) {
+  long long foo = 0, bar;
+  asm volatile("INST %0, %1" : "=r"(foo) : "r"(bar)); // expected-error {{GNU-style inline assembly is disabled}}
+  return;
+}