Mbed-os: Callback object comparison not working

[SeppoTakalo]

Description

Example code:

#include "mbed.h"

void another_callback(nsapi_event_t status, intptr_t param)
{
    printf("another_callback called\r\n");
}

mbed::Callback<void(nsapi_event_t, intptr_t)> test(mbed::Callback<void(nsapi_event_t, intptr_t)> arg)
{
    return arg;
}

int main()
{
    printf("Status callback example!\r\n");

    mbed::Callback<void(nsapi_event_t, intptr_t)> status_cb = callback(&another_callback);
    mbed::Callback<void(nsapi_event_t, intptr_t)> another_cb = test(status_cb);

    if (another_cb == status_cb) {
        printf("Callbacks are the same\r\n");
    } else {
        printf("Callbacks are NOT SAME\r\n");
    }
}

I would expect callbacks to return true for the comparison operation as those are exacly copies of the same object.

Directly copying the callbacks leads the comparison to return true, but passing it as a parameter to functions seems to lead it to return false.

Issue request type

[ ] Question
[ ] Enhancement
[X] Bug

[SeppoTakalo]

@geky Please review

FYI: @kjbracey-arm

[ciarmcom]

Internal Jira reference: https://jira.arm.com/browse/MBOCUSTRIA-770

[kjbracey-arm]

Basic cause is that the copy helper for a "function" callback only copies the function pointer, not the "object" pointer, leaving it uninitialised, so the memcmp sees a difference in that second word of the callback.

I can't really follow the code though - I don't really understand why the complexity of the copy helper, compared to the assumption that memcmp is adequate for an equality test.

Seems contradictory. Either there should be an equal helper, or no copy helper, I would have thought.

Adding an equal helper seems like the minimal change consistent with current form. Use it like this:

friend bool operator==(const Callback &l, const Callback &r)
{
    return l._ops == r._ops && (l._ops == NULL || l._ops->equal(l, r));
}

[kjbracey-arm]

Still, that's only going to increase code size further. Can we drop the copy helper, and just memcpy the thing instead? It seems that we're making so many assumptions about layout, that we effectively only work on raw pointers that would be memcpyable. What's the use case for the copy and dtor?

[SeppoTakalo]

@geky We are still waiting for your input.

Is this going to be fixed, or should I use function pointers instead of Callback objects?

[pan-]

IIRC the equality operator using memcmp was here for _compatibility reasons_ as it was present in the Function class of mbed OS 3 and one wanted to preserve that behaviour when Callback was written.

More generally, it is not possible to implement a correct == operator if there is no equality operator defined for the underlying function like object.

[SeppoTakalo]

Then, regarding the https://github.com/ARMmbed/mbed-os/pull/9424 should I continue to use the function pointer as comparing pointers have known result.

Otherwise we cannot remove particular listeners.

[kjbracey-arm]

Well, we can stick with the memcmp, but make sure that function_move does a memset to zero before its new, like its generate does. I think that will cover it.

[kjbracey-arm]

Actually, I think the copy constructor should have the memset. That also covers copy construction from unattached Callbacks, which seem to have the same issue.

[SeppoTakalo]

Kevin's suggestion works, so this is now fixed in https://github.com/ARMmbed/mbed-os/pull/9424

To be more precise: https://github.com/ARMmbed/mbed-os/pull/9424/commits/95f311c6ecc2f93d86066a1987e14a2b1e828997

[evedon]

As per last comment, the issue was fixed in #9424