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

[Trevor Saunders <tbsaunde at tbsaunde dot org>]

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