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

[Gonzalo BG]

>
> 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<FunctionDecl>(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::set<std::string> with a std::vector<std::string>.
Rationale: 1) we get a std::vector<string> 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


On Tue, Nov 12, 2013 at 5:38 AM, Nico Weber <thakis at chromium.org> wrote:

> 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?
>
>
> On Mon, Nov 11, 2013 at 11:24 AM, Gonzalo BG <gonzalobg88 at gmail.com>wrote:
>
>> 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
>>
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20131112/f73fa6d1/attachment.html>
-------------- next part --------------
Index: include/clang/Driver/Options.td
===================================================================
--- include/clang/Driver/Options.td	(revision 194458)
+++ include/clang/Driver/Options.td	(working copy)
@@ -535,6 +535,12 @@
 def findirect_virtual_calls : Flag<["-"], "findirect-virtual-calls">, Alias<fapple_kext>;
 def finline_functions : Flag<["-"], "finline-functions">, Group<clang_ignored_f_Group>;
 def finline : Flag<["-"], "finline">, Group<clang_ignored_f_Group>;
+def finstrument_functions_exclude_function_list : CommaJoined<["-"],
+  "finstrument-functions-exclude-function-list=">, Group<f_Group>, Flags<[CC1Option]>,
+  HelpText<"Exclude given (mangled, if C++) functions from function instrumentation.">;
+def finstrument_functions_exclude_file_list : CommaJoined<["-"],
+  "finstrument-functions-exclude-file-list=">, Group<f_Group>, Flags<[CC1Option]>,
+  HelpText<"Exclude given path segments from function instrumentation.">;
 def finstrument_functions : Flag<["-"], "finstrument-functions">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Generate calls to instrument function entry and exit">;
 def fkeep_inline_functions : Flag<["-"], "fkeep-inline-functions">, Group<clang_ignored_f_Group>;
Index: include/clang/Frontend/CodeGenOptions.h
===================================================================
--- include/clang/Frontend/CodeGenOptions.h	(revision 194458)
+++ include/clang/Frontend/CodeGenOptions.h	(working copy)
@@ -131,6 +131,12 @@
   /// A list of dependent libraries.
   std::vector<std::string> DependentLibraries;
 
+  /// Functions to exclude from function instrumentation.
+  std::vector<std::string> InstrumentFunctionExclusionsFunctions;
+
+  /// Path segments to exclude from function instrumentation, eg, '/bits'
+  std::vector<std::string> InstrumentFunctionExclusionsPathSegments;
+
 public:
   // Define accessors/mutators for code generation options of enumeration type.
 #define CODEGENOPT(Name, Bits, Default)
Index: lib/CodeGen/CodeGenFunction.cpp
===================================================================
--- lib/CodeGen/CodeGenFunction.cpp	(revision 194458)
+++ lib/CodeGen/CodeGenFunction.cpp	(working copy)
@@ -29,6 +29,7 @@
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/Operator.h"
+#include <map>
 using namespace clang;
 using namespace CodeGen;
 
@@ -232,7 +233,7 @@
   // Emit function epilog (to return).
   EmitReturnBlock();
 
-  if (ShouldInstrumentFunction())
+  if (ShouldInstrumentFunction(CurFn))
     EmitFunctionInstrumentation("__cyg_profile_func_exit");
 
   // Emit debug descriptor for function end.
@@ -277,13 +278,77 @@
     EmitDeclMetadata();
 }
 
+#if !defined(_MSC_VER)
+// Assume that __cxa_demangle is provided by libcxxabi (except for Windows).
+extern "C" char *__cxa_demangle(const char *mangled_name, char *output_buffer,
+                                size_t *length, int *status);
+#endif
+
 /// ShouldInstrumentFunction - Return true if the current function should be
 /// instrumented with __cyg_profile_func_* calls
