Re: [PATCH] Fix PR64078

[Bernd Edlinger]

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

[Jeff Law]

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

[Bernd Edlinger]

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

[Jeff Law]

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

[Tom de Vries]

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

[Tom de Vries]

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

[Bernd Edlinger]

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

[Tom de Vries]

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

[Tom de Vries]

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

[Bernd Edlinger]

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

[Tom de Vries]

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

[Jeff Law]

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

[Marek Polacek]

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

[Bernd Edlinger]

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

[Marek Polacek]

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

[Bernd Edlinger]

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

[Jeff Law]

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

[Bernd Edlinger]

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

[Jeff Law]

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

[Marek Polacek]

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

RE: [PATCH] Fix PR64078

[Bernd Edlinger]

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.
  

[PATCH] Fix PR64078

[Bernd Edlinger]

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.

But it may also happen randomly when the sanitizer tries to dump memory around 
an object,
that lies next to a non-accessible page, the sanitizer prints ""
in this case, which is not what the test case expects here.  As a work around I 
added
a large alignment attribute, to make sure, that the object cannot be at a page 
boundary.


Boot-strapped and regression-tested x86_64-linux-gnu.
OK for trunk?


Thanks,
Bernd.
  2015-09-06  Bernd Edlinger  

PR testsuite/64078
* c-c++-common/ubsan/object-size-9.c (s): Add alignment attribute.
(f2, f3): Add inline attribute.
* c-c++-common/ubsan/object-size-10.c (a, b): Add alignment attribute.


patch-pr64078.diff
Description: Binary data