Re: [PATCH] Fix PR64078
On 09/19/16 23:27, Jeff Law wrote: > On 09/19/2016 03:08 PM, Bernd Edlinger wrote: >>> >>> Would it work to break this up into distinct tests, exit()-ing from each >>> function rather than returning back to main? >>> >> >> Yes. I think how this test is designed, each function must be inlined, >> or it will fail anyway. It was for instance impossible to pass the >> ubsan test, if -fno-inline was used as RUNTESTFLAGS. > Presumably the dg-skip-if is ensuring that we're only testing with -O2 > turned on. >> >> Therefore it works as well, if main avoids to return and calls >> exit(0) instead, with a specific comment of course. >> >> See https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01985.html > That works for me. > > jeff OK, thanks. Then I will commit this on trunk and active branches. Bernd. 2016-09-22 Bernd Edlinger Tom de Vries PR testsuite/77411 * c-c++-common/ubsan/object-size-9.c: Call __builtin_exit in C++. Index: c-c++-common/ubsan/object-size-9.c === --- c-c++-common/ubsan/object-size-9.c (Revision 240355) +++ c-c++-common/ubsan/object-size-9.c (Arbeitskopie) @@ -93,5 +93,9 @@ main (void) #endif f4 (12); f5 (12); +#ifdef __cplusplus + /* Stack may be smashed by f2/f3 above. */ + __builtin_exit (0); +#endif return 0; }
Re: [PATCH] Fix PR64078
On 09/19/2016 03:08 PM, Bernd Edlinger wrote: Would it work to break this up into distinct tests, exit()-ing from each function rather than returning back to main? Yes. I think how this test is designed, each function must be inlined, or it will fail anyway. It was for instance impossible to pass the ubsan test, if -fno-inline was used as RUNTESTFLAGS. Presumably the dg-skip-if is ensuring that we're only testing with -O2 turned on. Therefore it works as well, if main avoids to return and calls exit(0) instead, with a specific comment of course. See https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01985.html That works for me. jeff
Re: [PATCH] Fix PR64078
On 09/19/16 22:19, Jeff Law wrote: > On 09/15/2016 04:29 AM, Tom de Vries wrote: >> On 31/08/16 07:42, Tom de Vries wrote: >>> On 30/08/16 11:38, Bernd Edlinger wrote: On 08/30/16 10:21, Tom de Vries wrote: > On 29/08/16 18:43, Bernd Edlinger wrote: >> Thanks! >> >> Actually my patch missed to fix one combination: -m32 with -fpic >> >> make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c >> --tool_opts >> '-m32 -fpic'" >> >> FAIL: c-c++-common/ubsan/object-size-9.c -O2 execution test >> FAIL: c-c++-common/ubsan/object-size-9.c -O2 -flto >> -fno-use-linker-plugin -flto-partition=none execution test >> >> The problem here is that the functions f2 and f3 access a stack- >> based object out of bounds and that is inlined in main and >> therefore smashes the return address of main in this case. >> >> A possible fix could look like follows: >> >> Index: object-size-9.c >> === >> --- object-size-9.c(revision 239794) >> +++ object-size-9.c(working copy) >> @@ -93,5 +93,9 @@ >> #endif >> f4 (12); >> f5 (12); >> +#ifdef __cplusplus >> + /* Stack may be smashed by f2/f3 above. */ >> + __builtin_exit (0); >> +#endif >> return 0; >> } >> >> >> Do you think that this should be fixed too? > > I think it should be fixed. Ideally, we'd prevent the out-of-bounds > writes to have harmful effects, but I'm not sure how to enforce that. > > This works for me: > ... > diff --git a/gcc/testsuite/c-c++-common/ubsan/object-size-9.c > b/gcc/testsuite/c-c++-common/ubsan/object-size-9.c > index 46f1fb9..fec920d 100644 > --- a/gcc/testsuite/c-c++-common/ubsan/object-size-9.c > +++ b/gcc/testsuite/c-c++-common/ubsan/object-size-9.c > @@ -31,6 +31,7 @@ static struct C > f2 (int i) > { > struct C x; > + struct C x2; > x.d[i] = 'z'; > return x; > } > @@ -45,6 +46,7 @@ static struct C > f3 (int i) > { > struct C x; > + struct C x2; > char *p = x.d; > p += i; > *p = 'z'; > ... > > But I have no idea how stable this solution is. > At least x2 could not be opimized away, as it is no POD, but there is no guarantee, that x2 comes after x on the stack. Another possibility, which seems to work as well: Index: gcc/testsuite/c-c++-common/ubsan/object-size-9.c === --- gcc/testsuite/c-c++-common/ubsan/object-size-9.c(revision 239794) +++ gcc/testsuite/c-c++-common/ubsan/object-size-9.c(working copy) @@ -30,7 +30,7 @@ f1 (struct T x, int i) static struct C f2 (int i) { - struct C x; + struct C x __attribute__ ((aligned(16))); x.d[i] = 'z'; return x; } @@ -44,7 +44,7 @@ f2 (int i) static struct C f3 (int i) { - struct C x; + struct C x __attribute ((aligned(16))); char *p = x.d; p += i; *p = 'z'; >>> >>> Works for me. >> >> OK for trunk, 5 & 6 branch? >> >> Thanks, >> - Tom >> >> >> 0001-Fix-object-size-9.c-with-fpic.patch >> >> >> Fix object-size-9.c with -fpic >> >> 2016-09-15 Bernd Edlinger >> Tom de Vries >> >> PR testsuite/77411 >> * c-c++-common/ubsan/object-size-9.c (f2, f3): Declare struct C >> variable >> with __attribute__((aligned(16))). > Just so I'm sure on this stuff. > > The tests exist to verify that ubsan detects the out-of-bounds writes. > ubsan isn't terminating the process, so we end up with a smashed stack? > > I fail to see how using aligned like this should consistently work. It > feels like a hack that just happens to work now. > > Would it work to break this up into distinct tests, exit()-ing from each > function rather than returning back to main? > Yes. I think how this test is designed, each function must be inlined, or it will fail anyway. It was for instance impossible to pass the ubsan test, if -fno-inline was used as RUNTESTFLAGS. Therefore it works as well, if main avoids to return and calls exit(0) instead, with a specific comment of course. See https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01985.html Bernd.
Re: [PATCH] Fix PR64078
On 09/15/2016 04:29 AM, Tom de Vries wrote: On 31/08/16 07:42, Tom de Vries wrote: On 30/08/16 11:38, Bernd Edlinger wrote: On 08/30/16 10:21, Tom de Vries wrote: On 29/08/16 18:43, Bernd Edlinger wrote: Thanks! Actually my patch missed to fix one combination: -m32 with -fpic make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c --tool_opts '-m32 -fpic'" FAIL: c-c++-common/ubsan/object-size-9.c -O2 execution test FAIL: c-c++-common/ubsan/object-size-9.c -O2 -flto -fno-use-linker-plugin -flto-partition=none execution test The problem here is that the functions f2 and f3 access a stack- based object out of bounds and that is inlined in main and therefore smashes the return address of main in this case. A possible fix could look like follows: Index: object-size-9.c === --- object-size-9.c(revision 239794) +++ object-size-9.c(working copy) @@ -93,5 +93,9 @@ #endif f4 (12); f5 (12); +#ifdef __cplusplus + /* Stack may be smashed by f2/f3 above. */ + __builtin_exit (0); +#endif return 0; } Do you think that this should be fixed too? I think it should be fixed. Ideally, we'd prevent the out-of-bounds writes to have harmful effects, but I'm not sure how to enforce that. This works for me: ... diff --git a/gcc/testsuite/c-c++-common/ubsan/object-size-9.c b/gcc/testsuite/c-c++-common/ubsan/object-size-9.c index 46f1fb9..fec920d 100644 --- a/gcc/testsuite/c-c++-common/ubsan/object-size-9.c +++ b/gcc/testsuite/c-c++-common/ubsan/object-size-9.c @@ -31,6 +31,7 @@ static struct C f2 (int i) { struct C x; + struct C x2; x.d[i] = 'z'; return x; } @@ -45,6 +46,7 @@ static struct C f3 (int i) { struct C x; + struct C x2; char *p = x.d; p += i; *p = 'z'; ... But I have no idea how stable this solution is. At least x2 could not be opimized away, as it is no POD, but there is no guarantee, that x2 comes after x on the stack. Another possibility, which seems to work as well: Index: gcc/testsuite/c-c++-common/ubsan/object-size-9.c === --- gcc/testsuite/c-c++-common/ubsan/object-size-9.c(revision 239794) +++ gcc/testsuite/c-c++-common/ubsan/object-size-9.c(working copy) @@ -30,7 +30,7 @@ f1 (struct T x, int i) static struct C f2 (int i) { - struct C x; + struct C x __attribute__ ((aligned(16))); x.d[i] = 'z'; return x; } @@ -44,7 +44,7 @@ f2 (int i) static struct C f3 (int i) { - struct C x; + struct C x __attribute ((aligned(16))); char *p = x.d; p += i; *p = 'z'; Works for me. OK for trunk, 5 & 6 branch? Thanks, - Tom 0001-Fix-object-size-9.c-with-fpic.patch Fix object-size-9.c with -fpic 2016-09-15 Bernd Edlinger Tom de Vries PR testsuite/77411 * c-c++-common/ubsan/object-size-9.c (f2, f3): Declare struct C variable with __attribute__((aligned(16))). Just so I'm sure on this stuff. The tests exist to verify that ubsan detects the out-of-bounds writes. ubsan isn't terminating the process, so we end up with a smashed stack? I fail to see how using aligned like this should consistently work. It feels like a hack that just happens to work now. Would it work to break this up into distinct tests, exit()-ing from each function rather than returning back to main? jeff
Re: [PATCH] Fix PR64078
On 31/08/16 07:42, Tom de Vries wrote: On 30/08/16 11:38, Bernd Edlinger wrote: On 08/30/16 10:21, Tom de Vries wrote: On 29/08/16 18:43, Bernd Edlinger wrote: Thanks! Actually my patch missed to fix one combination: -m32 with -fpic make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c --tool_opts '-m32 -fpic'" FAIL: c-c++-common/ubsan/object-size-9.c -O2 execution test FAIL: c-c++-common/ubsan/object-size-9.c -O2 -flto -fno-use-linker-plugin -flto-partition=none execution test The problem here is that the functions f2 and f3 access a stack- based object out of bounds and that is inlined in main and therefore smashes the return address of main in this case. A possible fix could look like follows: Index: object-size-9.c === --- object-size-9.c(revision 239794) +++ object-size-9.c(working copy) @@ -93,5 +93,9 @@ #endif f4 (12); f5 (12); +#ifdef __cplusplus + /* Stack may be smashed by f2/f3 above. */ + __builtin_exit (0); +#endif return 0; } Do you think that this should be fixed too? I think it should be fixed. Ideally, we'd prevent the out-of-bounds writes to have harmful effects, but I'm not sure how to enforce that. This works for me: ... diff --git a/gcc/testsuite/c-c++-common/ubsan/object-size-9.c b/gcc/testsuite/c-c++-common/ubsan/object-size-9.c index 46f1fb9..fec920d 100644 --- a/gcc/testsuite/c-c++-common/ubsan/object-size-9.c +++ b/gcc/testsuite/c-c++-common/ubsan/object-size-9.c @@ -31,6 +31,7 @@ static struct C f2 (int i) { struct C x; + struct C x2; x.d[i] = 'z'; return x; } @@ -45,6 +46,7 @@ static struct C f3 (int i) { struct C x; + struct C x2; char *p = x.d; p += i; *p = 'z'; ... But I have no idea how stable this solution is. At least x2 could not be opimized away, as it is no POD, but there is no guarantee, that x2 comes after x on the stack. Another possibility, which seems to work as well: Index: gcc/testsuite/c-c++-common/ubsan/object-size-9.c === --- gcc/testsuite/c-c++-common/ubsan/object-size-9.c(revision 239794) +++ gcc/testsuite/c-c++-common/ubsan/object-size-9.c(working copy) @@ -30,7 +30,7 @@ f1 (struct T x, int i) static struct C f2 (int i) { - struct C x; + struct C x __attribute__ ((aligned(16))); x.d[i] = 'z'; return x; } @@ -44,7 +44,7 @@ f2 (int i) static struct C f3 (int i) { - struct C x; + struct C x __attribute ((aligned(16))); char *p = x.d; p += i; *p = 'z'; Works for me. OK for trunk, 5 & 6 branch? Thanks, - Tom Fix object-size-9.c with -fpic 2016-09-15 Bernd Edlinger Tom de Vries PR testsuite/77411 * c-c++-common/ubsan/object-size-9.c (f2, f3): Declare struct C variable with __attribute__((aligned(16))). --- gcc/testsuite/c-c++-common/ubsan/object-size-9.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gcc/testsuite/c-c++-common/ubsan/object-size-9.c b/gcc/testsuite/c-c++-common/ubsan/object-size-9.c index 46f1fb9..4cd8529 100644 --- a/gcc/testsuite/c-c++-common/ubsan/object-size-9.c +++ b/gcc/testsuite/c-c++-common/ubsan/object-size-9.c @@ -30,7 +30,7 @@ f1 (struct T x, int i) static struct C f2 (int i) { - struct C x; + struct C x __attribute__((aligned(16))); x.d[i] = 'z'; return x; } @@ -44,7 +44,7 @@ f2 (int i) static struct C f3 (int i) { - struct C x; + struct C x __attribute__((aligned(16))); char *p = x.d; p += i; *p = 'z';
Re: [PATCH] Fix PR64078
On 30/08/16 11:38, Bernd Edlinger wrote: On 08/30/16 10:21, Tom de Vries wrote: On 29/08/16 18:43, Bernd Edlinger wrote: Thanks! Actually my patch missed to fix one combination: -m32 with -fpic make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c --tool_opts '-m32 -fpic'" FAIL: c-c++-common/ubsan/object-size-9.c -O2 execution test FAIL: c-c++-common/ubsan/object-size-9.c -O2 -flto -fno-use-linker-plugin -flto-partition=none execution test The problem here is that the functions f2 and f3 access a stack- based object out of bounds and that is inlined in main and therefore smashes the return address of main in this case. A possible fix could look like follows: Index: object-size-9.c === --- object-size-9.c(revision 239794) +++ object-size-9.c(working copy) @@ -93,5 +93,9 @@ #endif f4 (12); f5 (12); +#ifdef __cplusplus + /* Stack may be smashed by f2/f3 above. */ + __builtin_exit (0); +#endif return 0; } Do you think that this should be fixed too? I think it should be fixed. Ideally, we'd prevent the out-of-bounds writes to have harmful effects, but I'm not sure how to enforce that. This works for me: ... diff --git a/gcc/testsuite/c-c++-common/ubsan/object-size-9.c b/gcc/testsuite/c-c++-common/ubsan/object-size-9.c index 46f1fb9..fec920d 100644 --- a/gcc/testsuite/c-c++-common/ubsan/object-size-9.c +++ b/gcc/testsuite/c-c++-common/ubsan/object-size-9.c @@ -31,6 +31,7 @@ static struct C f2 (int i) { struct C x; + struct C x2; x.d[i] = 'z'; return x; } @@ -45,6 +46,7 @@ static struct C f3 (int i) { struct C x; + struct C x2; char *p = x.d; p += i; *p = 'z'; ... But I have no idea how stable this solution is. At least x2 could not be opimized away, as it is no POD, but there is no guarantee, that x2 comes after x on the stack. Another possibility, which seems to work as well: Index: gcc/testsuite/c-c++-common/ubsan/object-size-9.c === --- gcc/testsuite/c-c++-common/ubsan/object-size-9.c(revision 239794) +++ gcc/testsuite/c-c++-common/ubsan/object-size-9.c(working copy) @@ -30,7 +30,7 @@ f1 (struct T x, int i) static struct C f2 (int i) { - struct C x; + struct C x __attribute__ ((aligned(16))); x.d[i] = 'z'; return x; } @@ -44,7 +44,7 @@ f2 (int i) static struct C f3 (int i) { - struct C x; + struct C x __attribute ((aligned(16))); char *p = x.d; p += i; *p = 'z'; Works for me. Thanks, - Tom
Re: [PATCH] Fix PR64078
On 08/30/16 10:21, Tom de Vries wrote: > On 29/08/16 18:43, Bernd Edlinger wrote: >> Thanks! >> >> Actually my patch missed to fix one combination: -m32 with -fpic >> >> make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c --tool_opts >> '-m32 -fpic'" >> >> FAIL: c-c++-common/ubsan/object-size-9.c -O2 execution test >> FAIL: c-c++-common/ubsan/object-size-9.c -O2 -flto >> -fno-use-linker-plugin -flto-partition=none execution test >> >> The problem here is that the functions f2 and f3 access a stack- >> based object out of bounds and that is inlined in main and >> therefore smashes the return address of main in this case. >> >> A possible fix could look like follows: >> >> Index: object-size-9.c >> === >> --- object-size-9.c(revision 239794) >> +++ object-size-9.c(working copy) >> @@ -93,5 +93,9 @@ >> #endif >> f4 (12); >> f5 (12); >> +#ifdef __cplusplus >> + /* Stack may be smashed by f2/f3 above. */ >> + __builtin_exit (0); >> +#endif >> return 0; >> } >> >> >> Do you think that this should be fixed too? > > I think it should be fixed. Ideally, we'd prevent the out-of-bounds > writes to have harmful effects, but I'm not sure how to enforce that. > > This works for me: > ... > diff --git a/gcc/testsuite/c-c++-common/ubsan/object-size-9.c > b/gcc/testsuite/c-c++-common/ubsan/object-size-9.c > index 46f1fb9..fec920d 100644 > --- a/gcc/testsuite/c-c++-common/ubsan/object-size-9.c > +++ b/gcc/testsuite/c-c++-common/ubsan/object-size-9.c > @@ -31,6 +31,7 @@ static struct C > f2 (int i) > { > struct C x; > + struct C x2; > x.d[i] = 'z'; > return x; > } > @@ -45,6 +46,7 @@ static struct C > f3 (int i) > { > struct C x; > + struct C x2; > char *p = x.d; > p += i; > *p = 'z'; > ... > > But I have no idea how stable this solution is. > At least x2 could not be opimized away, as it is no POD, but there is no guarantee, that x2 comes after x on the stack. Another possibility, which seems to work as well: Index: gcc/testsuite/c-c++-common/ubsan/object-size-9.c === --- gcc/testsuite/c-c++-common/ubsan/object-size-9.c(revision 239794) +++ gcc/testsuite/c-c++-common/ubsan/object-size-9.c(working copy) @@ -30,7 +30,7 @@ f1 (struct T x, int i) static struct C f2 (int i) { - struct C x; + struct C x __attribute__ ((aligned(16))); x.d[i] = 'z'; return x; } @@ -44,7 +44,7 @@ f2 (int i) static struct C f3 (int i) { - struct C x; + struct C x __attribute ((aligned(16))); char *p = x.d; p += i; *p = 'z'; Thanks Bernd.
Re: [PATCH] Fix PR64078
On 29/08/16 18:43, Bernd Edlinger wrote: make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c --tool_opts '-m32 -fpic'" FAIL: c-c++-common/ubsan/object-size-9.c -O2 execution test FAIL: c-c++-common/ubsan/object-size-9.c -O2 -flto -fno-use-linker-plugin -flto-partition=none execution test Filed PR77411 - object-size-9.c -fpic -m32 failure Thanks, - Tom
Re: [PATCH] Fix PR64078
On 29/08/16 18:43, Bernd Edlinger wrote: Thanks! Actually my patch missed to fix one combination: -m32 with -fpic make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c --tool_opts '-m32 -fpic'" FAIL: c-c++-common/ubsan/object-size-9.c -O2 execution test FAIL: c-c++-common/ubsan/object-size-9.c -O2 -flto -fno-use-linker-plugin -flto-partition=none execution test The problem here is that the functions f2 and f3 access a stack- based object out of bounds and that is inlined in main and therefore smashes the return address of main in this case. A possible fix could look like follows: Index: object-size-9.c === --- object-size-9.c (revision 239794) +++ object-size-9.c (working copy) @@ -93,5 +93,9 @@ #endif f4 (12); f5 (12); +#ifdef __cplusplus + /* Stack may be smashed by f2/f3 above. */ + __builtin_exit (0); +#endif return 0; } Do you think that this should be fixed too? I think it should be fixed. Ideally, we'd prevent the out-of-bounds writes to have harmful effects, but I'm not sure how to enforce that. This works for me: ... diff --git a/gcc/testsuite/c-c++-common/ubsan/object-size-9.c b/gcc/testsuite/c-c++-common/ubsan/object-size-9.c index 46f1fb9..fec920d 100644 --- a/gcc/testsuite/c-c++-common/ubsan/object-size-9.c +++ b/gcc/testsuite/c-c++-common/ubsan/object-size-9.c @@ -31,6 +31,7 @@ static struct C f2 (int i) { struct C x; + struct C x2; x.d[i] = 'z'; return x; } @@ -45,6 +46,7 @@ static struct C f3 (int i) { struct C x; + struct C x2; char *p = x.d; p += i; *p = 'z'; ... But I have no idea how stable this solution is. Thanks, - Tom
Re: [PATCH] Fix PR64078
Thanks! Actually my patch missed to fix one combination: -m32 with -fpic make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c --tool_opts '-m32 -fpic'" FAIL: c-c++-common/ubsan/object-size-9.c -O2 execution test FAIL: c-c++-common/ubsan/object-size-9.c -O2 -flto -fno-use-linker-plugin -flto-partition=none execution test The problem here is that the functions f2 and f3 access a stack- based object out of bounds and that is inlined in main and therefore smashes the return address of main in this case. A possible fix could look like follows: Index: object-size-9.c === --- object-size-9.c (revision 239794) +++ object-size-9.c (working copy) @@ -93,5 +93,9 @@ #endif f4 (12); f5 (12); +#ifdef __cplusplus + /* Stack may be smashed by f2/f3 above. */ + __builtin_exit (0); +#endif return 0; } Do you think that this should be fixed too? Bernd. On 08/29/16 09:59, Tom de Vries wrote: > On 17/09/15 20:08, Marek Polacek wrote: >> On Thu, Sep 17, 2015 at 08:06:48PM +0200, Bernd Edlinger wrote: >>> Hi, >>> >>> On Thu, 17 Sep 2015 10:39:04, Jeff Law wrote: On 09/17/2015 09:00 AM, Marek Polacek wrote: > On Wed, Sep 09, 2015 at 07:48:15PM +0200, Bernd Edlinger wrote: >> Hi, >> >> On Wed, 9 Sep 2015 09:31:33, Jeff Law wrote: >>> You could probably make the function static or change its >>> visibility via >>> a function attribute (there's a visibility attribute which can >>> take the >>> values default, hidden protected or internal). Default visibility >>> essentially means the function can be overridden. I think >>> changing it >>> to "protected" might work. Note if we do that, we may need some >>> kind of >>> target selector on the test since not all targets support the >>> various >>> visibility attributes. >>> >> >> Yes, it works both ways: static works, and __attribute__ >> ((visibility ("protected"))) works too: >> >> make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c >> --target_board='unix{-fpic,-mcmodel=medium,-fpic\ >> -mcmodel=medium,-mcmodel=large,-fpic\ -mcmodel=large}'" >> >> has all tests passed, but.. >> >> make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c >> --target_board='unix{-fno-inline}'" >> >> still fails in the same way for all workarounds: inline, static, >> and __attribute__ ((visibility ("protected"))). >> >> Maybe "static" would be preferable? > > Yeah, I'd go with static if that helps. I'd rather avoid playing games > with visibility. Static is certainly easier and doesn't rely on targets implementing all the visibility capabilities. So static is probably the best approach. >>> >>> That's fine for me too, so is the original patch OK for trunk with >>> s/inline/static/ ? >> >> Yes. >> > > Hi, > > I've backported this fix to the 5 branch. > > Thanks, > - Tom >
Re: [PATCH] Fix PR64078
On 17/09/15 20:08, Marek Polacek wrote: On Thu, Sep 17, 2015 at 08:06:48PM +0200, Bernd Edlinger wrote: Hi, On Thu, 17 Sep 2015 10:39:04, Jeff Law wrote: On 09/17/2015 09:00 AM, Marek Polacek wrote: On Wed, Sep 09, 2015 at 07:48:15PM +0200, Bernd Edlinger wrote: Hi, On Wed, 9 Sep 2015 09:31:33, Jeff Law wrote: You could probably make the function static or change its visibility via a function attribute (there's a visibility attribute which can take the values default, hidden protected or internal). Default visibility essentially means the function can be overridden. I think changing it to "protected" might work. Note if we do that, we may need some kind of target selector on the test since not all targets support the various visibility attributes. Yes, it works both ways: static works, and __attribute__ ((visibility ("protected"))) works too: make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c --target_board='unix{-fpic,-mcmodel=medium,-fpic\ -mcmodel=medium,-mcmodel=large,-fpic\ -mcmodel=large}'" has all tests passed, but.. make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c --target_board='unix{-fno-inline}'" still fails in the same way for all workarounds: inline, static, and __attribute__ ((visibility ("protected"))). Maybe "static" would be preferable? Yeah, I'd go with static if that helps. I'd rather avoid playing games with visibility. Static is certainly easier and doesn't rely on targets implementing all the visibility capabilities. So static is probably the best approach. That's fine for me too, so is the original patch OK for trunk with s/inline/static/ ? Yes. Hi, I've backported this fix to the 5 branch. Thanks, - Tom
Re: [PATCH] Fix PR64078
On Thu, Sep 17, 2015 at 08:06:48PM +0200, Bernd Edlinger wrote: > Hi, > > On Thu, 17 Sep 2015 10:39:04, Jeff Law wrote: > > > > On 09/17/2015 09:00 AM, Marek Polacek wrote: > >> On Wed, Sep 09, 2015 at 07:48:15PM +0200, Bernd Edlinger wrote: > >>> Hi, > >>> > >>> On Wed, 9 Sep 2015 09:31:33, Jeff Law wrote: > You could probably make the function static or change its visibility via > a function attribute (there's a visibility attribute which can take the > values default, hidden protected or internal). Default visibility > essentially means the function can be overridden. I think changing it > to "protected" might work. Note if we do that, we may need some kind of > target selector on the test since not all targets support the various > visibility attributes. > > >>> > >>> Yes, it works both ways: static works, and __attribute__ ((visibility > >>> ("protected"))) works too: > >>> > >>> make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c > >>> --target_board='unix{-fpic,-mcmodel=medium,-fpic\ > >>> -mcmodel=medium,-mcmodel=large,-fpic\ -mcmodel=large}'" > >>> > >>> has all tests passed, but.. > >>> > >>> make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c > >>> --target_board='unix{-fno-inline}'" > >>> > >>> still fails in the same way for all workarounds: inline, static, and > >>> __attribute__ ((visibility ("protected"))). > >>> > >>> Maybe "static" would be preferable? > >> > >> Yeah, I'd go with static if that helps. I'd rather avoid playing games > >> with visibility. > > Static is certainly easier and doesn't rely on targets implementing all > > the visibility capabilities. So static is probably the best approach. > > > > That's fine for me too, so is the original patch OK for trunk with > s/inline/static/ ? Yes. Marek
RE: [PATCH] Fix PR64078
Hi, On Thu, 17 Sep 2015 10:39:04, Jeff Law wrote: > > On 09/17/2015 09:00 AM, Marek Polacek wrote: >> On Wed, Sep 09, 2015 at 07:48:15PM +0200, Bernd Edlinger wrote: >>> Hi, >>> >>> On Wed, 9 Sep 2015 09:31:33, Jeff Law wrote: You could probably make the function static or change its visibility via a function attribute (there's a visibility attribute which can take the values default, hidden protected or internal). Default visibility essentially means the function can be overridden. I think changing it to "protected" might work. Note if we do that, we may need some kind of target selector on the test since not all targets support the various visibility attributes. >>> >>> Yes, it works both ways: static works, and __attribute__ ((visibility >>> ("protected"))) works too: >>> >>> make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c >>> --target_board='unix{-fpic,-mcmodel=medium,-fpic\ >>> -mcmodel=medium,-mcmodel=large,-fpic\ -mcmodel=large}'" >>> >>> has all tests passed, but.. >>> >>> make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c >>> --target_board='unix{-fno-inline}'" >>> >>> still fails in the same way for all workarounds: inline, static, and >>> __attribute__ ((visibility ("protected"))). >>> >>> Maybe "static" would be preferable? >> >> Yeah, I'd go with static if that helps. I'd rather avoid playing games >> with visibility. > Static is certainly easier and doesn't rely on targets implementing all > the visibility capabilities. So static is probably the best approach. > That's fine for me too, so is the original patch OK for trunk with s/inline/static/ ? Thanks Bernd.
Re: [PATCH] Fix PR64078
On 09/17/2015 09:00 AM, Marek Polacek wrote: On Wed, Sep 09, 2015 at 07:48:15PM +0200, Bernd Edlinger wrote: Hi, On Wed, 9 Sep 2015 09:31:33, Jeff Law wrote: You could probably make the function static or change its visibility via a function attribute (there's a visibility attribute which can take the values default, hidden protected or internal). Default visibility essentially means the function can be overridden. I think changing it to "protected" might work. Note if we do that, we may need some kind of target selector on the test since not all targets support the various visibility attributes. Yes, it works both ways: static works, and __attribute__ ((visibility ("protected"))) works too: make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c --target_board='unix{-fpic,-mcmodel=medium,-fpic\ -mcmodel=medium,-mcmodel=large,-fpic\ -mcmodel=large}'" has all tests passed, but.. make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c --target_board='unix{-fno-inline}'" still fails in the same way for all workarounds: inline, static, and __attribute__ ((visibility ("protected"))). Maybe "static" would be preferable? Yeah, I'd go with static if that helps. I'd rather avoid playing games with visibility. Static is certainly easier and doesn't rely on targets implementing all the visibility capabilities. So static is probably the best approach. jeff
Re: [PATCH] Fix PR64078
On Wed, Sep 09, 2015 at 07:48:15PM +0200, Bernd Edlinger wrote: > Hi, > > On Wed, 9 Sep 2015 09:31:33, Jeff Law wrote: > > You could probably make the function static or change its visibility via > > a function attribute (there's a visibility attribute which can take the > > values default, hidden protected or internal). Default visibility > > essentially means the function can be overridden. I think changing it > > to "protected" might work. Note if we do that, we may need some kind of > > target selector on the test since not all targets support the various > > visibility attributes. > > > > Yes, it works both ways: static works, and __attribute__ ((visibility > ("protected"))) works too: > > make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c > --target_board='unix{-fpic,-mcmodel=medium,-fpic\ > -mcmodel=medium,-mcmodel=large,-fpic\ -mcmodel=large}'" > > has all tests passed, but.. > > make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c > --target_board='unix{-fno-inline}'" > > still fails in the same way for all workarounds: inline, static, and > __attribute__ ((visibility ("protected"))). > > Maybe "static" would be preferable? Yeah, I'd go with static if that helps. I'd rather avoid playing games with visibility. Marek
RE: [PATCH] Fix PR64078
Hi, On Wed, 9 Sep 2015 09:31:33, Jeff Law wrote: > You could probably make the function static or change its visibility via > a function attribute (there's a visibility attribute which can take the > values default, hidden protected or internal). Default visibility > essentially means the function can be overridden. I think changing it > to "protected" might work. Note if we do that, we may need some kind of > target selector on the test since not all targets support the various > visibility attributes. > Yes, it works both ways: static works, and __attribute__ ((visibility ("protected"))) works too: make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c --target_board='unix{-fpic,-mcmodel=medium,-fpic\ -mcmodel=medium,-mcmodel=large,-fpic\ -mcmodel=large}'" has all tests passed, but.. make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c --target_board='unix{-fno-inline}'" still fails in the same way for all workarounds: inline, static, and __attribute__ ((visibility ("protected"))). Maybe "static" would be preferable? Thanks Bernd.
Re: [PATCH] Fix PR64078
On 09/09/2015 03:10 AM, Bernd Edlinger wrote: Hi Jeff, On Tue, 8 Sep 2015 13:27:12, Jeff Law wrote: On 09/07/2015 07:46 AM, Bernd Edlinger wrote: Hi, On Mon, 7 Sep 2015 12:07:00, Marek Polacek wrote: On Sun, Sep 06, 2015 at 07:21:13PM +0200, Bernd Edlinger wrote: Hi, we observed sporadic failures of the following two test cases (see PR64078): c-c++-common/ubsan/object-size-9.c and c-c++-common/ubsan/object-size-10.c For object-size-9.c this happens in a reproducible way when -fpic option is used: If that option is used, it is slightly less desirable to inline the functions, but if an explicit "inline" is added, the function is still in-lined, even if -fpic is used. So if we rely on the function being inlined I think it would be better to add the always_inline attribute. I tried to replace inline by __attribute__((always_inline)), but unfortunately it does not work: FAIL: c-c++-common/ubsan/object-size-9.c -O2 (test for excess errors) Excess errors: /home/ed/gnu/gcc-trunk/gcc/testsuite/c-c++-common/ubsan/object-size-9.c:47:1: warning: always_inline function might not be inlinable [-Wattributes] /home/ed/gnu/gcc-trunk/gcc/testsuite/c-c++-common/ubsan/object-size-9.c:32:1: warning: always_inline function might not be inlinable [-Wattributes] /home/ed/gnu/gcc-trunk/gcc/testsuite/c-c++-common/ubsan/object-size-9.c:47:1: error: inlining failed in call to always_inline 'C f3(int)': function body can be overwritten at link time /home/ed/gnu/gcc-trunk/gcc/testsuite/c-c++-common/ubsan/object-size-9.c:94:10: error: called from here the diagnostics are just a little different when the function is inlined or not. Can't you attack this problem by making sure the function is not interposable? How could I do that? You could probably make the function static or change its visibility via a function attribute (there's a visibility attribute which can take the values default, hidden protected or internal). Default visibility essentially means the function can be overridden. I think changing it to "protected" might work. Note if we do that, we may need some kind of target selector on the test since not all targets support the various visibility attributes. jeff
RE: [PATCH] Fix PR64078
Hi Jeff, On Tue, 8 Sep 2015 13:27:12, Jeff Law wrote: > > On 09/07/2015 07:46 AM, Bernd Edlinger wrote: >> Hi, >> >> On Mon, 7 Sep 2015 12:07:00, Marek Polacek wrote: >>> >>> On Sun, Sep 06, 2015 at 07:21:13PM +0200, Bernd Edlinger wrote: Hi, we observed sporadic failures of the following two test cases (see PR64078): c-c++-common/ubsan/object-size-9.c and c-c++-common/ubsan/object-size-10.c For object-size-9.c this happens in a reproducible way when -fpic option is used: If that option is used, it is slightly less desirable to inline the functions, but if an explicit "inline" is added, the function is still in-lined, even if -fpic is used. >>> >>> So if we rely on the function being inlined I think it would be better to >>> add >>> the always_inline attribute. >>> >> >> >> I tried to replace inline by __attribute__((always_inline)), but >> unfortunately it does not work: >> >> FAIL: c-c++-common/ubsan/object-size-9.c -O2 (test for excess errors) >> Excess errors: >> /home/ed/gnu/gcc-trunk/gcc/testsuite/c-c++-common/ubsan/object-size-9.c:47:1: >> warning: always_inline function might not be inlinable [-Wattributes] >> /home/ed/gnu/gcc-trunk/gcc/testsuite/c-c++-common/ubsan/object-size-9.c:32:1: >> warning: always_inline function might not be inlinable [-Wattributes] >> /home/ed/gnu/gcc-trunk/gcc/testsuite/c-c++-common/ubsan/object-size-9.c:47:1: >> error: inlining failed in call to always_inline 'C f3(int)': function body >> can be overwritten at link time >> /home/ed/gnu/gcc-trunk/gcc/testsuite/c-c++-common/ubsan/object-size-9.c:94:10: >> error: called from here >> >> the diagnostics are just a little different when the function is inlined or >> not. > Can't you attack this problem by making sure the function is not > interposable? > How could I do that? Thanks, Bernd
Re: [PATCH] Fix PR64078
On 09/07/2015 07:46 AM, Bernd Edlinger wrote: Hi, On Mon, 7 Sep 2015 12:07:00, Marek Polacek wrote: On Sun, Sep 06, 2015 at 07:21:13PM +0200, Bernd Edlinger wrote: Hi, we observed sporadic failures of the following two test cases (see PR64078): c-c++-common/ubsan/object-size-9.c and c-c++-common/ubsan/object-size-10.c For object-size-9.c this happens in a reproducible way when -fpic option is used: If that option is used, it is slightly less desirable to inline the functions, but if an explicit "inline" is added, the function is still in-lined, even if -fpic is used. So if we rely on the function being inlined I think it would be better to add the always_inline attribute. I tried to replace inline by __attribute__((always_inline)), but unfortunately it does not work: FAIL: c-c++-common/ubsan/object-size-9.c -O2 (test for excess errors) Excess errors: /home/ed/gnu/gcc-trunk/gcc/testsuite/c-c++-common/ubsan/object-size-9.c:47:1: warning: always_inline function might not be inlinable [-Wattributes] /home/ed/gnu/gcc-trunk/gcc/testsuite/c-c++-common/ubsan/object-size-9.c:32:1: warning: always_inline function might not be inlinable [-Wattributes] /home/ed/gnu/gcc-trunk/gcc/testsuite/c-c++-common/ubsan/object-size-9.c:47:1: error: inlining failed in call to always_inline 'C f3(int)': function body can be overwritten at link time /home/ed/gnu/gcc-trunk/gcc/testsuite/c-c++-common/ubsan/object-size-9.c:94:10: error: called from here the diagnostics are just a little different when the function is inlined or not. Can't you attack this problem by making sure the function is not interposable? Jeff
RE: [PATCH] Fix PR64078
Hi, On Mon, 7 Sep 2015 12:07:00, Marek Polacek wrote: > > On Sun, Sep 06, 2015 at 07:21:13PM +0200, Bernd Edlinger wrote: >> Hi, >> >> we observed sporadic failures of the following two test cases (see PR64078): >> c-c++-common/ubsan/object-size-9.c and c-c++-common/ubsan/object-size-10.c >> >> For object-size-9.c this happens in a reproducible way when -fpic option is >> used: >> If that option is used, it is slightly less desirable to inline the >> functions, but if an explicit >> "inline" is added, the function is still in-lined, even if -fpic is used. > > So if we rely on the function being inlined I think it would be better to add > the always_inline attribute. > I tried to replace inline by __attribute__((always_inline)), but unfortunately it does not work: FAIL: c-c++-common/ubsan/object-size-9.c -O2 (test for excess errors) Excess errors: /home/ed/gnu/gcc-trunk/gcc/testsuite/c-c++-common/ubsan/object-size-9.c:47:1: warning: always_inline function might not be inlinable [-Wattributes] /home/ed/gnu/gcc-trunk/gcc/testsuite/c-c++-common/ubsan/object-size-9.c:32:1: warning: always_inline function might not be inlinable [-Wattributes] /home/ed/gnu/gcc-trunk/gcc/testsuite/c-c++-common/ubsan/object-size-9.c:47:1: error: inlining failed in call to always_inline 'C f3(int)': function body can be overwritten at link time /home/ed/gnu/gcc-trunk/gcc/testsuite/c-c++-common/ubsan/object-size-9.c:94:10: error: called from here the diagnostics are just a little different when the function is inlined or not. Bernd.
Re: [PATCH] Fix PR64078
On Sun, Sep 06, 2015 at 07:21:13PM +0200, Bernd Edlinger wrote: > Hi, > > we observed sporadic failures of the following two test cases (see PR64078): > c-c++-common/ubsan/object-size-9.c and c-c++-common/ubsan/object-size-10.c > > For object-size-9.c this happens in a reproducible way when -fpic option is > used: > If that option is used, it is slightly less desirable to inline the > functions, but if an explicit > "inline" is added, the function is still in-lined, even if -fpic is used. So if we rely on the function being inlined I think it would be better to add the always_inline attribute. Marek