Re: ubsan PATCH to fix compile-time hog with operator overloading (PR sanitizer/78208)

[Marek Polacek <polacek@redhat.com>]

On Thu, Nov 17, 2016 at 09:10:51AM +0100, Jakub Jelinek wrote:
> On Wed, Nov 16, 2016 at 04:29:05PM -0800, Marek Polacek wrote:
> > Sorry.  So consider the following:
> > 
> > class S
> > {
> >   virtual void foo () = 0;
> > };
> > 
> > struct T {
> >   T &operator << (const char *s);
> > };
> > 
> > T t;
> > 
> > void
> > S::foo ()
> > {
> >   t << "a" << "b" << "c";
> > }
> > 
> > Before
> > 1498               if (flag_sanitize & (SANITIZE_NULL | SANITIZE_ALIGNMENT))
> > 1499                 ubsan_maybe_instrument_member_call (stmt, is_ctor);
> > 
> > the stmt will be
> > 
> > T::operator<< (T::operator<< (T::operator<< (&t, "a"), "b"), "c")
> > 
> > after ubsan_maybe_instrument_member_call it will be
> > 
> > T::operator<< (UBSAN_NULL (SAVE_EXPR <T::operator<< (T::operator<< (&t, "a"), "b")>, 4B, 0);, SAVE_EXPR <T::operator<< (T::operator<< (&t, "a"), "b")>;, "c")
> > 
> > and that's what is saved into the hash table.  Then another stmt will be the
> > inner
> > 
> > T::operator<< (T::operator<< (&t, "a"), "b")
> > 
> > which we instrument and put into the hash table, and so on.  But those
> > SAVE_EXPRs aren't the same.  So we have a T::operator<< call that has nested
> > T::operator<< calls and we kind of recursively instrument all of them.
> 
> But those SAVE_EXPRs are the same.  If you put a breakpoint on the:
> 489	  if (op)
> 490	    CALL_EXPR_ARG (stmt, 0) = op;
> line 490 in c-ubsan.c, on the above short testcase is called 2 times
> (the T<<operator<< (&t, "a") is not instrumented, as the argument is known
> not to be NULL), rather than 3 times.
> When trying larger testcase like below I count ~ 120 calls (counted by hand,
> so it is possible it was the right 119 times).
> If this has not been linear, the testcase would be compiling for years.
> Yes, there will be 119 SAVE_EXPRs, and when you -fdump-tree-original it,
> it will be just insanely huge, but each SAVE_EXPR appears exactly twice
> in its containing SAVE_EXPR and the second time cp_genericize_r sees
> the SAVE_EXPR, it will do the
> 1138	  /* Other than invisiref parms, don't walk the same tree twice.  */
> 1139	  if (p_set->contains (stmt))
> 1140	    {
> 1141	      *walk_subtrees = 0;
> 1142	      return NULL_TREE;
> 1143	    }
> early exit.
 
Clearly I misread things here.  I blame the cold I've caught!

In any case, I'm sorry for wasting your time and I'll just close the PR.

	Marek