[PATCH] D46326: ThinLTO+CFI: short-circuit direct calls to jump table entries

Eli Friedman via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed May 9 15:07:56 PDT 2018


efriedma added inline comments.


================
Comment at: llvm/lib/IR/Value.cpp:506
+      continue;
+    U.set(New);
+  }
----------------
dmikulin wrote:
> vlad.tsyrklevich wrote:
> > dmikulin wrote:
> > > vlad.tsyrklevich wrote:
> > > > dmikulin wrote:
> > > > > efriedma wrote:
> > > > > > This looks like it doesn't handle the case of a function with a function pointer argument correctly.
> > > > > I thought getCalledFunction on a CallInst takes care of it... 
> > > > It checks that the call is a direct call, but the use might be an argument of the function not the called function, e.g.
> > > > ```
> > > > void bar();
> > > > foo(&bar);
> > > > ```
> > > > 
> > > > Iterating through the uses of `bar` you'll hit the call instruction and replace `foo(&bar)` with `foo(&bar.cfi)` though it shouldn't be. You need to check that getCalledFunction is the one you actually expect.
> > > I see... I thought there'd be a more straightforward direct call check, but I can't seem to find anything in the code. Changed to check if the getCalledFunction() is the same as what we're trying to replace, i.e. ''this'.
> > I haven't tested it but I think this will not work for `foo(&foo)`, e.g. it will change it to `foo.cfi(&foo.cfi)` instead of `foo.cfi(&foo)`.
> I can't seem to reproduce it without at least having the fptr argument wrapped in a bitcast, which makes this check work. I'm thinking I'd need a recursively defined fptr type to avoid the cast. 
> I don't necessarily like the roundabout way of checking if it's a direct call. Is there a better way?
Yes, it's impossible to have a function with itself as an argument... at least for now (it might become possible with the typeless pointer work, but no idea if/when that will happen).

Looking around a bit, it looks like there's a helper CallSite::isCallee which is meant for this sort of check; it takes a `Use*`.


https://reviews.llvm.org/D46326





More information about the llvm-commits mailing list