[MSVC] Improved __noop support (https://llvm.org/bugs/show_bug.cgi?id=14081)

[ili.filippov]

Summary

noop is declared with manually checked arguments ("t") that prevents any checking of argument completeness. noop can be called with void arguments now.
It is declared that noop has constant zero return value. So noop can be assigned as a pointer now.
Special flag is added which indicates that parsing is inside noop expression. Functions aren't marked as referenced if they are noop parameters. This prevents from template instantiation of function that is used only in __noop expression.

This behavior is the same as MS compiler behavior.

[rnk]

I'm concerned that there are other ways for code inside __noop() to trigger template instantiation, which will cause us to do semantic analysis in a weird state.

lib/Sema/SemaExpr.cpp

13244

s/match/mark/

13248

Why do we need to mark non-FunctionDecls referenced? Surely they can trigger template instantiation of other function decls.

13248

Can this just be if (InsideNoop) return; at the beginning of the function?

test/CodeGen/builtin-ms-noop.cpp

37

This test will not pass in NDEBUG mode because the IR names are skipped. You can either pattern match the alloca, or you can store to a global.

[• chatur01]

test/Sema/builtin-ms-noop-errors.cpp

17

I find checking this note weird. I know __noop is defined as taking ... from the Builtins.def file, but ASTContext::GetBuiltinType decides to ignore it and generate the empty argument list instead.

[rsmith]

lib/Parse/ParseExpr.cpp

1473–1478

Code in Parse shouldn't be inspecting the AST like this. If you really need different parse rules for the argument of __noop, maybe make it a keyword rather than a builtin?

1480–1481

Can you enter an unevaluated context here rather than inventing a new sort of context for this case? Global state in Sema doesn't seem like the right way to handle this -- if you trigger an instantiation from within this context, you should not instantiate the template in noop mode (it might end up being used outside noop mode too).

[ili.filippov]

Minor fixes of test and MarkDeclRefReferenced function.

[ili.filippov]

This is a kind reminder to review the patch.

lib/Parse/ParseExpr.cpp

1473–1478

Firstly I tried to make noop as a separate keyword however noop has a builtin behavior in 95% cases. For example noop will be a normal variable after "int noop;" declaration. I suppose that it will be inefficient to detect all these cases for a new key-word.

1480–1481

Entering unevaluated context doesn’t prevent from template instantiation here. Global state points that we are inside __noop parsing now.

lib/Sema/SemaExpr.cpp

13248

Non-FunctionDecls are marked as referenced because this prevents warnings about unused variables

test/Sema/builtin-ms-noop-errors.cpp

17

This patch doesn’t influence on this. I simply added this case (error + note) in a test. Deleted them.

[ili.filippov]

Ping

[ili.filippov]

Ping...

[rnk]

I don't think you've addressed Richard's concerns convincingly. He feels that having more Sema-global contexts is the wrong way to go, and if this is the cost of more accurate __noop support, then maybe it's not worth it.

include/clang/Basic/Builtins.def

 LANGBUILTIN(__GetExceptionInfo, "v*.", "ntu", ALL_MS_LANGUAGES)
 LANGBUILTIN(_InterlockedCompareExchange, "LiLiD*LiLi", "n", ALL_MS_LANGUAGES)
 LANGBUILTIN(_InterlockedCompareExchangePointer, "v*v*D*v*v*", "n", ALL_MS_LANGUAGES)
 LANGBUILTIN(_InterlockedDecrement,       "LiLiD*",     "n", ALL_MS_LANGUAGES)
 LANGBUILTIN(_InterlockedExchangeAdd,     "LiLiD*Li",   "n", ALL_MS_LANGUAGES)
 LANGBUILTIN(_InterlockedExchange,        "LiLiD*Li",   "n", ALL_MS_LANGUAGES)
 LANGBUILTIN(_InterlockedExchangePointer, "v*v*D*v*",   "n", ALL_MS_LANGUAGES)
 LANGBUILTIN(_InterlockedIncrement,       "LiLiD*",     "n", ALL_MS_LANGUAGES)
-LANGBUILTIN(__noop,           "i.",  "n", ALL_MS_LANGUAGES)
+LANGBUILTIN(__noop,           "i.",  "tn", ALL_MS_LANGUAGES)
 LANGBUILTIN(__readfsdword,    "ULiULi", "n", ALL_MS_LANGUAGES)
 LANGBUILTIN(__va_start,       "vc**.", "nt", ALL_MS_LANGUAGES)
 
 // Microsoft library builtins.
 LIBBUILTIN(_setjmpex, "iJ", "fj",   "setjmpex.h", ALL_MS_LANGUAGES)
 
 // C99 library functions
 // C99 stdlib.h

include/clang/Sema/Sema.h

   /// error or warning was produced.
   ///
   /// This value is used to suppress printing of redundant template
   /// instantiation backtraces when there are multiple errors in the
   /// same instantiation. FIXME: Does this belong in Sema? It's tough
   /// to implement it anywhere else.
   ActiveTemplateInstantiation LastTemplateInstantiationErrorContext;
 
+  /// \brief This indicator indicates if current parsing is doing
+  /// inside arguments of __noop builtin.
+  bool InsideNoop;
+
+  /// \brief RAII object used to change InsideNoop indicator.
+  class InsideNoopRAII {
+    Sema &Self;
+    bool OldInsideNoop;
+
+  public:
+    InsideNoopRAII(Sema &Self, bool NewInsideNoop)
+        : Self(Self), OldInsideNoop(Self.InsideNoop) {
+      Self.InsideNoop = NewInsideNoop || Self.InsideNoop;
+    }
+
+    ~InsideNoopRAII() { Self.InsideNoop = OldInsideNoop; }
+  };
+
   /// \brief The current index into pack expansion arguments that will be
   /// used for substitution of parameter packs.
   ///
   /// The pack expansion index will be -1 to indicate that parameter packs
   /// should be instantiated as themselves. Otherwise, the index specifies
   /// which argument within the parameter pack will be used for substitution.
   int ArgumentPackSubstitutionIndex;
 

lib/AST/ExprConstant.cpp

bool IntExprEvaluator::TryEvaluateBuiltinObjectSize(const CallExpr *E) {
   return Success(Size, E);
 }
 
 bool IntExprEvaluator::VisitCallExpr(const CallExpr *E) {
   switch (unsigned BuiltinOp = E->getBuiltinCallee()) {
   default:
     return ExprEvaluatorBaseTy::VisitCallExpr(E);
 
+  case Builtin::BI__noop:
+    return Success(0, E);
+
   case Builtin::BI__builtin_object_size: {
     if (TryEvaluateBuiltinObjectSize(E))
       return true;
 
     // If evaluating the argument has side-effects, we can't determine the size
     // of the object, and so we lower it to unknown now. CodeGen relies on us to
     // handle all cases where the expression has side-effects.
     if (E->getArg(0)->HasSideEffects(Info.Ctx)) {

lib/Parse/ParseExpr.cpp

case tok::lesslessless: {  // p-e: p-e '<<<' argument-expression-list '>>>'
       if (Tok.is(tok::code_completion)) {
         Actions.CodeCompleteCall(getCurScope(), LHS.get(), None);
         cutOffParsing();
         return ExprError();
       }
 
       if (OpKind == tok::l_paren || !LHS.isInvalid()) {
         if (Tok.isNot(tok::r_paren)) {
+          unsigned BuiltinId = 0;
+          if (LHS.isUsable()) {
+            Expr *Fn = LHS.get()->IgnoreParens();
+            if (DeclRefExpr *DRE = dyn_cast_or_null<DeclRefExpr>(Fn)) {
+              FunctionDecl *FnDecl =
+                  dyn_cast_or_null<FunctionDecl>(DRE->getDecl());
+              BuiltinId = (FnDecl ? FnDecl->getBuiltinID() : 0);
+            }
+          }
+          Sema::InsideNoopRAII SubstIndex(Actions,
+                                          BuiltinId == Builtin::BI__noop);
           if (ParseExpressionList(ArgExprs, CommaLocs, [&] {
                 Actions.CodeCompleteCall(getCurScope(), LHS.get(), ArgExprs);
              })) {
             (void)Actions.CorrectDelayedTyposInExpr(LHS);
             LHS = ExprError();
           } else if (LHS.isInvalid()) {
             for (auto &E : ArgExprs)
               Actions.CorrectDelayedTyposInExpr(E);

lib/Sema/Sema.cpp

: ExternalSource(nullptr),
     NSArrayDecl(nullptr), ArrayWithObjectsMethod(nullptr),
     NSDictionaryDecl(nullptr), DictionaryWithObjectsMethod(nullptr),
     MSAsmLabelNameCounter(0),
     GlobalNewDeleteDeclared(false),
     TUKind(TUKind),
     NumSFINAEErrors(0),
     CachedFakeTopLevelModule(nullptr),
     AccessCheckingSFINAE(false), InNonInstantiationSFINAEContext(false),
-    NonInstantiationEntries(0), ArgumentPackSubstitutionIndex(-1),
-    CurrentInstantiationScope(nullptr), DisableTypoCorrection(false),
-    TyposCorrected(0), AnalysisWarnings(*this), ThreadSafetyDeclCache(nullptr),
-    VarDataSharingAttributesStack(nullptr), CurScope(nullptr),
-    Ident_super(nullptr), Ident___float128(nullptr)
+    NonInstantiationEntries(0), InsideNoop(false),
+    ArgumentPackSubstitutionIndex(-1), CurrentInstantiationScope(nullptr),
+    DisableTypoCorrection(false), TyposCorrected(0), AnalysisWarnings(*this),
+    ThreadSafetyDeclCache(nullptr), VarDataSharingAttributesStack(nullptr),
+    CurScope(nullptr), Ident_super(nullptr), Ident___float128(nullptr)
 {
   TUScope = nullptr;
 
   LoadedExternalKnownNamespaces = false;
   for (unsigned I = 0; I != NSAPI::NumNSNumberLiteralMethods; ++I)
     NSNumberLiteralMethods[I] = nullptr;
 
   if (getLangOpts().ObjC1)

lib/Sema/SemaChecking.cpp

Sema::CheckBuiltinFunctionCall(FunctionDecl *FDecl, unsigned BuiltinID,
   case Builtin::BI__builtin_classify_type:
     if (checkArgCount(*this, TheCall, 1)) return true;
     TheCall->setType(Context.IntTy);
     break;
   case Builtin::BI__builtin_constant_p:
     if (checkArgCount(*this, TheCall, 1)) return true;
     TheCall->setType(Context.IntTy);
     break;
+  case Builtin::BI__noop:
+    TheCall->setType(Context.IntTy);
+    break;
   case Builtin::BI__sync_fetch_and_add:
   case Builtin::BI__sync_fetch_and_add_1:
   case Builtin::BI__sync_fetch_and_add_2:
   case Builtin::BI__sync_fetch_and_add_4:
   case Builtin::BI__sync_fetch_and_add_8:
   case Builtin::BI__sync_fetch_and_add_16:
   case Builtin::BI__sync_fetch_and_sub:
   case Builtin::BI__sync_fetch_and_sub_1:

lib/Sema/SemaExpr.cpp

static void MarkExprReferenced(Sema &SemaRef, SourceLocation Loc,
   CXXMethodDecl *DM = MD->getCorrespondingMethodInClass(MostDerivedClassDecl);
   if (!DM || DM->isPure())
     return;
   SemaRef.MarkAnyDeclReferenced(Loc, DM, OdrUse);
 } 
 
 /// \brief Perform reference-marking and odr-use handling for a DeclRefExpr.
 void Sema::MarkDeclRefReferenced(DeclRefExpr *E) {
+  // If we are inside call of __noop intrinsic we shouldn't mark function as
+  // referenced. We do so not to instantiate template functions. However we
+  // should mark variables as usual to avoid warnings about unused variables.
+  // This behaviour is the same as Microsoft compiler behaviour.
+  if (InsideNoop && isa<FunctionDecl>(E->getDecl()))
+    return;
+
   // TODO: update this with DR# once a defect report is filed.
   // C++11 defect. The address of a pure member should not be an ODR use, even
   // if it's a qualified reference.
   bool OdrUse = true;
   if (CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(E->getDecl()))
     if (Method->isVirtual())
       OdrUse = false;
   MarkExprReferenced(*this, E->getLocation(), E->getDecl(), E, OdrUse);
 }
 
 /// \brief Perform reference-marking and odr-use handling for a MemberExpr.
 void Sema::MarkMemberReferenced(MemberExpr *E) {
   // C++11 [basic.def.odr]p2:
   //   A non-overloaded function whose name appears as a potentially-evaluated
   //   expression or a member of a set of candidate functions, if selected by
   //   overload resolution when referred to from a potentially-evaluated
   //   expression, is odr-used, unless it is a pure virtual function and its
   //   name is not explicitly qualified.
   bool OdrUse = true;
   if (!E->hasQualifier()) {

test/CodeGen/builtin-ms-noop.cpp

 // RUN: %clang_cc1 -fms-extensions -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
 
 struct A {
   ~A() {}
 };
 
+int *p;
+
 extern "C" int f() {
 // CHECK: define i32 @f()
 // CHECK-NOT: call void @_ZN1AD1Ev
 // CHECK: ret i32 0
   return __noop(A());
 };
 
 extern "C" int g() {
   return __noop;
 // CHECK: define i32 @g()
 // CHECK: ret i32 0
 }
 
 extern "C" int h() {
   return (__noop);
 // CHECK: define i32 @h()
 // CHECK: ret i32 0
 }
 
 extern "C" int i() {
   return __noop + 1;
 // CHECK: define i32 @i()
 // CHECK: ret i32 1
 }
+
+extern "C" void j() {
+  p = __noop;
+// CHECK: define void @j()
+// CHECK: store i32* null, i32** @p
+}

test/Sema/builtin-ms-noop-errors.cpp

+// RUN: %clang_cc1 -fms-extensions -fdelayed-template-parsing -fms-compatibility -Wunused -verify %s
+
+class A {
+public:
+  ~A(){};
+};
+template<typename T> static A function1(int a) { sdklfj sdfjsd sdfksdf }
+template<typename T> static int function2() { fwedq rfgr dfgsdg }
+template<typename T> int function3(int a) { fwedq rfgr dfgsdg }
+int Z(){return 0;}
+int Z1(int){return 0;}
+void Z2(){}
+
+void D1() {
+  int var1, var2, var3, var4, var5, var6, var7, var8;
+  ((__noop))(function2<void>());
+  __noop(Z(), Z(), (function2<int>()), Z1(function2<int>()));
+  __noop(Z2());
+  __noop(function2<void>() + var1 + var2);
+  __noop(function1<int>(var3));
+  __noop(function2<int>() + Z1(var4) - function3<float>(var5));
+  __noop(function1<void>(function3<int>(__noop(var6 + function3<int>(var7)))));
+  __noop(function1<int>(Z1(var8)));
+}
+
+int D2() {
+  int arr[] = {9,8,7,6,5,4,3,2,1,0};
+  int T2 = arr[__noop() + 1];
+  int T3 = arr[__noop + 2];
+  return T2 + T3;
+}
+
+int D3() {
+  class A {};
+  int *A1 = __noop; // expected-warning{{expression which evaluates to zero treated as a null pointer constant of type 'int *'}}
+  int *A2 = __noop(); // expected-warning{{expression which evaluates to zero treated as a null pointer constant of type 'int *'}}
+  int *A3;
+  A3 = __noop; // expected-warning{{expression which evaluates to zero treated as a null pointer constant of type 'int *'}}
+  A3 = __noop(); // expected-warning{{expression which evaluates to zero treated as a null pointer constant of type 'int *'}}
+  int (*A4)() = __noop; // expected-warning{{expression which evaluates to zero treated as a null pointer constant of type 'int (*)()'}}
+  int (*A5)();
+  A5 = __noop; // expected-warning{{expression which evaluates to zero treated as a null pointer constant of type 'int (*)()'}}
+  void (*A6)();
+  A6 = __noop; // expected-warning{{expression which evaluates to zero treated as a null pointer constant of type 'void (*)()'}}
+  const int (*A7)(const float);
+  A7 = __noop(2); // expected-warning{{expression which evaluates to zero treated as a null pointer constant of type 'const int (*)(const float)'}}
+  void (A::*A8)();
+  A8 = __noop;
+  return A1[0] + A2[0] + A3[0] + A4() + A5();
+}
+
+void D4() {
+  int arr[10];
+  int T1 = arr[__noop(4 asdfasdf^*^&$^, 5) + __noop]; //expected-error{{expected ')'}} expected-note{{to match this '('}}
+  &__noop; // expected-error{{cannot take the address of an rvalue of type 'int'}}
+  &__noop(); // expected-error{{cannot take the address of an rvalue of type 'int'}}
+  int A = __noop[5]; // expected-error{{subscripted value is not an array, pointer, or vector}}
+  int __noop = 10;
+  int A1 = __noop(); // expected-error{{called object type 'int' is not a function or function pointer}}
+}
+
+int D5() {
+  int a1 = __noop(42, 56);
+  int a2 = __noop;
+  int a3; a3 = __noop(5);
+  int a4 = (__noop)();
+  int a5 = (__noop);
+  int a6 = (__noop());
+  int a7 = ((__noop)(5));
+  int a8 = ((__noop))();
+  return a1 + a2 + a3 + a4 + a5 + a6 + a7 + a8;
+}
+
+int D7() {
+  __noop;
+  __noop();
+  int T1 = __noop;
+  int T2 = __noop();
+  int __noop = 12;
+  int *A = &__noop;
+  int T3 = __noop;
+  int T4 = *A;
+  return T1 + T2 + T3 + T4;
+}