Oops, hit send too early.<br><br><div class="gmail_quote">On Fri, Oct 5, 2012 at 4:08 PM, Alexey Samsonov <span dir="ltr"><<a href="mailto:samsonov@google.com" target="_blank">samsonov@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Richard,<div><br></div><div>The patches are cool</div><div><br></div><div><div>+/// \brief A description of a type.</div>
<div>+class TypeDescriptor {</div></div><div>Why don't use bitfields for TypeDescriptor? </div></blockquote><div>Or at least a comment (1 bit for sign, 15 bits for width, 16 bits for kind) etc.</div><div><br></div><div>
I think later we'll be able to re-use some of your Clang-style error messages in other sanitizer tools, which is great.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br></div><div><br></div><div><div>+FUNCTIONS.ubsan-i386 :=</div><div>+FUNCTIONS.ubsan-x86_64 :=</div><div>Do you need to define this analogous to the way we define AsanFunctions/TsanFunctions etc. I'm sure what this variable</div>
<div>means (and if it's needed at all) for "make" build system, however :(</div><div><br></div><div><br></div><br><div class="gmail_quote"><div><div class="h5">On Fri, Oct 5, 2012 at 12:28 PM, Kostya Serebryany <span dir="ltr"><<a href="mailto:kcc@google.com" target="_blank">kcc@google.com</a>></span> wrote:<br>
</div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">Other then the includes the patch looks great. <div>Two comments which you may want to address in this or future patches: </div>
<div> sanitizer_common has all required code to unwind and print symbolized stack. It might be useful for environments that don't have stack unwinder/symbolizer on SIGILL. </div>
<div> sanitizer_common has Printf which you may want to use instead of fprintf(stderr, ..), for consistency and to allow redirecting to another file. </div><div><br></div><div>--kcc <div><div><br><br><div class="gmail_quote">
On Fri, Oct 5, 2012 at 12:03 PM, 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>On Thu, Oct 4, 2012 at 10:48 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>On Thu, Oct 4, 2012 at 3:36 AM, Kostya Serebryany <span dir="ltr"><<a href="mailto:kcc@google.com" target="_blank">kcc@google.com</a>></span> wrote:<br></div><div class="gmail_quote"><div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br></div><div>asan/tsan/msan generally avoid #including system headers, especially in .h files. </div><div>Once you start porting the code to non-linux, you'll know why. </div><div>compiler-rt/lib/sanitizer_common contains lots of useful stuff that allows you to avoid using system headers. </div>
<div>WDYT? </div></blockquote><div><br></div></div><div>I was trying to steer clear of most system headers, but I've tried a bit harder now; new patch attached.</div></div></blockquote></div><div>Cool! </div><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><br></div><div>I'm not concerned about the includes in ubsan_diag.cc, since I intend for that code to be replaced in the medium term (and to be made user-replaceable -- some applications will want to provide their own reporting functionality). That only leaves <stdint.h> and <stddef.h>, which are both provided by Clang.</div>
</div></blockquote><div><br></div></div><div>Mmm. Even with those two we had issues on windows, which forced us to have include/sanitizer/common_interface_defs.h</div><div>Again, just for consistency (and for future windows porting) you may want to use common_interface_defs.h instead of system headers. </div>
<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> I'm not hugely interested in making the runtime build with any other compiler, since you need to have a Clang which can target your platform anyway in order for it to be useful.</div>
</div>
</blockquote></div></div><br><div><div>>> <span style="color:rgb(34,34,34);font-size:13px;font-family:arial,sans-serif">Since these symbols are already extern "C", their mangled names are just __ubsan_handle_divrem_overflow etc, so there's no difference from a debugging perspective. Is there a benefit to moving them out of the namespace?</span></div>
</div><div><span style="color:rgb(34,34,34);font-size:13px;font-family:arial,sans-serif">No (other than </span>consistency). </div><div><br></div>
</blockquote></div><br></div></div></div>
<br></div></div><div class="im">_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></div></blockquote></div><span class="HOEnZb"><font color="#888888"><br><br clear="all"><div><br></div>-- <br><div>Alexey Samsonov, MSK</div><br>
</font></span></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div>Alexey Samsonov, MSK</div><br>