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