[C++] Add -fnull-this-pointer

Hi,
according to Trevor, the assumption about THIS pointer being non-NULL breaks
several bigger C++ packages (definitly including Firefox, but I believe
kdevelop was mentioned, too).  This patch makes the feature to be controlable
by a dedicated flag.  I am not sure about the default. We now have ubsan check
for the bug so I would hope the codebases to be updated soon, but it did not
happen for Firefox for quite a while despite the fact that Martin Liska reported
it.

This patch defaults to -fno-null-this-pointer, but I would be OK with changing
the default and setting it on only in GCC 6. Main point of the patch is to
avoid need of those packages to be built with -fno-delete-null-pointer-checks
(which still subsumes the flag).

The patch is bit inconsistent, becuase C++ FE wil still assume that this pointer
is non-NULL when expanding multiple inheritance accesses.  We did this from
very beginning. I do not know FE enough to see if it is easy to change the
behaviour here or if it is desired.

Bootstrapped/regtsted x86_64-linux.

Honza

	* c-family/c.opt (fnull-this-pointer): New flag.
	(nonnull_arg_p): Honnor flag_null_this_pointer.

On 2016.01.19 at 13:11 +0100, Jan Hubicka wrote:
> according to Trevor, the assumption about THIS pointer being non-NULL breaks
> several bigger C++ packages (definitly including Firefox, but I believe
> kdevelop was mentioned, too).  This patch makes the feature to be controlable
> by a dedicated flag.  I am not sure about the default. We now have ubsan check
> for the bug so I would hope the codebases to be updated soon, but it did not
> happen for Firefox for quite a while despite the fact that Martin Liska reported
> it.
> 
> This patch defaults to -fno-null-this-pointer, but I would be OK with changing
> the default and setting it on only in GCC 6. Main point of the patch is to
> avoid need of those packages to be built with -fno-delete-null-pointer-checks
> (which still subsumes the flag).

I can confirm that for QT-5, Chromium and Kdevelop this optimization
needs to be disabled. So it looks like a very common issue in large C++
codebases.

On Tue, Jan 19, 2016 at 01:11:44PM +0100, Jan Hubicka wrote:
> Hi,
> according to Trevor, the assumption about THIS pointer being non-NULL breaks

That was Markus, not me.

> several bigger C++ packages (definitly including Firefox, but I believe
> kdevelop was mentioned, too).  This patch makes the feature to be controlable
> by a dedicated flag.  I am not sure about the default. We now have ubsan check
> for the bug so I would hope the codebases to be updated soon, but it did not
> happen for Firefox for quite a while despite the fact that Martin Liska reported
> it.
> 
> This patch defaults to -fno-null-this-pointer, but I would be OK with changing

fwiw I find the naming a bit confusing maybe I'm just tired but it takes
some puzlling for me to know which way is being strict and which way is
allowing this.

> the default and setting it on only in GCC 6. Main point of the patch is to
> avoid need of those packages to be built with -fno-delete-null-pointer-checks
> (which still subsumes the flag).

Personally I'd rather try and be strict.  I suspect it often will be
easy to find and fix the bugs when the optimization is enabled.  Of
course if some projects don't care they can pass flags themselves.

Trev

