Why hasn't patch for -finstrument-functions-exclude-file-list support been merged?

Hi,

I’m trying to use “-finstrument-functions” in an application and this feature seems to work fine.

However, I can’t exclude the standard library from getting instrumented. This means I cannot use any std library function in the “__cyg_profile_func_enter” and “__cyg_profile_func_exit” call backs. This is severely limiting.

GCC defines “-finstrument-functions-exclude-file-list=file,file,…” (see http://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html) which allows you to exclude instrumentation of some files (e.g. the std library headers!). Someone filled a bug report and submitted a patch in march but it hasn’t been merged yet: http://llvm.org/bugs/show_bug.cgi?id=15255 .

Anyone knows the reason? If I’m not able to exclude the standard library I can’t use instrumentation in my application.

Bests,
Gonzalo

It looks like the patch stalled in review. This is the last message on the thread: http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20130311/075917.html (“I will look at these issues with the patch and work on resolving them” and then nothing). Maybe you can try to pick up the patch?

Maybe you can try to pick up the patch?

I can try but i’ve never messed with a compiler before so please consider that I have no idea of what I’m doing and need some guidance.

First, I compiled ToT llvm/clang and applied the patch. There are 4 new tests in the patch. Everything passes. I tried a small example using the “-finstrument-exclude-file-list” feature and it works as expected.

In the original mailings: http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20130311/075910.html some issues were raised.

  • // Inline functions that are not externally visible mustn’t be instrumented.
  • // They create a named reference to the inlined function, as the first
  • // parameter to _cyg_profile* functions, which a linker will never be able
  • // to resolve.
  • if (const FunctionDecl *ActualFuncDecl =
  • dyn_cast(CurFuncDecl)) {
  • if (ActualFuncDecl->isInlined() &&
  • !ActualFuncDecl->isInlineDefinitionExternallyVisible())
  • return false;
  • }
    This seems like a separable, ready-to-commit bug fix.

I’ve re-submitted Matthews original patch.

std::set is pretty heavyweight, and isn’t buying us anything. I think just having one large string to search would be best. Comma probably isn’t the best separator character; embedded NULLs or something non-ASCII would be better.

I replaced std::setstd::string with a std::vectorstd::string. Rationale: 1) we get a std::vector as input anyways, 2) using a string with a delimiter is more complicated, 3) in particular NULLs, because some string algorithms like find stop at the first NULL. I looked in the docs but wasn’t able to find algorithms for working with this type of data structure (http://llvm.org/docs/ProgrammersManual.html#string-like-containers). Please point me to them in case I miss them. Otherwise, I don’t think is worth it to implement them for this case.

If Fn->isExternC(), there’s no point in demangling. Let’s save ourselves the work in that case.

How can I call isExternC()? Fn is of type llvm::Function but I’ve only been able to find clang::FunctionDecl::isExternC().

Our correctness did not depend on host’s cxa_demangle before… There’s also a fixme in compiler_rt about shipping our own cxa_demangle in order not to leak memory (it is used inside sanitizers, so it has special requirements about allocating memory).

Is there another better demangle function I should call instead? Or what should be done about this?

I didn’t see anything to handle escaped commas in the input, which GCC’s documentation mentions.

Can you give me hints about what to look for in the docs to be able to implement this? I haven’t been able to find anything about dealing with escaped commas or characters.H

I suggest that we look through macro expansions to find the FileID where the expansion occurred, then check that. Also, can we cache the result of this lookup based on the FileID? There are a lot of string searches we’re potentially doing here, many of which will be redundant.

What is the best way to do this? I’ve added a std::map for the caching but don’t know if this is the preferred solution. Any hints are welcome.

Bests,
Gonzalo

improved.diff (7.53 KB)