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

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

4310

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

include/clang/Basic/DiagnosticSemaKinds.td

def warn_cast_qual : Warning<"cast from %0 to %1 drops %select{const and "
   "volatile qualifiers|const qualifier|volatile qualifier}2">,
   InGroup<CastQual>, DefaultIgnore;
 def warn_cast_qual2 : Warning<"cast from %0 to %1 must have all intermediate "
   "pointers const qualified to be safe">, InGroup<CastQual>, DefaultIgnore;
 } // End of general sema category.
 
 // inline asm.
 let CategoryName = "Inline Assembly Issue" in {
+  def err_asm_disabled : Error<
+    "inline asm is not allowed with -fno-inline-asm">;
   def err_asm_invalid_lvalue_in_output : Error<"invalid lvalue in asm output">;
   def err_asm_invalid_output_constraint : Error<
     "invalid output constraint '%0' in asm">;
   def err_asm_invalid_lvalue_in_input : Error<
     "invalid lvalue in asm input for constraint '%0'">;
   def err_asm_invalid_input_constraint : Error<
     "invalid input constraint '%0' in asm">;
   def err_asm_invalid_type_in_input : Error<

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(NoInlineAsm       , 1, 0, "disable 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")

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 finline_asm : Flag<["-"], "finline-asm">, Group<f_Group>, Flags<[DriverOption]>;
+def fno_inline_asm : Flag<["-"], "fno-inline-asm">, Group<f_Group>, Flags<[CC1Option]>,
+  HelpText<"Disable inline assembly">;
+
 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">;

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");
 
+  // -fno-inline-asm
+  if (Args.hasFlag(options::OPT_fno_inline_asm, options::OPT_finline_asm,
+                   false))
+    CmdArgs.push_back("-fno-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))

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.NoInlineAsm = Args.hasArg(OPT_fno_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)) {

lib/Sema/SemaStmtAsm.cpp

 }
 
 StmtResult Sema::ActOnGCCAsmStmt(SourceLocation AsmLoc, bool IsSimple,
                                  bool IsVolatile, unsigned NumOutputs,
                                  unsigned NumInputs, IdentifierInfo **Names,
                                  MultiExprArg constraints, MultiExprArg Exprs,
                                  Expr *asmString, MultiExprArg clobbers,
                                  SourceLocation RParenLoc) {
+  // Check if inline asm is not allowed
+  if (getLangOpts().NoInlineAsm) {
+    return StmtError(Diag(AsmLoc,diag::err_asm_disabled));
+  }
   unsigned NumClobbers = clobbers.size();
   StringLiteral **Constraints =
     reinterpret_cast<StringLiteral**>(constraints.data());
   StringLiteral *AsmString = cast<StringLiteral>(asmString);
   StringLiteral **Clobbers = reinterpret_cast<StringLiteral**>(clobbers.data());
 
   SmallVector<TargetInfo::ConstraintInfo, 4> OutputConstraintInfos;
 

test/Sema/no-inline-asm.c

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