> 
> The patch is bit inconsistent, becuase C++ FE wil still assume that this pointer
> is non-NULL when expanding multiple inheritance accesses.  We did this from
> very beginning. I do not know FE enough to see if it is easy to change the
> behaviour here or if it is desired.
> 
> Bootstrapped/regtsted x86_64-linux.
> 
> Honza
> 
> 	* c-family/c.opt (fnull-this-pointer): New flag.
> 	(nonnull_arg_p): Honnor flag_null_this_pointer.
> Index: tree.c
> ===================================================================
> --- tree.c	(revision 232553)
> +++ tree.c	(working copy)
> @@ -14016,6 +14022,7 @@ nonnull_arg_p (const_tree arg)
>    /* THIS argument of method is always non-NULL.  */
>    if (TREE_CODE (TREE_TYPE (cfun->decl)) == METHOD_TYPE
>        && arg == DECL_ARGUMENTS (cfun->decl)
> +      && flag_null_this_pointer
>        && flag_delete_null_pointer_checks)
>      return true;
>  
> Index: c-family/c.opt
> ===================================================================
> --- c-family/c.opt	(revision 232553)
> +++ c-family/c.opt	(working copy)
> @@ -1321,6 +1321,10 @@ Enum(ivar_visibility) String(public) Val
>  EnumValue
>  Enum(ivar_visibility) String(package) Value(IVAR_VISIBILITY_PACKAGE)
>  
> +fnull-this-pointer
> +C++ ObjC++ Optimization Report Var(flag_null_this_pointer)
> +Allow calling methods of NULL pointer
> +
>  fnonansi-builtins
>  C++ ObjC++ Var(flag_no_nonansi_builtin, 0)
>  
> Index: doc/invoke.texi
> ===================================================================
> --- doc/invoke.texi	(revision 232553)
> +++ doc/invoke.texi	(working copy)
> @@ -232,6 +232,7 @@ Objective-C and Objective-C++ Dialects}.
>  -fobjc-std=objc1 @gol
>  -fno-local-ivars @gol
>  -fivar-visibility=@r{[}public@r{|}protected@r{|}private@r{|}package@r{]} @gol
> +-fnull-this-pointer @gol
>  -freplace-objc-classes @gol
>  -fzero-link @gol
>  -gen-decls @gol
> @@ -2361,6 +2362,11 @@ errors if these functions are not inline
>  Disable Wpedantic warnings about constructs used in MFC, such as implicit
>  int and getting a pointer to member function via non-standard syntax.
>  
> +@item -fnull-this-pointer
> +@opindex fnull-this-pointer
> +Disable optimization which take advantage of the fact that calling method
> +of @code{NULL} pointer is undefined.
> +
>  @item -fno-nonansi-builtins
>  @opindex fno-nonansi-builtins
>  Disable built-in declarations of functions that are not mandated by

On Tue, Jan 19, 2016 at 2:18 PM, Trevor Saunders <tbsaunde@tbsaunde.org> wrote:
> On Tue, Jan 19, 2016 at 01:11:44PM +0100, Jan Hubicka wrote:
>> Hi,
>> according to Trevor, the assumption about THIS pointer being non-NULL breaks
>
> That was Markus, not me.
>
>> several bigger C++ packages (definitly including Firefox, but I believe
>> kdevelop was mentioned, too).  This patch makes the feature to be controlable
>> by a dedicated flag.  I am not sure about the default. We now have ubsan check
>> for the bug so I would hope the codebases to be updated soon, but it did not
>> happen for Firefox for quite a while despite the fact that Martin Liska reported
>> it.
>>
>> This patch defaults to -fno-null-this-pointer, but I would be OK with changing
>
> fwiw I find the naming a bit confusing maybe I'm just tired but it takes
> some puzlling for me to know which way is being strict and which way is
> allowing this.
>
>> the default and setting it on only in GCC 6. Main point of the patch is to
>> avoid need of those packages to be built with -fno-delete-null-pointer-checks
>> (which still subsumes the flag).
>
> Personally I'd rather try and be strict.  I suspect it often will be
> easy to find and fix the bugs when the optimization is enabled.  Of
> course if some projects don't care they can pass flags themselves.

Agreed.  As we already have a flag that can be used as a workaround I don't see
a reason to add another more specific one.  That just makes it a
lesser incentive
for people to fix their code.

Richard.