-bool CodeGenFunction::ShouldInstrumentFunction() {
+bool CodeGenFunction::ShouldInstrumentFunction(llvm::Function *Fn) {
+  typedef std::vector<std::string>::const_iterator CIt;
   if (!CGM.getCodeGenOpts().InstrumentFunctions)
     return false;
   if (!CurFuncDecl || CurFuncDecl->hasAttr<NoInstrumentFunctionAttr>())
     return false;
+
+  // 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;
+  }
+
+  SourceLocation SLoc = CurFuncDecl->getLocation();
+  static std::map<SourceLocation, std::string> cache;
+
+  if (SLoc.isFileID()) {
+    if (cache.find(SLoc) == cache.end()) {
+      ASTContext &ctx = CurFuncDecl->getASTContext();
+      const SourceManager &SM = ctx.getSourceManager();
+
+      PresumedLoc PLoc = SM.getPresumedLoc(SLoc);
+      cache[SLoc] = PLoc.getFilename();
+    }
+    std::string FunctionDeclPath = cache[SLoc];
+
+    const std::vector<std::string> &PathSearch =
+      CGM.getCodeGenOpts().InstrumentFunctionExclusionsPathSegments;
+
+    for (CIt i = PathSearch.begin(), e = PathSearch.end(); i != e; ++i) {
+      if(FunctionDeclPath.find(*i) != std::string::npos) {
+        return false;
+      }
+    }
+  }
+
+  // Demangle the name if it's mangled
+  std::string FunctionName = Fn->getName();
+#if !defined(_MSC_VER)
+  int status = 0;
+  char *result = __cxa_demangle(FunctionName.c_str(), 0, 0, &status);
+
+  assert((status == 0 || status == -2) && "Couldn't demangle name.");
+
+  if (status == 0) {
+    FunctionName = result;
+    free(result);
+  }
+#endif
+
+  const std::vector<std::string> &FunctionSearch =
+    CGM.getCodeGenOpts().InstrumentFunctionExclusionsFunctions;
+  for (CIt i = FunctionSearch.begin(), e = FunctionSearch.end(); i != e; ++i) {
+    if(FunctionName.find(*i) != std::string::npos) {
+      return false;
+    }
+  }
+
   return true;
 }
 
@@ -567,7 +632,7 @@
     DI->EmitFunctionStart(GD, FnType, CurFn, Builder);
   }
 
-  if (ShouldInstrumentFunction())
+  if (ShouldInstrumentFunction(CurFn))
     EmitFunctionInstrumentation("__cyg_profile_func_enter");
 
   if (CGM.getCodeGenOpts().InstrumentForProfiling)
Index: lib/CodeGen/CodeGenFunction.h
===================================================================
--- lib/CodeGen/CodeGenFunction.h	(revision 194458)
+++ lib/CodeGen/CodeGenFunction.h	(working copy)
@@ -1202,7 +1202,7 @@
 
   /// ShouldInstrumentFunction - Return true if the current function should be
   /// instrumented with __cyg_profile_func_* calls
-  bool ShouldInstrumentFunction();
+  bool ShouldInstrumentFunction(llvm::Function *Fn);
 
   /// EmitFunctionInstrumentation - Emit LLVM code to call the specified
   /// instrumentation function with the current function and the call site, if
Index: lib/Driver/Tools.cpp
===================================================================
--- lib/Driver/Tools.cpp	(revision 194458)
+++ lib/Driver/Tools.cpp	(working copy)
@@ -2620,6 +2620,12 @@
   Args.AddAllArgs(CmdArgs, options::OPT_fdata_sections);
 
   Args.AddAllArgs(CmdArgs, options::OPT_finstrument_functions);
+  if (Args.hasArg(options::OPT_finstrument_functions)) {
+    Args.AddAllArgs(CmdArgs,
+                    options::OPT_finstrument_functions_exclude_file_list);
+    Args.AddAllArgs(CmdArgs,
+                    options::OPT_finstrument_functions_exclude_function_list);
+  }
 
   if (Args.hasArg(options::OPT_ftest_coverage) ||
       Args.hasArg(options::OPT_coverage))
Index: lib/Frontend/CompilerInvocation.cpp
===================================================================
--- lib/Frontend/CompilerInvocation.cpp	(revision 194458)
+++ lib/Frontend/CompilerInvocation.cpp	(working copy)
@@ -432,6 +432,13 @@
   }
 
   Opts.InstrumentFunctions = Args.hasArg(OPT_finstrument_functions);
+  if (Opts.InstrumentFunctions) {
+    Opts.InstrumentFunctionExclusionsFunctions
+        = Args.getAllArgValues(OPT_finstrument_functions_exclude_function_list);
+    Opts.InstrumentFunctionExclusionsPathSegments
+        = Args.getAllArgValues(OPT_finstrument_functions_exclude_file_list);
+  }
+
   Opts.InstrumentForProfiling = Args.hasArg(OPT_pg);
   Opts.EmitOpenCLArgMetadata = Args.hasArg(OPT_cl_kernel_arg_info);
   Opts.DebugCompilationDir = Args.getLastArgValue(OPT_fdebug_compilation_dir);