32-Bit machine creates broken JPEG2000 file in trunk

[gcode-importer]

Originally reported on Google Code with ID 316

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

Reported by szukw000 on 2014-03-25 19:56:39

---

- _Attachment: [openjpeg-trunk-r2793.dif.gz](https://storage.googleapis.com/google-code-attachments/openjpeg/issue-316/comment-0/openjpeg-trunk-r2793.dif.gz)_

[gcode-importer]

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:110: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:119:2:
warning: conversion to '__off_t' from 'OPJ_OFF_T' may alter its value [-Wconversion]

Reported by malaterre on 2014-03-26 12:09:19

• Labels added: Priority-Critical, Milestone-Release2.1
• Labels removed: Priority-Medium

[gcode-importer]

$ 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]

Reported by malaterre on 2014-03-26 12:20:23

[gcode-importer]

$ 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;
 }

Reported by malaterre on 2014-03-26 12:34:46

[gcode-importer]

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.

Reported by julienmalik on 2014-03-26 13:00:02

[gcode-importer]

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

Reported by julienmalik on 2014-03-26 13:15:09

[gcode-importer]

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/specific.m4;h=de940f214f4a03a62695a0ed8dd646ab59cad537;hb=HEAD#l171

Reported by julienmalik on 2014-03-26 13:23:36

[gcode-importer]

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.

Reported by julienmalik on 2014-03-26 14:28:40

[gcode-importer]

The macminig4 showing the issue also has gcc 4.7

Reported by julienmalik on 2014-03-26 14:32:09

[gcode-importer]

Issue 314 has been merged into this issue.

Reported by malaterre on 2014-03-26 14:58:25

[gcode-importer]

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.

Reported by malaterre on 2014-03-26 15:01:30

[gcode-importer]

This issue was updated by revision r2803.

Reported by malaterre on 2014-03-26 15:10:41

[gcode-importer]

This issue was updated by revision r2804.

Reported by malaterre on 2014-03-26 15:16:39

[gcode-importer]

This issue was updated by revision r2805.

Reported by malaterre on 2014-03-26 15:16:50

[gcode-importer]

This issue was updated by revision r2808.

Reported by malaterre on 2014-03-26 16:20:16

[gcode-importer]

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 :(

Reported by malaterre on 2014-03-26 16:28:55

[gcode-importer]

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

Reported by malaterre on 2014-03-27 07:58:59

[gcode-importer]

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.

Reported by julienmalik on 2014-03-27 08:36:06

[gcode-importer]

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...

Reported by julienmalik on 2014-03-27 08:41:13

[gcode-importer]

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

Reported by szukw000 on 2014-03-27 09:05:28

[gcode-importer]

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

Reported by szukw000 on 2014-03-27 09:34:35

[gcode-importer]

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

Reported by szukw000 on 2014-03-27 09:57:14

[gcode-importer]

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.

Reported by malaterre on 2014-03-27 10:15:34

[gcode-importer]

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/specific.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.

Reported by julienmalik on 2014-03-27 10:21:12

[gcode-importer]

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

Reported by szukw000 on 2014-03-27 10:48:37

[gcode-importer]

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.

Reported by malaterre on 2014-03-27 10:50:29

[gcode-importer]

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

Reported by szukw000 on 2014-03-27 11:31:09

[gcode-importer]

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

Reported by malaterre on 2014-03-27 11:45:04

[gcode-importer]

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

Reported by malaterre on 2014-03-27 12:31:32

[gcode-importer]

This issue was closed by revision r2815.

Reported by malaterre on 2014-03-27 15:08:00

• Status changed: Fixed

[gcode-importer]

Nice catch ! Thanks.

Reported by julienmalik on 2014-03-27 16:35:08