Fix a crash with variably-modified parameter types in a naked function

[aaron.ballman]

Summary

Naked functions have no prolog, so it's not valid to emit prolog code to evaluate the variably-modified type. This fixes Issue 50541.

[erichkeane]

Other than this 1 thing, LGTM. I DO find myself wondering how much of the CXXMethodDecl bit in the branch above is valid for 'naked' as well. If it ISN'T we might consider moving this loop AND that to EmitFunctionPrologue.

clang/lib/CodeGen/CodeGenFunction.cpp

1200

Since you're touching it... this looks like a 'range-for' loop :D

[aaron.ballman]

Updating based on review feedback

[aaron.ballman]

In D122341#3403587, @erichkeane wrote:

Other than this 1 thing, LGTM. I DO find myself wondering how much of the CXXMethodDecl bit in the branch above is valid for 'naked' as well. If it ISN'T we might consider moving this loop AND that to EmitFunctionPrologue.

There's a different kind of bug there. MSVC disallows using the naked attribute on a member function. Clang doesn't enforce this: https://godbolt.org/z/6jKGbzYTq GCC has no such restriction, but GCC doesn't have many diagnostics in this area anyway. We may want to reconsider whether allowing the naked attribute on a non static member function is a good idea. We probably don't want to allow it in MS compatibility mode at the very least. But I think this can be a change for another day. WDYT?

clang/lib/CodeGen/CodeGenFunction.cpp

1200

Sure, I can do that.

[erichkeane]

In D122341#3405008, @aaron.ballman wrote:

In D122341#3403587, @erichkeane wrote:

Other than this 1 thing, LGTM. I DO find myself wondering how much of the CXXMethodDecl bit in the branch above is valid for 'naked' as well. If it ISN'T we might consider moving this loop AND that to EmitFunctionPrologue.

There's a different kind of bug there. MSVC disallows using the naked attribute on a member function. Clang doesn't enforce this: https://godbolt.org/z/6jKGbzYTq GCC has no such restriction, but GCC doesn't have many diagnostics in this area anyway. We may want to reconsider whether allowing the naked attribute on a non static member function is a good idea. We probably don't want to allow it in MS compatibility mode at the very least. But I think this can be a change for another day. WDYT?

Ah! I didn't know the bit about MSVC! I think that makes it even more interesting. From what I can tell however, GCC actually DOES 'do the right thing' with member functions, though the motivation for them is somewhat suspicious. I definitely agree we don't want to allow it in MS Compat mode (and agree that is a different patch). As for in GCC mode, a part of me wants to just DO it and see what the fallout is. The only value I can see for these as non-static member functions is in a template (or perhaps as a virtual function? *shudders*), and I don't see much overlap in the usage there.

clang/test/CodeGen/attr-naked.c

31

so in the test, both 'entry' and '%0'/'%1' above are unstable names. You might want to replace both with wildcards. THOUGH, I see other parts of this test have the param names, so IDK? Either way, 'entry' isn't necessarily a stable label.

[aaron.ballman]

Updated the tests

[aaron.ballman]

In D122341#3405269, @erichkeane wrote:

In D122341#3405008, @aaron.ballman wrote:

In D122341#3403587, @erichkeane wrote:

Other than this 1 thing, LGTM. I DO find myself wondering how much of the CXXMethodDecl bit in the branch above is valid for 'naked' as well. If it ISN'T we might consider moving this loop AND that to EmitFunctionPrologue.

There's a different kind of bug there. MSVC disallows using the naked attribute on a member function. Clang doesn't enforce this: https://godbolt.org/z/6jKGbzYTq GCC has no such restriction, but GCC doesn't have many diagnostics in this area anyway. We may want to reconsider whether allowing the naked attribute on a non static member function is a good idea. We probably don't want to allow it in MS compatibility mode at the very least. But I think this can be a change for another day. WDYT?

Ah! I didn't know the bit about MSVC! I think that makes it even more interesting. From what I can tell however, GCC actually DOES 'do the right thing' with member functions, though the motivation for them is somewhat suspicious. I definitely agree we don't want to allow it in MS Compat mode (and agree that is a different patch). As for in GCC mode, a part of me wants to just DO it and see what the fallout is. The only value I can see for these as non-static member functions is in a template (or perhaps as a virtual function? *shudders*), and I don't see much overlap in the usage there.

I don't have very strong opinions on whether we restrict in non-Microsoft mode, but from what I can tell, the Itanium ABI does do some interesting things for some member functions (having to do with virtual table tables, also having to do with targets that mandate returning this, but otherwise don't seem to do any interesting things in the prolog. So it may be sensible to restrict some situations while still allowing it on member functions. I think for the moment, I'll just restrict on Microsoft mode under the assumption that Itanium is "okay enough" for the moment. But I'll do that in a follow-up patch.

clang/test/CodeGen/attr-naked.c

31

