[PATCH] D111660: [FuncSpec] Make sure function is actually the callee before trying to specialize.

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:665
       auto &CS = *cast<CallBase>(U);
+      if (CS.getCalledFunction() != F) {
+        continue;
----------------
duck-37 wrote:
> SjoerdMeijer wrote:
> > Hmmm, interesting. Can this lead to a miscompute, or other problems?  I think we need to add a test to trigger that (which this patch fixes)?
> Yes, this can lead to miscompiles and crashes if not handled properly. The problem is that I *believe* this issue only arises in very specific scenarios and only when we start allowing specialization of functions that can get their address taken. Otherwise this is never triggered on problematic functions. 
> 
> Since allowing that is outside of the scope of this PR, I thought it would be best to add the test for that in a separate patch I'm planning on submitting later.
I don't think we can or should submit code that isn't covered by tests, so I think that leaves us 2 options here:
- if we can't trigger this now, we could turn this into an assert.
- if we can trigger the problematic behaviour in a later patch, then it should be be part of that (and covered with tests there).

These options are not mutually exclusive, we could add an assert now, change that later? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111660