* [PATCH] Zero vptr in dtor for -fsanitize=vptr.
@ 2017-10-27 10:51 Martin Liška
2017-10-27 11:09 ` Jakub Jelinek
0 siblings, 1 reply; 19+ messages in thread
From: Martin Liška @ 2017-10-27 10:51 UTC (permalink / raw)
To: gcc-patches; +Cc: Jason Merrill, Jakub Jelinek
[-- Attachment #1: Type: text/plain, Size: 759 bytes --]
Hello.
This is small improvement that can catch a virtual call after a lifetime
scope of an object.
Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
Ready to be installed?
Martin
gcc/cp/ChangeLog:
2017-10-27 Martin Liska <mliska@suse.cz>
* decl.c (begin_destructor_body): In case of disabled recovery,
we can zero object in order to catch virtual calls after
an object lifetime.
gcc/testsuite/ChangeLog:
2017-10-27 Martin Liska <mliska@suse.cz>
* g++.dg/ubsan/vptr-12.C: New test.
---
gcc/cp/decl.c | 3 ++-
gcc/testsuite/g++.dg/ubsan/vptr-12.C | 26 ++++++++++++++++++++++++++
2 files changed, 28 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/g++.dg/ubsan/vptr-12.C
[-- Attachment #2: 0001-Zero-vptr-in-dtor-for-fsanitize-vptr.patch --]
[-- Type: text/x-patch, Size: 1323 bytes --]
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 15a8d283353..69636e30008 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -15281,7 +15281,8 @@ begin_destructor_body (void)
/* Clobbering an empty base is harmful if it overlays real data. */
&& !is_empty_class (current_class_type))
{
- if (sanitize_flags_p (SANITIZE_VPTR))
+ if (sanitize_flags_p (SANITIZE_VPTR)
+ && (flag_sanitize_recover & SANITIZE_VPTR) == 0)
{
tree fndecl = builtin_decl_explicit (BUILT_IN_MEMSET);
tree call = build_call_expr (fndecl, 3,
diff --git a/gcc/testsuite/g++.dg/ubsan/vptr-12.C b/gcc/testsuite/g++.dg/ubsan/vptr-12.C
new file mode 100644
index 00000000000..96c8473d757
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ubsan/vptr-12.C
@@ -0,0 +1,26 @@
+// { dg-do run }
+// { dg-shouldfail "ubsan" }
+// { dg-options "-fsanitize=vptr -fno-sanitize-recover=vptr" }
+
+struct MyClass
+{
+ virtual ~MyClass () {}
+ virtual void
+ Doit ()
+ {
+ }
+};
+
+int
+main ()
+{
+ MyClass *c = new MyClass;
+ c->~MyClass ();
+ c->Doit ();
+
+ return 0;
+}
+
+// { dg-output "\[^\n\r]*vptr-12.C:19:\[0-9]*: runtime error: member call on address 0x\[0-9a-fA-F]* which does not point to an object of type 'MyClass'(\n|\r\n|\r)" }
+// { dg-output "0x\[0-9a-fA-F]*: note: object has invalid vptr(\n|\r\n|\r)" }
+
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
2017-10-27 10:51 [PATCH] Zero vptr in dtor for -fsanitize=vptr Martin Liška
@ 2017-10-27 11:09 ` Jakub Jelinek
2017-10-27 11:26 ` Martin Liška
0 siblings, 1 reply; 19+ messages in thread
From: Jakub Jelinek @ 2017-10-27 11:09 UTC (permalink / raw)
To: Martin Liška; +Cc: gcc-patches, Jason Merrill
On Fri, Oct 27, 2017 at 12:47:12PM +0200, Martin Liška wrote:
> Hello.
>
> This is small improvement that can catch a virtual call after a lifetime
> scope of an object.
>
>
> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>
> Ready to be installed?
The decl.c change seems to be only incremental change from a not publicly
posted patch rather than the full diff against trunk.
> 2017-10-27 Martin Liska <mliska@suse.cz>
>
> * decl.c (begin_destructor_body): In case of disabled recovery,
> we can zero object in order to catch virtual calls after
> an object lifetime.
>
> gcc/testsuite/ChangeLog:
>
> 2017-10-27 Martin Liska <mliska@suse.cz>
>
> * g++.dg/ubsan/vptr-12.C: New test.
> ---
> gcc/cp/decl.c | 3 ++-
> gcc/testsuite/g++.dg/ubsan/vptr-12.C | 26 ++++++++++++++++++++++++++
> 2 files changed, 28 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/g++.dg/ubsan/vptr-12.C
>
>
> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
> index 15a8d283353..69636e30008 100644
> --- a/gcc/cp/decl.c
> +++ b/gcc/cp/decl.c
> @@ -15281,7 +15281,8 @@ begin_destructor_body (void)
> /* Clobbering an empty base is harmful if it overlays real data. */
> && !is_empty_class (current_class_type))
> {
> - if (sanitize_flags_p (SANITIZE_VPTR))
> + if (sanitize_flags_p (SANITIZE_VPTR)
> + && (flag_sanitize_recover & SANITIZE_VPTR) == 0)
> {
> tree fndecl = builtin_decl_explicit (BUILT_IN_MEMSET);
> tree call = build_call_expr (fndecl, 3,
Jakub
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
2017-10-27 11:09 ` Jakub Jelinek
@ 2017-10-27 11:26 ` Martin Liška
2017-10-27 11:48 ` Jakub Jelinek
0 siblings, 1 reply; 19+ messages in thread
From: Martin Liška @ 2017-10-27 11:26 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches, Jason Merrill
[-- Attachment #1: Type: text/plain, Size: 222 bytes --]
On 10/27/2017 12:52 PM, Jakub Jelinek wrote:
> The decl.c change seems to be only incremental change from a not publicly
> posted patch rather than the full diff against trunk.
Sorry for that. Sending full patch.
Martin
[-- Attachment #2: 0001-Zero-vptr-in-dtor-for-fsanitize-vptr.patch --]
[-- Type: text/x-patch, Size: 2473 bytes --]
From df0cc0c2da18b150b1f0fbef418450a223470d7f Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Thu, 19 Oct 2017 11:10:19 +0200
Subject: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
gcc/cp/ChangeLog:
2017-10-27 Martin Liska <mliska@suse.cz>
* decl.c (begin_destructor_body): In case of disabled recovery,
we can zero object in order to catch virtual calls after
an object lifetime.
gcc/testsuite/ChangeLog:
2017-10-27 Martin Liska <mliska@suse.cz>
* g++.dg/ubsan/vptr-12.C: New test.
---
gcc/cp/decl.c | 14 +++++++++++++-
gcc/testsuite/g++.dg/ubsan/vptr-12.C | 26 ++++++++++++++++++++++++++
2 files changed, 39 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/g++.dg/ubsan/vptr-12.C
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 42b52748e2a..69636e30008 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -15280,7 +15280,19 @@ begin_destructor_body (void)
if (flag_lifetime_dse
/* Clobbering an empty base is harmful if it overlays real data. */
&& !is_empty_class (current_class_type))
- finish_decl_cleanup (NULL_TREE, build_clobber_this ());
+ {
+ if (sanitize_flags_p (SANITIZE_VPTR)
+ && (flag_sanitize_recover & SANITIZE_VPTR) == 0)
+ {
+ tree fndecl = builtin_decl_explicit (BUILT_IN_MEMSET);
+ tree call = build_call_expr (fndecl, 3,
+ current_class_ptr, integer_zero_node,
+ TYPE_SIZE_UNIT (current_class_type));
+ finish_decl_cleanup (NULL_TREE, call);
+ }
+ else
+ finish_decl_cleanup (NULL_TREE, build_clobber_this ());
+ }
/* And insert cleanups for our bases and members so that they
will be properly destroyed if we throw. */
diff --git a/gcc/testsuite/g++.dg/ubsan/vptr-12.C b/gcc/testsuite/g++.dg/ubsan/vptr-12.C
new file mode 100644
index 00000000000..96c8473d757
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ubsan/vptr-12.C
@@ -0,0 +1,26 @@
+// { dg-do run }
+// { dg-shouldfail "ubsan" }
+// { dg-options "-fsanitize=vptr -fno-sanitize-recover=vptr" }
+
+struct MyClass
+{
+ virtual ~MyClass () {}
+ virtual void
+ Doit ()
+ {
+ }
+};
+
+int
+main ()
+{
+ MyClass *c = new MyClass;
+ c->~MyClass ();
+ c->Doit ();
+
+ return 0;
+}
+
+// { dg-output "\[^\n\r]*vptr-12.C:19:\[0-9]*: runtime error: member call on address 0x\[0-9a-fA-F]* which does not point to an object of type 'MyClass'(\n|\r\n|\r)" }
+// { dg-output "0x\[0-9a-fA-F]*: note: object has invalid vptr(\n|\r\n|\r)" }
+
--
2.14.2
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
2017-10-27 11:26 ` Martin Liška
@ 2017-10-27 11:48 ` Jakub Jelinek
2017-10-27 13:53 ` Martin Liška
0 siblings, 1 reply; 19+ messages in thread
From: Jakub Jelinek @ 2017-10-27 11:48 UTC (permalink / raw)
To: Martin Liška; +Cc: gcc-patches, Jason Merrill
On Fri, Oct 27, 2017 at 01:16:08PM +0200, Martin Liška wrote:
> On 10/27/2017 12:52 PM, Jakub Jelinek wrote:
> > The decl.c change seems to be only incremental change from a not publicly
> > posted patch rather than the full diff against trunk.
>
> Sorry for that. Sending full patch.
Thanks.
> --- a/gcc/cp/decl.c
> +++ b/gcc/cp/decl.c
> @@ -15280,7 +15280,19 @@ begin_destructor_body (void)
> if (flag_lifetime_dse
> /* Clobbering an empty base is harmful if it overlays real data. */
> && !is_empty_class (current_class_type))
> - finish_decl_cleanup (NULL_TREE, build_clobber_this ());
> + {
> + if (sanitize_flags_p (SANITIZE_VPTR)
> + && (flag_sanitize_recover & SANITIZE_VPTR) == 0)
> + {
> + tree fndecl = builtin_decl_explicit (BUILT_IN_MEMSET);
> + tree call = build_call_expr (fndecl, 3,
> + current_class_ptr, integer_zero_node,
> + TYPE_SIZE_UNIT (current_class_type));
I wonder if it wouldn't be cheaper to just use thisref = {}; rather than
memset, pretty much the same thing as build_clobber_this () emits, except
for the TREE_VOLATILE. Also, build_clobber_this has:
if (vbases)
exprstmt = build_if_in_charge (exprstmt);
so it doesn't clobber if not in charge, not sure if it applies here too.
So maybe easiest would be add a bool argument to build_clobber_this which
would say whether it is a clobber or real clearing?
> + finish_decl_cleanup (NULL_TREE, call);
> + }
> + else
> + finish_decl_cleanup (NULL_TREE, build_clobber_this ());
> + }
>
> /* And insert cleanups for our bases and members so that they
> will be properly destroyed if we throw. */
> diff --git a/gcc/testsuite/g++.dg/ubsan/vptr-12.C b/gcc/testsuite/g++.dg/ubsan/vptr-12.C
> new file mode 100644
> index 00000000000..96c8473d757
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/ubsan/vptr-12.C
> @@ -0,0 +1,26 @@
> +// { dg-do run }
> +// { dg-shouldfail "ubsan" }
> +// { dg-options "-fsanitize=vptr -fno-sanitize-recover=vptr" }
> +
> +struct MyClass
> +{
> + virtual ~MyClass () {}
> + virtual void
> + Doit ()
> + {
> + }
Why not put all the above 4 lines into a single one, the dtor already uses
that kind of formatting.
> +};
> +
> +int
> +main ()
> +{
> + MyClass *c = new MyClass;
> + c->~MyClass ();
> + c->Doit ();
> +
> + return 0;
> +}
> +
> +// { dg-output "\[^\n\r]*vptr-12.C:19:\[0-9]*: runtime error: member call on address 0x\[0-9a-fA-F]* which does not point to an object of type 'MyClass'(\n|\r\n|\r)" }
> +// { dg-output "0x\[0-9a-fA-F]*: note: object has invalid vptr(\n|\r\n|\r)" }
> +
Unnecessary empty line at end.
Jakub
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
2017-10-27 11:48 ` Jakub Jelinek
@ 2017-10-27 13:53 ` Martin Liška
2017-10-27 13:57 ` Jakub Jelinek
0 siblings, 1 reply; 19+ messages in thread
From: Martin Liška @ 2017-10-27 13:53 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches, Jason Merrill
[-- Attachment #1: Type: text/plain, Size: 2889 bytes --]
On 10/27/2017 01:26 PM, Jakub Jelinek wrote:
> On Fri, Oct 27, 2017 at 01:16:08PM +0200, Martin Liška wrote:
>> On 10/27/2017 12:52 PM, Jakub Jelinek wrote:
>>> The decl.c change seems to be only incremental change from a not publicly
>>> posted patch rather than the full diff against trunk.
>>
>> Sorry for that. Sending full patch.
>
> Thanks.
>
>> --- a/gcc/cp/decl.c
>> +++ b/gcc/cp/decl.c
>> @@ -15280,7 +15280,19 @@ begin_destructor_body (void)
>> if (flag_lifetime_dse
>> /* Clobbering an empty base is harmful if it overlays real data. */
>> && !is_empty_class (current_class_type))
>> - finish_decl_cleanup (NULL_TREE, build_clobber_this ());
>> + {
>> + if (sanitize_flags_p (SANITIZE_VPTR)
>> + && (flag_sanitize_recover & SANITIZE_VPTR) == 0)
>> + {
>> + tree fndecl = builtin_decl_explicit (BUILT_IN_MEMSET);
>> + tree call = build_call_expr (fndecl, 3,
>> + current_class_ptr, integer_zero_node,
>> + TYPE_SIZE_UNIT (current_class_type));
>
> I wonder if it wouldn't be cheaper to just use thisref = {}; rather than
> memset, pretty much the same thing as build_clobber_this () emits, except
> for the TREE_VOLATILE. Also, build_clobber_this has:
> if (vbases)
> exprstmt = build_if_in_charge (exprstmt);
> so it doesn't clobber if not in charge, not sure if it applies here too.
> So maybe easiest would be add a bool argument to build_clobber_this which
> would say whether it is a clobber or real clearing?
Hello.
Did that in newer version of the patch, good idea!
>
>> + finish_decl_cleanup (NULL_TREE, call);
>> + }
>> + else
>> + finish_decl_cleanup (NULL_TREE, build_clobber_this ());
>> + }
>>
>> /* And insert cleanups for our bases and members so that they
>> will be properly destroyed if we throw. */
>> diff --git a/gcc/testsuite/g++.dg/ubsan/vptr-12.C b/gcc/testsuite/g++.dg/ubsan/vptr-12.C
>> new file mode 100644
>> index 00000000000..96c8473d757
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/ubsan/vptr-12.C
>> @@ -0,0 +1,26 @@
>> +// { dg-do run }
>> +// { dg-shouldfail "ubsan" }
>> +// { dg-options "-fsanitize=vptr -fno-sanitize-recover=vptr" }
>> +
>> +struct MyClass
>> +{
>> + virtual ~MyClass () {}
>> + virtual void
>> + Doit ()
>> + {
>> + }
>
> Why not put all the above 4 lines into a single one, the dtor already uses
> that kind of formatting.
Sure.
>
>> +};
>> +
>> +int
>> +main ()
>> +{
>> + MyClass *c = new MyClass;
>> + c->~MyClass ();
>> + c->Doit ();
>> +
>> + return 0;
>> +}
>> +
>> +// { dg-output "\[^\n\r]*vptr-12.C:19:\[0-9]*: runtime error: member call on address 0x\[0-9a-fA-F]* which does not point to an object of type 'MyClass'(\n|\r\n|\r)" }
>> +// { dg-output "0x\[0-9a-fA-F]*: note: object has invalid vptr(\n|\r\n|\r)" }
>> +
>
> Unnecessary empty line at end.
Likewise.
Martin
>
> Jakub
>
[-- Attachment #2: 0001-Zero-vptr-in-dtor-for-fsanitize-vptr.patch --]
[-- Type: text/x-patch, Size: 3698 bytes --]
From b1da5f4de8b630f284627f422b902d28cd1d408b Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Thu, 19 Oct 2017 11:10:19 +0200
Subject: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
gcc/cp/ChangeLog:
2017-10-27 Martin Liska <mliska@suse.cz>
* decl.c (build_clobber_this): Rename to ...
(build_this_constructor): ... this. Add argument clobber_p.
(start_preparsed_function): Use the argument.
(begin_destructor_body): In case of disabled recovery,
we can zero object in order to catch virtual calls after
an object lifetime.
gcc/testsuite/ChangeLog:
2017-10-27 Martin Liska <mliska@suse.cz>
* g++.dg/ubsan/vptr-12.C: New test.
---
gcc/cp/decl.c | 18 ++++++++++++++----
gcc/testsuite/g++.dg/ubsan/vptr-12.C | 22 ++++++++++++++++++++++
2 files changed, 36 insertions(+), 4 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/ubsan/vptr-12.C
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 519aa06a0f9..ee48d1c157e 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -14639,8 +14639,12 @@ implicit_default_ctor_p (tree fn)
/* Clobber the contents of *this to let the back end know that the object
storage is dead when we enter the constructor or leave the destructor. */
+/* Clobber or zero (depending on CLOBBER_P argument) the contents of *this
+ to let the back end know that the object storage is dead
+ when we enter the constructor or leave the destructor. */
+
static tree
-build_clobber_this ()
+build_this_constructor (bool clobber_p)
{
/* Clobbering an empty base is pointless, and harmful if its one byte
TYPE_SIZE overlays real data. */
@@ -14657,7 +14661,9 @@ build_clobber_this ()
ctype = CLASSTYPE_AS_BASE (ctype);
tree clobber = build_constructor (ctype, NULL);
- TREE_THIS_VOLATILE (clobber) = true;
+
+ if (clobber_p)
+ TREE_THIS_VOLATILE (clobber) = true;
tree thisref = current_class_ref;
if (ctype != current_class_type)
@@ -15086,7 +15092,7 @@ start_preparsed_function (tree decl1, tree attrs, int flags)
because part of the initialization might happen before we enter the
constructor, via AGGR_INIT_ZERO_FIRST (c++/68006). */
&& !implicit_default_ctor_p (decl1))
- finish_expr_stmt (build_clobber_this ());
+ finish_expr_stmt (build_this_constructor (true));
if (!processing_template_decl
&& DECL_CONSTRUCTOR_P (decl1)
@@ -15301,7 +15307,11 @@ begin_destructor_body (void)
if (flag_lifetime_dse
/* Clobbering an empty base is harmful if it overlays real data. */
&& !is_empty_class (current_class_type))
- finish_decl_cleanup (NULL_TREE, build_clobber_this ());
+ {
+ bool s = (sanitize_flags_p (SANITIZE_VPTR)
+ && (flag_sanitize_recover & SANITIZE_VPTR) == 0);
+ finish_decl_cleanup (NULL_TREE, build_this_constructor (!s));
+ }
/* And insert cleanups for our bases and members so that they
will be properly destroyed if we throw. */
diff --git a/gcc/testsuite/g++.dg/ubsan/vptr-12.C b/gcc/testsuite/g++.dg/ubsan/vptr-12.C
new file mode 100644
index 00000000000..51b9d36d3f2
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ubsan/vptr-12.C
@@ -0,0 +1,22 @@
+// { dg-do run }
+// { dg-shouldfail "ubsan" }
+// { dg-options "-fsanitize=vptr -fno-sanitize-recover=vptr" }
+
+struct MyClass
+{
+ virtual ~MyClass () {}
+ virtual void Doit () {}
+};
+
+int
+main ()
+{
+ MyClass *c = new MyClass;
+ c->~MyClass ();
+ c->Doit ();
+
+ return 0;
+}
+
+// { dg-output "\[^\n\r]*vptr-12.C:19:\[0-9]*: runtime error: member call on address 0x\[0-9a-fA-F]* which does not point to an object of type 'MyClass'(\n|\r\n|\r)" }
+// { dg-output "0x\[0-9a-fA-F]*: note: object has invalid vptr(\n|\r\n|\r)" }
--
2.14.2
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
2017-10-27 13:53 ` Martin Liška
@ 2017-10-27 13:57 ` Jakub Jelinek
2017-10-27 18:18 ` Jason Merrill
0 siblings, 1 reply; 19+ messages in thread
From: Jakub Jelinek @ 2017-10-27 13:57 UTC (permalink / raw)
To: Martin Liška, Jason Merrill, Nathan Sidwell; +Cc: gcc-patches
On Fri, Oct 27, 2017 at 03:48:41PM +0200, Martin Liška wrote:
> --- a/gcc/cp/decl.c
> +++ b/gcc/cp/decl.c
> @@ -14639,8 +14639,12 @@ implicit_default_ctor_p (tree fn)
> /* Clobber the contents of *this to let the back end know that the object
> storage is dead when we enter the constructor or leave the destructor. */
>
> +/* Clobber or zero (depending on CLOBBER_P argument) the contents of *this
> + to let the back end know that the object storage is dead
> + when we enter the constructor or leave the destructor. */
> +
> static tree
> -build_clobber_this ()
> +build_this_constructor (bool clobber_p)
I think build_clobber_this is better name, but will defer final review
to Jason or Nathan. Also, seems there was already a function comment
and you've added yet another one, instead of ammending the first one.
Otherwise LGTM.
Jakub
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
2017-10-27 13:57 ` Jakub Jelinek
@ 2017-10-27 18:18 ` Jason Merrill
2017-10-27 18:22 ` Jakub Jelinek
0 siblings, 1 reply; 19+ messages in thread
From: Jason Merrill @ 2017-10-27 18:18 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Martin Liška, Nathan Sidwell, gcc-patches List
On Fri, Oct 27, 2017 at 9:52 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Oct 27, 2017 at 03:48:41PM +0200, Martin Liška wrote:
>> --- a/gcc/cp/decl.c
>> +++ b/gcc/cp/decl.c
>> @@ -14639,8 +14639,12 @@ implicit_default_ctor_p (tree fn)
>> /* Clobber the contents of *this to let the back end know that the object
>> storage is dead when we enter the constructor or leave the destructor. */
>>
>> +/* Clobber or zero (depending on CLOBBER_P argument) the contents of *this
>> + to let the back end know that the object storage is dead
>> + when we enter the constructor or leave the destructor. */
>> +
>> static tree
>> -build_clobber_this ()
>> +build_this_constructor (bool clobber_p)
>
> I think build_clobber_this is better name, but will defer final review
> to Jason or Nathan. Also, seems there was already a function comment
> and you've added yet another one, instead of ammending the first one.
Agreed.
If the point is to clear the vptr, why are you also clearing the rest
of the object?
Jason
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
2017-10-27 18:18 ` Jason Merrill
@ 2017-10-27 18:22 ` Jakub Jelinek
2017-10-27 18:34 ` Nathan Sidwell
0 siblings, 1 reply; 19+ messages in thread
From: Jakub Jelinek @ 2017-10-27 18:22 UTC (permalink / raw)
To: Jason Merrill; +Cc: Martin Liška, Nathan Sidwell, gcc-patches List
On Fri, Oct 27, 2017 at 02:10:10PM -0400, Jason Merrill wrote:
> On Fri, Oct 27, 2017 at 9:52 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Fri, Oct 27, 2017 at 03:48:41PM +0200, Martin Liška wrote:
> >> --- a/gcc/cp/decl.c
> >> +++ b/gcc/cp/decl.c
> >> @@ -14639,8 +14639,12 @@ implicit_default_ctor_p (tree fn)
> >> /* Clobber the contents of *this to let the back end know that the object
> >> storage is dead when we enter the constructor or leave the destructor. */
> >>
> >> +/* Clobber or zero (depending on CLOBBER_P argument) the contents of *this
> >> + to let the back end know that the object storage is dead
> >> + when we enter the constructor or leave the destructor. */
> >> +
> >> static tree
> >> -build_clobber_this ()
> >> +build_this_constructor (bool clobber_p)
> >
> > I think build_clobber_this is better name, but will defer final review
> > to Jason or Nathan. Also, seems there was already a function comment
> > and you've added yet another one, instead of ammending the first one.
>
> Agreed.
>
> If the point is to clear the vptr, why are you also clearing the rest
> of the object?
Can there be multiple vptr pointers in the object or is there just one?
Even if there can be multiple, perhaps earlier destructors would
have cleared those other vptr pointers though.
Jakub
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
2017-10-27 18:22 ` Jakub Jelinek
@ 2017-10-27 18:34 ` Nathan Sidwell
2017-10-27 18:37 ` Jakub Jelinek
0 siblings, 1 reply; 19+ messages in thread
From: Nathan Sidwell @ 2017-10-27 18:34 UTC (permalink / raw)
To: Jakub Jelinek, Jason Merrill; +Cc: Martin Liška, gcc-patches List
On 10/27/2017 02:18 PM, Jakub Jelinek wrote:
> On Fri, Oct 27, 2017 at 02:10:10PM -0400, Jason Merrill wrote:
>> If the point is to clear the vptr, why are you also clearing the rest
>> of the object?
>
> Can there be multiple vptr pointers in the object or is there just one?
> Even if there can be multiple, perhaps earlier destructors would
> have cleared those other vptr pointers though.
There can be multiple vptrs in an object (multiple polymorphic bases).
However, each such case will have its own base dtor invoked, as you
postulated. In fact, there may be a base dtor invoked that maps onto
the single vptr, in the cases when we're singly inheriting a polymorphic
base.
nathan
[polymorphic here includes the case of having virtual bases].
--
Nathan Sidwell
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
2017-10-27 18:34 ` Nathan Sidwell
@ 2017-10-27 18:37 ` Jakub Jelinek
2017-10-27 20:37 ` Nathan Sidwell
0 siblings, 1 reply; 19+ messages in thread
From: Jakub Jelinek @ 2017-10-27 18:37 UTC (permalink / raw)
To: Nathan Sidwell; +Cc: Jason Merrill, Martin Liška, gcc-patches List
On Fri, Oct 27, 2017 at 02:30:39PM -0400, Nathan Sidwell wrote:
> On 10/27/2017 02:18 PM, Jakub Jelinek wrote:
> > On Fri, Oct 27, 2017 at 02:10:10PM -0400, Jason Merrill wrote:
>
> > > If the point is to clear the vptr, why are you also clearing the rest
> > > of the object?
> >
> > Can there be multiple vptr pointers in the object or is there just one?
> > Even if there can be multiple, perhaps earlier destructors would
> > have cleared those other vptr pointers though.
>
> There can be multiple vptrs in an object (multiple polymorphic bases).
> However, each such case will have its own base dtor invoked, as you
> postulated. In fact, there may be a base dtor invoked that maps onto the
> single vptr, in the cases when we're singly inheriting a polymorphic base.
But when singly inheriting a polymorphic base and thus mapped to the same
vptr all but the last dtor will not be in charge, right?
So, if using build_clobber_this for this, instead of clobbering what we
clobber we'd just clear the single vptr (couldn't clobber the rest, even
if before the store, because that would make the earlier other vptr stores
dead).
Jakub
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
2017-10-27 18:37 ` Jakub Jelinek
@ 2017-10-27 20:37 ` Nathan Sidwell
2017-11-03 14:25 ` Martin Liška
0 siblings, 1 reply; 19+ messages in thread
From: Nathan Sidwell @ 2017-10-27 20:37 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Jason Merrill, Martin Liška, gcc-patches List
On 10/27/2017 02:34 PM, Jakub Jelinek wrote:
> But when singly inheriting a polymorphic base and thus mapped to the same
> vptr all but the last dtor will not be in charge, right?
Correct.
> So, if using build_clobber_this for this, instead of clobbering what we
> clobber we'd just clear the single vptr (couldn't clobber the rest, even
> if before the store, because that would make the earlier other vptr stores
> dead).
ok (I'd not looked at the patch to see if in chargeness was signficant)
nathan
--
Nathan Sidwell
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
2017-10-27 20:37 ` Nathan Sidwell
@ 2017-11-03 14:25 ` Martin Liška
2017-11-03 14:31 ` Marek Polacek
2017-11-03 15:21 ` Jason Merrill
0 siblings, 2 replies; 19+ messages in thread
From: Martin Liška @ 2017-11-03 14:25 UTC (permalink / raw)
To: Nathan Sidwell, Jakub Jelinek; +Cc: Jason Merrill, gcc-patches List
[-- Attachment #1: Type: text/plain, Size: 691 bytes --]
On 10/27/2017 09:44 PM, Nathan Sidwell wrote:
> On 10/27/2017 02:34 PM, Jakub Jelinek wrote:
>
>> But when singly inheriting a polymorphic base and thus mapped to the same
>> vptr all but the last dtor will not be in charge, right?
>
> Correct.
>
>> So, if using build_clobber_this for this, instead of clobbering what we
>> clobber we'd just clear the single vptr (couldn't clobber the rest, even
>> if before the store, because that would make the earlier other vptr stores
>> dead).
>
> ok (I'd not looked at the patch to see if in chargeness was signficant)
>
> nathan
>
Hello.
I'm sending v2 which only zeros vptr of object.
Ready to be installed after finishing tests?
Martin
[-- Attachment #2: 0001-Zero-vptr-in-dtor-for-fsanitize-vptr.patch --]
[-- Type: text/x-patch, Size: 2691 bytes --]
From 098932be5472656c834b402038accb0b861afcc1 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Thu, 19 Oct 2017 11:10:19 +0200
Subject: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
gcc/cp/ChangeLog:
2017-11-03 Martin Liska <mliska@suse.cz>
* decl.c (begin_destructor_body): In case of VPTR sanitization
(with disabled recovery), zero vptr in order to catch virtual calls
after lifetime of an object.
gcc/testsuite/ChangeLog:
2017-10-27 Martin Liska <mliska@suse.cz>
* g++.dg/ubsan/vptr-12.C: New test.
---
gcc/cp/decl.c | 20 +++++++++++++++++++-
gcc/testsuite/g++.dg/ubsan/vptr-12.C | 22 ++++++++++++++++++++++
2 files changed, 41 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/g++.dg/ubsan/vptr-12.C
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index d88c78f348b..d45cc29e636 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -15241,7 +15241,25 @@ begin_destructor_body (void)
if (flag_lifetime_dse
/* Clobbering an empty base is harmful if it overlays real data. */
&& !is_empty_class (current_class_type))
- finish_decl_cleanup (NULL_TREE, build_clobber_this ());
+ {
+ if (sanitize_flags_p (SANITIZE_VPTR)
+ && (flag_sanitize_recover & SANITIZE_VPTR) == 0)
+ {
+ tree binfo = TYPE_BINFO (current_class_type);
+ tree ref
+ = cp_build_indirect_ref (current_class_ptr, RO_NULL,
+ tf_warning_or_error);
+
+ tree vtbl_ptr = build_vfield_ref (ref, TREE_TYPE (binfo));
+ tree vtbl = build_zero_cst (TREE_TYPE (vtbl_ptr));
+ tree stmt = cp_build_modify_expr (input_location, vtbl_ptr,
+ NOP_EXPR, vtbl,
+ tf_warning_or_error);
+ finish_decl_cleanup (NULL_TREE, stmt);
+ }
+ else
+ finish_decl_cleanup (NULL_TREE, build_clobber_this ());
+ }
/* And insert cleanups for our bases and members so that they
will be properly destroyed if we throw. */
diff --git a/gcc/testsuite/g++.dg/ubsan/vptr-12.C b/gcc/testsuite/g++.dg/ubsan/vptr-12.C
new file mode 100644
index 00000000000..be5c074dfc1
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ubsan/vptr-12.C
@@ -0,0 +1,22 @@
+// { dg-do run }
+// { dg-shouldfail "ubsan" }
+// { dg-options "-fsanitize=vptr -fno-sanitize-recover=vptr" }
+
+struct MyClass
+{
+ virtual ~MyClass () {}
+ virtual void Doit () {}
+};
+
+int
+main ()
+{
+ MyClass *c = new MyClass;
+ c->~MyClass ();
+ c->Doit ();
+
+ return 0;
+}
+
+// { dg-output "\[^\n\r]*vptr-12.C:16:\[0-9]*: runtime error: member call on address 0x\[0-9a-fA-F]* which does not point to an object of type 'MyClass'(\n|\r\n|\r)" }
+// { dg-output "0x\[0-9a-fA-F]*: note: object has invalid vptr(\n|\r\n|\r)" }
--
2.14.3
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
2017-11-03 14:25 ` Martin Liška
@ 2017-11-03 14:31 ` Marek Polacek
2017-11-05 18:41 ` Martin Liška
2017-11-03 15:21 ` Jason Merrill
1 sibling, 1 reply; 19+ messages in thread
From: Marek Polacek @ 2017-11-03 14:31 UTC (permalink / raw)
To: Martin Liška
Cc: Nathan Sidwell, Jakub Jelinek, Jason Merrill, gcc-patches List
On Fri, Nov 03, 2017 at 03:25:25PM +0100, Martin Liška wrote:
> On 10/27/2017 09:44 PM, Nathan Sidwell wrote:
> > On 10/27/2017 02:34 PM, Jakub Jelinek wrote:
> >
> >> But when singly inheriting a polymorphic base and thus mapped to the same
> >> vptr all but the last dtor will not be in charge, right?
> >
> > Correct.
> >
> >> So, if using build_clobber_this for this, instead of clobbering what we
> >> clobber we'd just clear the single vptr (couldn't clobber the rest, even
> >> if before the store, because that would make the earlier other vptr stores
> >> dead).
> >
> > ok (I'd not looked at the patch to see if in chargeness was signficant)
> >
> > nathan
> >
>
> Hello.
>
> I'm sending v2 which only zeros vptr of object.
>
> Ready to be installed after finishing tests?
> Martin
> From 098932be5472656c834b402038accb0b861afcc1 Mon Sep 17 00:00:00 2001
> From: marxin <mliska@suse.cz>
> Date: Thu, 19 Oct 2017 11:10:19 +0200
> Subject: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
>
> gcc/cp/ChangeLog:
>
> 2017-11-03 Martin Liska <mliska@suse.cz>
>
> * decl.c (begin_destructor_body): In case of VPTR sanitization
> (with disabled recovery), zero vptr in order to catch virtual calls
> after lifetime of an object.
>
> gcc/testsuite/ChangeLog:
>
> 2017-10-27 Martin Liska <mliska@suse.cz>
>
> * g++.dg/ubsan/vptr-12.C: New test.
> ---
> gcc/cp/decl.c | 20 +++++++++++++++++++-
> gcc/testsuite/g++.dg/ubsan/vptr-12.C | 22 ++++++++++++++++++++++
> 2 files changed, 41 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/g++.dg/ubsan/vptr-12.C
>
> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
> index d88c78f348b..d45cc29e636 100644
> --- a/gcc/cp/decl.c
> +++ b/gcc/cp/decl.c
> @@ -15241,7 +15241,25 @@ begin_destructor_body (void)
> if (flag_lifetime_dse
> /* Clobbering an empty base is harmful if it overlays real data. */
> && !is_empty_class (current_class_type))
> - finish_decl_cleanup (NULL_TREE, build_clobber_this ());
> + {
> + if (sanitize_flags_p (SANITIZE_VPTR)
> + && (flag_sanitize_recover & SANITIZE_VPTR) == 0)
> + {
> + tree binfo = TYPE_BINFO (current_class_type);
> + tree ref
> + = cp_build_indirect_ref (current_class_ptr, RO_NULL,
> + tf_warning_or_error);
> +
> + tree vtbl_ptr = build_vfield_ref (ref, TREE_TYPE (binfo));
> + tree vtbl = build_zero_cst (TREE_TYPE (vtbl_ptr));
> + tree stmt = cp_build_modify_expr (input_location, vtbl_ptr,
> + NOP_EXPR, vtbl,
> + tf_warning_or_error);
> + finish_decl_cleanup (NULL_TREE, stmt);
> + }
> + else
> + finish_decl_cleanup (NULL_TREE, build_clobber_this ());
> + }
>
> /* And insert cleanups for our bases and members so that they
> will be properly destroyed if we throw. */
> diff --git a/gcc/testsuite/g++.dg/ubsan/vptr-12.C b/gcc/testsuite/g++.dg/ubsan/vptr-12.C
> new file mode 100644
> index 00000000000..be5c074dfc1
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/ubsan/vptr-12.C
> @@ -0,0 +1,22 @@
> +// { dg-do run }
> +// { dg-shouldfail "ubsan" }
> +// { dg-options "-fsanitize=vptr -fno-sanitize-recover=vptr" }
> +
> +struct MyClass
> +{
> + virtual ~MyClass () {}
> + virtual void Doit () {}
> +};
> +
> +int
> +main ()
> +{
> + MyClass *c = new MyClass;
> + c->~MyClass ();
> + c->Doit ();
> +
> + return 0;
> +}
> +
> +// { dg-output "\[^\n\r]*vptr-12.C:16:\[0-9]*: runtime error: member call on address 0x\[0-9a-fA-F]* which does not point to an object of type 'MyClass'(\n|\r\n|\r)" }
> +// { dg-output "0x\[0-9a-fA-F]*: note: object has invalid vptr(\n|\r\n|\r)" }
I think the last dg-output shouldn't have any regexps at the end, so:
// { dg-output "0x\[0-9a-fA-F]*: note: object has invalid vptr" }
Marek
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
2017-11-03 14:25 ` Martin Liška
2017-11-03 14:31 ` Marek Polacek
@ 2017-11-03 15:21 ` Jason Merrill
2017-11-06 8:27 ` Martin Liška
1 sibling, 1 reply; 19+ messages in thread
From: Jason Merrill @ 2017-11-03 15:21 UTC (permalink / raw)
To: Martin Liška; +Cc: Nathan Sidwell, Jakub Jelinek, gcc-patches List
On Fri, Nov 3, 2017 at 10:25 AM, Martin Liška <mliska@suse.cz> wrote:
> On 10/27/2017 09:44 PM, Nathan Sidwell wrote:
>> On 10/27/2017 02:34 PM, Jakub Jelinek wrote:
>>
>>> But when singly inheriting a polymorphic base and thus mapped to the same
>>> vptr all but the last dtor will not be in charge, right?
>>
>> Correct.
>>
>>> So, if using build_clobber_this for this, instead of clobbering what we
>>> clobber we'd just clear the single vptr (couldn't clobber the rest, even
>>> if before the store, because that would make the earlier other vptr stores
>>> dead).
>>
>> ok (I'd not looked at the patch to see if in chargeness was signficant)
>>
>> nathan
>>
>
> Hello.
>
> I'm sending v2 which only zeros vptr of object.
>
> Ready to be installed after finishing tests?
Surely we also want to check TYPE_CONTAINS_VPTR_P.
Jason
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
2017-11-03 14:31 ` Marek Polacek
@ 2017-11-05 18:41 ` Martin Liška
0 siblings, 0 replies; 19+ messages in thread
From: Martin Liška @ 2017-11-05 18:41 UTC (permalink / raw)
To: Marek Polacek
Cc: Nathan Sidwell, Jakub Jelinek, Jason Merrill, gcc-patches List
On 11/03/2017 03:31 PM, Marek Polacek wrote:
> I think the last dg-output shouldn't have any regexps at the end, so:
>
> // { dg-output "0x\[0-9a-fA-F]*: note: object has invalid vptr" }
Thanks for that.
I'll fix it.
Martin
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
2017-11-03 15:21 ` Jason Merrill
@ 2017-11-06 8:27 ` Martin Liška
2017-11-14 16:26 ` Martin Liška
2017-11-14 17:01 ` Jason Merrill
0 siblings, 2 replies; 19+ messages in thread
From: Martin Liška @ 2017-11-06 8:27 UTC (permalink / raw)
To: Jason Merrill; +Cc: Nathan Sidwell, Jakub Jelinek, gcc-patches List
[-- Attachment #1: Type: text/plain, Size: 1054 bytes --]
On 11/03/2017 04:21 PM, Jason Merrill wrote:
> On Fri, Nov 3, 2017 at 10:25 AM, Martin Liška <mliska@suse.cz> wrote:
>> On 10/27/2017 09:44 PM, Nathan Sidwell wrote:
>>> On 10/27/2017 02:34 PM, Jakub Jelinek wrote:
>>>
>>>> But when singly inheriting a polymorphic base and thus mapped to the same
>>>> vptr all but the last dtor will not be in charge, right?
>>>
>>> Correct.
>>>
>>>> So, if using build_clobber_this for this, instead of clobbering what we
>>>> clobber we'd just clear the single vptr (couldn't clobber the rest, even
>>>> if before the store, because that would make the earlier other vptr stores
>>>> dead).
>>>
>>> ok (I'd not looked at the patch to see if in chargeness was signficant)
>>>
>>> nathan
>>>
>>
>> Hello.
>>
>> I'm sending v2 which only zeros vptr of object.
>>
>> Ready to be installed after finishing tests?
>
> Surely we also want to check TYPE_CONTAINS_VPTR_P.
>
> Jason
>
Done that in attached patch.
Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
Ready to be installed?
Martin
[-- Attachment #2: 0001-Zero-vptr-in-dtor-for-fsanitize-vptr-v2.patch --]
[-- Type: text/x-patch, Size: 2727 bytes --]
From 882becbc5446f304a9445281c6f778b80086ce39 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Thu, 19 Oct 2017 11:10:19 +0200
Subject: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
gcc/cp/ChangeLog:
2017-11-03 Martin Liska <mliska@suse.cz>
* decl.c (begin_destructor_body): In case of VPTR sanitization
(with disabled recovery), zero vptr in order to catch virtual calls
after lifetime of an object.
gcc/testsuite/ChangeLog:
2017-10-27 Martin Liska <mliska@suse.cz>
* g++.dg/ubsan/vptr-12.C: New test.
---
gcc/cp/decl.c | 21 ++++++++++++++++++++-
gcc/testsuite/g++.dg/ubsan/vptr-12.C | 22 ++++++++++++++++++++++
2 files changed, 42 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/g++.dg/ubsan/vptr-12.C
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 0ce8f2d3435..48d2760afde 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -15247,7 +15247,26 @@ begin_destructor_body (void)
if (flag_lifetime_dse
/* Clobbering an empty base is harmful if it overlays real data. */
&& !is_empty_class (current_class_type))
- finish_decl_cleanup (NULL_TREE, build_clobber_this ());
+ {
+ if (sanitize_flags_p (SANITIZE_VPTR)
+ && (flag_sanitize_recover & SANITIZE_VPTR) == 0
+ && TYPE_CONTAINS_VPTR_P (current_class_type))
+ {
+ tree binfo = TYPE_BINFO (current_class_type);
+ tree ref
+ = cp_build_indirect_ref (current_class_ptr, RO_NULL,
+ tf_warning_or_error);
+
+ tree vtbl_ptr = build_vfield_ref (ref, TREE_TYPE (binfo));
+ tree vtbl = build_zero_cst (TREE_TYPE (vtbl_ptr));
+ tree stmt = cp_build_modify_expr (input_location, vtbl_ptr,
+ NOP_EXPR, vtbl,
+ tf_warning_or_error);
+ finish_decl_cleanup (NULL_TREE, stmt);
+ }
+ else
+ finish_decl_cleanup (NULL_TREE, build_clobber_this ());
+ }
/* And insert cleanups for our bases and members so that they
will be properly destroyed if we throw. */
diff --git a/gcc/testsuite/g++.dg/ubsan/vptr-12.C b/gcc/testsuite/g++.dg/ubsan/vptr-12.C
new file mode 100644
index 00000000000..f23bbc3fd10
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ubsan/vptr-12.C
@@ -0,0 +1,22 @@
+// { dg-do run }
+// { dg-shouldfail "ubsan" }
+// { dg-options "-fsanitize=vptr -fno-sanitize-recover=vptr" }
+
+struct MyClass
+{
+ virtual ~MyClass () {}
+ virtual void Doit () {}
+};
+
+int
+main ()
+{
+ MyClass *c = new MyClass;
+ c->~MyClass ();
+ c->Doit ();
+
+ return 0;
+}
+
+// { dg-output "\[^\n\r]*vptr-12.C:16:\[0-9]*: runtime error: member call on address 0x\[0-9a-fA-F]* which does not point to an object of type 'MyClass'(\n|\r\n|\r)" }
+// { dg-output "0x\[0-9a-fA-F]*: note: object has invalid vptr" }
--
2.14.3
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
2017-11-06 8:27 ` Martin Liška
@ 2017-11-14 16:26 ` Martin Liška
2017-11-14 17:01 ` Jason Merrill
1 sibling, 0 replies; 19+ messages in thread
From: Martin Liška @ 2017-11-14 16:26 UTC (permalink / raw)
To: Jason Merrill; +Cc: Nathan Sidwell, Jakub Jelinek, gcc-patches List
PING^1
On 11/06/2017 09:27 AM, Martin Liška wrote:
> On 11/03/2017 04:21 PM, Jason Merrill wrote:
>> On Fri, Nov 3, 2017 at 10:25 AM, Martin Liška <mliska@suse.cz> wrote:
>>> On 10/27/2017 09:44 PM, Nathan Sidwell wrote:
>>>> On 10/27/2017 02:34 PM, Jakub Jelinek wrote:
>>>>
>>>>> But when singly inheriting a polymorphic base and thus mapped to the same
>>>>> vptr all but the last dtor will not be in charge, right?
>>>>
>>>> Correct.
>>>>
>>>>> So, if using build_clobber_this for this, instead of clobbering what we
>>>>> clobber we'd just clear the single vptr (couldn't clobber the rest, even
>>>>> if before the store, because that would make the earlier other vptr stores
>>>>> dead).
>>>>
>>>> ok (I'd not looked at the patch to see if in chargeness was signficant)
>>>>
>>>> nathan
>>>>
>>>
>>> Hello.
>>>
>>> I'm sending v2 which only zeros vptr of object.
>>>
>>> Ready to be installed after finishing tests?
>>
>> Surely we also want to check TYPE_CONTAINS_VPTR_P.
>>
>> Jason
>>
>
> Done that in attached patch.
> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>
> Ready to be installed?
> Martin
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
2017-11-06 8:27 ` Martin Liška
2017-11-14 16:26 ` Martin Liška
@ 2017-11-14 17:01 ` Jason Merrill
2017-11-15 9:24 ` Martin Liška
1 sibling, 1 reply; 19+ messages in thread
From: Jason Merrill @ 2017-11-14 17:01 UTC (permalink / raw)
To: Martin Liška; +Cc: Nathan Sidwell, Jakub Jelinek, gcc-patches List
OK.
On Mon, Nov 6, 2017 at 3:27 AM, Martin Liška <mliska@suse.cz> wrote:
> On 11/03/2017 04:21 PM, Jason Merrill wrote:
>> On Fri, Nov 3, 2017 at 10:25 AM, Martin Liška <mliska@suse.cz> wrote:
>>> On 10/27/2017 09:44 PM, Nathan Sidwell wrote:
>>>> On 10/27/2017 02:34 PM, Jakub Jelinek wrote:
>>>>
>>>>> But when singly inheriting a polymorphic base and thus mapped to the same
>>>>> vptr all but the last dtor will not be in charge, right?
>>>>
>>>> Correct.
>>>>
>>>>> So, if using build_clobber_this for this, instead of clobbering what we
>>>>> clobber we'd just clear the single vptr (couldn't clobber the rest, even
>>>>> if before the store, because that would make the earlier other vptr stores
>>>>> dead).
>>>>
>>>> ok (I'd not looked at the patch to see if in chargeness was signficant)
>>>>
>>>> nathan
>>>>
>>>
>>> Hello.
>>>
>>> I'm sending v2 which only zeros vptr of object.
>>>
>>> Ready to be installed after finishing tests?
>>
>> Surely we also want to check TYPE_CONTAINS_VPTR_P.
>>
>> Jason
>>
>
> Done that in attached patch.
> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>
> Ready to be installed?
> Martin
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
2017-11-14 17:01 ` Jason Merrill
@ 2017-11-15 9:24 ` Martin Liška
0 siblings, 0 replies; 19+ messages in thread
From: Martin Liška @ 2017-11-15 9:24 UTC (permalink / raw)
To: Jason Merrill; +Cc: Nathan Sidwell, Jakub Jelinek, gcc-patches List
[-- Attachment #1: Type: text/plain, Size: 150 bytes --]
Thanks for review. I actually noticed your introduction of
cp_build_fold_indirect_ref after I installed my patch.
I'm testing following fix.
Martin
[-- Attachment #2: 0001-Fix-fallout-of-fsanitize-vptr.patch --]
[-- Type: text/x-patch, Size: 965 bytes --]
From 63d9cff5c183f3614cff527ff991e1586a9efa5b Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Wed, 15 Nov 2017 10:01:51 +0100
Subject: [PATCH] Fix fallout of -fsanitize=vptr.
gcc/cp/ChangeLog:
2017-11-15 Martin Liska <mliska@suse.cz>
* decl.c (begin_destructor_body): Use cp_build_fold_indirect_ref
instead of cp_build_indirect_ref.
---
gcc/cp/decl.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 041893db937..7e16f7b415b 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -15253,8 +15253,7 @@ begin_destructor_body (void)
{
tree binfo = TYPE_BINFO (current_class_type);
tree ref
- = cp_build_indirect_ref (current_class_ptr, RO_NULL,
- tf_warning_or_error);
+ = cp_build_fold_indirect_ref (current_class_ptr);
tree vtbl_ptr = build_vfield_ref (ref, TREE_TYPE (binfo));
tree vtbl = build_zero_cst (TREE_TYPE (vtbl_ptr));
--
2.14.3
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2017-11-15 9:05 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-27 10:51 [PATCH] Zero vptr in dtor for -fsanitize=vptr Martin Liška
2017-10-27 11:09 ` Jakub Jelinek
2017-10-27 11:26 ` Martin Liška
2017-10-27 11:48 ` Jakub Jelinek
2017-10-27 13:53 ` Martin Liška
2017-10-27 13:57 ` Jakub Jelinek
2017-10-27 18:18 ` Jason Merrill
2017-10-27 18:22 ` Jakub Jelinek
2017-10-27 18:34 ` Nathan Sidwell
2017-10-27 18:37 ` Jakub Jelinek
2017-10-27 20:37 ` Nathan Sidwell
2017-11-03 14:25 ` Martin Liška
2017-11-03 14:31 ` Marek Polacek
2017-11-05 18:41 ` Martin Liška
2017-11-03 15:21 ` Jason Merrill
2017-11-06 8:27 ` Martin Liška
2017-11-14 16:26 ` Martin Liška
2017-11-14 17:01 ` Jason Merrill
2017-11-15 9:24 ` Martin Liška
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).