Ah, good to know -- I'll correct that.

[aaron.ballman]

Thanks for the reviews! I've commit in 488c772920566354075f7933eedbe4358c128bd2.

clang/docs/ReleaseNotes.rst

- Allow `-Wno-gnu` to silence GNU extension diagnostics for pointer arithmetic
   diagnostics. Fixes `Issue 54444 <https://github.com/llvm/llvm-project/issues/54444>`_.
 - Placeholder constraints, as in `Concept auto x = f();`, were not checked when modifiers
   like ``auto&`` or ``auto**`` were added. These constraints are now checked.
   This fixes  `Issue 53911 <https://github.com/llvm/llvm-project/issues/53911>`_
   and  `Issue 54443 <https://github.com/llvm/llvm-project/issues/54443>`_.
 - Previously invalid member variables with template parameters would crash clang.
   Now fixed by setting identifiers for them.
   This fixes `Issue 28475 (PR28101) <https://github.com/llvm/llvm-project/issues/28475>`_.
-
 - Now allow the `restrict` and `_Atomic` qualifiers to be used in conjunction
   with `__auto_type` to match the behavior in GCC. This fixes
   `Issue 53652 <https://github.com/llvm/llvm-project/issues/53652>`_.
+- No longer crash when specifying a variably-modified parameter type in a
+  function with the ``naked`` attribute. This fixes
+  `Issue 50541 <https://github.com/llvm/llvm-project/issues/50541>`_.
 
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - ``-Wliteral-range`` will warn on floating-point equality comparisons with
   constants that are not representable in a casted value. For example,
   ``(float) f == 0.1`` is always false.
 - ``-Winline-namespace-reopened-noninline`` now takes into account that the

clang/lib/CodeGen/CodeGenFunction.cpp

 
       EmitTypeCheck(
           isa<CXXConstructorDecl>(MD) ? TCK_ConstructorCall : TCK_MemberCall,
           Loc, CXXABIThisValue, ThisTy, CXXABIThisAlignment, SkippedChecks);
     }
   }
 
   // If any of the arguments have a variably modified type, make sure to
-  // emit the type size.
-  for (FunctionArgList::const_iterator i = Args.begin(), e = Args.end();
-       i != e; ++i) {
-    const VarDecl *VD = *i;
-
-    // Dig out the type as written from ParmVarDecls; it's unclear whether
-    // the standard (C99 6.9.1p10) requires this, but we're following the
-    // precedent set by gcc.
-    QualType Ty;
-    if (const ParmVarDecl *PVD = dyn_cast<ParmVarDecl>(VD))
-      Ty = PVD->getOriginalType();
-    else
-      Ty = VD->getType();
+  // emit the type size, but only if the function is not naked. Naked functions
+  // have no prolog to run this evaluation.
+  if (!FD || !FD->hasAttr<NakedAttr>()) {
+    for (const VarDecl *VD : Args) {
+      // Dig out the type as written from ParmVarDecls; it's unclear whether
+      // the standard (C99 6.9.1p10) requires this, but we're following the
+      // precedent set by gcc.
+      QualType Ty;
+      if (const ParmVarDecl *PVD = dyn_cast<ParmVarDecl>(VD))
+        Ty = PVD->getOriginalType();
+      else
+        Ty = VD->getType();
 
       if (Ty->isVariablyModifiedType())
         EmitVariablyModifiedType(Ty);
+    }
   }
   // Emit a location at the end of the prologue.
   if (CGDebugInfo *DI = getDebugInfo())
     DI->EmitLocation(Builder, StartLoc);
-
   // TODO: Do we need to handle this in two places like we do with
   // target-features/target-cpu?
   if (CurFuncDecl)
     if (const auto *VecWidth = CurFuncDecl->getAttr<MinVectorWidthAttr>())
       LargestVectorWidth = VecWidth->getVectorWidth();
 }
 
 void CodeGenFunction::EmitFunctionBody(const Stmt *Body) {

clang/test/CodeGen/attr-naked.c

 // Make sure not to generate prolog or epilog for naked functions.
 __attribute((naked)) void t3(int x) {
 // CHECK: define{{.*}} void @t3(i32 noundef %0)
 // CHECK-NOT: alloca
 // CHECK-NOT: store
 // CHECK: unreachable
 }
 
+// Make sure naked functions do not attempt to evaluate parameters with a
+// variably-modified type. Naked functions get no prolog, so this evaluation
+// should not take place.
+__attribute__((naked)) void t4(int len, char x[len]) {
+  // CHECK: define{{.*}} void @t4(i32 noundef{{.*}}, i8* noundef{{.*}})
+  // CHECK: unreachable
+}
+
 // CHECK: attributes [[NAKED_OPTNONE]] = { naked noinline nounwind optnone{{.*}} }
 // CHECK: attributes [[NAKED]] = { naked noinline nounwind{{.*}} }