Please change NeedsUbsanRT, not needsUbsanRT(). Otherwise, LGTM On 22 Nov 2012 04:32, "Alexey Samsonov" <[email protected]> wrote:
> Hi Joey, > > On Thu, Nov 22, 2012 at 4:20 PM, Joey Gouly <[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]] >> Sent: 22 November 2012 12:08 >> To: Joey Gouly >> Cc: Richard Smith; Nuno Lopes; [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]> 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]> >> > To: <[email protected]>; <[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] >> 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
