On Tue, Jan 19, 2016 at 2:18 PM, Trevor Saunders <tbsau...@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
