There is a pitfall in intel-fpga-bbb/samples/tutorial/03_ccip when shared_ptr and reinterpret_cast are used togethor

[MegaStone]

I encounter a bug in my application when I am trying to allocate FPGA buffers following samples in 03_ccip. There is a pitfall in these samples. And this pitfall is easy to trigger if you maintain your C++ code in a modular fashion.

All samples allocate and use shared buffers in this way.

auto buf_handle = fpga.allocBuffer(getpagesize());
auto buf = reinterpret_cast<volatile char*>(buf_handle->c_type());
...
buf[0] = 0;

This is OK if your design is simple enough to be coded in the main() scope. However, it is a common practice to seperate the allocation of resources from the actual usuage of them. To this end, the allocation and usage of resources may appear in different scopes.

volatile char* buf = nullptr;
... {       // scopes may be introduced by functions or if/for/while blocks 
    auto buf_handle = fpga.allocBuffer(getpagesize());
    buf = reinterpret_cast<volatile char*>(buf_handle->c_type());
    ...  // both buf_handle and its referred object is destroyed!!!
}
...  {
buf[0] = 0; // buf is dangling
}

It's common to structure codes with many scopes. No error or warning is reported in GCC. But a segmentation fault is triggered if you execute this code. And if you are not familiar with shared_ptr, you are doomed to waste a large amount of time to debug this code.

The pitfall is constructed by synergy of std::shared_ptr returned by fpga.allocBuffer(int) and reinterpret_cast. Since the ownership of std::shared_ptr<mpf_shared_buffer> buf_handle is not transferred to buf with reinterpret_cast, the allocated buffer is destroyed at the ends of the first scope since buf_handle is destroyed here. Then buf[0] is a dangling pointer that cannot be detected by the compiler.

A better sample should use explicit shared_ptr type to avoid this pitfall.

volatile char* buf = nullptr;
std::shared_ptr<mpf_shared_buffer> buf_handle;
... {       
    buf_handle = fpga.allocBuffer(getpagesize());
    buf = reinterpret_cast<volatile char*>(buf_handle->c_type());
    ...
}
...  {
buf[0] = 0;
}

P.S. I don't think managing shared buffer with shared_ptr is a good idea. Shared buffer should be alive as long as the FPGA may refer to it. But I do not think it practical to add FPGA's reference to shared_ptr::use_count. If the purpose here is to avoid the use of explicit deallocation of shared buffer, Resource acquisition is initialization (RAII) may be a better idiom.

[r-rojo]

Hi @MegaStone, thanks for bringing this up to our attention.
I agree that one may fall into these sorts of pitfalls when stealing a shared_ptr into a raw pointer, especially when the raw pointer is declared in an outer scope. I also agree that using "auto" keyword makes things less clear.
As for using RAII, we do use this with the shared buffers by limiting their creation to factory functions and by wrapping them in shared_ptr objects. We do, however, break one of its principles by allowing raw access to the raw pointer of the object with the c_type() function.
This is done to provide developers with the most flexibility while still avoiding "explicit deallocation".
To help developers avoid these problems, we will update our samples to be explicit about the types being used (by not using auto) and include comments about the dangers in assigning the raw pointer to a non-shared_ptr variable.
In addition, we will add similar notes highlighting the dangers in our API documentation for the c_type() functions.
Again, thank you for alerting us to this and let us know if you have any more comments or questions.