[PATCH] D21446: Comprehensive static instrumentation (2/3): Clang support

Thu Jun 16 14:04:58 PDT 2016

kcc added inline comments.

================
Comment at: docs/CSI.rst:7
@@ +6,3 @@
+
+CSI:LLVM is a framework providing comprehensive static instrumentation via the
+compiler in order to simplify the task of building efficient and effective
----------------
The intro paragraph is important to attract users. 
That's up to you, but I would try to make it shorter. 
E.g. 
"framework providing comprehensive static instrumentation via the compiler"
might be replaced with 
"framework for comprehensive compile-time instrumentation"
etc. 

On the other hand, the intro lacks a single most important sentence, in bold, explaining why CSI is cool. Something like "With CSI the tool writers will not need to hack the compiler"

================
Comment at: docs/CSI.rst:22
@@ +21,3 @@
+To ensure high performance of CSI tools, CSI:LLVM ideally should be configured
+to enable link-time optimization (LTO), and the GNU ``gold linker`` is a
+prerequisite for enabling LTO for CSI:LLVM.  (See
----------------
gold is not the only linker that supports LTO. 
Say something like "linker needs to support LTO [link]"

================
Comment at: docs/CSI.rst:29
@@ +28,3 @@
+
+To create a CSI tool, add ``#include <csi.h>`` at the top of the tool source
+and implement function bodies for the hooks relevant to the tool.
----------------
Here you may want a bit more detail. 
E.g. "a simple CSI tool may consist of a single C++ file".  

================
Comment at: docs/CSI.rst:38
@@ +37,3 @@
+
+  % clang++ -c -emit-llvm null-tool.c -o null-tool.o
+  % clang++ -c -emit-llvm my-tool.cpp -o my-tool.o
----------------
-emit-llvm here is confusing, at least for me. 
Either explain why you need it here (to combine real object file with LLVM) or find another way...

Ideally, this and linking the null tool should be hidden under -fcsi or some such,
but this could be done in later patches. 

================
Comment at: docs/CSI.rst:48
@@ +47,3 @@
+
+The LLVM/Clang used to build the tool does not have to be CSI:LLVM, as long
+as it generates LLVM bitcode compatible with CSI:LLVM.
----------------
Even if this is true, the statement is a bit risky. 
What if the LLVM bit code changes? 
I would omit this unless you really need it. 

================
Comment at: docs/CSI.rst:58
@@ +57,3 @@
+
+* Modify paths in the build process to point to CSI:LLVM (including its Clang
+  driver).
----------------
Too complex. Just tell the user that they need to use fresh clang. 

================
Comment at: docs/CSI.rst:61
@@ +60,3 @@
+* When building object files for the TIX, pass additional arguments ``-fcsi``
+  and ``-emit-llvm`` to the Clang driver, which produces CSI instrumented
+  object files.
----------------
-emit-llvm again. It should be hidden inside -fcsi. 
It's ok to fix it later, but then please explain in the docs that it's temporary

================
Comment at: docs/CSI.rst:64
@@ +63,3 @@
+* During the linking stage for the TIX, add additional arguments
+  ``-fuse-ld=gold`` and ``-flto`` and add the tool object file (e.g.
+  ``my-tool.o``) to be statically linked to the TIX.
----------------
again, gold is not the only LTO-enabled linker. 

================
Comment at: docs/CSI.rst:79
@@ +78,3 @@
+the static library of the CSI runtime to produce the final TIX.  The runtime
+archive is distributed under the ``build/lib/clang/<VERSION>/lib/<OS>``
+directory. We plan to investigate means of linking with the runtime
----------------
this too needs to be hidden under -fcsi

================
Comment at: docs/CSI.rst:125
@@ +124,3 @@
+  // Value representing unknown CSI ID
+  #define UNKNOWN_CSI_ID ((csi_id_t)-1)
+
----------------
avoid the C preprocessor when you can (const var is usually better)

================
Comment at: docs/CSI.rst:148
@@ +147,3 @@
+``init_priority`` attribute), however, the execution order of that constructor
+relative to ``__csi_init`` is undefined.
+
----------------
On Linux, there is preinit_array... Just saying... 

================
Comment at: docs/CSI.rst:196
@@ +195,3 @@
+
+ void __csi_bb_entry(const csi_id_t bb_id);
+ void __csi_bb_exit(const csi_id_t bb_id);
----------------
didn't you want properties here? 
E.g. I have one use case for bb_entry property: whether this BB is important for coverage instrumentation or may be omitted. 

(there are plenty of papers on this, e.g. http://users.sdsc.edu/~mtikir/publications/papers/issta02.pdf)

================
Comment at: docs/CSI.rst:216
@@ +215,3 @@
+
+  void __csi_before_call(const csi_id_t call_id, const csi_id_t func_id);
+  void __csi_after_call(const csi_id_t call_id, const csi_id_t func_id);
----------------
maybe use callee_id instead of func_id?

================
Comment at: docs/CSI.rst:245
@@ +244,3 @@
+  // the same basic block.
+  #define CSI_PROP_LOAD_READ_BEFORE_WRITE_IN_BB 0x1
+
----------------
avoid #defines.
const or enum should be enough

================
Comment at: docs/CSI.rst:266
@@ +265,3 @@
+locations in the source code.  The FED tables are indexed by the program
+object's ID.  The accessors for the FED tables are shown below:
+
----------------
ask around, and look in other places (esan?) if there is something like this already
(other than DWARF, of course).
FED might be useful for other tools, but there is a chance it's already implemented 
somewhere (we have something similar, but more specialized in the sanitizers)

================
Comment at: docs/CSI.rst:271
@@ +270,3 @@
+  typedef struct {
+    char * filename;
+    int32_t line_number;
----------------
const char ?

================
Comment at: docs/CSI.rst:294
@@ +293,3 @@
+Currently the FED tables are initialized by default, which incurs some runtime
+overhead.  We are considering providing explicit initialization calls for the
+FED tables in the future as an optimization, which allows the runtime to
----------------
or consider lazy initialization.
The smaller is the API the better 

================
Comment at: docs/CSI.rst:303
@@ +302,3 @@
+
+* One limitation to LTO is that, it cannot fully optimize dynamic libraries,
+  since dynamic libraries must be compiled as position independent code (PIC),
----------------
pcc@ wanted to comment on this. 

================
Comment at: docs/CSI.rst:312
@@ +311,3 @@
+* On systems where LTO is not used, the TIX produced by linking a program with
+  a CSI tool will still function correctly, but might not be optimized.  Null
+  hooks might not be elided, for example, meaning that linking an instrumented
----------------
Might not? Or will not? 

================
Comment at: include/clang/Basic/LangOptions.def:258
@@ -257,1 +257,3 @@
 
+LANGOPT(ComprehensiveStaticInstrumentation, 1, 0, "Turn on Comprehensive Static Instrumentation.")
+
----------------
please use 80 chars per line here and everywhere else. 

================
Comment at: lib/Driver/Tools.cpp:4944
@@ -4943,1 +4943,3 @@
 
+  if (Args.hasArg(options::OPT_fcsi)) {
+    Args.AddLastArg(CmdArgs, options::OPT_fcsi);
----------------
Most of the LLVM code does not use {} for 1-line statements like this (according to the style guide). 


Repository:
  rL LLVM

http://reviews.llvm.org/D21446



