nickdesaulniers requested changes to this revision. nickdesaulniers added a comment. This revision now requires changes to proceed.
In D147714#4249274 <https://reviews.llvm.org/D147714#4249274>, @efriedma wrote: > Any thoughts on diagnostics here? If I'm not mistaken, with this patch, if > you request an impossible tail call, you get a crash with very little useful > information. (Although, see > https://discourse.llvm.org/t/rfc-improving-clangs-middle-and-back-end-diagnostics/69261 > ) Patching this in, and running through the original test cases as reported in https://github.com/llvm/llvm-project/issues/54964#issue-1207256071: $ clang++ x.cpp x.cpp:5:32: error: cannot perform a tail call to function 'base' because its signature is incompatible with the calling function int f1() { [[clang::musttail]] return base(5); } ^ x.cpp:1:1: note: target function has different number of parameters (expected 0 but has 1) int base(int); ^ x.cpp:5:14: note: tail call required by 'musttail' attribute here int f1() { [[clang::musttail]] return base(5); } ^ x.cpp:6:57: error: cannot perform a tail call to function 'base' because its signature is incompatible with the calling function int f2(int x, int y /* unused */) { [[clang::musttail]] return base(5); } ^ x.cpp:1:1: note: target function has different number of parameters (expected 2 but has 1) int base(int); ^ x.cpp:6:39: note: tail call required by 'musttail' attribute here int f2(int x, int y /* unused */) { [[clang::musttail]] return base(5); } ^ 2 errors generated. so that's pretty informative to the user. Changing `musttail` to `nonportable_musttail`: $ clang++ x.cpp cannot guarantee tail call due to mismatched parameter counts %call = musttail call noundef i32 @_Z4basei(i32 noundef 5) in function _Z2f1v fatal error: error in backend: Broken function found, compilation aborted! clang++: error: clang frontend command failed with exit code 70 (use -v to see invocation) clang version 17.0.0 (g...@github.com:llvm/llvm-project.git 36e13ec194d1601499e0069b33b9bd8f69887e1f) Target: x86_64-unknown-linux-gnu Thread model: posix InstalledDir: /android0/llvm-project/llvm/build/bin clang++: note: diagnostic msg: ******************** PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT: Preprocessed source(s) and associated run script(s) are located at: clang++: note: diagnostic msg: /tmp/x-299fe3.cpp clang++: note: diagnostic msg: /tmp/x-299fe3.sh clang++: note: diagnostic msg: ******************** So at the very least, this should use the existing clang support for backend diagnostics rather than crashing. Please see how `!srcloc` works for inline asm and `__attribute__((warning("")))`. It should be straightforward to attach that ad-hoc metadata to call sites with that statement attribute. Also, "woof" to the attribute name. Taking a step back: can't we just fix the backends or modify `CheckTypesMatch` (or add a new method) to detect partial matches? Is there actually a safety issue on any known target? If not, creating a separate attribute feels like working around a possible bug in a particular backend when that should probably be fixed instead. Reading https://github.com/llvm/llvm-project/issues/54964, I'm curious if folks have pursued @efriedma 's suggestion #2 from https://github.com/llvm/llvm-project/issues/54964#issuecomment-1101612886? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147714/new/ https://reviews.llvm.org/D147714 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
