[Annotalysis] Fixes virtual method calls when type is unknown

This bug fixes a failure in annotalysis that is caused when gcc does
not return the correct static type for the callee of a virtual method.

Bootstrapped and passed GCC regression testsuite on x86_64-unknown-linux-gnu.

Okay for branches/annotalysis and google/main?

  -DeLesley


2011-06-30   DeLesley Hutchins  <delesley@google.com>
        * tree-threadsafe-analyze.c (handle_call_gs): Fixes case where
the virtual
        method callee has unknown type.


+            {
+              fdecl = lang_hooks.get_virtual_function_decl (callee, objtype);
+            }
         }
     }

On 11-06-30 13:01 , Delesley Hutchins wrote:
> This bug fixes a failure in annotalysis that is caused when gcc does
> not return the correct static type for the callee of a virtual method.
>
> Bootstrapped and passed GCC regression testsuite on x86_64-unknown-linux-gnu.
>
> Okay for branches/annotalysis and google/main?

OK with some minor formatting nits below.

> 2011-06-30   DeLesley Hutchins<delesley@google.com>
>          * tree-threadsafe-analyze.c (handle_call_gs): Fixes case where
> the virtual
>          method callee has unknown type.

Blank line after address line.
Watch for line wrapping past 80 columns.


>             tree objtype = TREE_TYPE (TREE_TYPE (OBJ_TYPE_REF_OBJECT (callee)));
> -          fdecl = lang_hooks.get_virtual_function_decl (callee, objtype);
> +          /* Check to make sure objtype is a valid type.
> +           * OBJ_TYPE_REF_OBJECT does not always return the correct
> static type of the callee.
> +           * For example:  Given  foo(void* ptr) { ((Foo*)
> ptr)->doSomething(); }
> +           * objtype will be void, not Foo.  Whether or not this
> happens depends on the details
> +           * of how a particular call is lowered to GIMPLE, and there
> is no easy fix that works
> +           * in all cases.  For now, we simply rely on gcc's type
> information; if that information
> +           * is not accurate, then the analysis will be less precise.
> +           */

Don't use leading '*' in multi-line comments.
s/gcc/GCC/

> +          if (TREE_CODE(objtype) == RECORD_TYPE)  /* if objtype is an
> object type... */

Comments at the end of a line are rarely used.  No need to add one in 
this case.

> +            {
> +              fdecl = lang_hooks.get_virtual_function_decl (callee, objtype);
> +            }

No need to add braces here.


Thanks.  Diego.

On Thu, Jun 30, 2011 at 12:09 PM, Diego Novillo <dnovillo@google.com> wrote:
>
> On 11-06-30 13:01 , Delesley Hutchins wrote:
>>
>> Okay for branches/annotalysis and google/main?
>
> OK with some minor formatting nits below.

Please also cherry-pick this into the google/gcc_4-6 branch.  I've
sent you instructions out-of-band for doing so.

Ollie

Hi,

