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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
