View Issue Details

IDProjectCategoryView StatusLast Update
0027167LazarusLCLpublic2015-01-21 18:10
ReporterDerit Agustin Assigned ToJuha Manninen  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
PlatformwindowsOSwin32 
Product Version1.3 (SVN) 
Summary0027167: TControlbar Arithmetic overflow
Descriptioni'm trying create 2 button in controlbar i found error when run
TagsNo tags attached.
Fixed in Revisionr47481
LazTarget-
Widgetset
Attached Files

Relationships

related to 0027173 closedFlorian FPC Exception 'External: SIGFPE' raised when using variable of subrange type as divisor. 

Activities

Derit Agustin

2014-12-15 00:35

reporter  

controlbar.zip (128,564 bytes)

Vojtech Cihak

2014-12-15 00:56

reporter   ~0079810

Two users already reported it on the forum. Both cases was under Win (Win32 and Win64).

Unfortunately, I cannot reproduce it with Linux and I have no real Win system.

Crash is at line 598 of cotrolbar.inc
aNewRow := (Y - (FInitDrag.Y - AMoveBand.InitTop)) div RowSize;

See: http://forum.lazarus.freepascal.org/index.php/topic,26613.msg163807.html#msg163807

Derit Agustin

2014-12-15 01:27

reporter   ~0079811

thanks for reply :)

Juha Manninen

2014-12-15 11:24

developer   ~0079818

Last edited: 2014-12-15 11:38

View 2 revisions

Interesting. It is a compiler bug in FPC trunk.
I tested with a temporary variable, reusing an existing Integer:
  aLimit := FRowSize; // Type conversion TRowSize -> Integer.
  aNewRow := (Y - (FInitDrag.Y - AMoveBand.InitTop)) div aLimit;

The assignment aLimit := FRowSize; throws a SIGFPE which should never happen.
FRowSize type is TRowSize = 1..MaxInt; which is a subset of Integer, thus even range errors should not happen.
The value in FRowSize during assignment was 26 which is correct and valid.

With FPC 2.6.4 it works well. Derit, you did not mention your compiler version. Is it FPC trunk?
Can you also verify that you have a 32-bit WinXP. Other cases (including me) used 64-bit Windows. At least I used a 32-bit compiler on a 64-bit Windows.

I made a workaround fix in r47202. I changed TRowSize = Integer.
Can somebody please create a bug report for FPC with a minimal test program to reproduce this problem. We can mark this issue as related and then finally revert the correct type when FPC is fixed.

Jonas Maebe

2014-12-15 11:35

manager   ~0079819

> The assignment aLimit := FRowSize; throws a SIGFPE
> which should never happen.

Note that the x87's archaic execution model means that if an FPU exception occurs, it will be thrown when the *next* FPU instruction is executed (unless you insert fwait instructions after every fpu instruction, which slows down execution a lot). So the actual exception probably happens before this line.

FPC does not contain a code generation option to add fwait instructions everywhere and there are no plans to introduce one either. Try testing on a platform that does not use a broken fpu, such as Win64 (which uses the SSE unit instead of the x87).

Derit Agustin

2014-12-15 11:42

reporter   ~0079821

@juha i use lazarus trunk and fpc trunk

Juha Manninen

2014-12-15 11:53

developer   ~0079822

Jonas, TRowSize = 1..MaxInt; is an integer type, not a floating point type.
Why is FPU involved here?

Jonas Maebe

2014-12-15 12:02

manager   ~0079823

