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

Thu Mar 24 07:37:40 PDT 2022

aaron.ballman added a comment.

In D122341#3405269 <https://reviews.llvm.org/D122341#3405269>, @erichkeane wrote:

> 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.

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.

================
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
----------------
erichkeane wrote:
> 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.
Ah, good to know -- I'll correct that.

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

https://reviews.llvm.org/D122341