After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 162778 - Crash when using "addition" mode on a grayscale image
Crash when using "addition" mode on a grayscale image
Status: RESOLVED FIXED
Product: GIMP
Classification: Other
Component: General
2.2.x
Other Linux
: High critical
: 2.2
Assigned To: GIMP Bugs
GIMP Bugs
: 158871 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2005-01-03 03:00 UTC by Josh Lee
Modified: 2008-01-15 12:44 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Dissasembly of 0x08283381 (6.69 KB, text/plain)
2005-01-05 20:28 UTC, Josh Lee
  Details
workaround the problem by disabling the code (5.07 KB, patch)
2005-01-09 00:24 UTC, Sven Neumann
committed Details | Review
Attached patch against 2.2.10 to disable use of movntq instruction and re-enable MMX acceleration for addition composite mode (4.48 KB, patch)
2006-04-08 18:39 UTC, Mukund Sivaraman
none Details | Review
New patch with no #if (4.62 KB, patch)
2006-04-10 14:13 UTC, Mukund Sivaraman
committed Details | Review
Patch to remove more movntq instructions from P2 MMX-only code in CVS HEAD (431 bytes, patch)
2006-05-10 09:27 UTC, Mukund Sivaraman
committed Details | Review

Description Josh Lee 2005-01-03 03:00:51 UTC
Gimp crashes when you use addition mode and the image is grayscale.

1. Make an image, doesn't matter how big.
2. Put it in grayscale mode.
3. Hit 'duplicate layer'.
4. Change the mode of the upper layer to addition.
5. Kaboom.

