[PATCH] D20590: [esan|cfrag] Add struct infomation printing for the cfrag tool.

[Qin Zhao via llvm-commits]

zhaoqin added inline comments.

================
Comment at: lib/esan/cache_frag.cpp:23
@@ +22,3 @@
+// This should be kept consistent with LLVM's EfficiencySanitizer StructInfoTy.
+struct StructInfoTy {
+  const char *StructName;
----------------
filcab wrote:
> No need to have `Ty` in the name.
It was from my habit of using struct name_t in C.
Removed.

================
Comment at: lib/esan/cache_frag.cpp:25
@@ +24,3 @@
+  const char *StructName;
+  u32 NumOfFields;
+  u64 *Counters;
----------------
filcab wrote:
> Maybe put the `u32` at the end?
> It'll end up being the same wasted space, but it might be easier to extend and keep binary compatibility, if we want (unsure we'd want that, though).
Why u32 at the end would keep binary compatibility? Won't anything append after won't change this layout? The only benefit I can see is some potential space saving in the future.

Adding ToolType back to ToolInfo/CacheFragInfo would be the most likely change in the future, and it would break any compatibility, so I won't worry too much about this.

================
Comment at: lib/esan/cache_frag.cpp:38
@@ -25,1 +37,3 @@
 
+static void printStructInfo(CacheFragTy *CacheFrag) {
+  // We print StructInfo for debugging purpose.
----------------
filcab wrote:
> aizatsky wrote:
> > Not sure about this, but maybe add "print" member methods to structs?
> +1
I am not sure if I want to add anything to the struct created from the ESan pass.
It may increase the chance that the struct layout defined in the runtime is different from the struct layout defined in the compiler.
I would rather delete this routine as it is mainly for debugging.

================
Comment at: lib/esan/cache_frag.cpp:40
@@ +39,3 @@
+  // We print StructInfo for debugging purpose.
+  if ((uptr)Verbosity() >= 3) {
+    for (u32 i = 0; i < CacheFrag->NumOfStructs; ++i) {
----------------
aizatsky wrote:
> why (uptr) casts?
From other examples.

lib/sanitizer_common/sanitizer_common.h:INLINE int Verbosity() {
lib/sanitizer_common/sanitizer_common.h:    if ((uptr)Verbosity() >= (level)) Report(__VA_ARGS__); \
lib/sanitizer_common/sanitizer_common.h:    if ((uptr)Verbosity() >= (level)) Printf(__VA_ARGS__); \


http://reviews.llvm.org/D20590