← Back to index

Problem copying std::function

Flyingsand
#21321
Hello.

I have an issue where I get a crash when starting a custom-built timer for my game, but only in release mode. After thinking about it more, I believe I know what the problem is, but I want to check here to make sure.

My timer stores its callback in a std::function object. Whenever a timer is created and started, it adds itself to a global list of timers that run every iteration of the game loop. The global timers list is initialized with the memory allocated by the platform layer, and allocates its nodes using a pool allocation strategy. The crash happens when the copy-assignment operator is executed when assigning the timer object to the data of the list node. I get a EXC_BAD_ACCESS (GPFLT) exception, which indicates some kind of error with the memory I'm operating with.

Since the timer object is created in the game (DLL) layer (as a local variable) and then copied into the global timers list (which was initialized with the memory allocated by the platform layer), my suspicion is that the exception is happening because it can't copy executable code across module boundaries. Either that, or std::function can't reference executable code across a module boundary. This either isn't happening, or isn't checked when in debug mode. That's the only thing I can think of that is causing this crash. Does that make sense?

If that is indeed the problem, is there a solution to this? If I don't allocate the global timers list with the memory from the platform layer (and just use malloc for every node), everything works fine. But that means the timers won't be serialized with the rest of the game memory.

Thanks.
Edited by Flyingsand on Reason: Initial post
Mārtiņš Možeiko
#21322
std::function does not copy executable code. That would be too much overhead and would not work on many platforms where allocating executable memory pages is not allowed).

Does the crash happens after dll reload or in same session?
You talk about serializing timer list - is this for saving memory between reloads? Because that won't work as std::function will keep pointer to executable code it needs to execute. And executable code changes addresses when compiler runs next time.

Are you using static C/C++ runtime for compiling your platform & game code? Because std::function does allocations on heap, so if heap is different something can break there.

Another potential reason is that you are using different compiler options (optimization level, _DEBUG vs NDEBUG define, etc...) Because std::function is header-only implementation, which means its data layout is heavily dependent on your compiler settings. If it has in its implementation #ifdef _DEBUG or similar then the data layout could be different in place where you create std::function from where you use it.

I would suggest to debug crash more carefully - what is call stack, what instruction exactly crashes, is it reading or writing, what kind of data it is accessing, etc...
Flyingsand
#21324
No, the crash does not happen across DLL reload. It's during the same session. I have spent a fair bit of time debugging it, but it's not giving me a whole lot to work with. That being said, I'm definitely open to tips on how to debug it better if there's something I'm missing.

I'll walk through what's happening and provide the stack trace to better illustrate it.

Here is the timer: 
 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
struct bbTimer {
    fsr32 duration;
    fsr32 interval;
    fsr32 t, ti;
    bool active;
    
    std::function<void(bbTimer*)> callback;
    
    bbTimer(fsr32 duration, fsr32 interval);
    bbTimer(fsr32 duration, fsr32 interval, void(*func)(bbTimer*));
    
    bbTimer(const bbTimer &copy);
    bbTimer& operator=(const bbTimer&);
    
    template <typename F>
    bbTimer(fsr32 duration, fsr32 interval, F func)
        : bbTimer(duration, interval)
    {
        callback = func;
    }
    
    void start();
    void stop();
};


I can create a timer like this: 
1
2
3
4
5
bbTimer timer(2.f, bbTimeInterval60FPS);
timer.callback = [](bbTimer *t) {
    printf("hi, %f\n", t->t);
};
timer.start();


Calling start on the timer adds itself to the timers list: 
1
2
3
4
5
void
bbTimer::start() {
    active = true;
    timers.push_back(*this);
}


