Thanks, committed as r168510! ________________________________________ From: [email protected] [[email protected]] On Behalf Of Richard Smith [[email protected]] Sent: 22 November 2012 20:55 To: Alexey Samsonov Cc: Joey Gouly; Nuno Lopes; Richard Smith; [email protected] Subject: Re: [cfe-commits] [PATCH] PR14306: Move -fbounds-checking to-fsanitize=bounds
Please change NeedsUbsanRT, not needsUbsanRT(). Otherwise, LGTM On 22 Nov 2012 04:32, "Alexey Samsonov" <[email protected]<mailto:[email protected]>> wrote: Hi Joey, On Thu, Nov 22, 2012 at 4:20 PM, Joey Gouly <[email protected]<mailto:[email protected]>> wrote: Hi Alexey, I've attached just the changes to darwin-sanitizer-ld.c, look good? Yeah, that's fine. You can leave a single "// CHECK-DYN-BOUNDS-NOT: libclang_rt.ubsan_osx.a" (w/o undefined dynamic_lookup) in the last test case. -fsanitize=bounds looks good to me as well, but you should probably wait for approval from Richard or Nuno. Thanks! Thanks, Joey From: Alexey Samsonov [mailto:[email protected]<mailto:[email protected]>] Sent: 22 November 2012 12:08 To: Joey Gouly Cc: Richard Smith; Nuno Lopes; [email protected]<mailto:[email protected]> Subject: Re: [cfe-commits] [PATCH] PR14306: Move -fbounds-checking to-fsanitize=bounds Hi Joey, On Thu, Nov 22, 2012 at 3:39 PM, Joey Gouly <[email protected]<mailto:[email protected]>> wrote: Hi Richard & Nuno, > > What I'm not sure is if 'bounds' should be included in the ubsan group. > > While it also checks for undefined behaviour, it won't produce any nice > > diagnostics like the other ubsan checkers. This bound checker just crashes > > the program when it detects a violation. > > I agree that it would be preferable to produce a call into the ubsan > runtime to produce a diagnostic for this, but I don't think that needs > to block this patch. We should decide one way or the other, though, > and not link in the ubsan runtime for -fsanitize=bounds if we're not > going to use it. I was planning on looking into using ubsan for diagnostics after this initial patch. I updated the patch to not link with the ubsan-rt if -fsanitize=bounds is passed. Sounds good. Can you please also test this behavior in test/Driver/darwin-sanitizer-ld.c? Ok to commit? > Presumably there will be a followup patch to LLVM, to remove the > vestigial 'Penalty' argument; we could at that time add an argument to > the createBoundsCheckingPass function to specify how to handle a > failure. There is a patch to remove the penalty arg, it has been approved for committing, but I'm waiting for the Clang patch to land before I commit it. I have also started to look into adding a new parameter to select between traps and runtime calls. Thanks, Joey > FWIW, I would prefer if 'bounds' was included in the ubsan group. I'm just > raising the concern that not everyone may agree. > > Nuno > > ----- Original Message ----- From: "Joey Gouly" > <[email protected]<mailto:[email protected]>> > To: <[email protected]<mailto:[email protected]>>; > <[email protected]<mailto:[email protected]>> > Sent: Wednesday, November 21, 2012 11:52 AM > Subject: [cfe-commits] [PATCH] PR14306: Move -fbounds-checking > to-fsanitize=bounds > > > > Hi all, > > Attached is the patch to change the -fbounds-checking flag to > -fsanitize=bounds, and also put it under the ubsan flag as well. > > Note: I removed the bounds checking penalty parameter, but that is in a > separate patch. > > Please review! > > Thanks, > Joey _______________________________________________ cfe-commits mailing list [email protected]<mailto:[email protected]> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits -- Alexey Samsonov, MSK -- Alexey Samsonov, MSK _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
