[PATCH] D20483: [esan] EfficiencySanitizer working set tool fastpath

[Qin Zhao via llvm-commits]

zhaoqin added inline comments.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:71
@@ +70,3 @@
+static const uint64_t ShadowOffs[4] = { // Indexed by scale
+  0x0000130000000000ull,
+  0x0000220000000000ull,
----------------
bruening wrote:
> filcab wrote:
> > aizatsky wrote:
> > > bruening wrote:
> > > > aizatsky wrote:
> > > > > uh-oh. These are really magical constants copied.
> > > > > Unfortunately there's no proper way to share code between runtime / llvm. I talked with our cube and the idea is to introduce a separate header (e.g. esan_compile_interface.h) and put all shareable constants there. At least that would make it clear what you are sharing. At best - you can manually synchronize the file by copying. WDYT?
> > > > Yes, this lack of ability to share is something I was surprised at in the other sanitizers.  I talked to kcc about it early on when starting this project and it sounded like copying a handful of constants was something everyone was willing to live with and that they did not plan to push for any better approach, so I followed suit here.
> > > > 
> > > > Sure, a separate header sounds good.  Is there a plan to do this for the other sanitizers?
> > > > 
> > > > I assume the header would go into include/llvm/Transforms/ and should probably follow the CamelCase naming.
> > > > Sure, a separate header sounds good. Is there a plan to do this for the other sanitizers?
> > > 
> > > Everyone agrees that the way it is now is very painful and we need to try this. If a separate header works for you - we would definitely steer others in the same direction.
> > > 
> > > > I assume the header would go into include/llvm/Transforms/ and should probably follow the CamelCase naming.
> > > 
> > > Yes. Even if you don't decide to copy it to runtime, it would clearly document all inter-dependencies and will make updating easier.
> > > Yes. Even if you don't decide to copy it to runtime, it would clearly document all inter-dependencies and will make updating easier.
> > 
> > I don't like that half-way thing. Please, if you're making it a header signalizing that the stuff there is shared, then do it on both places, not just one.
> Thinking more about this, I'm not sure it makes sense to treat just these two shadow constants as different from all of the other shared interface constants such as all of the runtime library function names:
> 
>   static const char *const EsanInitName = "__esan_init";
> 
> This includes the load and store handler names ("__esan_aligned_load8", e.g.), which are generated in EfficiencySanitizer.cpp.
> 
> The cleanest thing would be to use macros to turn __esan_init into a string or a var, and include that header in both places.  We'd have to remove the loop that generates __esan_aligned_loadX and hardcode each one.
> We'd put the ToolType enum in there too.
> 
> However, compiler-rt is not supposed to directly include an llvm header, right?  If it's not allowed, and we'd rather have the loop that generates those names, and we already have a nice compiler-rt/lib/esan/esan_interface_internal.h header for the runtime, and nobody else is going to include a new llvm header, perhaps simply clearly labeling the interface (functions, tooltype, shadow consts) in a section at the top of EfficiencySanitizer.cpp is the best option.
IMHO, there are two possible benefits of adding a header file here:
1. avoid duplication
2. clear separation/documentation about inter-dependencies between runtime.

However, since we do not include an llvm header,
we cannot avoid duplication.

And there is only one EfficiencySanitizer.cpp, it looks to me that a clear section about the duplicated part is easier to track than a separate header file.


http://reviews.llvm.org/D20483