[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#439794, @zhaoqin wrote:

> In that case, http://reviews.llvm.org/D20488 has add test to check the tool number passed to __esan_init.


It's already there, in cache_frag_basic.ll.

Re: this change:
I think it's better, but I don't really like one thing here: Finding out what's getting included in the anonymous structure has us browsing the source code in several different locations.
Having to track what's getting pushed onto the `SmallVector` seems harder to follow without much gain.
(I think the current solution is acceptable, but I'd prefer to have it be a bit easier to read/follow)

How about:
The createEsanInitToolGV just dispatches to the appropriate (tool-specific) function and returns the same as that function (possibly creates the module string and passes it to the next function too, if you want to factor that out to here);
The tool-specific function:

  Gets all the info/adds constants
  Sets up an array with the information to be placed in the structure
  Calls into a helper "create a GV for tool init" function (optional if you want to return the GV from the tool-specific function, at the expense of code duplication)

That way, each function deals only with one thing. Instead of having dispatch + (some) setup + GV creation all mixed in the same function. The creation of the structure is very easy to read, and you can easily make sure both places (here and compiler-rt) have a matching structure definition.
Adding a tool-specific function will also avoid introducing more bugs, since you'd only need to touch the dispatcher + your own tool's struct creator. The dispatcher wouldn't need to know anything about how the GV is created.

What do you think?


================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:133
@@ -117,2 +132,3 @@
   void initializeCallbacks(Module &M);
+  void createCacheFragGV(Module &M, SmallVector<Constant *, 4> &ToolInfoVec);
   GlobalVariable *createEsanInitToolGV(Module &M);
----------------
That `Vec` in the name feels weird. Why not just `ToolInfoData` or even `ToolData`? No need repeating something that's in the type ("saying" that it's a vector).


http://reviews.llvm.org/D20541