wxEventFunctor.m_handlerAddr looks dangerous

[Jan Engelhardt]

In wxWidgets 3.0.0's include/wx/event.h, one can find:

wxEventFunctorFunctor(const Functor& handler)
: m_handler(handler), m_handlerAddr(&handler)
{ }

virtual bool IsMatching(const wxEventFunctor &functor) const
{
if ( wxTypeId(functor) != wxTypeId(*this) )
return false;

typedef wxEventFunctorFunctor<EventTag, Functor> FunctorThis;
const FunctorThis& other = static_cast<const FunctorThis&>(functor);

// The only reliable/portable way to compare two functors is by
// identity:
return m_handlerAddr == other.m_handlerAddr;
}

However, I believe the comparison is not reliable enough. Consider
this sample code which has two different handlers, but whose object
address happens to be the same:

---8<---
#include <wx/wx.h>
#include <cstdio>
class Main : public wxFrame {
public:
Main(void);
private:
void sub1(void);
void sub2(void);
wxButton *button1, *button2;
};
void Main::sub1(void)
{
auto func1 = [this](wxCommandEvent &) {
wxMessageDialog(this, "Hello world!").ShowModal();
};
printf("%p\n", &func1);
button1 = new wxButton(this, wxID_ANY, "Welcome it");
Bind(wxEVT_BUTTON, func1, button1->GetId());
}
void Main::sub2(void)
{
auto func2 = [this](wxCommandEvent &) {
wxMessageDialog(this, "Goodbye cruel world!").ShowModal();
};
printf("%p\n", &func2);
button2 = new wxButton(this, wxID_ANY, "Do away with it");
Bind(wxEVT_BUTTON, func2, button2->GetId());
}
Main::Main(void) : wxFrame(NULL, wxID_ANY, wxEmptyString) {
sub1();
sub2();
auto sz = new wxBoxSizer(wxHORIZONTAL);
sz->Add(button1, 1);
sz->Add(button2, 1);
SetSizer(sz);
sz->SetSizeHints(this);
}
class MyApp : public wxApp {
private: bool OnInit(void) {
(new Main)->Show();
return true;
};
};
IMPLEMENT_APP(MyApp);
--->8---


Breakpoint 1, Main::__lambda0::operator() (__closure=0x756708) at q.cpp:14
14 wxMessageDialog(this, "Hello world!").ShowModal();
(gdb) up
#1 0x000000000040c3d7 in wxEventFunctorFunctor<wxEventTypeTag<wxCommandEvent>, Main::sub1()::__lambda0>::operator()(wxEvtHandler *, wxEvent &) (
this=0x756700, event=...) at /usr/include/wx-3.0/wx/event.h:516
516 m_handler(static_cast<EventArg&>(event));
(gdb) p m_handlerAddr
warning: RTTI symbol not found for class 'wxEventFunctorFunctor<wxEventTypeTag<wxCommandEvent>, Main::sub1()::{lambda(wxCommandEvent&)#1}>'
$1 = (const void *) 0x7fffffffdc80


The special thing about lambdas is that they are not function pointers
(those would be unique), but are, in g++, class-like objects with an
operator() function. Given that, the problem with using the == test
inside IsMatching would also apply to non-lambda functors.

What you would probably want is to use the address of the anonymous
(unique) function inside the lambda which g++ internally calls

