With the release of GCC 6.0 in Rawhide I'm having a build warning/error[1,2] with OpenImageIO I'm not sure what to do with (other than adding a flag to ignore it).
Upstream is looking into it but currently thinks that the pugixml API is requiring a method that GCC 6.0 doesn't like:
[ 3%] Building CXX object src/libutil/CMakeFiles/OpenImageIO_Util.dir/filter.cpp.o cd /builddir/build/BUILD/oiio-Release-1.6.9/build/linux/src/libutil && /usr/bin/c++ -DNDEBUG -DOpenImageIO_Util_EXPORTS -DUSE_EXTERNAL_PUGIXML=1 -DUSE_FIELD3D=1 -DUSE_FREETYPE -DUSE_OCIO=1 -DUSE_OPENCV -DUSE_OPENEXR_VERSION2=1 -DUSE_OPENSSL=1 -D__STDC_CONSTANT_MACROS -D__STDC_LIMIT_MACROS -I/usr/include/OpenEXR -I/usr/include/libraw -I/builddir/build/BUILD/oiio-Release-1.6.9/src/include -I/builddir/build/BUILD/oiio-Release-1.6.9/build/linux/include/OpenImageIO -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m32 -march=i686 -mtune=atom -fasynchronous-unwind-tables -O2 -g -DNDEBUG -fPIC -Wall -Werror -fno-math-errno -Wno-error=unused-local-typedefs -Wno-unused-local-typedefs -Wno-unused-result -o CMakeFiles/OpenImageIO_Util.dir/filter.cpp.o -c /builddir/build/BUILD/oiio-Release-1.6.9/src/libutil/filter.cpp In file included from /builddir/build/BUILD/oiio-Release-1.6.9/src/include/OpenImageIO/pugiconfig.hpp:41:0, from /builddir/build/BUILD/oiio-Release-1.6.9/src/include/OpenImageIO/pugixml.hpp:20, from /builddir/build/BUILD/oiio-Release-1.6.9/src/libOpenImageIO/formatspec.cpp:45: /builddir/build/BUILD/oiio-Release-1.6.9/src/include/OpenImageIO/pugixml.cpp: In member function 'void OpenImageIO::v1_6::pugi::xml_document::create()': /builddir/build/BUILD/oiio-Release-1.6.9/src/include/OpenImageIO/pugixml.cpp:5143:58: error: placement new constructing an object of type 'OpenImageIO::v1_6::pugi::impl::xml_document_struct' and size '44' in a region of type 'char [1]' and size '1' [-Werror=placement-new] _root = new (page->data) impl::xml_document_struct(page); ^
Any ideas would be appreciated.
Thanks, Richard
[1] https://koji.fedoraproject.org/koji/taskinfo?taskID=12804782 [2] https://kojipkgs.fedoraproject.org//work/tasks/4782/12804782/build.log
On 02/03/2016 05:59 PM, Richard Shaw wrote:
In member function 'void OpenImageIO::v1_6::pugi::xml_document::create()': /builddir/build/BUILD/oiio-Release-1.6.9/src/include/OpenImageIO/pugixml.cpp:5143:58: error: placement new constructing an object of type 'OpenImageIO::v1_6::pugi::impl::xml_document_struct' and size '44' in a region of type 'char [1]' and size '1' [-Werror=placement-new] _root = new (page->data) impl::xml_document_struct(page);
It's the use of char data[1] as a flexible array member. I'm not sure if this is valid C++. Warning about it is certainly appropriate.
You should remove the data member, use sizeof(xml_memory_page) instead of offsetof(xml_memory_page, data), and replace page->data with reinterpret_cast<char *>(page) + sizeof (impl::xml_memory_page), or ideally, have xml_memory_page::construct() return both pointers.
You probably should check for wraparound in the size computations as well, to avoid allocating less memory than requested.
Florian
On Wed, Feb 3, 2016 at 11:15 AM, Florian Weimer fweimer@redhat.com wrote:
On 02/03/2016 05:59 PM, Richard Shaw wrote:
In member function 'void
OpenImageIO::v1_6::pugi::xml_document::create()':
/builddir/build/BUILD/oiio-Release-1.6.9/src/include/OpenImageIO/pugixml.cpp:5143:58:
error: placement new constructing an object of type 'OpenImageIO::v1_6::pugi::impl::xml_document_struct' and size '44' in a region of type 'char [1]' and size '1' [-Werror=placement-new] _root = new (page->data) impl::xml_document_struct(page);
It's the use of char data[1] as a flexible array member. I'm not sure if this is valid C++. Warning about it is certainly appropriate.
I passed your suggestions on to upstream, they're usually pretty responsive.
Thanks! Richard
On 03/02/16 10:59 -0600, Richard Shaw wrote:
With the release of GCC 6.0 in Rawhide I'm having a build warning/error[1,2] with OpenImageIO I'm not sure what to do with (other than adding a flag to ignore it).
tl;dr either add -Wno-error=placement-new for now or try the workaround at the bottom of this mail.
Upstream is looking into it but currently thinks that the pugixml API is requiring a method that GCC 6.0 doesn't like:
No, the code is trying to place a large object in a tiny buffer, and GCC issues a warning about that, because it looks suspect. Because the package uses -Werror (which I won't rant about now) that warning becomes an error and so breaks the build.
It looks like the code is possibly safe though, meaning the warning is a false-positive. The code appears to be using an emulated form of C99 flexible-array member (which isn't supported in standard C++). I assume there is a 1-byte array at the end of the object, and then they over-allocating for the object so they can store something else in the location beginning at the 1-byte array e.g.
#include <stdlib.h>
struct X { enum Type { Int, Double }; Type type; char data[1]; };
int main() { X* p = (X*)malloc(sizeof(X) + sizeof(double) - 1); *(double*)p->data = 1.0; p->type = X::Double; }
(This example ignores alignment requirements, so isn't OK, the real code in OpenImageIO might be OK).
I'll take a closer look, but if this is doing something reasonable then we'll need to make GCC's warning smarter, so it allows cases like this.
/builddir/build/BUILD/oiio-Release-1.6.9/src/include/OpenImageIO/pugixml.cpp:5143:58: error: placement new constructing an object of type 'OpenImageIO::v1_6::pugi::impl::xml_document_struct' and size '44' in a region of type 'char [1]' and size '1' [-Werror=placement-new] _root = new (page->data) impl::xml_document_struct(page);
A workaround would be to make it too hard for the compiler to see the problem:
void* ptr = page->data; _root = new (ptr) impl::xml_document_struct(page);
This way GCC doesn't see that the address refers to a 1-byte array.
On 03/02/16 17:30 +0000, Jonathan Wakely wrote:
On 03/02/16 10:59 -0600, Richard Shaw wrote:
With the release of GCC 6.0 in Rawhide I'm having a build warning/error[1,2] with OpenImageIO I'm not sure what to do with (other than adding a flag to ignore it).
tl;dr either add -Wno-error=placement-new for now or try the workaround at the bottom of this mail.
Upstream is looking into it but currently thinks that the pugixml API is requiring a method that GCC 6.0 doesn't like:
No, the code is trying to place a large object in a tiny buffer, and GCC issues a warning about that, because it looks suspect. Because the package uses -Werror (which I won't rant about now) that warning becomes an error and so breaks the build.
It looks like the code is possibly safe though, meaning the warning is a false-positive. The code appears to be using an emulated form of C99 flexible-array member (which isn't supported in standard C++). I assume there is a 1-byte array at the end of the object, and then they over-allocating for the object so they can store something else in the location beginning at the 1-byte array e.g.
#include <stdlib.h>
struct X { enum Type { Int, Double }; Type type; char data[1]; };
int main() { X* p = (X*)malloc(sizeof(X) + sizeof(double) - 1); *(double*)p->data = 1.0;
Oops, I pasted the wrong version of the example. To reproduce the same warning the line above should be:
double* d = new (p->data) double(1.0);
p->type = X::Double; }
Jonathan Wakely wrote:
A workaround would be to make it too hard for the compiler to see the problem:
void* ptr = page->data; _root = new (ptr) impl::xml_document_struct(page);
This way GCC doesn't see that the address refers to a 1-byte array.
Why not simply:
char data[ #ifndef __GNUC__ 1 #endif ];
or:
#ifdef __GNUC__ char data[]; #else char data[1]; #endif
or if you want GCC to accept this even in strict standards-compliant modes:
#ifdef __GNUC__ __extension__ char data[]; #else char data[1]; #endif
?
Kevin Kofler
On 04/02/16 00:08 +0100, Kevin Kofler wrote:
Jonathan Wakely wrote:
A workaround would be to make it too hard for the compiler to see the problem:
void* ptr = page->data; _root = new (ptr) impl::xml_document_struct(page);
This way GCC doesn't see that the address refers to a 1-byte array.
Why not simply:
char data[ #ifndef __GNUC__ 1 #endif ];
or:
#ifdef __GNUC__ char data[]; #else char data[1]; #endif
or if you want GCC to accept this even in strict standards-compliant modes:
#ifdef __GNUC__ __extension__ char data[]; #else char data[1]; #endif
?
Changing from a 1-byte array to the flexible array member potentially changes the object's size, which would be an ABI change. I didn't look at the rest of the code to know if that would be safe, or if it would break something.
Suppressing the warning with the void* kluge won't break things, it will just allow the build to proceeed and generate the same code as before.
-
Florian Weimer -
Jonathan Wakely -
Kevin Kofler -
Richard Shaw