<div class="gmail_quote">On Tue, Aug 14, 2012 at 1:41 AM, Kostya Serebryany <span dir="ltr"><<a 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">
<br><br><div class="gmail_quote"><div class="im">On Sat, Aug 11, 2012 at 6:48 AM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br></div>
<div><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>Hi,</div><div><br></div><div>There are three different (and mostly orthogonal, design-wise) areas where I would like to make improvements to Clang's undefined behavior checking:</div>
<div><br></div><div>1) Completeness of checks. There are integer undefined behaviors which -ftrapv and -fcatch-undefined-behavior don't catch, and there is almost no checking available for any other undefined behaviors outside of those and the ones caught by {Address,Thread,Memory} Sanitizer.</div>
<div><br></div><div>I would like to add support for checking the following additional undefined behaviors, which Clang does not currently appear to be able to check for:</div><div><br></div><div> - Using a null pointer or empty glvalue (a dereferenced null pointer) as the object expression in a class member access expression or member function call.</div>
<div> - Using an object of polymorphic class type where either the static and dynamic types are incompatible or the object's lifetime has not started or has ended, in a delete-expression, class member access expression, member function call, derived-to-virtual-base conversion, dynamic_cast, or typeid, or using the value of a reference bound to such a glvalue.</div>
<div> - Base-to-derived cast on wrong polymorphic type</div><div> - Binding a reference to an inappropriately-aligned address or to an empty lvalue</div><div> - VLA size evaluates to a nonpositive value</div><div> - Reaching the end of a function with a non-void return type, in C++ (in C, this is only UB if the caller uses the return value)</div>
<div> - Overflow in conversion from floating point to smaller floating point or to integral type</div><div> - Overflow in conversion from integral type to floating point (int128 to float, or when converting to __fp16)</div>
<div> - Converting an out-of-range integer to an enumerated type</div><div> - Floating-point arithmetic overflow</div><div> - Pointer arithmetic which leaves the bounds which can be determined by llvm.objectsize</div><div>
- Pointer subtraction between pointers which can be determined to be in different objects</div><div> - Signed << where the LHS is negative, or where the result doesn't fit in the result type (C99, C11) or doesn't fit in the corresponding unsigned type (C++11)</div>
<div> - Assignment where the LHS and RHS overlap but are not equal</div><div><br></div><div>Regehr et al's IOC has code to handle a few of these checks already, which should be straightforward to apply to Clang trunk (assuming the IOC guys are still happy with that?). Low overhead would be an explicit goal of all of these checks.</div>
<div><br></div><div><br></div><div>2) Command-line interface. We currently have the following options to enable various flavors of undefined behavior checks:</div><div> -ftrapv</div><div> -fcatch-undefined-behavior</div>
<div> -faddress-sanitizer</div><div> -fthread-sanitizer</div><div><br></div><div>I would like for us to have a single argument which enables all undefined behavior checking which can be performed cheaply and with no changes to address space layout or ABI; I propose we extend -fcatch-undefined-behavior to be that argument. IOC adds the -ftrapv checks to -fcatch-undefined-behavior (unless we're in -fwrapv mode), so we can easily pick up that part of this change.</div>
<div><br></div><div>IOC also adds:</div><div> -fcatch-undefined-ansic-behavior</div><div> -fcatch-undefined-c99-behavior</div><div> -fcatch-undefined-cxx0x-behavior</div><div> -fcatch-undefined-cxx98-behavior</div><div>
-fcatch-undefined-nonarith-behavior</div><div><br></div><div>I think we should support this kind of configuration through a mechanism similar to warning flags: -fcatch-undefined-behavior=c++11 -fno-catch-undefined-behavior=null-reference, for instance.</div>
<div><br></div><div>Also, I think we should consider renaming this switch (and/or possibly the -f*-sanitizer switches) to provide a consistent set of names, but I don't have a concrete proposal for that.</div><div>
<br></div><div><br></div><div><div>3) Handling and reporting of undefined behavior, once caught. The sanitizers produce decent information (which is both useful and detailed), but it could be prettier. </div></div></blockquote>
<div><br></div></div></div><div>Can you be more specific? </div></div></blockquote><div><br></div><div>Sure. Here's a few things which I think could be improved:</div><div><br></div><div>* The point at which the error is trapped doesn't include a column number. (I'd also like a code snippet with a caret, where possible, but there are tradeoffs to be carefully considered there...)</div>
<div><br></div><div>* Symbols don't get demangled (I know, this is WIP).</div><div><br></div><div>* The shadow byte output is currently only really useful if you understand how ASAN works in a reasonable amount of detail. We don't provide any explanation of what they mean in the output, and even the documentation doesn't explain what the various redzone values mean. I think we can do a lot better on this particular point; I gave some sample output in my original message which shows the direction I'm thinking of.</div>
<div><br></div><div>* The 'stats' output does not seem relevant in the case where ASAN traps an issue.</div><div><br></div><div>* There seems to be a tension between producing extremely detailed output (including things like bp and sp), which would be useful for a failure in a long-running process, and producing readable output (omitting such things, which are usually not interesting, and are redundant if the problem can be reproduced in a debugger). I'm not really sure what the right tradeoff is here. I certainly don't want to remove any useful information from the output.</div>
<div><br></div><div>In essence, I'd like the ASAN output to be as accessible as our compile-time diagnostics. And yes, I am volunteering to help resolve the above points, if you agree that we should improve these things.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div>IOC is in some ways quite ambitious here, and provides options to not only diagnose a problem, but also to call a function which can produce a value for the operation and recover. I'm not proposing adding such a feature to Clang at this time (I'll leave that to the IOC folks).</div>
<div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br></div><div>I would like to propose adding a runtime library for the -fcatch-undefined-behavior checks, and emit calls to relevant diagnostic functions instead of calls to @llvm.trap. I'm planning on having one library entry point per flavor of check, named __ubsan_report_<check_name>, which would be passed relevant operand values, and a pointer to a structure containing information about the context (file, line and column number, operand types as strings, and possibly a code snippet to display with the diagnostic).</div>
</blockquote><div><br></div></div><div>Do you plan to have these library functions to be marked as never-return? (if not, it will cost quite a bit of performance)</div></div></blockquote><div><br></div><div>Yes, that was certainly my initial plan (although John Regehr has suggested that there may be value in being able to continue after such a check failure). I would especially welcome your input into ways to emit the checks in IR, and how to design the runtime library interface, to minimize the performance burden of the checks, since I know you've done a lot of research into that.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div>Also note, that asan/tsan use the debug info to get file/line info. Do you plan to do the same, or you want to pass this info explicitly to the callback? (this will cost additional binary size and maybe some perf)</div>
</div></blockquote><div><br></div><div>I would like to preserve more information than can easily be extracted from the debug info (in particular, line numbers and column ranges for operands of problematic operations), so I'm intending on passing that info explicitly to the callback, but naturally if the measured impact of this is too high, I'll need to reconsider.</div>
<div><br></div><div>I think the perf impact can be mitigated by forcing the check call out of the hot path, either through just block placement, or by pushing the extra arg emission into a separate noinline function, which would be called on a check failure with a minimal set of arguments. Something like:</div>
<div><br></div><div><div>int f(int x, int y) {</div><div> return x << y;</div><div>}</div></div><div>=></div><div>linkonce_odr unnamed_addr const char __ubsan_str_8dfd1c41b32683bab10e108ac2851f2a = " return x << y;\n";</div>
<div><div>__attribute__((noreturn, noinline)) static void __ubsan_check_failure_1(int a, int b) {</div></div><div><div> static const struct LocationInfo __ubsan_locs_1[] = {</div><div><div> __FILE__, 2, 11, 12, // '<<'</div>
<div> __FILE__, 2, 9, 9, // LHS</div><div> __FILE__, 2, 14, 14 // RHS</div></div><div> };</div></div><div> __ubsan_report_left_shift_out_of_range(a, b, 32, "int", __ubsan_locs_1, str_8dfd1c41b32683bab10e108ac2851f2a);</div>
<div><div>}</div></div><div>int f(int x, int y) {</div><div> if (__builtin_expect((unsigned)y >= 32 || ((unsigned)x >> (31-y)), 0))</div><div> __ubsan_check_failure_1(x, y);</div><div><div> return x << y;</div>
<div>}</div></div></div>