Other then the includes the patch looks great. Two comments which you may want to address in this or future patches: 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. sanitizer_common has Printf which you may want to use instead of fprintf(stderr, ..), for consistency and to allow redirecting to another file.
--kcc On Fri, Oct 5, 2012 at 12:03 PM, Kostya Serebryany <[email protected]> wrote: > > > On Thu, Oct 4, 2012 at 10:48 PM, Richard Smith <[email protected]>wrote: > >> On Thu, Oct 4, 2012 at 3:36 AM, Kostya Serebryany <[email protected]> wrote: >> >>> >>> asan/tsan/msan generally avoid #including system headers, especially in >>> .h files. >>> Once you start porting the code to non-linux, you'll know why. >>> compiler-rt/lib/sanitizer_common contains lots of useful stuff that >>> allows you to avoid using system headers. >>> WDYT? >>> >> >> I was trying to steer clear of most system headers, but I've tried a bit >> harder now; new patch attached. >> > Cool! > > >> >> 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. >> > > Mmm. Even with those two we had issues on windows, which forced us to > have include/sanitizer/common_interface_defs.h > Again, just for consistency (and for future windows porting) you may want > to use common_interface_defs.h instead of system headers. > > >> 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. >> > > >> 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? > No (other than consistency). > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
