[PATCH] D122341: Fix a crash with variably-modified parameter types in a naked function

Thu Mar 24 06:30:27 PDT 2022

erichkeane added a comment.

In D122341#3405008 <https://reviews.llvm.org/D122341#3405008>, @aaron.ballman wrote:

> In D122341#3403587 <https://reviews.llvm.org/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.

================
Comment at: clang/test/CodeGen/attr-naked.c:31
+  // CHECK: define{{.*}} void @t4(i32 noundef %0, i8* noundef %1)
+  // CHECK-NEXT: entry:
+  // CHECK-NEXT: unreachable
----------------
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.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122341/new/

https://reviews.llvm.org/D122341