> Trev
>
>>
>> The patch is bit inconsistent, becuase C++ FE wil still assume that this pointer
>> is non-NULL when expanding multiple inheritance accesses.  We did this from
>> very beginning. I do not know FE enough to see if it is easy to change the
>> behaviour here or if it is desired.
>>
>> Bootstrapped/regtsted x86_64-linux.
>>
>> Honza
>>
>>       * c-family/c.opt (fnull-this-pointer): New flag.
>>       (nonnull_arg_p): Honnor flag_null_this_pointer.
>> Index: tree.c
>> ===================================================================
>> --- tree.c    (revision 232553)
>> +++ tree.c    (working copy)
>> @@ -14016,6 +14022,7 @@ nonnull_arg_p (const_tree arg)
>>    /* THIS argument of method is always non-NULL.  */
>>    if (TREE_CODE (TREE_TYPE (cfun->decl)) == METHOD_TYPE
>>        && arg == DECL_ARGUMENTS (cfun->decl)
>> +      && flag_null_this_pointer
>>        && flag_delete_null_pointer_checks)
>>      return true;
>>
>> Index: c-family/c.opt
>> ===================================================================
>> --- c-family/c.opt    (revision 232553)
>> +++ c-family/c.opt    (working copy)
>> @@ -1321,6 +1321,10 @@ Enum(ivar_visibility) String(public) Val
>>  EnumValue
>>  Enum(ivar_visibility) String(package) Value(IVAR_VISIBILITY_PACKAGE)
>>
>> +fnull-this-pointer
>> +C++ ObjC++ Optimization Report Var(flag_null_this_pointer)
>> +Allow calling methods of NULL pointer
>> +
>>  fnonansi-builtins
>>  C++ ObjC++ Var(flag_no_nonansi_builtin, 0)
>>
>> Index: doc/invoke.texi
>> ===================================================================
>> --- doc/invoke.texi   (revision 232553)
>> +++ doc/invoke.texi   (working copy)
>> @@ -232,6 +232,7 @@ Objective-C and Objective-C++ Dialects}.
>>  -fobjc-std=objc1 @gol
>>  -fno-local-ivars @gol
>>  -fivar-visibility=@r{[}public@r{|}protected@r{|}private@r{|}package@r{]} @gol
>> +-fnull-this-pointer @gol
>>  -freplace-objc-classes @gol
>>  -fzero-link @gol
>>  -gen-decls @gol
>> @@ -2361,6 +2362,11 @@ errors if these functions are not inline
>>  Disable Wpedantic warnings about constructs used in MFC, such as implicit
>>  int and getting a pointer to member function via non-standard syntax.
>>
>> +@item -fnull-this-pointer
>> +@opindex fnull-this-pointer
>> +Disable optimization which take advantage of the fact that calling method
>> +of @code{NULL} pointer is undefined.
>> +
>>  @item -fno-nonansi-builtins
>>  @opindex fno-nonansi-builtins
>>  Disable built-in declarations of functions that are not mandated by

On 2016.01.19 at 15:23 +0100, Richard Biener wrote:
> On Tue, Jan 19, 2016 at 2:18 PM, Trevor Saunders <tbsaunde@tbsaunde.org> wrote:
> > On Tue, Jan 19, 2016 at 01:11:44PM +0100, Jan Hubicka wrote:
> >> Hi,
> >> according to Trevor, the assumption about THIS pointer being non-NULL breaks
> >
> > That was Markus, not me.
> >
> >> several bigger C++ packages (definitly including Firefox, but I believe
> >> kdevelop was mentioned, too).  This patch makes the feature to be controlable
> >> by a dedicated flag.  I am not sure about the default. We now have ubsan check
> >> for the bug so I would hope the codebases to be updated soon, but it did not
> >> happen for Firefox for quite a while despite the fact that Martin Liska reported
> >> it.
> >>
> >> This patch defaults to -fno-null-this-pointer, but I would be OK with changing
> >
> > fwiw I find the naming a bit confusing maybe I'm just tired but it takes
> > some puzlling for me to know which way is being strict and which way is
> > allowing this.
> >
> >> the default and setting it on only in GCC 6. Main point of the patch is to
> >> avoid need of those packages to be built with -fno-delete-null-pointer-checks
> >> (which still subsumes the flag).
> >
> > Personally I'd rather try and be strict.  I suspect it often will be
> > easy to find and fix the bugs when the optimization is enabled.  Of
> > course if some projects don't care they can pass flags themselves.
> 
> Agreed.  As we already have a flag that can be used as a workaround I don't see
> a reason to add another more specific one.  That just makes it a
> lesser incentive
> for people to fix their code.

