[PATCH] D50016: IR: Add entry/exit instrumentation symbols to the libcall list.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Needs a test.



================
Comment at: llvm/lib/Transforms/Utils/EntryExitInstrumenter.cpp:23
                        Instruction *InsertionPt, DebugLoc DL) {
+  // If you add code to recognize a new instrumentation function here, the
+  // function also needs to be added to include/llvm/IR/RuntimeLibcalls.def
----------------
pcc wrote:
> pcc wrote:
> > tejohnson wrote:
> > > pcc wrote:
> > > > tejohnson wrote:
> > > > > Can you include RuntimeLibcalls.def and assert that the name is in that list?
> > > > I could, but I don't think we need to do that unless there's evidence that people are ignoring the comment.
> > > I'd prefer an assert, because I think it would be easy to miss the comment if the second if block is extended with another func name, or a subsequent new if block is added below it.
> > How about just putting a comment in all the places where people are likely to add new code to this function (before ifs, at the end)?
> I suppose another idea is to do something like
> ```
> #define HANDLE_LIBCALL(name, value) const char LibCallName_##name[] = value;
> #include "llvm/IR/RuntimeLibcalls.def"
> ```
> in a header somewhere, and then change this code to use `LibCallName_FOO`.
Sure that would work too (I was originally envisioning something like what is done in IRSymtab.cpp except assert that IsBuiltinFunc, but your idea is more efficient). Why would it need to be in a header rather than in this file?

Better to enforce consistency at compile time if possible IMO.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D50016