On Thu, Jun 30, 2011 at 10:01:58AM -0700, Delesley Hutchins wrote:
> This bug fixes a failure in annotalysis that is caused when gcc does
> not return the correct static type for the callee of a virtual method.
> 
> Bootstrapped and passed GCC regression testsuite on x86_64-unknown-linux-gnu.
> 
> Okay for branches/annotalysis and google/main?
> 
>   -DeLesley
> 
> 
> 2011-06-30   DeLesley Hutchins  <delesley@google.com>
>         * tree-threadsafe-analyze.c (handle_call_gs): Fixes case where
> the virtual
>         method callee has unknown type.
> 
> 
> Index: gcc/testsuite/g++.dg/thread-ann/thread_annot_lock-79.C
> ===================================================================
> --- gcc/testsuite/g++.dg/thread-ann/thread_annot_lock-79.C	(revision 0)
> +++ gcc/testsuite/g++.dg/thread-ann/thread_annot_lock-79.C	(revision 0)
> @@ -0,0 +1,54 @@
> +// Test virtual method calls in cases where the static type is unknown
> +// This is a "good" test case that should not incur any thread safety warning.
> +// { dg-do compile }
> +// { dg-options "-Wthread-safety -O" }
> +
> +#include "thread_annot_common.h"
> +
> +class Foo {
> +public:
> +  Mutex m;
> +
> +  Foo();
> +  virtual ~Foo();
> +  virtual void doSomething() EXCLUSIVE_LOCKS_REQUIRED(m) = 0;
> +};
> +
> +
> +class FooDerived : public Foo {
> +public:
> +  FooDerived();
> +  virtual ~FooDerived();
> +  virtual void doSomething();
> +};
> +
> +
> +/**
> + * This is a test case for a bug wherein annotalysis would crash when
> analyzing
> + * a virtual method call.
> + *
> + * The following three functions represent cases where gcc loses the static
> + * type of foo in the expression foo->doSomething() when it drops down to
> + * gimple.  Annotalysis should technically issue a thread-safe
> warning in these
> + * cases, but because the type is missing, it should silently accept them
> + * instead.  See tree-threadsafe-analyze.c::handle_call_gs.
> + */
> +
> +// reinterpret_cast
> +void foo1(void* ptr)
> +{
> +  reinterpret_cast<Foo*>(ptr)->doSomething();
> +}
> +
> +// C-style cast
> +void foo2(int* buf)
> +{
> +  ((Foo*) buf)->doSomething();
> +}
> +
> +// new expression, with no local variable
> +void foo3()
> +{
> +  (new FooDerived)->doSomething();
> +}
> +
> Index: gcc/tree-threadsafe-analyze.c
> ===================================================================
> --- gcc/tree-threadsafe-analyze.c	(revision 175385)
> +++ gcc/tree-threadsafe-analyze.c	(working copy)
> @@ -2446,7 +2446,18 @@
>        if (TREE_CODE (callee) == OBJ_TYPE_REF)
>          {
>            tree objtype = TREE_TYPE (TREE_TYPE (OBJ_TYPE_REF_OBJECT (callee)));
> -          fdecl = lang_hooks.get_virtual_function_decl (callee, objtype);
> +          /* Check to make sure objtype is a valid type.
> +           * OBJ_TYPE_REF_OBJECT does not always return the correct
> static type of the callee.
> +           * For example:  Given  foo(void* ptr) { ((Foo*)
> ptr)->doSomething(); }
> +           * objtype will be void, not Foo.  Whether or not this
> happens depends on the details
> +           * of how a particular call is lowered to GIMPLE, and there
> is no easy fix that works
> +           * in all cases.  For now, we simply rely on gcc's type
> information; if that information
> +           * is not accurate, then the analysis will be less precise.
> +           */
> +          if (TREE_CODE(objtype) == RECORD_TYPE)  /* if objtype is an
> object type... */
> +            {
> +              fdecl = lang_hooks.get_virtual_function_decl (callee, objtype);
> +            }
>          }
>      }

If you in some way rely on static types of those pointers, you may be
in for a number of unpleasant surprises, because these types do not
really have any meaning at all.  Just out of curiosity, I does your
analysis crash also on the testcase below (add the Mutex field if it
is necessary)?

Also, I have just very briefly glanced at the code in handle_call_gs
but still it seems to me that the code dealing with virtual calls
assumes virtual functions are never overridden...?

And pretty please, use -p diff option when creating patches to be
posted here.

Thanks,

Martin


namespace std {
  typedef __SIZE_TYPE__ size_t;
}
inline void* operator new(std::size_t, void* __p) throw() {
  return __p;
}
extern "C" void abort (void);

struct Foo {
public:
  char stuff[1024];
};

class Bar {
public:
  virtual int test (void) { return 1; }
};

Foo ft;
Foo *f = &ft;

int main()
{
  Bar *b;
  b = new (&f) Bar();
  return b->test();
}

> If you in some way rely on static types of those pointers, you may be
> in for a number of unpleasant surprises, because these types do not
> really have any meaning at all.

Annotalysis is just a static analyzer, so if the types are somehow
inaccurate (as they are in certain cases), then the only ill effect
will be that the analysis issues a warning where it shouldn't, or
fails to issue a warning where it should.  Of course, no analysis
that's based on types will ever be sound for C++, because C++ is not
type-safe. We merely assume that the declared types represent the
intentions of the programmer, and hope that the declared types are
preserved in gimple with some degree of fidelity.

