[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

[Nick Desaulniers via Phabricator via cfe-commits]

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 (git at 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