Re: [boost] GIL Image Processing Library - Undefined Behavior

Hi again, since there was no response to my earlier questions about undefined behavior, I'd like to bring this up again: GIL makes extensive use of reinterpret_cast and gil_reinterpret_cast. The former is, for example, used in code similar to the following: short * imagedata = new short[3*image_size]; rgb16s_t * rgbdata = (rgb16s_t *)imagedata; gil_reinterpret_cast is defined as template <class OutPtr, class In> OutPtr gil_reinterpret_cast(In * p) { return static_cast<OutPtr>(static_cast<void *>(p)); } I suspect that undefined behavior results in both cases. IIRC, there is no guarantee that the memory layout of an rgb array is equal to that of a corresponding scalar array with three times as many elements (although this will probably work in practice). Likewise, a static_cast from void * is only allowed back to the original pointer type, not to a different type. Can someone more familiar with the fine points of C++ confirm these suspicions? If they are true, I think the issue should be resolved before inclusion into boost. Regards Ulli -- ________________________________________________________________ | | | Ullrich Koethe Universitaet Hamburg / University of Hamburg | | FB Informatik / Dept. of Informatics | | AB Kognitive Systeme / Cognitive Systems Group | | | | Phone: +49 (0)40 42883-2573 Vogt-Koelln-Str. 30 | | Fax: +49 (0)40 42883-2572 D - 22527 Hamburg | | Email: u.koethe@computer.org Germany | | koethe@informatik.uni-hamburg.de | | WWW: http://kogs-www.informatik.uni-hamburg.de/~koethe/ | |________________________________________________________________|

Ulli wrote:
GIL makes extensive use of reinterpret_cast and gil_reinterpret_cast. The former is, for example, used in code similar to the following:
short * imagedata = new short[3*image_size]; rgb16s_t * rgbdata = (rgb16s_t *)imagedata;
We do indeed cast the user-supplied channel* to pixel_t*, so that we can construct an interleaved view of the user-supplied memory. While in theory it is possible to have a problem, we have tested this on many platforms, Win, Mac, Win CE, Win Mobile 5, 32-bit, 64-bit, with gcc 3.4, gcc 4.0, Visual studio 7, 8, and with various channel types, 8,16,32 bit, and so far have never had a problem. We have even tried funny channels, like struct { short a; bool b; }; While the compiler pads the above channel itself, it never pads the pixel. And in all the users that have tried GIL no one has ever complained. We can avoid this cast fairly easily, but we have chosen to keep it there as it makes GIL both simpler and faster. Today GIL uses a native C pointer to represent an interleaved pixel iterator, and it uses a native C reference to represent an interleaved pixel reference. Instead we could provide alternative models of iterator and reference, respectively, similar to the planar iterator and reference models. The iterator and the reference will internally contain a pointer to the first channel. The iterator's increment will advance the underlying channel pointer by "num_channels" steps. We will hopefully provide the example tomorrow together with the rest of the GIL examples. Hailin is working on cleaning up the examples. We do not recommend using the above wrappers, however, as we would like to keep GIL constructs as slim as possible. Unnecessarily wrapping a pointer in a class increases the possibility for abstraction penalty. For example, in the STL that comes with Visual Studio vector::iterator is not a raw pointer, but is wrapped in a class. While for simple uses this has no performance implications, we have observed that complex uses like std::sort do exhibit abstraction penalty. What we could easily do is have a compile-time assertion to ensure that there will be no alignment problems, and changing the compiler alignment options may still be better than using the wrappers above. The other cases where we cast is to reinterpret the buffer of bytes that stores a variant (like any_image_view). We have not discovered a way to do this better than static_cast<OutPtr>(static_cast<void *>(p)). Using reinterpret_cast apparently is less safe. Suggestions are most welcome. Thanks, Lubomir
participants (2)
-
Lubomir Bourdev
-
Ullrich Koethe