Re: [PATCH 3/3] Fix ubsan tests by disabling of an optimization.

[Richard Biener <richard dot guenther at gmail dot com>]

On Thu, Jul 9, 2015 at 6:37 PM, Jeff Law <law@redhat.com> wrote:
> On 07/09/2015 09:41 AM, Jakub Jelinek wrote:
>>
>> On Thu, Jul 09, 2015 at 09:34:25AM -0600, Jeff Law wrote:
>>>
>>> On 07/09/2015 08:13 AM, Jakub Jelinek wrote:
>>>>
>>>> On Thu, Jul 09, 2015 at 03:56:35PM +0200, mliska wrote:
>>>>>
>>>>> ---
>>>>>   gcc/testsuite/g++.dg/ubsan/vptr-1.C | 2 +-
>>>>>   gcc/testsuite/g++.dg/ubsan/vptr-2.C | 2 +-
>>>>>   gcc/testsuite/g++.dg/ubsan/vptr-3.C | 2 +-
>>>>>   3 files changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>>
>>>> I'd actually think it would be better to give up on the
>>>> UBSAN_* internal calls in tail merging.  Those internal pass
>>>> arguments based on their gimple_location, so tail merging breaks
>>>> them.
>>>
>>> So I think the larger question here is should differences in gimple
>>> locations prevent tail merging?  I'd tend to think not, which then begs
>>> the
>>
>>
>> Generally no.
>>
>>> question, are the UBSAN calls special enough to warrant an exception?
>>
>>
>> ASAN internal calls too I suppose.  And yes, I believe they are special
>> enough to warrant an exception.  The speciality is in them being lowered
>> later on into a call that is passed as one argument a structure containing
>> file:line into derived from that location, and for those calls that info
>> is
>> very important (by using -fsanitize=address or -fsanitize=undefined
>> the user already says that he doesn't care that much about generated code
>> quality, the extra overhead is already there).  Another option would be
>> for
>> -fsanitize=address or undefined etc. to disable various optimization
>> options
>> (it does already disable some like non-null optimizations, because it is
>> essential, but I believe there is no reason not to perform tail merging
>> even with those options, as long as there are none of the problematic
>> internal calls involved, or if the locus is the same.  One could consider
>> gimple_location as yet another parameter to those internal calls, not
>> emitted directly just to avoid wasting compiler memory.
>
> I figured you'd say something along these lines :-)  Essentially the
> argument is the line numbers are absolutely core to what the sanitizers
> provide by way their diagnostics.  Getting them wrong because we tail merged
> is likely to cause huge amounts of confusion.
>
> Martin -- if you could have the existence of ASAN or UBSAN calls inhibit
> tail merging.  I guess you could potentially check the location information
> and still allow if the location information on those calls matches, but I
> doubt that's worth the effort.

But the warning on the "bogus" line will still be warranted, so user goes and
fixes it.  Then tail-merge no longer applies and he gets the warning on the
other warranted line.

So in the end tail-merging caused him to fix _two_ bugs instead of just
the single one he'd run into otherwise!

-> I don't see any issue with tail-merging those.

Richard.

> Jeff
>