> Just out of curiosity, I does your
> analysis crash also on the testcase below (add the Mutex field if it
> is necessary)?

No, the static type of b (in b->test()) remains Bar* in gimple, so the
analysis works fine (i.e. the analyzer will not crash).

> Also, I have just very briefly glanced at the code in handle_call_gs
> but still it seems to me that the code dealing with virtual calls
> assumes virtual functions are never overridden...?

It doesn't matter whether it's overridden or not, since any overriding
versions are required to have the same type signature as the original.
 Annotalysis is merely an extended type-checker.

> And pretty please, use -p diff option when creating patches to be posted here.

My apologies; will do.

  -DeLesley

Hi,

On Fri, Jul 01, 2011 at 09:31:30AM -0700, Delesley Hutchins wrote:
> > Just out of curiosity, I does your
> > analysis crash also on the testcase below (add the Mutex field if it
> > is necessary)?
> 
> No, the static type of b (in b->test()) remains Bar* in gimple, so the
> analysis works fine (i.e. the analyzer will not crash).
> 

Sorry, I've had a look only just now and you do the analysis before
early inlining and as long as you do that, the type is really Bar and
you are fine as far as far as placement new is concerned.

However, on the second thought, I still think you need to handle the
case when BINFO_VIRTUALS are NULL in cp_get_virtual_function_decl
(BTW, there is a non-langhook variant in gimple-fold.c called
gimple_get_virt_method_for_binfo) because of the case below.

Martin


// { dg-do compile }
// { dg-options "-Wthread-safety -O" }
//#include "thread_annot_common.h"

class Anc {
public:
  int a;
};

class Foo : public Anc {
public:
  //  Mutex m;

  Foo();
  virtual ~Foo();
  virtual void doSomething() = 0;// EXCLUSIVE_LOCKS_REQUIRED(m) = 0;
};


class FooDerived : public Foo {
public:
  FooDerived();
  virtual ~FooDerived();
  virtual void doSomething();
};

// reinterpret_cast
void foo1(Anc* ptr)
{
  reinterpret_cast<Foo*>(ptr)->doSomething();
}

// C-style cast
void foo2(Anc* buf)
{
  ((Foo*) buf)->doSomething();
}

This is indeed a problem.  Good catch; thanks!

  -DeLesley

On Fri, Jul 1, 2011 at 11:50 AM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> On Fri, Jul 01, 2011 at 09:31:30AM -0700, Delesley Hutchins wrote:
>> > Just out of curiosity, I does your
>> > analysis crash also on the testcase below (add the Mutex field if it
>> > is necessary)?
>>
>> No, the static type of b (in b->test()) remains Bar* in gimple, so the
>> analysis works fine (i.e. the analyzer will not crash).
>>
>
> Sorry, I've had a look only just now and you do the analysis before
> early inlining and as long as you do that, the type is really Bar and
> you are fine as far as far as placement new is concerned.
>
> However, on the second thought, I still think you need to handle the
> case when BINFO_VIRTUALS are NULL in cp_get_virtual_function_decl
> (BTW, there is a non-langhook variant in gimple-fold.c called
> gimple_get_virt_method_for_binfo) because of the case below.
>
> Martin
>
>
> // { dg-do compile }
> // { dg-options "-Wthread-safety -O" }
> //#include "thread_annot_common.h"
>
> class Anc {
> public:
>  int a;
> };
>
> class Foo : public Anc {
> public:
>  //  Mutex m;
>
>  Foo();
>  virtual ~Foo();
>  virtual void doSomething() = 0;// EXCLUSIVE_LOCKS_REQUIRED(m) = 0;
> };
>
>
> class FooDerived : public Foo {
> public:
>  FooDerived();
>  virtual ~FooDerived();
>  virtual void doSomething();
> };
>
> // reinterpret_cast
> void foo1(Anc* ptr)
> {
>  reinterpret_cast<Foo*>(ptr)->doSomething();
> }
>
> // C-style cast
> void foo2(Anc* buf)
> {
>  ((Foo*) buf)->doSomething();
> }
>
>
>