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

[Joey Gouly]

Thanks, committed as r168510!

________________________________________
From: metafoo at gmail.com [metafoo at gmail.com] On Behalf Of Richard Smith [richard at metafoo.co.uk]
Sent: 22 November 2012 20:55
To: Alexey Samsonov
Cc: Joey Gouly; Nuno Lopes; Richard Smith; cfe-commits at cs.uiuc.edu
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" <samsonov at google.com<mailto:samsonov at google.com>> wrote:
Hi Joey,

On Thu, Nov 22, 2012 at 4:20 PM, Joey Gouly <joey.gouly at arm.com<mailto: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<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<mailto: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<mailto: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<mailto:joey.gouly at arm.com>>
> To: <cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>>; <richard at metafoo.co.uk<mailto: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<mailto:cfe-commits at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits




--
Alexey Samsonov, MSK



--
Alexey Samsonov, MSK