I don't know. I don't know even whether it actually is involved here (maybe it's involved in the last instruction for the previous line or so). In some cases we do use the fpu on i386 to perform 64 bit inline move operations though, although based on just this code fragment I don't think that would happen here.

Jonas Maebe

2014-12-15 12:13

manager   ~0079824

A few other things to take into account:
* not all platforms can differentiate between floating point and integer division exceptions. I don't know how it is with Win32
* 1..MaxInt can be interpreted as an unsigned type, while Integer is a signed type. Maybe the former causes some operations to be performed using unsigned arithmetic (or 64 bit arithmetic in case of a detected mixing of signed and unsigned types).

Cyrax

2014-12-15 18:08

reporter   ~0079830

Excerpt from assembler dialog when the exception happens:
---
0055DAC7 29c8 sub %ecx,%eax
0055DAC9 99 cltd
0055DACA f775c0 divl -0x40(%ebp)
---

Unrelated note : for some reason GDB won't show locations for clauses if they reside in include file. LCL and whole Lazarus is built with debug info included.

Juha Manninen

2014-12-15 19:22

developer   ~0079832

Last edited: 2014-12-15 19:23

View 2 revisions

Cyrax, I think you tested with the original code where RowSize is a divider.
Could you try assigning it to a temporary integer (which also throws exception) and then maybe isolate a smaller example to reproduce it.

Unfortunately I don't have time to dive deeper into this issue.

Cyrax

2014-12-15 20:26

reporter   ~0079834

Excerpt from same place when the fix is applied:
---
0055DABD 8b4df4 mov -0xc(%ebp),%ecx
0055DAC0 99 cltd
0055DAC1 f7b980030000 idivl 0x380(%ecx)
0055DAC7 8945dc mov %eax,-0x24(%ebp)
---

Juha, I will try to isolate it.

ocean

2014-12-15 22:23

reporter  

error.pp (161 bytes)   
program error;

{$mode Delphi}

uses sysutils;

type a = 1..MaxInt;

var b: a;
    c: integer;

begin
 b := 3;
 c := -5 div b;
 writeln(c);
end.
error.pp (161 bytes)   

ocean

2014-12-15 22:25

reporter   ~0079835

Attached gives error in fpc2.7.1 win32

Do-wan Kim

2014-12-16 03:54

reporter   ~0079836

Last edited: 2014-12-16 05:33

View 3 revisions

nx86mat.pas line 531,

            {Division depends on the right type.}
            if is_signed(right.resultdef) or (is_signed(left.resultdef) and is_in_limit(right.resultdef,left.resultdef)) then
              op:=A_IDIV
            else
              op:=A_DIV;

How about like this?

http://stackoverflow.com/questions/7131764/visual-c-generates-div-instead-of-idiv-x86-integer-arithmetic

Patch may satified 1 and 2 in "ISO C99, 6.5.7p3" and it works.

Do-wan Kim

2014-12-16 05:01

reporter  

ranged_division_overflow_nx86mat.pas.patch (566 bytes)   
Index: compiler/x86/nx86mat.pas
===================================================================
--- compiler/x86/nx86mat.pas	(revision 29300)
+++ compiler/x86/nx86mat.pas	(working copy)
@@ -528,7 +528,7 @@
               emit_reg_reg(A_XOR,opsize,regd,regd);
 
             {Division depends on the right type.}
-            if is_signed(right.resultdef) then
+            if is_signed(right.resultdef) or (is_signed(left.resultdef) and is_in_limit(right.resultdef,left.resultdef)) then
               op:=A_IDIV
             else
               op:=A_DIV;

Cyrax

2014-12-16 06:55

reporter   ~0079837

Last edited: 2014-12-16 09:04

View 3 revisions

Assembler excerpt from ocean's error.pp program:
---
error.pp:14 c := -5 div b;
00401563 b8fbffffff mov $0xfffffffb,%eax
00401568 99 cltd
00401569 f73500a04100 divl 0x41a000
0040156F a310a04100 mov %eax,0x41a010
---

Command line arguments for FPC : -MObjFPC -Scghi -gw2 -godwarfsets -gl -l -vewnhibq -Filib\i386-win32 -Fu. -FUlib\i386-win32 -FEbin\i386-win32-win32\

EDIT: Can this bug report to be moved to FPC side?

Juha Manninen

2014-12-16 08:17

developer   ~0079838

> EDIT: Can this bug report to be moved to FPC side?

No, this report is about TControlbar. Somebody should create a new report for FPC.

It seems the DIV operator is needed for the bug to appear. In my test it happened during the assignment from TRowSize to Integer, before any divisions. Did the debugger point to a wrong line? I don't know.

Michl

2014-12-16 09:48

developer   ~0079839

I don't know if this is a bug or not, but a simple solution:

In my case:

ABand.Control.Top is 0
aBevel is 2
---> (0 - 2) div FRow --> it is not in the guilty Range(1..MaxInt)!

Simple Programm shows the same behavior:


program Project1;

type
  TRange = 1..MaxInt;
var
  Range: TRange;

begin
  Range:=1;
  WriteLn(-2 div Range);
  ReadLn;
end.


Patch added.

Michl

2014-12-16 09:48

developer  

controlbar.inc.patch (752 bytes)   
Index: lcl/include/controlbar.inc
===================================================================
--- lcl/include/controlbar.inc	(revision 47172)
+++ lcl/include/controlbar.inc	(working copy)
@@ -354,7 +354,7 @@
     aLeft := Math.min(ClientWidth - aBevel - ABand.Width, ABand.Control.Left - aBevel);
     aLeft := Math.max(aLeft, aBevel + cFullGrabber);
   end;
-  aTop := aBevel + Math.max(Math.min(j, (ABand.Control.Top - aBevel) div RowSize), 0) * RowSize;
+  aTop := aBevel + Math.max(Math.min(j, (ABand.Control.Top - aBevel) div Integer(RowSize)), 0) * RowSize;
   ABand.Height := CalcBandHeight(ABand.Control);
   { check whether the new band overlap any other }
   aRect := Rect(aLeft, aTop, aLeft + ABand.Width, aTop + ABand.Height);
controlbar.inc.patch (752 bytes)   

Derit Agustin

2014-12-16 11:55

reporter   ~0079846

error fixed :D from rev 47205

Juha Manninen

2014-12-16 12:47

developer   ~0079847

No Derit, r47202 added a workaround for TControlbar until the real problem in FPC is fixed.

Derit Agustin

2014-12-16 13:01

reporter   ~0079848

ok, but I do not find this bug after r47205

Cyrax

2014-12-17 03:03

reporter   ~0079859

Reported at FPC side : http://bugs.freepascal.org/view.php?id=27173

Do-wan Kim

2014-12-19 15:44

reporter  

division_overflow_nx86mat.pas.patch (811 bytes)   
Index: compiler/x86/nx86mat.pas
===================================================================
--- compiler/x86/nx86mat.pas	(revision 29305)
+++ compiler/x86/nx86mat.pas	(working copy)
@@ -528,7 +528,9 @@
               emit_reg_reg(A_XOR,opsize,regd,regd);
 
             {Division depends on the right type.}
-            if is_signed(right.resultdef) then
+			{ A_DIV generate SIGFPE when result does not fit register size(edx still remains after divide) after full-register-size unsigned division. 
+			  To prevent this, full-register-size signed division should be used A_IDIV. }
+            if is_signed(right.resultdef) or (is_signed(left.resultdef) and (left.resultdef.size={$ifdef x86_64}8{$else}4{$endif x86_64})) then
               op:=A_IDIV
             else
               op:=A_DIV;

Do-wan Kim

2014-12-19 15:53

reporter   ~0079901

Last edited: 2014-12-19 15:56

View 2 revisions

When edx:eax fair is not fit eax by unsigned divided result, SIGFPE generated.

movl $2,%edx
movl $-1,%eax
movl $2,%ecx <= change to $4, no error.
divl %ecx <= SIGFPE int overflow

Sorry for my short English x(

Juha Manninen

2014-12-19 17:58

developer   ~0079903

Do-wan Kim, please add your comments and patches to the related FPC report. This report is about TControlBar in Lazarus LCL.

Do-wan Kim

2014-12-19 23:53

reporter   ~0079905

yep :)

Juha Manninen

2015-01-21 18:10

developer   ~0080545

The workaround is reverted.

Issue History

Date Modified Username Field Change
2014-12-15 00:35 Derit Agustin New Issue
2014-12-15 00:35 Derit Agustin File Added: controlbar.zip
2014-12-15 00:56 Vojtech Cihak Note Added: 0079810
2014-12-15 01:27 Derit Agustin Note Added: 0079811
2014-12-15 11:24 Juha Manninen LazTarget => -
2014-12-15 11:24 Juha Manninen Note Added: 0079818
2014-12-15 11:24 Juha Manninen Assigned To => Juha Manninen
2014-12-15 11:24 Juha Manninen Status new => confirmed
2014-12-15 11:35 Jonas Maebe Note Added: 0079819
2014-12-15 11:38 Juha Manninen Note Edited: 0079818 View Revisions
2014-12-15 11:42 Derit Agustin Note Added: 0079821
2014-12-15 11:53 Juha Manninen Note Added: 0079822
2014-12-15 12:02 Jonas Maebe Note Added: 0079823
2014-12-15 12:13 Jonas Maebe Note Added: 0079824
2014-12-15 18:08 Cyrax Note Added: 0079830
2014-12-15 19:22 Juha Manninen Note Added: 0079832
2014-12-15 19:23 Juha Manninen Note Edited: 0079832 View Revisions
2014-12-15 20:26 Cyrax Note Added: 0079834
2014-12-15 22:23 ocean File Added: error.pp
2014-12-15 22:25 ocean Note Added: 0079835
2014-12-16 03:54 Do-wan Kim Note Added: 0079836
2014-12-16 04:42 Do-wan Kim Note Edited: 0079836 View Revisions
2014-12-16 05:01 Do-wan Kim File Added: ranged_division_overflow_nx86mat.pas.patch
2014-12-16 05:33 Do-wan Kim Note Edited: 0079836 View Revisions
2014-12-16 06:55 Cyrax Note Added: 0079837
2014-12-16 06:57 Cyrax Note Edited: 0079837 View Revisions
2014-12-16 08:17 Juha Manninen Note Added: 0079838
2014-12-16 09:04 Cyrax Note Edited: 0079837 View Revisions
2014-12-16 09:48 Michl Note Added: 0079839
2014-12-16 09:48 Michl File Added: controlbar.inc.patch
2014-12-16 11:55 Derit Agustin Note Added: 0079846
2014-12-16 12:47 Juha Manninen Note Added: 0079847
2014-12-16 13:01 Derit Agustin Note Added: 0079848
2014-12-17 03:03 Cyrax Note Added: 0079859
2014-12-17 10:31 Juha Manninen Relationship added related to 0027173
2014-12-19 15:44 Do-wan Kim File Added: division_overflow_nx86mat.pas.patch
2014-12-19 15:53 Do-wan Kim Note Added: 0079901
2014-12-19 15:56 Do-wan Kim Note Edited: 0079901 View Revisions
2014-12-19 17:58 Juha Manninen Note Added: 0079903
2014-12-19 23:53 Do-wan Kim Note Added: 0079905
2015-01-21 18:10 Juha Manninen Fixed in Revision => r47481
2015-01-21 18:10 Juha Manninen Note Added: 0080545
2015-01-21 18:10 Juha Manninen Status confirmed => resolved
2015-01-21 18:10 Juha Manninen Resolution open => fixed