[cfe-commits] [PATCH] PR14306: Move -fbounds-checking to-fsanitize=bounds

[Richard Smith]

Please change NeedsUbsanRT, not needsUbsanRT(). Otherwise, LGTM
On 22 Nov 2012 04:32, "Alexey Samsonov" <samsonov at google.com> wrote:

> Hi Joey,
>
> On Thu, Nov 22, 2012 at 4:20 PM, Joey Gouly <joey.gouly at arm.com> 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:samsonov at google.com]
>> Sent: 22 November 2012 12:08
>> To: Joey Gouly
>> Cc: Richard Smith; Nuno Lopes; cfe-commits at cs.uiuc.edu
>> 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 <joey.gouly at arm.com> 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" <joey.gouly at arm.com>
>> > To: <cfe-commits at cs.uiuc.edu>; <richard at metafoo.co.uk>
>> > 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
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>>
>>
>> --
>> Alexey Samsonov, MSK
>>
>
>
>
> --
> Alexey Samsonov, MSK
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121122/c356ae67/attachment.html>