This is an archive of the discontinued LLVM Phabricator instance.

Comprehensive static instrumentation (2/3): Clang support
AbandonedPublic

Authored by tdenniston on Jun 16 2016, 12:53 PM.

Details

Summary

This diff encompasses clang support for CSI.

Diff Detail

Repository
rL LLVM

Event Timeline

tdenniston retitled this revision from to Comprehensive static instrumentation (2/3): Clang support.
tdenniston updated this object.
tdenniston added reviewers: kcc, zhaoqin, bruening.
tdenniston set the repository for this revision to rL LLVM.
tdenniston added a project: Restricted Project.
tdenniston added subscribers: cfe-commits, eugenis, pcc and 2 others.
neboat added a subscriber: neboat.Jun 16 2016, 1:41 PM
kcc added inline comments.Jun 16 2016, 2:04 PM
docs/CSI.rst
7

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"

22

gold is not the only linker that supports LTO.
Say something like "linker needs to support LTO [link]"

29

Here you may want a bit more detail.
E.g. "a simple CSI tool may consist of a single C++ file".

38

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

48

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.

58

Too complex. Just tell the user that they need to use fresh clang.

61

-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

64

again, gold is not the only LTO-enabled linker.

79

this too needs to be hidden under -fcsi

125

avoid the C preprocessor when you can (const var is usually better)

148

On Linux, there is preinit_array... Just saying...

196

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)

216

maybe use callee_id instead of func_id?

245

avoid #defines.
const or enum should be enough

266

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)

271

const char ?

294

or consider lazy initialization.
The smaller is the API the better

303

pcc@ wanted to comment on this.

312

Might not? Or will not?

include/clang/Basic/LangOptions.def
258

please use 80 chars per line here and everywhere else.

lib/Driver/Tools.cpp
4944

Most of the LLVM code does not use {} for 1-line statements like this (according to the style guide).

tdenniston abandoned this revision.Jun 27 2016, 7:54 AM

Abandoning. We are resubmitting the CSI patches in smaller increments.