Well, -fno-delete-null-pointer-checks is a big hammer, that disables way
to many optimizations than really necessary.

On Tue, Jan 19, 2016 at 03:37:02PM +0100, Markus Trippelsdorf wrote:
> > Agreed.  As we already have a flag that can be used as a workaround I don't see
> > a reason to add another more specific one.  That just makes it a
> > lesser incentive
> > for people to fix their code.
> 
> Well, -fno-delete-null-pointer-checks is a big hammer, that disables way
> to many optimizations than really necessary.

But then perhaps it will be better incentive for the projects to fix their
cruft.  With a specialized option they will keep broken code forever.

	Jakub

On Jan 19, 2016, at 6:41 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> But then perhaps it will be better incentive for the projects to fix their
> cruft.  With a specialized option they will keep broken code forever.

Flags are forever.

> On Jan 19, 2016, at 6:41 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> > But then perhaps it will be better incentive for the projects to fix their
> > cruft.  With a specialized option they will keep broken code forever.
> 
> Flags are forever.

OK, in https://gcc.gnu.org/ml/gcc-patches/2016-01/msg01435.html I included the
recomendation of https://gcc.gnu.org/ml/gcc-patches/2016-01/msg01435.html
and https://gcc.gnu.org/ml/gcc-patches/2016-01/msg01435.html to handle the issue.

Honza

