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

[Qin Zhao via llvm-commits]

zhaoqin added a comment.

In http://reviews.llvm.org/D20541#440870, @filcab wrote:

> 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)


I feel we are going circle here!!!
In my original CL, the full struct description and definition is at one place, i.e., in the tool specific createGV function.
I am asked to split it so the header (ToolType/UnitName) would be put at beginning of the struct for all different types of Tool GV.
Now it is hard to track what the struct is because of the split!!!

> 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?


I am not sure what's the difference from my original CL, where the struct is declared in each tool-specific create GV?
The original CL had a static struct declaration and then filled the entries and created the GV at the end of the function.

  StructType *CacheFragType =
    StructType::get(Int32Ty, IntptrTy, nullptr);
  ...
  GlobalVariable *CacheFragGV = new GlobalVariable(
      M, CacheFragType, true, GlobalVariable::InternalLinkage,
      ConstantStruct::get(CacheFragType,
                          ConstantInt::get(Int32Ty, Options.ToolType),
                          ConstantExpr::getPointerCast(UnitName, IntptrTy),
                          nullptr));

It seems clearer than pushing a vector in a separate the function and then create the GV without knowing what's inside.

Also, how do you guarantee the UnitName would be the first entry if each tool-specific function "Gets all the info/adds constants"?


================
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);
----------------
filcab wrote:
> 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).
It is not about knowing what type it is at where it is declared. It is for knowing what type it is where it is used.
If someone reading the code sees ToolInfoVec, it is clear that ToolInfoVec is a vec for ToolInfo with multiple entries and extensible without looking for its declaration.
In contrast, ToolInfoData tells much less information, ToolInfoEntries would better. But I feel TooInfoVec is even better.



http://reviews.llvm.org/D20541