<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 6/26/12 4:18 AM, Kostya Serebryany
wrote:<br>
</div>
<blockquote
cite="mid:CAN=P9pjhNMVS489Dx9MKiPo70ZzJYgCPK44eJKUZ65xuFJ8Uhw@mail.gmail.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
<div style="font-family: arial,helvetica,sans-serif;"><font
size="2">Updated the patch to use the new interface for TLS
(GlobalVariable::GeneralDynamicTLSModel).
<div>Comments? <br>
</div>
</font></div>
</blockquote>
<br>
First, there's the non-technical question of whether this belongs in
LLVM mainline or not in its current state. Do you have users of
this tool in the same way that you had users of ASan? What
platforms have you tested this tool on? Do you get false positives
when you link with native code libraries, and if so, is this false
positive rate low? I get the impression that this tool isn't as
mature as TSan or ASan, so I'm wondering whether it's really ready
to go in yet.<br>
<br>
Second, on the technical side, I think dealing with the external
code problem has to be addressed before the code can be committed; I
suspect the tool will return too many false positives if you don't
address it (if you have experiments indicating otherwise, please let
us know). I'm assuming from the comment at the beginning of the
pass's implementation that it's still unaddressed.<br>
<br>
I think instrumenting all library code is unrealistic; you'd have to
have every library on your system compiled with the sanitizer (at
least for large GUI applications). I think there's a few other
approaches that you can take:<br>
<br>
1) Assume external code works properly. To do this, you first
ensure that memory allocated by external code is always marked as
initialized. Second, when passing memory objects from internal,
instrumented code to external library code, you mark the memory as
initialized (since you're assuming the external code will initialize
it).<br>
<br>
Of course, you can do things like special casing the C library to
reduce false negatives, and you can instruct users to compile
library code with the compiler to find more bugs.<br>
<br>
2) Use something like SAFECode's CompleteChecks pass to determine
which checks are on memory objects handled completely within the
analyzed portion of the program and which checks can access memory
handled by external code. Checks on memory objects handled by
external code always pass (or just generate a warning instead of an
error, or something like that). This approach requires a good
points-to analysis algorithm. SAFECode uses DSA which is
unification-based. You can either use that or, if you prefer, use a
subset based algorithm like Anderson's. I think there's one
implementation of Anderson's for LLVM out there, but I don't know
how robust it is.<br>
<br>
3) Use dynamic binary instrumentation to handle external code.<br>
<br>
The first option is probably the easiest to implement. The second
option requires a points-to analysis that can go into LLVM
mainline. The third option is a fair amount of work but probably
easier than using points-to analysis and will probably yield a
better debugging tool.<br>
<br>
Third, other technical nit-picks:<br>
<br>
1) IMHO, in general, every method and function should have a comment
explaining what it does and the meaning of its inputs and return
values. Descriptions should use complete sentences.<br>
<br>
2) You may want to consider using a class derived from InstVisitor
instead of writing large if/then/else or switch code.<br>
<br>
3) If I understand correctly, a "shadow instruction" propagates
information about initialization status through a computation (so,
for example, if you have a c = add a, b instruction, there's a
shadow instruction that computes c's initialization status based on
a and b's initialization status). It looks like you add shadow
instructions for all instructions and then "fill in" the operands to
the shadow instructions by iterating over the instructions a second
time. Is this correct?<br>
<br>
If so, there's an alternative approach: if you fetch the dominator
tree for the function and visit instructions in dominator tree
order, then you'll visit all definitions before all uses. That will
alleviate the need to make two passes over all the instructions.
The only tricky part is phi nodes, but for those, you add shadow
instructions (phi nodes with undef operands) to them first and then
visit all the other instructions in dominator tree order; you fill
in the operands of the shadow phi nodes at the end.<br>
<br>
I'm not sure which approach is faster (I don't think LLVM's
dominator tree pass is the most efficient implementation), but it's
something to think about.<br>
<br>
-- John T.<br>
<br>
<blockquote
cite="mid:CAN=P9pjhNMVS489Dx9MKiPo70ZzJYgCPK44eJKUZ65xuFJ8Uhw@mail.gmail.com"
type="cite">
<div style="font-family: arial, helvetica, sans-serif"><font
size="2">
<div><br>
</div>
<div><br>
<br>
<div class="gmail_quote">
On Fri, Jun 22, 2012 at 8:26 PM, Kostya Serebryany <span
dir="ltr"><<a moz-do-not-send="true"
href="mailto:kcc@google.com" target="_blank">kcc@google.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="font-family:arial,helvetica,sans-serif"><font
size="2">Any comments? Objections?
<div>May I commit with post-commit review? </div>
<div><br>
</div>
<div>--kcc
<div>
<div class="h5"><br>
<br>
<div class="gmail_quote">
On Tue, Jun 19, 2012 at 11:33 AM, Kostya
Serebryany <span dir="ltr"><<a
moz-do-not-send="true"
href="mailto:kcc@google.com"
target="_blank">kcc@google.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote"
style="margin:0 0 0 .8ex;border-left:1px
#ccc solid;padding-left:1ex">
<div
style="font-family:arial,helvetica,sans-serif"><font
size="2">Hello,
<div><br>
</div>
<div>Please review the following patch
which adds MemorySanitizer
instrumentation pass. </div>
<div>For the detailed description see <a
moz-do-not-send="true"
href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2012-June/050821.html"
target="_blank">http://lists.cs.uiuc.edu/pipermail/llvmdev/2012-June/050821.html</a></div>
<div>The patch contains
instrumentation pass itself and
basic LLVM-ish tests. </div>
<div>Clang flag (-fmemory-sanitizer),
the run-time library and more tests
will follow. </div>
<div><br>
</div>
<div>Code review URL: <a
moz-do-not-send="true"
href="http://codereview.appspot.com/6298091/"
target="_blank">http://codereview.appspot.com/6298091/</a></div>
<div><br>
</div>
<div>Thanks, </div>
<div><br>
</div>
<div>--kcc </div>
</font></div>
</blockquote>
</div>
<br>
</div>
</div>
</div>
</font></div>
</blockquote>
</div>
<br>
</div>
</font></div>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
llvm-commits mailing list
<a class="moz-txt-link-abbreviated" href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>
<a class="moz-txt-link-freetext" href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a>
</pre>
</blockquote>
<br>
<br>
</body>
</html>