| Message ID | 1c507aae804089fe39102391ff554d51a20ba391.1436450591.git.mliska@suse.cz |
|---|---|
| State | New |
| Headers | show |
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. Jakub
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 question, are the UBSAN calls special enough to warrant an exception? Jeff
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. Jakub
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. Jeff
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 >
On 07/10/2015 02:19 AM, Richard Biener wrote: > > But the warning on the "bogus" line will still be warranted, so user goes and > fixes it. But when the user gets the "bogus" line, he may look at the code and determine that the reported line can't possibly be executed -- so they get confused, assume the warning is bogus and ignore it. Then tail-merge no longer applies and he gets the warning on the > other warranted line. That assumes that we'd get to both paths. We may not. The paths may be totally independent. Jeff
diff --git a/gcc/testsuite/g++.dg/ubsan/vptr-1.C b/gcc/testsuite/g++.dg/ubsan/vptr-1.C index f4260c1..2183575 100644 --- a/gcc/testsuite/g++.dg/ubsan/vptr-1.C +++ b/gcc/testsuite/g++.dg/ubsan/vptr-1.C @@ -1,5 +1,5 @@ // { dg-do run { target { ilp32 || lp64 } } } -// { dg-options "-fsanitize=vptr" } +// { dg-options "-fsanitize=vptr -fno-tree-tail-merge" } struct S { diff --git a/gcc/testsuite/g++.dg/ubsan/vptr-2.C b/gcc/testsuite/g++.dg/ubsan/vptr-2.C index 2aa7046..30047f0 100644 --- a/gcc/testsuite/g++.dg/ubsan/vptr-2.C +++ b/gcc/testsuite/g++.dg/ubsan/vptr-2.C @@ -1,5 +1,5 @@ // { dg-do run { target { ilp32 || lp64 } } } -// { dg-options "-fsanitize=vptr" } +// { dg-options "-fsanitize=vptr -fno-tree-tail-merge" } struct S { diff --git a/gcc/testsuite/g++.dg/ubsan/vptr-3.C b/gcc/testsuite/g++.dg/ubsan/vptr-3.C index 916d8ef..71c52bd 100644 --- a/gcc/testsuite/g++.dg/ubsan/vptr-3.C +++ b/gcc/testsuite/g++.dg/ubsan/vptr-3.C @@ -1,5 +1,5 @@ // { dg-do run { target { ilp32 || lp64 } } } -// { dg-options "-fsanitize=vptr" } +// { dg-options "-fsanitize=vptr -fno-tree-tail-merge" } struct S {