32-Bit machine creates broken JPEG2000 file in trunk

[GoogleCodeExporter]

This issue has been triggered by

 [OpenJPEG] cannot opj_decompress file compressed with opj_compress !

The patch has been tested first on a 32-Bit and then on a 64-Bit machine.

winfried

Original issue reported on code.google.com by szukw...@arcor.de on 25 Mar 2014 at 7:56

Attachments:

• openjpeg-trunk-r2793.dif.gz

[GoogleCodeExporter]

Confirmed:

http://my.cdash.org/viewBuildError.php?type=1&buildid=585003

/home/voxxl/Dashboards/MyTests/openjpeg-continuous/src/lib/openjp2/openjpeg.c: 
In function 'opj_skip_from_file':
/home/voxxl/Dashboards/MyTests/openjpeg-continuous/src/lib/openjp2/openjpeg.c:11
0:2: warning: conversion to '__off_t' from 'OPJ_OFF_T' may alter its value 
[-Wconversion]
/home/voxxl/Dashboards/MyTests/openjpeg-continuous/src/lib/openjp2/openjpeg.c: 
In function 'opj_seek_from_file':
/home/voxxl/Dashboards/MyTests/openjpeg-continuous/src/lib/openjp2/openjpeg.c:11
9:2: warning: conversion to '__off_t' from 'OPJ_OFF_T' may alter its value 
[-Wconversion]

Original comment by mathieu.malaterre on 26 Mar 2014 at 12:09

• Added labels: Milestone-Release2.1, Priority-Critical
• Removed labels: Priority-Medium

[GoogleCodeExporter]

$ gcc -o toto.o ../openjpeg-nightly/cmake/TestFileOffsetBits.c 
../openjpeg-nightly/cmake/TestFileOffsetBits.c: In function ‘main’:
../openjpeg-nightly/cmake/TestFileOffsetBits.c:7:3: warning: left shift count 
>= width of type [enabled by default]
../openjpeg-nightly/cmake/TestFileOffsetBits.c:7:3: warning: left shift count 
>= width of type [enabled by default]
../openjpeg-nightly/cmake/TestFileOffsetBits.c:7:3: warning: left shift count 
>= width of type [enabled by default]
../openjpeg-nightly/cmake/TestFileOffsetBits.c:7:3: warning: left shift count 
>= width of type [enabled by default]

Original comment by mathieu.malaterre on 26 Mar 2014 at 12:20

[GoogleCodeExporter]

$ svn di cmake/
Index: cmake/TestFileOffsetBits.c
===================================================================
--- cmake/TestFileOffsetBits.c  (révision 2798)
+++ cmake/TestFileOffsetBits.c  (copie de travail)
@@ -3,8 +3,7 @@
 int main(int argc, char **argv)
 {
   /* Cause a compile-time error if off_t is smaller than 64 bits */
-#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
-  int off_t_is_large[ (LARGE_OFF_T % 2147483629 == 721 && LARGE_OFF_T % 
2147483647 == 1) ? 1 : -1 ];  
+  int off_t_is_large[ sizeof(off_t) >= 8 ? 1 : -1 ];  
   return 0;
 }

Original comment by mathieu.malaterre on 26 Mar 2014 at 12:34

[GoogleCodeExporter]

Still trying to understand the problem...

The patch provided by winfried should not be necessary : OPJ_FSEEK should map 
to fseeko (and not fseek). When LFS is activated (nothing to be done on 64 bits 
machine, enable macro _FILE_OFFSET_BITS=64 on 32 bit machine), then off_t is 
turned into a 64 bit integer, so OPJ_FSEEK takes a 64 bit integer as input.

The problem lies elsewhere IMHO.

http://my.cdash.org/viewConfigure.php?buildid=585003 :
-- Checking for 64-bit off_t
-- Checking for 64-bit off_t - present
-- Checking for fseeko/ftello
-- Checking for fseeko/ftello - present

This is wrong. On a 32 bit machine we should have 'Checking for 64-bit off_t - 
present with _FILE_OFFSET_BITS=64' instead

I guess TestFileOffsetBits.c generates a warning instead of an error is the 
problem.
So probably your diff is enough, Mathieu, if it does generates an error on this 
machine.

The original incantation with bitshift and all comes from the equivalent 
autoconf module if I remember well.

Original comment by julien.m...@gmail.com on 26 Mar 2014 at 1:00

[GoogleCodeExporter]

See 
http://git.savannah.gnu.org/gitweb/?p=autoconf.git;a=blob;f=lib/autoconf/specifi
c.m4;h=de940f214f4a03a62695a0ed8dd646ab59cad537;hb=HEAD#l87