It doesn't matter whether you do the addition first or the grayscale first. It
works on both solid white and solid black images.
Comment 1 Sven Neumann 2005-01-03 10:14:54 UTC
I cannot reproduce this problem. Please provide further information on your
system. Please tell us what CPU you are using and what compiler was used to
compile GIMP  It would also be good to know if running gimp with the
--no-cpu-accel command-line option makes any difference. Please do also run gimp
with the --verbose option and paste the line about "Processor instruction sets"
to this bug report.
Comment 2 Josh Lee 2005-01-04 00:48:17 UTC
"Processor instruction sets: +mmx -sse -sse2 -3dnow -altivec -vis"
I am using the standard packages from ftp.us.debian.org, unstable. (I put my
distro in the 'os details' field but it doesn't seem to show up here).
I am running a pair of Pentium II 350 Mhz.

Here is the output from when I create the image to when it crashes.
> loading menu '/usr/share/gimp/2.0/menus/image-menu.xml' for /image-menubar
> (script-fu:8379): LibGimpBase-WARNING **: script-fu: wire_read(): error
> Illegal instruction
(Exit code 132)

When I use the --no-cpu-accel option, it doesn't crash at all, so that's
probably a workaround.

I don't have debugging symbols.
Comment 3 Sven Neumann 2005-01-04 01:51:00 UTC
Did you report this in the Debian bug-tracker as well (using reportbug)?
Comment 4 Josh Lee 2005-01-04 02:45:25 UTC
I submitted this to Debian, and it's at
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=288508

Here's a backtrace. Who knows how useful it may be.

Program received signal SIGILL, Illegal instruction.

Thread 16384 (LWP 11514)

  • #0 gimp_composite_addition_va8_va8_va8_mmx
  • #1 gimp_composite_dispatch
  • #2 initial_region
  • #3 eq_histogram_lut_setup
  • #4 eq_histogram_lut_setup
  • #5 pixel_regions_process_parallel
  • #6 combine_regions
  • #7 gimp_image_get_new_preview
  • #8 gimp_viewable_get_new_preview
  • #9 gimp_undo_create_preview
  • #10 gimp_undo_create_preview
  • #11 g_child_watch_add
    from /usr/lib/libglib-2.0.so.0
  • #12 g_main_depth
    from /usr/lib/libglib-2.0.so.0
  • #13 g_main_context_dispatch
    from /usr/lib/libglib-2.0.so.0
  • #14 g_main_context_dispatch
    from /usr/lib/libglib-2.0.so.0
  • #15 g_main_loop_run
    from /usr/lib/libglib-2.0.so.0
  • #16 app_run
  • #17 main

Comment 5 Helvetix Victorinox 2005-01-05 04:21:39 UTC
The fact that you have a SMP system is important information.  I have not tested
this on a multi-cpu system.  Although I don't believe I violate any rules at the
assembly language level, I would not be surprised to learn that the accelerated
code has some SMP problems.

Can you give the instruction at address 0x08283381?
Comment 6 Josh Lee 2005-01-05 20:28:22 UTC
Created attachment 35500 [details]
Dissasembly of 0x08283381

The attachment is a full dumb of the assembly; below is the line in question:
0x08283381 <gimp_composite_addition_va8_va8_va8_mmx+65>:	movntq
%mm1,(%esi)
Comment 7 Helvetix Victorinox 2005-01-06 04:58:48 UTC
This is my oversight.  Despite the fact that I put this in the MMX
implementation, the movntq is available only on CPUs with the SSE instruction
set.  This is a bug.

In the interm, until the fix is ready, users of CPUs that do NOT implement SSE,
should use the --no-cpu-accel command line option as a temporary work-around.
Comment 8 Ari Pollak 2005-01-06 05:05:57 UTC
that's odd, it seems like every MMX page I've searched for makes mention of movntq.
Comment 9 Sven Neumann 2005-01-06 15:04:18 UTC
Helvetix, can you come up with a quick fix so that we can release 2.2.2 with it?
Comment 10 Helvetix Victorinox 2005-01-06 17:33:19 UTC
The Pentium 4 processor manual, Volume 2, Instruction Set Reference says, that
the MOVNTQ instruction will generate a protected mode exception #UD if the CPUID
does not have the SSE feature flag set.  In this case, the MMX feature flag is
set (Pentium 3 supported MMX, but not SSE or SSE2), but not the SSE feature flag.

I'm working the fix now.
Comment 11 Sven Neumann 2005-01-08 13:01:53 UTC
Just a quick note to inform you that I would want to release 2.2.2 this weekend,
preferably tonight.
Comment 12 Sven Neumann 2005-01-09 00:24:27 UTC
Created attachment 35688 [details] [review]
workaround the problem by disabling the code

This patch disables the broken MMX acceleration routine. I plan to use this for
the 2.2.2 release.
Comment 13 Sven Neumann 2005-01-09 20:21:24 UTC
As I am going to do the 2.2.2 release tonight, I have applied this change to the
stable branch (and to the stable branch only):

2005-01-09  Sven Neumann  <sven@gimp.org>

	* app/composite/gimp-composite-mmx-installer.c
	* app/composite/gimp-composite-mmx-test.c
	* app/composite/gimp-composite-mmx.[ch]: workaround bug #162778 by
	disabling MMX acceleration for grayscale addition mode.
Comment 14 Manish Singh 2005-01-09 22:46:09 UTC
*** Bug 158871 has been marked as a duplicate of this bug. ***
Comment 15 Sven Neumann 2005-02-18 22:29:10 UTC
Moving this bug to the 2.4 milestone since there's a workaround in the gimp-2-2
branch.
Comment 16 Philippe Verdy 2005-10-04 08:18:13 UTC
MOVNTQ is not an MMX or SSE instruction. It's an Intel specific instruction. 
You can perform the same thing with other instructions.

Using it will crash for example on AMD Athlon 64 and AMD Turion (the mobile 
version of Athlon 64) which DO feature the 3DNow, MMX, SSE, and SSE2 
capabilities (but not the Intel HyperThreading) and WP (memory write 
protection bit).
Comment 17 Federico Mena Quintero 2006-01-06 17:39:46 UTC
Is this supposed to work in 2.2.10 out of the box?  Or does it need a patch?

2.2.10 has different code that that in the patch in comment #12, so I don't know if the code got changed since then.  In 2.2.10 the ADDITION mode *is* #ifdefed out for RGB8, and there seem to be no VA8 constants.
Comment 18 Mukund Sivaraman 2006-01-06 18:57:28 UTC
Neo:

The instruction does exist on x86_64 (both Intel and AMD):

ftp://download.intel.com/design/Pentium4/manuals/25366617.pdf
http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/26569.pdf

It also seems to exist on 32-bit Athlons (although I don't know when exactly it was introduced.. will require a careful examination of various CPUs capabilities).

----

Helvetix is right here that the MOVNTQ instruction caused *this* particular bug and although it's used in MMX code here, it's introduced on SSE capable CPUs. The movq instruction seems to do the exact same thing semantically except it has different cache behaviour which affects performance. You should be able to s/movntq/movq/ from what I read in the processor documentation.

If you revert your earlier patch, you get left with an #if 0 / #endif block which selects the only existing MOVNTQ instruction in that file. If you instead select the MOVQ part (i.e., if it were #if 1) after reverting your earlier patch in this bug, it should be usable on a PII MMX system.

Helvetix has s/movntq/movq/ in places to fix this issue.

Comment 19 Sven Neumann 2006-04-04 14:51:38 UTC
What's the current state on this bug report? Can someone please summarize what combinations are broken and what needs to be done to fix those issues?
Comment 20 Mukund Sivaraman 2006-04-08 18:39:38 UTC
Created attachment 62986 [details] [review]
Attached patch against 2.2.10 to disable use of movntq instruction and re-enable MMX acceleration for addition composite mode

This is the last place where the movntq instruction was used. It has been disabled now and movq is used instead.
Comment 21 Mukund Sivaraman 2006-04-08 18:44:40 UTC
Please do not use the above patch as it does not build.. am checking out why something which used to build before doesn't anymore.

Comment 22 Mukund Sivaraman 2006-04-08 19:09:08 UTC
The patch is above OK.

I had recently changed compilers to GCC 4.x and the x86 assembly code in app/composite/ doesn't build with GCC 4.x. Fedora's GIMP SRPM seems to have a patch for this issue, but this is a different bug.

Comment 23 Sven Neumann 2006-04-10 13:53:20 UTC
Wouldn't it be better to actually do the required changes instead of using ifdefs?
Comment 24 Mukund Sivaraman 2006-04-10 14:13:51 UTC
Created attachment 63123 [details] [review]
New patch with no #if

Btw the patch is already in CVS HEAD.. helvetix seems to have forgotten to update the 2.2 branch (and hence this bug)
Comment 25 Sven Neumann 2006-04-11 20:15:56 UTC
Thanks a lot for the patch and for your patience. I have committed it to the gimp-2-2 branch now.

2006-04-11  Sven Neumann  <sven@gimp.org>

	* app/composite/gimp-composite-mmx-installer.c
	* app/composite/gimp-composite-mmx-test.c
	* app/composite/gimp-composite-mmx.[ch]: disable use of movntq
	instruction and re-enable MMX acceleration for addition composite
	mode (bug #162778, patch by Mukund).
Comment 26 Tom Williams 2006-05-06 15:24:56 UTC
When compiling gimp 2.2.11 on my 32-bit Linux system (Slackware 8 base w/ mods), I get this error compiling gimp-composite-mmx.c:

if gcc -DHAVE_CONFIG_H -I. -I. -I../.. -I../.. -I../../app -I/opt/gnome/include/glib-2.0 -I/opt/gnome/lib/glib-2.0/include   -I/usr/include -DG_LOG_DOMAIN=\"Gimp-Composite\"  -DGIMP_DISABLE_DEPRECATED -DGDK_MULTIHEAD_SAFE -DGTK_MULTIHEAD_SAFE -mmmx -g -O2 -Wall -MT libcompositemmx_a-gimp-composite-mmx.o -MD -MP -MF ".deps/libcompositemmx_a-gimp-composite-mmx.Tpo" -c -o libcompositemmx_a-gimp-composite-mmx.o `test -f 'gimp-composite-mmx.c' || echo './'`gimp-composite-mmx.c; \
then mv -f ".deps/libcompositemmx_a-gimp-composite-mmx.Tpo" ".deps/libcompositemmx_a-gimp-composite-mmx.Po"; else rm -f ".deps/libcompositemmx_a-gimp-composite-mmx.Tpo"; exit 1; fi
gimp-composite-mmx.c:834: warning: 'mmx_op_overlay' defined but not used
gimp-composite-mmx.c: In function 'gimp_composite_burn_rgba8_rgba8_rgba8_mmx':
gimp-composite-mmx.c:148: error: can't find a register in class 'GENERAL_REGS' while reloading 'asm'
make[3]: *** [libcompositemmx_a-gimp-composite-mmx.o] Error 1
make[3]: Leaving directory `/home/tom/build/gimp-2.2.11/app/composite'
make[2]: *** [all-recursive] Error 1
make[2]: Leaving directory `/home/tom/build/gimp-2.2.11/app'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/home/tom/build/gimp-2.2.11'
make: *** [all] Error 2

real    10m14.351s
user    7m9.191s
sys     1m31.102s
tom@deathstar:~/build/gimp-2.2.11$ 

Build environment:

2.6.16.11 kernel
glibc-2.4
gcc 4.1.1 (20060505 snapshot)

Peace...

Peace...
Comment 27 Tom Williams 2006-05-06 15:25:59 UTC
(In reply to comment #26)
> When compiling gimp 2.2.11 on my 32-bit Linux system (Slackware 8 base w/
> mods), I get this error compiling gimp-composite-mmx.c:
> 
> if gcc -DHAVE_CONFIG_H -I. -I. -I../.. -I../.. -I../../app
> -I/opt/gnome/include/glib-2.0 -I/opt/gnome/lib/glib-2.0/include  
> -I/usr/include -DG_LOG_DOMAIN=\"Gimp-Composite\"  -DGIMP_DISABLE_DEPRECATED
> -DGDK_MULTIHEAD_SAFE -DGTK_MULTIHEAD_SAFE -mmmx -g -O2 -Wall -MT
> libcompositemmx_a-gimp-composite-mmx.o -MD -MP -MF
> ".deps/libcompositemmx_a-gimp-composite-mmx.Tpo" -c -o
> libcompositemmx_a-gimp-composite-mmx.o `test -f 'gimp-composite-mmx.c' || echo
> './'`gimp-composite-mmx.c; \
> then mv -f ".deps/libcompositemmx_a-gimp-composite-mmx.Tpo"
> ".deps/libcompositemmx_a-gimp-composite-mmx.Po"; else rm -f
> ".deps/libcompositemmx_a-gimp-composite-mmx.Tpo"; exit 1; fi
> gimp-composite-mmx.c:834: warning: 'mmx_op_overlay' defined but not used
> gimp-composite-mmx.c: In function 'gimp_composite_burn_rgba8_rgba8_rgba8_mmx':
> gimp-composite-mmx.c:148: error: can't find a register in class 'GENERAL_REGS'
> while reloading 'asm'
> make[3]: *** [libcompositemmx_a-gimp-composite-mmx.o] Error 1
> make[3]: Leaving directory `/home/tom/build/gimp-2.2.11/app/composite'
> make[2]: *** [all-recursive] Error 1
> make[2]: Leaving directory `/home/tom/build/gimp-2.2.11/app'
> make[1]: *** [all-recursive] Error 1
> make[1]: Leaving directory `/home/tom/build/gimp-2.2.11'
> make: *** [all] Error 2
> 
> real    10m14.351s
> user    7m9.191s
> sys     1m31.102s
> tom@deathstar:~/build/gimp-2.2.11$ 
> 
> Build environment:
> 
> 2.6.16.11 kernel
> glibc-2.4
> gcc 4.1.1 (20060505 snapshot)
> 
> Peace...
> 

I forgot to add I'm running on a Pentium II 350MHz box.

Peace...
Comment 28 Mukund Sivaraman 2006-05-06 16:05:12 UTC
The error doesn't have anything to do with the type of CPU you are using. It has to do with your compiler failing on compiling the assembly syntax in the C code. GCC 4.1.x has problems with this code. There is a patch in FC5's SRPMS to fix this.

Btw, your bug report is not the same as the bug in this thread. You can open another bug report if you want to, where perhaps that patch can be incorporated into the mainline tree.

Comment 29 Tom Williams 2006-05-06 16:18:55 UTC
(In reply to comment #28)
> The error doesn't have anything to do with the type of CPU you are using. It
> has to do with your compiler failing on compiling the assembly syntax in the C
> code. GCC 4.1.x has problems with this code. There is a patch in FC5's SRPMS to
> fix this.

Thanks.

> Btw, your bug report is not the same as the bug in this thread. You can open
> another bug report if you want to, where perhaps that patch can be incorporated
> into the mainline tree.

The reason I posted my comments here is the other bug report I opened:

http://bugzilla.gnome.org/show_bug.cgi?id=158871

was flagged as a dupe of this one.  I'll look into the gcc 4.1.x issue.

Peace...

Comment 30 Mukund Sivaraman 2006-05-06 19:57:25 UTC
Well your other bug in #158871 *is* a duplicate of this bug (#162778). But it is not the same as the bug you reported in comment #27. You are not able to figure the difference between your current problem and the previous bug you reported?

In bug #158871, the tests fail (during a run of the compiled test program) whereas   your bug in comment #27 is a compile error.

Comment 31 Mukund Sivaraman 2006-05-10 09:27:35 UTC
Created attachment 65151 [details] [review]
Patch to remove more movntq instructions from P2 MMX-only code in CVS HEAD
Comment 32 Sven Neumann 2006-05-10 09:35:41 UTC
2006-05-10  Sven Neumann  <sven@gimp.org>

	* app/composite/gimp-composite-mmx.c
	(gimp_composite_swap_rgba8_rgba8_rgba8_mmx): applied patch from
	Mukund that replaces remaining movntq instructions in MMX assembly
	(bug #162778).