On Wed, Dec 21, 2016 at 9:58 AM, Mike Holmes <mike.hol...@linaro.org> wrote: > On 21 December 2016 at 10:49, Maxim Uvarov <maxim.uva...@linaro.org> wrote: > >> On 12/21/16 18:31, Mike Holmes wrote: >> > >> > >> > On 21 December 2016 at 10:26, Maxim Uvarov <maxim.uva...@linaro.org >> > <mailto:maxim.uva...@linaro.org>> wrote: >> > >> > On debian jessie gcc unable to compile current code >> > with error: >> > odp_packet.c: In function 'odp_packet_trunc_head': >> > odp_packet.c:314:46: error: array subscript is above array bounds >> > [-Werror=array-bounds] >> > to->buf_hdr.seg[i].len = from->buf_hdr.seg[num + i].len; >> > >> > The problem is that it breaks compilation only on .len line: >> > for (i = 0; i < num; i++) { >> > to->buf_hdr.seg[i].hdr = from->buf_hdr.seg[num + i].hdr; >> > to->buf_hdr.seg[i].data = from->buf_hdr.seg[num + i].data; >> > to->buf_hdr.seg[i].len = from->buf_hdr.seg[num + i].len; >> > } >> > >> > If that line is commented out compilation passes. If lines are >> reordered >> > than compilation also passes. Because there is no warning on .hdr >> > and .data >> > it looks like compiler error. Additional check for preconfigured >> value >> > is workaround to that situation. >> > >> > Signed-off-by: Maxim Uvarov <maxim.uva...@linaro.org >> > <mailto:maxim.uva...@linaro.org>> >> > --- >> > platform/linux-generic/odp_packet.c | 2 ++ >> > 1 file changed, 2 insertions(+) >> > >> > diff --git a/platform/linux-generic/odp_packet.c >> > b/platform/linux-generic/odp_packet.c >> > index 0d3fd05..f3e0a12 100644 >> > --- a/platform/linux-generic/odp_packet.c >> > +++ b/platform/linux-generic/odp_packet.c >> > @@ -309,6 +309,8 @@ static inline void >> > copy_num_segs(odp_packet_hdr_t *to, odp_packet_hdr_t *from, >> > int i; >> > >> > for (i = 0; i < num; i++) { >> > + if (odp_unlikely((num + i) >= >> CONFIG_PACKET_MAX_SEGS)) >> > + ODP_ABORT("packet segmenation error\n"); >> > >> > >> > Does this impact performance, if so should it be optionally compiled in >> > for that GCC version ? >> > >> >> >> CONFIG_PACKET_MAX_SEG is currently 2. And according to our tests it will >> add one if check for odp_packet_trunc_head() and odp_packet_trunc_tail() >> calls. So overhead is one if instruction.
I agree that this is a bit of a kludge. I'm currently debugging Bug 2789 where things fail if CONFIG_PACKET_MAX_SEG > 2. I'd like to see if the fix for that doesn't result in a cleaner implementation in this area that may not expose this issue. >> >> >> It fails on Jessie, which is: >> # gcc -v >> Using built-in specs. >> COLLECT_GCC=gcc >> COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/4.9/lto-wrapper >> Target: x86_64-linux-gnu >> Configured with: ../src/configure -v --with-pkgversion='Debian 4.9.2-10' >> --with-bugurl=file:///usr/share/doc/gcc-4.9/README.Bugs >> --enable-languages=c,c++,java,go,d,fortran,objc,obj-c++ --prefix=/usr >> --program-suffix=-4.9 --enable-shared --enable-linker-build-id >> --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix >> --with-gxx-include-dir=/usr/include/c++/4.9 --libdir=/usr/lib >> --enable-nls --with-sysroot=/ --enable-clocale=gnu >> --enable-libstdcxx-debug --enable-libstdcxx-time=yes >> --enable-gnu-unique-object --disable-vtable-verify --enable-plugin >> --with-system-zlib --disable-browser-plugin --enable-java-awt=gtk >> --enable-gtk-cairo >> --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-4.9-amd64/jre >> --enable-java-home >> --with-jvm-root-dir=/usr/lib/jvm/java-1.5.0-gcj-4.9-amd64 >> --with-jvm-jar-dir=/usr/lib/jvm-exports/java-1.5.0-gcj-4.9-amd64 >> --with-arch-directory=amd64 >> --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --enable-objc-gc >> --enable-multiarch --with-arch-32=i586 --with-abi=m64 >> --with-multilib-list=m32,m64,mx32 --enable-multilib --with-tune=generic >> --enable-checking=release --build=x86_64-linux-gnu >> --host=x86_64-linux-gnu --target=x86_64-linux-gnu >> Thread model: posix >> gcc version 4.9.2 (Debian 4.9.2-10) >> >> >> And passes on my Ubuntu which is (4.8 or 4.9): >> gcc-4.9 -v >> Using built-in specs. >> COLLECT_GCC=gcc-4.9 >> COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/4.9/lto-wrapper >> Target: x86_64-linux-gnu >> Configured with: ../src/configure -v --with-pkgversion='Ubuntu >> 4.9.3-0ubuntu4' >> --with-bugurl=file:///usr/share/doc/gccgo-4.9/README.Bugs >> --enable-languages=c,c++,go --prefix=/usr --program-suffix=-4.9 >> --enable-shared --enable-linker-build-id --libexecdir=/usr/lib >> --without-included-gettext --enable-threads=posix >> --with-gxx-include-dir=/usr/include/c++/4.9 --libdir=/usr/lib >> --enable-nls --with-sysroot=/ --enable-clocale=gnu >> --enable-libstdcxx-time=yes --enable-gnu-unique-object >> --disable-vtable-verify --disable-libquadmath --enable-plugin >> --with-system-zlib --enable-multiarch --disable-werror >> --with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32 >> --enable-multilib --with-tune=generic --enable-checking=release >> --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu >> Thread model: posix >> gcc version 4.9.3 (Ubuntu 4.9.3-0ubuntu4) >> >> >> And for arm it looks like it failed with some 5.x gcc version. So it >> might be not easy to define exactly which version is it. >> >> >> > Perhaps we need to just add a comment then - possibly a doxygen one maybe > a @note ? > So that anyone editing in future will not re break it by mistake, they > might not test on a legacy compiler > > >> >> >> > >> > >> > to->buf_hdr.seg[i].hdr = from->buf_hdr.seg[num + >> > i].hdr; >> > to->buf_hdr.seg[i].data = from->buf_hdr.seg[num + >> > i].data; >> > to->buf_hdr.seg[i].len = from->buf_hdr.seg[num + >> > i].len; >> > -- >> > 2.7.1.250.gff4ea60 >> > >> > >> > >> > >> > -- >> > Mike Holmes >> > Program Manager - Linaro Networking Group >> > Linaro.org <http://www.linaro.org/>* **│ *Open source software for ARM >> SoCs >> > "Work should be fun and collaborative, the rest follows" >> > >> > __ >> > >> > >> >> > > > -- > Mike Holmes > Program Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs > "Work should be fun and collaborative, the rest follows"