000000000040b98e t Main::sub1()::{lambda(wxCommandEvent&)#1}::operator()(wxCommandEvent&) const

That, however, is not exposed in the C++ language.

[Vadim Zeitlin]

On Wed, 26 Feb 2014 22:40:57 +0100 (CET) Jan Engelhardt wrote:

JE> In wxWidgets 3.0.0's include/wx/event.h, one can find:
JE>
JE> wxEventFunctorFunctor(const Functor& handler)
JE> : m_handler(handler), m_handlerAddr(&handler)
JE> { }
JE>
JE> virtual bool IsMatching(const wxEventFunctor &functor) const
JE> {
JE> if ( wxTypeId(functor) != wxTypeId(*this) )
JE> return false;
JE>
JE> typedef wxEventFunctorFunctor<EventTag, Functor> FunctorThis;
JE> const FunctorThis& other = static_cast<const FunctorThis&>(functor);
JE>
JE> // The only reliable/portable way to compare two functors is by
JE> // identity:
JE> return m_handlerAddr == other.m_handlerAddr;
JE> }
JE>
JE> However, I believe the comparison is not reliable enough. Consider
JE> this sample code which has two different handlers, but whose object
JE> address happens to be the same:

I do see the problem (and thanks for reporting it), but I don't see any
solution. How else can we compare them, really?

This is just another argument in favour of completely dropping all this
IsMatching() stuff. We should return some (opaque) wxEventHandlerConnection
object from Bind() and have Unbind() taking it. But this:

1. Wouldn't help existing code.
2. Still needs to be done...


Regards,
VZ

[Jan Engelhardt]

Post by Vadim Zeitlin
JE>
JE> wxEventFunctorFunctor(const Functor& handler)
JE> : m_handler(handler), m_handlerAddr(&handler)
JE> { }
JE> What you would probably want is to use the address of the anonymous
JE> (unique) function inside the lambda which g++ internally calls
JE> 000000000040b98e t Main::sub1()::{lambda(wxCommandEvent&)#1}::operator()(wxCommandEvent&) const
JE> That, however, is not exposed in the C++ language.
I do see the problem (and thanks for reporting it), but I don't see any
solution. How else can we compare them, really?

It actually does seem to be possible to refer to possibly-anonymous
functors function... check out this gem:

diff --git a/include/wx/event.h b/include/wx/event.h
index 61ec19c..5278a13 100644
--- a/include/wx/event.h
+++ b/include/wx/event.h
@@ -501,7 +501,7 @@ public:
typedef typename EventTag::EventClass EventArg;

wxEventFunctorFunctor(const Functor& handler)
- : m_handler(handler), m_handlerAddr(&handler)
+ : m_handler(handler), m_handlerAddr(&Functor::operator())
{ }

virtual void operator()(wxEvtHandler *WXUNUSED(handler), wxEvent& event)
@@ -536,7 +536,7 @@ private:
Functor m_handler;

// Use the address of the original functor for comparison in IsMatching:
- const void *m_handlerAddr;
+ decltype(&Functor::operator()) m_handlerAddr;

// Provide a dummy default ctor for type info purposes
wxEventFunctorFunctor() { }


This would seem to require no changes to user code.

The alternative to
decltype(&Functor::operator()) m_handlerAddr;
is
__typeof__(&Functor::operator()) m_handlerAddr;
Other than that, there is
const void *m_handlerAddr;
in combination with
m_handlerAddr(reinterpret_cast<const void *>(&Functor::operator()))
but this will elicit diagnostics (and will raise eyebrows).


There is another way, someway, which is replacing in event.h

m_handlerAddr(&m_handler)

because the passed functor is copied during construction of a
wxEventFunctorFunctor instance. This is likely to break Unbinding;
that said, it would seem one cannot unbind functors that have already
gone out of scope anyway…

[Vadim Zeitlin]

On Fri, 28 Feb 2014 02:31:58 +0100 (CET) Jan Engelhardt wrote:

JE> On Thursday 2014-02-27 23:47, Vadim Zeitlin wrote:
JE>
JE> >JE> In wxWidgets 3.0.0's include/wx/event.h, one can find:
JE> >JE>
JE> >JE> wxEventFunctorFunctor(const Functor& handler)
JE> >JE> : m_handler(handler), m_handlerAddr(&handler)
JE> >JE> { }
JE> >
JE> >JE> What you would probably want is to use the address of the anonymous
JE> >JE> (unique) function inside the lambda which g++ internally calls
JE> >JE> 000000000040b98e t Main::sub1()::{lambda(wxCommandEvent&)#1}::operator()(wxCommandEvent&) const
JE> >JE> That, however, is not exposed in the C++ language.
JE> >
JE> > I do see the problem (and thanks for reporting it), but I don't see any
JE> >solution. How else can we compare them, really?
JE>
JE> It actually does seem to be possible to refer to possibly-anonymous
JE> functors function... check out this gem:
JE>
JE> diff --git a/include/wx/event.h b/include/wx/event.h
JE> index 61ec19c..5278a13 100644
JE> --- a/include/wx/event.h
JE> +++ b/include/wx/event.h
JE> @@ -501,7 +501,7 @@ public:
JE> typedef typename EventTag::EventClass EventArg;
JE>
JE> wxEventFunctorFunctor(const Functor& handler)
JE> - : m_handler(handler), m_handlerAddr(&handler)
JE> + : m_handler(handler), m_handlerAddr(&Functor::operator())
JE> { }
JE>
JE> virtual void operator()(wxEvtHandler *WXUNUSED(handler), wxEvent& event)
JE> @@ -536,7 +536,7 @@ private:
JE> Functor m_handler;
JE>
JE> // Use the address of the original functor for comparison in IsMatching:
JE> - const void *m_handlerAddr;
JE> + decltype(&Functor::operator()) m_handlerAddr;
JE>
JE> // Provide a dummy default ctor for type info purposes
JE> wxEventFunctorFunctor() { }
JE>
JE>
JE> This would seem to require no changes to user code.
JE>
JE> The alternative to
JE> decltype(&Functor::operator()) m_handlerAddr;
JE> is
JE> __typeof__(&Functor::operator()) m_handlerAddr;
JE> Other than that, there is
JE> const void *m_handlerAddr;
JE> in combination with
JE> m_handlerAddr(reinterpret_cast<const void *>(&Functor::operator()))
JE> but this will elicit diagnostics (and will raise eyebrows).

We still support (even in 3.2) compilers without support for decltype nor
__typeof__ (VC++ < 10, I think), so we'd have to use the cast to void*. And
I'm not even sure if the usual union trick could be used here to avoid the
warnings.

And there is also the fact that wxEventFunctorFunctor is used for function
pointers, if I'm reading the code correctly, which don't have operator() so
we'd need to separate these two cases...

JE> There is another way, someway, which is replacing in event.h
JE>
JE> m_handlerAddr(&m_handler)
JE>
JE> because the passed functor is copied during construction of a
JE> wxEventFunctorFunctor instance.

Sorry, is something missing here? Otherwise I don't understand what should
be replaced with what.

Thanks for looking at this!
VZ

[Jan Engelhardt]

Post by Vadim Zeitlin
We still support (even in 3.2) compilers without support for decltype nor
__typeof__ (VC++ < 10, I think), so we'd have to use the cast to void*. And
I'm not even sure if the usual union trick could be used here to avoid the
warnings.

I do not think so; to declare that union, you would need to know the type
of the operator() function, which... you don't except by way of
typeof()/decltype().

Post by Vadim Zeitlin
And there is also the fact that wxEventFunctorFunctor is used for function
pointers, if I'm reading the code correctly, which don't have operator() so
we'd need to separate these two cases...

Correctly spotted.

Post by Vadim Zeitlin
JE> There is another way, someway, which is replacing in event.h
JE> m_handlerAddr(&m_handler)
JE> because the passed functor is copied during construction of a
JE> wxEventFunctorFunctor instance.

diff --git a/include/wx/event.h b/include/wx/event.h
index 61ec19c..8dd2946 100644
--- a/include/wx/event.h
+++ b/include/wx/event.h
@@ -501,7 +501,7 @@ public:
typedef typename EventTag::EventClass EventArg;

wxEventFunctorFunctor(const Functor& handler)
- : m_handler(handler), m_handlerAddr(&handler)
+ : m_handler(handler), m_handlerAddr(reinterpret_cast<const void *>(&m_handler))
{ }

virtual void operator()(wxEvtHandler *WXUNUSED(handler), wxEvent& event)
------

(Again, only meant for class-like functors, not function pointers.)
I however do not advocate this patch; it very likely will break
Unbind(classlikefunctor) as IsMatching() will always return false.

[Vadim Zeitlin]

On Fri, 28 Feb 2014 03:03:37 +0100 (CET) Jan Engelhardt wrote:

JE> On Friday 2014-02-28 02:42, Vadim Zeitlin wrote:
JE> >
JE> > We still support (even in 3.2) compilers without support for decltype nor
JE> >__typeof__ (VC++ < 10, I think), so we'd have to use the cast to void*. And
JE> >I'm not even sure if the usual union trick could be used here to avoid the
JE> >warnings.
JE>
JE> I do not think so; to declare that union, you would need to know the type
JE> of the operator() function, which... you don't except by way of
JE> typeof()/decltype().

I thought of using "void (*)()" in the union. This would clearly be still
ugly and still require a cast but perhaps we could at least make it compile
without warnings...

JE> diff --git a/include/wx/event.h b/include/wx/event.h
JE> index 61ec19c..8dd2946 100644
JE> --- a/include/wx/event.h
JE> +++ b/include/wx/event.h
JE> @@ -501,7 +501,7 @@ public:
JE> typedef typename EventTag::EventClass EventArg;
JE>
JE> wxEventFunctorFunctor(const Functor& handler)
JE> - : m_handler(handler), m_handlerAddr(&handler)
JE> + : m_handler(handler), m_handlerAddr(reinterpret_cast<const void *>(&m_handler))
JE> { }
JE>
JE> virtual void operator()(wxEvtHandler *WXUNUSED(handler), wxEvent& event)
JE> ------
JE>
JE> (Again, only meant for class-like functors, not function pointers.)
JE> I however do not advocate this patch; it very likely will break
JE> Unbind(classlikefunctor) as IsMatching() will always return false.

Yes, so it's not really going to improve the situation.

I persist in thinking that returning connection identifier from Bind() and
using it in Unbind() is the only way to make this work correctly in all
cases, and it's probably the simplest one, too, on balance. Unfortunately I
really don't see myself doing this now, I'd need to run into a problem with
Unbind() in one of my projects to motivate me to do this :-/ So while any
patches doing this would be definitely welcome, I don't have any plans to
do anything about this right now, except mentioning the potential problem
in the documentation.

Regards,
VZ

[Jan Engelhardt]

Post by Vadim Zeitlin
JE> I do not think so; to declare that union, you would need to know the type
JE> of the operator() function, which... you don't except by way of
JE> typeof()/decltype().
I thought of using "void (*)()" in the union. This would clearly be still
ugly and still require a cast but perhaps we could at least make it compile
without warnings...
JE> typedef typename EventTag::EventClass EventArg;
JE>
JE> wxEventFunctorFunctor(const Functor& handler)
JE> - : m_handler(handler), m_handlerAddr(&handler)
JE> + : m_handler(handler), m_handlerAddr(reinterpret_cast<const void *>(&m_handler))

reinterpret_cast<void (wxEventFunctorFunctor::*)(void)>(&m_handler)

that seems to fly without warnings.

Post by Vadim Zeitlin
I persist in thinking that returning connection identifier from Bind() and
using it in Unbind() is the only way to make this work correctly in all
cases [...]

I concur.

Post by Vadim Zeitlin
I don't have any plans to do anything about this right now, except
mentioning the potential problem in the documentation.

Good enough for me. (Since I don't use Unbind and thus don't run into
problems ATM - as Unbind seems to be the only place requiring
IsMatching.)