Original comment by julien.m...@gmail.com on 26 Mar 2014 at 1:15

[GoogleCodeExporter]

Unrelated, but seems like we should do that too, for MacOSX :
http://git.savannah.gnu.org/gitweb/?p=autoconf.git;a=blob;f=lib/autoconf/specifi
c.m4;h=de940f214f4a03a62695a0ed8dd646ab59cad537;hb=HEAD#l171

Original comment by julien.m...@gmail.com on 26 Mar 2014 at 1:23

[GoogleCodeExporter]

It is most probably a compiler/libc/? version issue.

I checked the build logs of OrfeoToolbox deb packages, which include the same 
check since they include a version of openjpeg v2.

Same source package, built on three ubuntu version, for i386 arch :
https://launchpadlibrarian.net/157813014/buildlog_ubuntu-precise-i386.otb_3.20.0
-0ppa~precise3_UPLOADING.txt.gz
https://launchpadlibrarian.net/157815482/buildlog_ubuntu-quantal-i386.otb_3.20.0
-0ppa~quantal4_UPLOADING.txt.gz
https://launchpadlibrarian.net/157730111/buildlog_ubuntu-raring-i386.otb_3.20.0-
0ppa~raring3_UPLOADING.txt.gz

You can grep for 'off_t'.
Only on precise (gcc 4.6) we have "Checking for 64-bit off_t - present with 
_FILE_OFFSET_BITS=64"
The two more recent distro (gcc 4.7) don't require it and output "Checking for 
64-bit off_t - present"

So the configuration-time check is broken on recent distro.

Original comment by julien.m...@gmail.com on 26 Mar 2014 at 2:28

[GoogleCodeExporter]

The macminig4 showing the issue also has gcc 4.7

Original comment by julien.m...@gmail.com on 26 Mar 2014 at 2:32

[GoogleCodeExporter]

Issue 314 has been merged into this issue.

Original comment by mathieu.malaterre on 26 Mar 2014 at 2:58

[GoogleCodeExporter]

Here is what I get with clang 3.0-6.2:

$ clang /tmp/valid.c
/tmp/valid.c:7:23: error: 'off_t_is_large' declared as an array with a negative 
size
  int off_t_is_large[ (LARGE_OFF_T % 2147483629 == 721 && LARGE_OFF_T % 2147483647 == 1) ? 1 : -1 ];  
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/valid.c:7:24: warning: shift count >= width of type 
[-Wshift-count-overflow]
  int off_t_is_large[ (LARGE_OFF_T % 2147483629 == 721 && LARGE_OFF_T % 2147483647 == 1) ? 1 : -1 ];  
                       ^~~~~~~~~~~
/tmp/valid.c:6:33: note: expanded from:
#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
                                ^  ~~
/tmp/valid.c:7:24: warning: shift count >= width of type 
[-Wshift-count-overflow]
  int off_t_is_large[ (LARGE_OFF_T % 2147483629 == 721 && LARGE_OFF_T % 2147483647 == 1) ? 1 : -1 ];  
                       ^~~~~~~~~~~
/tmp/valid.c:6:57: note: expanded from:
#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
                                                        ^  ~~
/tmp/valid.c:7:59: warning: shift count >= width of type 
[-Wshift-count-overflow]
  int off_t_is_large[ (LARGE_OFF_T % 2147483629 == 721 && LARGE_OFF_T % 2147483647 == 1) ? 1 : -1 ];  
                                                          ^~~~~~~~~~~
/tmp/valid.c:6:33: note: expanded from:
#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
                                ^  ~~
/tmp/valid.c:7:59: warning: shift count >= width of type 
[-Wshift-count-overflow]
  int off_t_is_large[ (LARGE_OFF_T % 2147483629 == 721 && LARGE_OFF_T % 2147483647 == 1) ? 1 : -1 ];  
                                                          ^~~~~~~~~~~
/tmp/valid.c:6:57: note: expanded from:
#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
                                                        ^  ~~
4 warnings and 1 error generated.

Original comment by mathieu.malaterre on 26 Mar 2014 at 3:01

[GoogleCodeExporter]

This issue was updated by revision r2803.

Original comment by mathieu.malaterre on 26 Mar 2014 at 3:10

[GoogleCodeExporter]

This issue was updated by revision r2804.

Original comment by mathieu.malaterre on 26 Mar 2014 at 3:16

[GoogleCodeExporter]

This issue was updated by revision r2805.

Original comment by mathieu.malaterre on 26 Mar 2014 at 3:16

[GoogleCodeExporter]

This issue was updated by revision r2808.

Original comment by mathieu.malaterre on 26 Mar 2014 at 4:20

[GoogleCodeExporter]

