On Mon, Mar 7, 2016 at 12:35 PM, Markus Trippelsdorf <mar...@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