On 2016.01.19 at 13:11 +0100, Jan Hubicka wrote:
> according to Trevor, the assumption about THIS pointer being non-NULL breaks
> several bigger C++ packages (definitly including Firefox, but I believe
> kdevelop was mentioned, too).  This patch makes the feature to be controlable
> by a dedicated flag.  I am not sure about the default. We now have ubsan check
> for the bug so I would hope the codebases to be updated soon, but it did not
> happen for Firefox for quite a while despite the fact that Martin Liska reported
> it.
> 
> This patch defaults to -fno-null-this-pointer, but I would be OK with changing
> the default and setting it on only in GCC 6. Main point of the patch is to
> avoid need of those packages to be built with -fno-delete-null-pointer-checks
> (which still subsumes the flag).
> 
> The patch is bit inconsistent, becuase C++ FE wil still assume that this pointer
> is non-NULL when expanding multiple inheritance accesses.  We did this from
> very beginning. I do not know FE enough to see if it is easy to change the
> behaviour here or if it is desired.
> 
> Bootstrapped/regtsted x86_64-linux.
> 
> Honza
> 
> 	* c-family/c.opt (fnull-this-pointer): New flag.
> 	(nonnull_arg_p): Honnor flag_null_this_pointer.
> Index: tree.c
> ===================================================================
> --- tree.c	(revision 232553)
> +++ tree.c	(working copy)
> @@ -14016,6 +14022,7 @@ nonnull_arg_p (const_tree arg)
>    /* THIS argument of method is always non-NULL.  */
>    if (TREE_CODE (TREE_TYPE (cfun->decl)) == METHOD_TYPE
>        && arg == DECL_ARGUMENTS (cfun->decl)
> +      && flag_null_this_pointer

This should be:
+      && !flag_null_this_pointer

Honza, can you please repost the patch. Richard said on IRC that he may
reconsider his rejection after all.

I've tested the patch on my Gentoo test machine and it fixes segfaults
in LLVM, QT, Chromium, Kdevelop.

If the patch gets accepted, changes.html and porting_to.html need to be
updated to reflect the new flag.

On Mon, Mar 7, 2016 at 12:35 PM, Markus Trippelsdorf
<markus@trippelsdorf.de> wrote:
> On 2016.01.19 at 13:11 +0100, Jan Hubicka wrote:
>> according to Trevor, the assumption about THIS pointer being non-NULL breaks
>> several bigger C++ packages (definitly including Firefox, but I believe
>> kdevelop was mentioned, too).  This patch makes the feature to be controlable
>> by a dedicated flag.  I am not sure about the default. We now have ubsan check
>> for the bug so I would hope the codebases to be updated soon, but it did not
>> happen for Firefox for quite a while despite the fact that Martin Liska reported
>> it.
>>
>> This patch defaults to -fno-null-this-pointer, but I would be OK with changing
>> the default and setting it on only in GCC 6. Main point of the patch is to
>> avoid need of those packages to be built with -fno-delete-null-pointer-checks
>> (which still subsumes the flag).
>>
>> The patch is bit inconsistent, becuase C++ FE wil still assume that this pointer
>> is non-NULL when expanding multiple inheritance accesses.  We did this from
>> very beginning. I do not know FE enough to see if it is easy to change the
>> behaviour here or if it is desired.
>>
>> Bootstrapped/regtsted x86_64-linux.
>>
>> Honza
>>
>>       * c-family/c.opt (fnull-this-pointer): New flag.
>>       (nonnull_arg_p): Honnor flag_null_this_pointer.
>> Index: tree.c
>> ===================================================================
>> --- tree.c    (revision 232553)
>> +++ tree.c    (working copy)
>> @@ -14016,6 +14022,7 @@ nonnull_arg_p (const_tree arg)
>>    /* THIS argument of method is always non-NULL.  */
>>    if (TREE_CODE (TREE_TYPE (cfun->decl)) == METHOD_TYPE
>>        && arg == DECL_ARGUMENTS (cfun->decl)
>> +      && flag_null_this_pointer
>
> This should be:
> +      && !flag_null_this_pointer
>
> Honza, can you please repost the patch. Richard said on IRC that he may
> reconsider his rejection after all.
>
> I've tested the patch on my Gentoo test machine and it fixes segfaults
> in LLVM, QT, Chromium, Kdevelop.
>
> If the patch gets accepted, changes.html and porting_to.html need to be
> updated to reflect the new flag.

Note that I'll only re-consider if we want to make this flag enabled by default.
(no strong opinion about that - but for example the recent -flifetime-dse
strengthening fallout is similar)

If it's supposed to be a flag people need to use explicitely I still stand with
my suggestion to force them using -fno-delete-null-pointer-checks.

Richard.

> --
> Markus

On Mon, Mar 07, 2016 at 12:57:20PM +0100, Richard Biener wrote:
> > Honza, can you please repost the patch. Richard said on IRC that he may
> > reconsider his rejection after all.
> >
> > I've tested the patch on my Gentoo test machine and it fixes segfaults
> > in LLVM, QT, Chromium, Kdevelop.
> >
> > If the patch gets accepted, changes.html and porting_to.html need to be
> > updated to reflect the new flag.
> 
> Note that I'll only re-consider if we want to make this flag enabled by default.

By enabled by default you mean what exactly?  That by default this will not
be treated as non-NULL anymore?  I'm against that, while there are large
codebases that are coded in "C++", there are plenty of properly written
packages and by deferring this optimization we'd just delay the fixing of
firefox etc.

> (no strong opinion about that - but for example the recent -flifetime-dse
> strengthening fallout is similar)

-flifetime-dse change is different, we default to -flifetime-dse=2, but have
a way for broken packages to workaround their bugs, so that situation is the
same as with -fno-delete-null-pointer-checks.

	Jakub

On Mon, Mar 7, 2016 at 1:03 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Mar 07, 2016 at 12:57:20PM +0100, Richard Biener wrote:
>> > Honza, can you please repost the patch. Richard said on IRC that he may
>> > reconsider his rejection after all.
>> >
>> > I've tested the patch on my Gentoo test machine and it fixes segfaults
>> > in LLVM, QT, Chromium, Kdevelop.
>> >
>> > If the patch gets accepted, changes.html and porting_to.html need to be
>> > updated to reflect the new flag.
>>
>> Note that I'll only re-consider if we want to make this flag enabled by default.
>
> By enabled by default you mean what exactly?  That by default this will not
> be treated as non-NULL anymore?

Yes.

> I'm against that, while there are large
> codebases that are coded in "C++", there are plenty of properly written
> packages and by deferring this optimization we'd just delay the fixing of
> firefox etc.

True.  Then I'm against a new flag.

>> (no strong opinion about that - but for example the recent -flifetime-dse
>> strengthening fallout is similar)
>
> -flifetime-dse change is different, we default to -flifetime-dse=2, but have
> a way for broken packages to workaround their bugs, so that situation is the
> same as with -fno-delete-null-pointer-checks.

Well, if anybody would have asked I'd have said we don't want that fine-grained
control either there - people could have just used -fno-lifetime-dse.  Unless
we want to change default behavior back to what GCC 5 did, but see above.

Richard.

>         Jakub

On 2016.03.07 at 13:03 +0100, Jakub Jelinek wrote:
> On Mon, Mar 07, 2016 at 12:57:20PM +0100, Richard Biener wrote:
> > (no strong opinion about that - but for example the recent -flifetime-dse
> > strengthening fallout is similar)
> 
> -flifetime-dse change is different, we default to -flifetime-dse=2, but have
> a way for broken packages to workaround their bugs, so that situation is the
> same as with -fno-delete-null-pointer-checks.

If the situation was the same there would be no -flifetime-dse=2 flag at
all. Affected users would be told to use -fno-lifetime-dse for broken
code, just like they are told to use -fno-delete-null-pointer-checks.

On Mon, Mar 7, 2016 at 1:14 PM, Markus Trippelsdorf
<markus@trippelsdorf.de> wrote:
> On 2016.03.07 at 13:03 +0100, Jakub Jelinek wrote:
>> On Mon, Mar 07, 2016 at 12:57:20PM +0100, Richard Biener wrote:
>> > (no strong opinion about that - but for example the recent -flifetime-dse
>> > strengthening fallout is similar)
>>
>> -flifetime-dse change is different, we default to -flifetime-dse=2, but have
>> a way for broken packages to workaround their bugs, so that situation is the
>> same as with -fno-delete-null-pointer-checks.
>
> If the situation was the same there would be no -flifetime-dse=2 flag at
> all. Affected users would be told to use -fno-lifetime-dse for broken
> code, just like they are told to use -fno-delete-null-pointer-checks.

Yes, I said I would have told them that.  But appearantly Jason had a
different opinion...

Richard.

> --
> Markus

Hi,
so I am not sure what is the consensus. Here are my two cents.
I added the code motivated by looking into multiple inheritance code, where
we often wind to

if (this != null)
  foo = this + offset;
else
  foo = NULL;

which is redundant.  I built libreoffice+LTO with and without this change and
get the following

   text         data     bss    dec     
61207687        3461668  480368 65149723        trunk
60849535        3461612  480496 64791643        disabling special code for THIS
60599827        3461612  480496 64541935        -fno-delete-null-pointer-checks

I.e. about 0.6% code size increase for this analysis ;)
This reproduce over other libreoffice's libraries.

While it is not goal of this optimization to increase the size of
libreoffice binary, I would say that the new coe seems surprisingly active
on this codebase (more than I would expect for one liner of this type).
I think it makes sense to shoot towards making this a default and convincing users
to clean the codebases.

Given that we already have sanitizer feature for few releases reporitng this (I
believe, did not double check), having flag disabled for this release and enabled
for next is only delaying the trouble by a year.

I don't see much extra pain in maintaining this flag and we may document it
as meant for transition and that it will be obsoletted eventually.

If I imagine I was a maintainer of a package that breaks now, I think I would
find an explicit flag in Makefile a decade later more infrmative than having
-fno-delete-null-pointer-checks but no longer being sure if my codebase doesn't
break the rule some other way.

So if we get into consensus that we want flag I will be happy to take care of
the patch (there was suggestion for better name that I can't find right now).
If we decide to get w/o a flag I am happy with current status where the
situation is documented in the changes.html and porting document and there is
well defined workaround.
Honza