The timers list is my own custom implementation of a cache-friendly linked list that can use memory that I pass to it. (Just to reiterate, if I don't initialize the timers list with the allocated memory from the platform layer so it uses malloc/free for each node, everything works fine in both debug and release configurations.)

push_back ends up calling allocate_and_init_node, which triggers the copy-assignment operator of bbTimer: 
 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
template <typename T, typename Allocator>
typename fsList<T, Allocator>::nodeType*
fsList<T, Allocator>::allocate_and_init_node(const T& data, nodeType *next, nodeType *prev)
{
    nodeType *node = (nodeType *)_allocator.allocate(sizeof(nodeType));
	if (node) {
		fs_memclear(node, sizeof(nodeType));
		node->data = data; <<<<<<<<<<<<<<<<<<< Copy-assignment here!
		node->next = next;
		node->prev = prev;
	}
	
    return node;
}

bbTimer&
bbTimer::operator=(const bbTimer &rhs)
{
    if (this != &rhs) {
        duration = rhs.duration; <<<<<<<<<<<<<<< Crashes here!
        interval = rhs.interval;
        active = rhs.active;
        t = rhs.t;
        ti = rhs.ti;
        callback = rhs.callback;
    }
    return (*this);
}


Here is the stack trace:
link

And here is the stack trace in assembly (looks like it's on a mov instruction):
link

In terms of serialization, you're absolutely right, it won't work with std::function for the same reason it won't for function pointers. So there's no firm reason why I need to allocate the list with the game memory (I made this assumption earlier when I didn't realize it wouldn't work). However, even though I can fix it pretty easily by just using malloc/free for each node, I still want to get to the bottom of this problem so I can actually understand what is wrong. (And so I don't do it again! :p)

Thanks for your help!
Edited by Flyingsand on
Mārtiņš Možeiko
#21325
What is rbx value on that second screenshot? If it is not 16 byte aligned, then your issue may be that your allocator is returning not correctly aligned memory. You need to align your returned Timer object (node->data address) by alignof(Timer) bytes.

Also: 
1
2
3
4
	nodeType *node = (nodeType *)_allocator.allocate(sizeof(nodeType));
	if (node) {
		fs_memclear(node, sizeof(nodeType));
		node->data = data;

you cannot do this with C++ objects. You need to use placement new instead. memclear will set everything to 0, but this potentially can put C++ object in invalid state (virtual table pointer cleared, other members not initialized correctly, etc...). You correctly initialize C++ object by calling placement new: 
1
2
3
		fs_memclear(node, sizeof(nodeType));
		new (&node->data) T; // assuming node->data is T type.
		node->data = data;

This will construct node->data with default constructor, then assign node->data. Ideally you want your _allocator.allocate return initialized memory with placement new directly by using "new (address) type(value)" and avoid calling memclear.

To avoid placement new for structures that don't need it (POD types) you'll need to specialize it by template type (std::is_pod<T>::value). For POD types you are allowed to call memclear.

Also look into move constructors to avoid extra heap allocations by std::function (and other potential C++ objects as members).
Edited by Mārtiņš Možeiko on
Flyingsand
#21326
Ah! Data alignment.. I should have thought of that. I've dealt with this issue many times before, so I don't know why it didn't occur to me. I think I got hung up on the idea that there was an issue copying std::function across a module boundary because I had a similar issue with file descriptors awhile back. Anyway..

Here is the value of rbx and xmm0 at the time of the crash: 
 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
 (lldb) print $rbx
 (unsigned long) $0 = 2199023704040
 (lldb) p/x $rbx
 (unsigned long) $1 = 0x000002000006d7e8
 (lldb) x $rbx
 0x2000006d7e8: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
 0x2000006d7f8: 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
 (lldb) print $xmm0
 (unsigned char __attribute__((ext_vector_type(16)))) $4 = (0x00, 0x00, 0x00, 0x40, 0x89, 0x88, 0x88, 0x3c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00)
 (lldb) print/x $xmm0
 (unsigned char __attribute__((ext_vector_type(16)))) $5 = (0x40000000, 0x3c888889, 0x00000000, 0x00000000)


So yes.. rbx is not 16-byte aligned. Fixing that solved the problem, so it now works fine in release configuration.

Also, thanks for the info on memclear with regard to C++ objects. I haven't used it with those kinds of objects (virtual functions), and don't typically use virtual functions anyway, but it's good to know. In fact, I'm tempted to rewrite the whole list because it started out as a pretty simple, fast, single parameter templated, cache-friendly list, but when I added in the option of allocation strategy, it got a bit messy and I don't like it.

And move constructors: yes, definitely. On my list of things to do. :)

Thanks again!
Edited by Flyingsand on
ratchetfreak
#21327
move construction is as easy as: 

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
template <typename T, typename Allocator>
typename fsList<T, Allocator>::nodeType*
fsList<T, Allocator>::allocate_and_init_node(T&& data, nodeType *next, nodeType *prev)
                                            //^by double ref 
{
    nodeType *node = (nodeType *)_allocator.allocate(sizeof(nodeType));
	if (node) {
		std::allocator_traits<Allocator>::construct(_allocator, node);
		node->data = std::move(data); //move-assignment here
		node->next = next;
		node->prev = prev;
	}
	
    return node;
}


std::allocator_traits<Allocator>::construct will forward any extra parameters to the constructor it ends up calling, that way you can move-emplace. There is a corresponding destruct to call on the other side.

I find tht using those functions has cleaner syntax than the placement new and direct destruct syntax.

Mārtiņš Možeiko
#21328
Move construction is as easy as that only if you correctly implement actual move constructor and move assignment operator in actual class.
ratchetfreak
#21334
mmozeiko
Move construction is as easy as that only if you correctly implement actual move constructor and move assignment operator in actual class.


Actually rule of 0 is better here, if your member fields all handle rule of 5 correctly there is no need to implement any of the rule of 5. (other than perhaps defaulting them).
Mārtiņš Možeiko
#21336
OP has custom copy operator in Timer class. That means he needs to implement move assignment/constructor. Simply adding std::move won't fix anything.
ratchetfreak
#21338
it's a member-by-member copy so it may as well be removed.
Flyingsand
#21339
ratchetfreak
it's a member-by-member copy so it may as well be removed.

Yeah, that's true. I actually explicitly added the copy constructor and assignment operator for debugging purposes, because I figured out when this issue first came up that it was happening during copy. But yes, they can be removed.
Reply to Thread
← Back to index