I can run  ctest -R issue316 from my amd64 box with `gcc -m32` now. We'll see 
how it goes on other machine. we are not C89 anymore :(

Original comment by mathieu.malaterre on 26 Mar 2014 at 4:28

[GoogleCodeExporter]

The initial code is UB:

     There's U.B. if off_t (not a C-defined type, by the way) is in 
fact narrow.  If it is, the sub-expression `(off_t) 1 << 62)' runs 
afoul of 6.5.7p3: 

        "[...] If the value of the right operand is [...] greater 
        than or equal to the width of the promoted left operand, 
        the behavior is undefined." 

ref:
https://groups.google.com/d/msg/comp.lang.c/Y8Kf3xJ57JQ/6Iqcu8tARugJ

Original comment by mathieu.malaterre on 27 Mar 2014 at 7:58

[GoogleCodeExporter]

Thanks for the clarification.
Maybe worth asking the autoconf guys why it is implemented like this, and why 
there is no update on their side to support newer compilers.

Original comment by julien.m...@gmail.com on 27 Mar 2014 at 8:36

[GoogleCodeExporter]

Also, just for the sake of clarity : the code does *not* compile with gcc 4.6, 
unless _FILE_OFFSET_BITS=64 is defined.
It was intended it would be the same on the other compilers.

Did you check it compiles with clang when adding _FILE_OFFSET_BITS=64 before 
the include ?

Anyway, being UB, I guess the only solution is to change the code...

Original comment by julien.m...@gmail.com on 27 Mar 2014 at 8:41

[GoogleCodeExporter]

Julien,

>Maybe worth asking the autoconf guys why it is implemented like this,
>and why there is no update on their side to support newer compilers.

The CMAKE guys are the autoconf guys? Or the autoconf guys are the CMAKE
guys? 

winfried

Original comment by szukw...@arcor.de on 27 Mar 2014 at 9:05

[GoogleCodeExporter]

I am back on the 32-Bit machine.

cmake version 2.8.5
gcc 4.8.2

cmake ..
-- The C compiler identification is GNU
-- Check for working C compiler: /usr/local/gcc-4.8.2/bin/gcc
-- Check for working C compiler: /usr/local/gcc-4.8.2/bin/gcc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done

-- Checking for 64-bit off_t
-- Checking for 64-bit off_t - present
-- Checking for fseeko/ftello
-- Checking for fseeko/ftello - present
-- Large File support - found

BUILD/src/lib/openjp2/opj_config_private.h:

/* #undef _LARGEFILE_SOURCE */
/* #undef _LARGE_FILES */
/* #undef _FILE_OFFSET_BITS */
#define OPJ_HAVE_FSEEKO ON

By the way:

/usr/local/src/OpenJPEG-2.0.0/openjpeg-trunk-r2793/src/lib/openjp2/tcd.c: In 
function 'opj_tcd_dc_level_shift_decode':
/usr/local/src/OpenJPEG-2.0.0/openjpeg-trunk-r2793/src/lib/openjp2/tcd.c:1695:83
: warning: incompatible implicit declaration of built-in function 'lrintf' 
[enabled by default]
                                         *l_current_ptr = opj_int_clamp((OPJ_INT32)lrintf(l_value) + l_tccp->m_dc_level_shift, l_min, l_max); ;

winfried

Original comment by szukw...@arcor.de on 27 Mar 2014 at 9:34

[GoogleCodeExporter]

I am back on a 64-Bit machine.

cmake version 2.8.11
gcc 4.8.2

-- The C compiler identification is GNU 4.8.2
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done

-- Checking for 64-bit off_t
-- Checking for 64-bit off_t - present
-- Checking for fseeko/ftello
-- Checking for fseeko/ftello - present
-- Large File support - found

openjpeg-2.0-trunk-r2801/BUILD/src/lib/openjp2/opj_config_private.h

/* #undef _LARGEFILE_SOURCE */
/* #undef _LARGE_FILES */
/* #undef _FILE_OFFSET_BITS */
#define OPJ_HAVE_FSEEKO ON

Does that mean your 'TestFileOffsetBits.c' is wrong?

winfried

Original comment by szukw...@arcor.de on 27 Mar 2014 at 9:57

[GoogleCodeExporter]

winfried, this is impossible, I have tested r2808 in all possible compilers and 
they all *failed* as expected. Could you please make sure you are using latest 
trunk.

Original comment by mathieu.malaterre on 27 Mar 2014 at 10:15

[GoogleCodeExporter]

Winfried,

Neither. There is nothing provided by cmake for LFS detection. So at the time I 
reproduced what is done in autoconf.

The content of the TestFileOffsetBits.c is a copy-paste of what is done in 
autoconf for LFS detection, as pointed earlier :
http://git.savannah.gnu.org/gitweb/?p=autoconf.git;a=blob;f=lib/autoconf/specifi
c.m4;h=de940f214f4a03a62695a0ed8dd646ab59cad537;hb=HEAD#l87

So if TestFileOffsetBits.c is not reliable anymore on recent compilers, chances 
are that autoconf has the bug too.
It is surprising that this has not been caught earlier by the myriad of 
autoconf-based projects.

A portable LFS detection being such a brain damage, it is safer to stick with a 
common solution between what we have at the cmake level in openjpeg, and what 
is done in autoconf.
And avoid trying to invent a new way of doing things specific to OpenJPEG.

Original comment by julien.m...@gmail.com on 27 Mar 2014 at 10:21

[GoogleCodeExporter]

openjpeg-2.0-trunk does use CMAKE only. No configure.ac, no autotools

Linux x86_64
cmake version 2.8.11
gcc 4.8.2

openjpeg-2.0-trunk-r2808/BUILD/src/lib/openjp2/opj_config_private.h:

/* #undef _LARGEFILE_SOURCE */
/* #undef _LARGE_FILES */
/* #undef _FILE_OFFSET_BITS */
#define OPJ_HAVE_FSEEKO ON

winfried

Original comment by szukw...@arcor.de on 27 Mar 2014 at 10:48

[GoogleCodeExporter]

winfried you need to delete the entire binary tree (rm -rf) otherwise cmake 
does not re-run the try-compile already cached in CMakeCache.txt.

Original comment by mathieu.malaterre on 27 Mar 2014 at 10:50

[GoogleCodeExporter]

Yes, I know.

machine: Linux 32-Bit
cmake  : 2.8.5
gcc    : 4.8.2

-- Check for working C compiler: /usr/local/gcc-4.8.2/bin/gcc
-- Check for working C compiler: /usr/local/gcc-4.8.2/bin/gcc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done

-- Checking for 64-bit off_t
-- Checking for 64-bit off_t - present with _FILE_OFFSET_BITS=64
-- Checking for fseeko/ftello
-- Checking for fseeko/ftello - present
-- Large File support - found

make:

/usr/local/src/OpenJPEG-2.0.0/openjpeg-trunk-r2812/src/lib/openjp2/tcd.c: In 
function 'opj_tcd_dc_level_shift_decode':
/usr/local/src/OpenJPEG-2.0.0/openjpeg-trunk-r2812/src/lib/openjp2/tcd.c:1695:83
: warning: incompatible implicit declaration of built-in function 'lrintf' 
[enabled by default]
                                         *l_current_ptr = opj_int_clamp((OPJ_INT32)lrintf(l_value) + l_tccp->m_dc_level_shift, l_min, l_max); ;

openjpeg-trunk-r2812/BUILD/src/lib/openjp2/opj_config_private.h:

/* #undef _LARGEFILE_SOURCE */
/* #undef _LARGE_FILES */
#define _FILE_OFFSET_BITS 64
#define OPJ_HAVE_FSEEKO ON

bin/opj_compress -i p6_rose.ppm  -o p6_rose.ppm.jp2

jiv p6_rose.ppm.jp2
 incorrect magic number
 error: cannot load image data
 cannot load image

winfried

Original comment by szukw...@arcor.de on 27 Mar 2014 at 11:31

[GoogleCodeExporter]

Steps:

$ cd bin32
$ export CFLAGS=-m32
$ cmake ..
$ make
$ ./bin/opj_compress -i lena512.pgm -o bla.jp2

[INFO] tile number 1 / 1
Generated outfile bla.jp2
$ file bla.jp2
bla.jp2: JPEG 2000 Part 1 (JP2)
$ file ./bin/opj_compress
./bin/opj_compress: ELF 32-bit LSB  executable, Intel 80386, version 1 (SYSV), 
dynamically linked (uses shared libs), for GNU/Linux 2.6.26, 
BuildID[sha1]=1da4dc27fc0e36e40fc68ef14d6ce47a5dcf961a, not stripped

Original comment by mathieu.malaterre on 27 Mar 2014 at 11:45

[GoogleCodeExporter]

All 32bits debian machines are affected, see: https://bugs.debian.org/742780

Original comment by mathieu.malaterre on 27 Mar 2014 at 12:31

[GoogleCodeExporter]

This issue was closed by revision r2815.

Original comment by mathieu.malaterre on 27 Mar 2014 at 3:08

• Changed state: Fixed

[GoogleCodeExporter]

Nice catch ! Thanks.

Original comment by julien.m...@gmail.com on 27 Mar 2014 at 4:35