[PATCH] D21753: Comprehensive Static Instrumentation (2/2): Clang flag

[Derek Bruening via cfe-commits]

bruening added inline comments.

================
Comment at: docs/CSI.rst:75
@@ +74,3 @@
+  % clang++ -c -O3 -g -fcsi -emit-llvm bar.cpp -o bar.o
+  % clang++ foo.o bar.o my-tool.o libclang_rt.csi-x86_64.a -fuse-ld=gold -flto -lrt -ldl -o foo
+
----------------
See below: the sanitizers pass the -f flag (-fcsi here) to the link line and have the library automatically linked in, which is a simpler usage model than the user having to name this static library explicitly.

================
Comment at: docs/CSI.rst:78
@@ +77,3 @@
+Notice that in the final stage of linking, the tool user also needs to link in
+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>``
----------------
This should not be necessary: as mentioned above, if -fcsi is passed to the link line you should be able to have clang automatically add the static csi library, just like is done for the sanitizers.

================
Comment at: docs/CSI.rst:81
@@ +80,3 @@
+directory. We plan to investigate means of linking with the runtime
+automatically in the future, but for the time being, the tool user should link
+it in explicitly.
----------------
Hmm, see above comments: is this already implemented and was deliberately split from this one for simplicity?

================
Comment at: docs/CSI.rst:97
@@ +96,3 @@
+* functions,
+* function exits,
+* basic blocks,
----------------
Wouldn't the after-hook here be the same as the after-hook for the function category?  Generally the reason to have a post-function or function-exit hook would be to view or change the return value: couldn't that be done equally easily from a post-function hook at the instruction after the call site?  I guess I'm asking why this is a separate category.

================
Comment at: docs/CSI.rst:99
@@ +98,3 @@
+* basic blocks,
+* call sites,
+* loads, and
----------------
It seems like there is some redundancy here?  This seems very similar to the "functions" category: I'm curious as to why they are separate?

================
Comment at: docs/CSI.rst:164
@@ +163,3 @@
+library loads.
+
+
----------------
Some tools also need thread-local handling: do you plan to provide thread initialization and exit hooks in the future?

================
Comment at: docs/CSI.rst:164
@@ +163,3 @@
+library loads.
+
+
----------------
bruening wrote:
> Some tools also need thread-local handling: do you plan to provide thread initialization and exit hooks in the future?
What about a fini or destructor function called at program exit?  Many profiling or analysis tools gather data and want to report it or dump it to a file at program exit.

================
Comment at: docs/CSI.rst:173
@@ +172,3 @@
+
+  void __csi_func_entry(const csi_id_t func_id);
+  void __csi_func_exit(const csi_id_t func_exit_id, const csi_id_t func_id);
----------------
Generally, tools that hook application functions want to examine the arguments.  How does a hook access (or modify) the application function's arguments?

================
Comment at: docs/CSI.rst:174
@@ +173,3 @@
+  void __csi_func_entry(const csi_id_t func_id);
+  void __csi_func_exit(const csi_id_t func_exit_id, const csi_id_t func_id);
+
----------------
Similarly, how does a hook access or change the return value?

================
Comment at: docs/CSI.rst:181
@@ +180,3 @@
+hook ``__csi_func_exit`` is invoked just before the function returns
+normally).  (We have not yet defined the API for exceptions.)
+The ``func_exit_id`` parameter allows the tool writer to distinguish the
----------------
s/normally)/normally/

================
Comment at: docs/CSI.rst:189
@@ +188,3 @@
+
+CSI also provide instrumentation hooks basic block entry and exit.
+A basic block consists of strands of instructions with no incoming branches
----------------
Grammar: provides, for

================
Comment at: docs/CSI.rst:212
@@ +211,3 @@
+
+CSI provides the following hooks for call sites:
+
----------------
See above: I'm not sure why both call sites and function entry hooks are needed?  Perhaps there could be some explanation of that here.

================
Comment at: docs/CSI.rst:221
@@ +220,3 @@
+parameter identifies the called function.  Note that it may not always be
+possible to CSI to produce the function ID corresponds to the called function
+statically --- for example, if a function is called indirectly
----------------
Grammar: s/to CSI/for CSI/; s/ID/ID that/

================
Comment at: docs/CSI.rst:258
@@ +257,3 @@
+plan to extend the CSI to include more property values and incorporate property
+into other types of hooks.
+
----------------
Hmm, there seems to be a missing feature in this interface design in general: static analysis or static operation of some kind.  Tools often want to take one action if a memory address is aligned, but a different one if it's not aligned (usually a fastpath when aligned and a slowpath when unaligned).  The compiler often knows whether a load or store is aligned, yet this interface forces the code that checks alignment and acts on it to be executed every single time at runtime, rather than executed just once statically.  I guess this is just an inherent limitation of this interface approach -- perhaps it could be discussed in the limitations section?

================
Comment at: docs/CSI.rst:264
@@ +263,3 @@
+CSI provides a front-end data (FED) table for each type of
+program objects to allow a tool to easily relate runtime events back to
+locations in the source code.  The FED tables are indexed by the program
----------------
Grammar: s/objects/object/

================
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),
----------------
s/that,/that/

================
Comment at: docs/CSI.rst:318
@@ +317,3 @@
+* CSI currently does not support instrumentation for exceptions and C++11 atomics.
+
+
----------------
High-level comments:

Without inlining of code in these function calls, it is hard to see how a high-performance tool can be built.  Something like ThreadSanitizer that does little or no inlined instrumentation and lives with high overhead could fit into this interface, but most tools will just not work well with this callout-only no-static-analysis interface: I would expect an order of magnitude performance loss or higher for other sanitizers or similar tools.  Are there plans to extend the interface to allow it to become a shared infrastructure for the existing sanitizers?  That would require large changes to the interface, it seems.  Maybe this is more of a comment for the RFC.

If the interface is not concerned with performance, and does not seem to be leveraging much compiler information or static analysis, I would have to step back and ask: what advantage would a tool writer gain from using CSI versus a pure-dynamic tool like Pin or DynamoRIO?  In these dynamic tool platforms, every hook here is also available, and such dynamic tools will operate on any binary including third-party libraries not amenable to recompilation.  I guess I would expect a compiler tool interface to be taking more advantage of the compiler, but I don't see much discussion here of future extensions to accomplish that.  Should there be any discussion in these docs as to advantages and disadvantages versus other tool platforms?

================
Comment at: docs/CSI.rst:333
@@ +332,3 @@
+  information known at compile time, such as whether a memory access is a
+  constant, whether a variable accessed is captured, and such.
+
----------------
OK, so this is a partial answer to the long previous comment.

================
Comment at: test/Lexer/has_feature_comprehensive_static_instrumentation.cpp:11
@@ +10,2 @@
+// CHECK-CSI: CsiEnabled
+// CHECK-NO-CSI: CsiDisabled
----------------
I think we also want a test/Driver/fcsi test that checks platforms by ensuring that -fcsi is reported as an unsupported option for other than Linux x86_64.  If the instrumentation always adds a call to some symbol in the runtime library it could also have a sanity check for that.


Repository:
  rL LLVM

http://reviews.llvm.org/D21753