[PATCH] D20541: [esan|cfrag] Create the cfrag variable for the runtime

[Filipe Cabecinhas via llvm-commits]

filcab added a comment.

In http://reviews.llvm.org/D20541#439341, @zhaoqin wrote:

> In http://reviews.llvm.org/D20541#439070, @filcab wrote:
>
> > Please add a test for the cache fragmentation tool's global variable.
>
>
> http://reviews.llvm.org/D20542 is the test for the arg passed to runtime.
>  Are you thinking of a different test in llvm?


Yes, I'd like an LLVM test checking we emit the correct tool number.

Thanks for working on this.

Filipe


================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:206
@@ +205,3 @@
+  // runtime library cache_frag tool.
+  // struct CacheFragTy {
+  //   int         ToolType;
----------------
I'd prefer having `CacheFragInfo` (`CacheFragCUInfo` since it's a per-compile unit thing?) or something similar. `Ty` doesn't add anything to the rest of the name, and is not explanatory.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:207
@@ +206,3 @@
+  // struct CacheFragTy {
+  //   int         ToolType;
+  //   const char *UnitName;
----------------
zhaoqin wrote:
> filcab wrote:
> > I don't really see a reason to include the tool type here. We're already emitting it in the call to `__esan_init`.
> One of the reason is to potentially support multiple tools in the same build/run, where each tool may need separate information.
> It is not in any near future plan, so no code refactoring for it, i.e., remove ToolType from __esan_init.
> But adding code with that in mind would help.
> 
>From what I remember, the plan is to only have one active tool at a time, especially with the different shadow scales, etc.

Please don't implement partial extension points unless there is a plan (I'm ok with planning for portability across OS if it's not a big problem. But I'd rather avoid doing small things "planning" possible functionality which would imply huge changes).
We're already saying this is a CacheFrag thing, so the tool was already picked.


http://reviews.llvm.org/D20541