[MCA] use std::function instead of function_ref when storing

[boomanaiden154]

This patch changes uses of llvm::function_ref for std::function when storing the callback inside of a class. The LLVM Programmer's manual mentions that llvm::function_ref is not safe to store as it contains pointers to external memory that are not guaranteed to exist in the future when it is stored.

This causes issues when setting callbacks inside of a class that manages MCA state. Passing a lambda directly to the set callback functions will end up causing UB/segfaults when the lambda is called as some external memory is now invalid. This is easy to work around (create a separate std::function, pass that into the function setting the callback), but isn't ideal.

[mshockwave]

Passing a lambda directly to the set callback functions will end up causing UB/segfaults when the lambda is called as some external memory is now invalid. This is easy to work around (create a separate std::function, pass that into the function setting the callback), but isn't ideal.

Yeah, in that case it definitely creates a dangling function pointer.

My original motivation behind using llvm::function_ref over std::function was that the latter is not cheap and sometime pretty expensive, and presumably it's not really difficult for the users (of IncrementalSrcManager and InstrBuilder) to store/manage the callbacks somewhere else. Do you find cases where storing callbacks outside these two classes challenging?

[dwblaikie]

Let's fix the correctness issue/API subtlety first (function_ref isn't guaranteed to work in this way, even if the caller is very careful about preserving the functor for the lifetime of the object)

It'd be pretty easy for callers to get cleaned up/have the functors inlined into the call sites, etc, and break this code.

Some other tool should be created/used if there's a need for a lightweight functor reference that can be preserved in a longer-term situation like this.

[boomanaiden154]

Doing some quick microbenchmarks, I'm only seeing a 2-3% performance regression for computing the cost of 10000 instructions, so I think fixing the correctness/API design issue is probably worth the performance cost. I'm going to land this and we can revisit this if the performance regression causes issues for people/or someone is willing to implement a lightweight function reference for this situation.