[PATCH] Add -finstruments-functions-exclude-{file, function}-list options

[Douglas Gregor]

On Mar 11, 2013, at 9:31 PM, Matthew Iselin <matthew at theiselins.net> wrote:

> Hi all,
> 
> This patch adds two options, -finstrument-functions-exclude-file-list= and -finstrument-functions-exclude-function=list=, which offer a means through which headers in various locations and individual functions can be excluded from instrumentation via -finstrument-functions. These options can be found in GCC alongside -finstrument-functions.
> 
> LLVM Bugzilla bug #15255 should be resolved by this patch.
> 
> This was sent to the llvm-commits list yesterday. I have been informed that this list is a better option for clang-specific patches (which makes sense, in hindsight).
> 
> Since the original patch submission yesterday, this patch has been updated to resolve GCC incompatibilities: the options should now function more or less equivalently to their GCC counterparts. The updated patch also adds code to avoid instrumenting inline functions that will never be externally visible. This should help avoid cases where an inline function has calls to __cyg_profile_* with a named reference to the inline function emitted within it, but is completely inlined and never has an actual symbol name present. This causes linker errors when trying to resolve the inline function names.
> 
> Excluded functions do not have __cyg_profile_* calls emitted in them. I have included a few tests to ensure the behaviour is as expected.

A few comments:

+
+  // 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<FunctionDecl>(CurFuncDecl)) {
+    if (ActualFuncDecl->isInlined() &&
+        !ActualFuncDecl->isInlineDefinitionExternallyVisible())
+      return false;
+  }

This seems like a separable, ready-to-commit bug fix.

+  if (SLoc.isFileID()) {
+    ASTContext &ctx = CurFuncDecl->getASTContext();
+    const SourceManager &SM = ctx.getSourceManager();
+
+    PresumedLoc PLoc = SM.getPresumedLoc(SLoc);
+    std::string FunctionDeclPath = PLoc.getFilename();
+

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.

+    // Unsure of an 'optimal' way to do this - we have (potentially) many
+    // haystacks but only one needle here - maybe this shouldn't be a set
+    // and should just be string with the full, comma separated, value?
+    const std::set<std::string> &PathSearch =
+      CGM.getCodeGenOpts().InstrumentFunctionExclusionsPathSegments;
+    Iterator = std::find_if(PathSearch.begin(), PathSearch.end(),
+                            CheckForSubstringInString(FunctionDeclPath));
+    if (Iterator != PathSearch.end()) {
+      return false;
+    }

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. 

+  // We need to demangle the name if it's mangled, but we also need to handle
+  // unmangled names. And Windows doesn't have libcxxabi. Yay!
+  std::string FunctionName = Fn->getName();
+#if !defined(_MSC_VER)
+  int status = 0;
+  char *result = __cxa_demangle(Fn->getName().str().c_str(), 0, 0, &status);
+
+  assert((status == 0 || status == -2) && "Couldn't demangle name.");
+
+  if (status == 0) {
+    FunctionName = result;
+    free(result);
+  }
+#endif

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

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

	- Doug