View Issue Details

IDProjectCategoryView StatusLast Update
0000628LDMud 3.3Portabilitypublic2009-05-05 15:27
Reporterzesstra Assigned Tozesstra  
PriorityhighSeveritymajorReproducibilityN/A
Status resolvedResolutionfixed 
Platformx86_64OSMacOS XOS Version10.5.x
Product Version3.3.718 
Target Version3.3.719Fixed in Version3.3.719 
Summary0000628: Inconsistent data for F_NUMBER is generated on LP64 systems
DescriptionF_NUMBER in interpret.c expects the number to be in the host format. It reads sizeof(p_int) chars following the F_NUMBER instruction into svp[1]->u.number.

Unfortunately, the compiler does generate two different F_NUMBER:
- L_NUMBER writes sizeof(p_int) chars into the bytecode after F_NUMBER.
- ins_number() writes a int32 into the bytecode after F_NUMBER.

I don't know why nobody noticed this problem, it is quite weird. ins_number() seems to be rarely used.
TagsNo tags attached.

Activities

2009-04-19 16:17

 

0001-Fix-F_NUMBER-generation.patch (4,152 bytes)   
From df56a1f99b435edc08d8a3570e9df0325c01b636 Mon Sep 17 00:00:00 2001
From: zesstra <zesstra@zesstra.de>
Date: Sun, 19 Apr 2009 22:14:14 +0200
Subject: [PATCH] Fix F_NUMBER generation.

The compiler generated F_NUMBER at more than one places. ins_number() did
only write a int32 into the bytecode, all the other places write a p_int,
which interpret.c expects.
The patch added ins_p_int(), upd_p_int() and read_p_int().
ins_number() calls ins_p_int() upon generating the F_NUMBER instruction.
Additionally upon storing a number (L_NUMBER) and in the unary '-'
upd_p_int() is called instead of a direct memcpy(). (memcpy() is faster,
but I think it is much more risky to forget a place to change...)

Signed-off-by: zesstra <zesstra@zesstra.de>
---
 src/prolang.y |   65 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 57 insertions(+), 8 deletions(-)

diff --git a/src/prolang.y b/src/prolang.y
index 4c55b9d..7ea8e29 100644
--- a/src/prolang.y
+++ b/src/prolang.y
@@ -2053,8 +2053,60 @@ read_long (mp_uint offset)
 } /* read_long() */
 
 /*-------------------------------------------------------------------------*/
+static INLINE void
+ins_p_int (p_int num)
+
+/* Add the number <num> to the A_PROGRAM area in a fixed byteorder.
+ */
+{
+    if (realloc_a_program(sizeof(num)))
+    {
+        /* F_NUMBER expects the number in the host format. Therefore memcpy()
+         * is OK. interpret.c will read the number with memcpy() as well.
+         * TODO: use a suitable PUT_ from bytecode.h (change in interpret.c as well!)
+         */
+        memcpy(mem_block[A_PROGRAM].block + CURRENT_PROGRAM_SIZE, &num, sizeof(num));
+        CURRENT_PROGRAM_SIZE += sizeof(num);
+    }
+    else
+    {
+        yyerrorf("Out of memory: program size %"PRIuMPINT"\n"
+                 , mem_block[A_PROGRAM].current_size + sizeof(num));
+    }
+} /* ins_p_int() */
+
+/*-------------------------------------------------------------------------*/
+static INLINE void
+upd_p_int (mp_uint offset, p_int num)
+
+/* Store the number <num> at <offset> in the A_PROGRAM area in a fixed byteorder.
+ */
+{
+    /* F_NUMBER expects the number in the host format. Therefore memcpy()
+     * is OK. interpret.c will read the number with memcpy() as well.
+     * TODO: use a suitable PUT_ from bytecode.h (change in interpret.c as well!)
+     */
+    memcpy(mem_block[A_PROGRAM].block + offset, &num, sizeof(num));
+
+} /* upd_p_int() */
+
+/*-------------------------------------------------------------------------*/
+static p_int
+read_p_int (mp_uint offset)
+
+/* Return the <number> stored at <offset> in the A_PROGRAM area.
+ */
+{
+    p_int number;
+    /* TODO: use GET_ function from bytecode.h */
+    memcpy(&number, mem_block[A_PROGRAM].block + offset, sizeof(number));
+
+    return number;
+} /* read_p_int() */
+
+/*-------------------------------------------------------------------------*/
 static void
-ins_number (long num)
+ins_number (p_int num)
 
 /* Insert code to push number <num> onto the stack.
  * The function tries to find the shortest sequence to do so.
@@ -2082,7 +2134,7 @@ ins_number (long num)
     else
     {
         ins_f_code(F_NUMBER);
-        ins_long(num);
+        ins_p_int(num);
     }
 } /* ins_number() */
 
@@ -9930,12 +9982,9 @@ expr0:
                 F_NUMBER )
           {
               p_int number;
-
-              memcpy(&number, &(mem_block[A_PROGRAM].block[last_expression+1])
-                    , sizeof(number));
+              number = read_p_int(last_expression + 1);
               number = -number;
-              memcpy(&(mem_block[A_PROGRAM].block[last_expression+1]), &number
-                    , sizeof(number));
+              upd_p_int(last_expression + 1, number);
           }
           else
           {
@@ -10106,7 +10155,7 @@ expr4:
           else
           {
               add_f_code(F_NUMBER);
-              memcpy(__PREPARE_INSERT__p, &$1, sizeof $1);
+              upd_p_int((char*)__PREPARE_INSERT__p - mem_block[A_PROGRAM].block, $1);
               current += 1 + sizeof (p_int);
               $$.type = Type_Number;
           }
-- 
1.6.1

zesstra

2009-04-19 16:18

administrator   ~0001059

The patch adds ins_p_int(), upd_p_int() and read_p_int().
ins_number() now calls ins_p_int() upon generating the F_NUMBER instruction.
Additionally upon storing a number (L_NUMBER) and in the unary '-'
upd_p_int() is called instead of a direct memcpy(). (memcpy() is faster,
but I think it is much more risky to forget a place to change...)

2009-05-05 14:46

 

0002-Fixes-insert_pop_value.patch (5,334 bytes)   
From d5c5882d90fe6add24add3c67db7336090a24fb3 Mon Sep 17 00:00:00 2001
From: zesstra <zesstra@zesstra.de>
Date: Mon, 4 May 2009 17:40:36 +0200
Subject: [PATCH 2/2] Fixes insert_pop_value().

insert_pop_value() removes the last value from the stack. If possible, it
patches the code to prevent that value from being computed at all (by
removing the last expression). Unfortunately the function assumed that all
opertions have no data following in the bytecode. This patches changes it
and enabled optimization/removal of operations with data.

Signed-off-by: zesstra <zesstra@zesstra.de>
---
 src/prolang.y |  111 ++++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 70 insertions(+), 41 deletions(-)

diff --git a/src/prolang.y b/src/prolang.y
index 7ea8e29..c01eac8 100644
--- a/src/prolang.y
+++ b/src/prolang.y
@@ -14145,58 +14145,87 @@ insert_pop_value (void)
  */
 
 {
-    if (last_expression == CURRENT_PROGRAM_SIZE-1)
+    /* We don't have to fear sideeffects and try to prevent
+     * the value from being generated if the last expression is not too long
+     * ago.
+     */
+    if (last_expression == CURRENT_PROGRAM_SIZE - 1)
     {
-        /* We don't have to fear sideeffects and try to prevent
-         * the value from being generated.
-         */
+         /* The following ops have no data in the bytecode. */
         switch ( mem_block[A_PROGRAM].block[last_expression])
         {
-        case F_ASSIGN:
-            mem_block[A_PROGRAM].block[last_expression] =
+                /* The following ops have no data in the bytecode. */
+            case F_ASSIGN:
+                mem_block[A_PROGRAM].block[last_expression] =
                 F_VOID_ASSIGN;
-            break;
-        case F_ADD_EQ:
-            mem_block[A_PROGRAM].block[last_expression] =
+                break;
+            case F_ADD_EQ:
+                mem_block[A_PROGRAM].block[last_expression] =
                 F_VOID_ADD_EQ;
-            break;
-        case F_PRE_INC:
-        case F_POST_INC:
-            mem_block[A_PROGRAM].block[last_expression] =
+                break;
+            case F_PRE_INC:
+            case F_POST_INC:
+                mem_block[A_PROGRAM].block[last_expression] =
                 F_INC;
-            break;
-        case F_PRE_DEC:
-        case F_POST_DEC:
-            mem_block[A_PROGRAM].block[last_expression] =
+                break;
+            case F_PRE_DEC:
+            case F_POST_DEC:
+                mem_block[A_PROGRAM].block[last_expression] =
                 F_DEC;
-            break;
-        case F_CONST0:
-        case F_CONST1:
-        case F_NCONST1:
-            mem_block[A_PROGRAM].current_size = last_expression;
-            break;
-        case F_CLIT:
-        case F_NCLIT:
-        case F_CSTRING0:
-        case F_CSTRING1:
-        case F_CSTRING2:
-            mem_block[A_PROGRAM].current_size = last_expression-1;
-            break;
-        case F_STRING:
-            mem_block[A_PROGRAM].current_size = last_expression-2;
-            break;
-        case F_NUMBER:
-            mem_block[A_PROGRAM].current_size = last_expression-4;
-            break;
-        default: ins_f_code(F_POP_VALUE);
+                break;
+            case F_CONST0:
+            case F_CONST1:
+            case F_NCONST1:
+                mem_block[A_PROGRAM].current_size = last_expression;
+                break;
+            default:
+                ins_f_code(F_POP_VALUE);
+                break;
         }
-        last_expression = -1;
+    }
+    else if (last_expression == CURRENT_PROGRAM_SIZE - 2)
+    {
+        /* The following ops are followed by 1 chars of data in the bytecode. */
+        switch ( mem_block[A_PROGRAM].block[last_expression])
+        {
+            case F_CLIT:
+            case F_NCLIT:
+            case F_CSTRING0:
+            case F_CSTRING1:
+            case F_CSTRING2:
+            case F_CSTRING3:
+                mem_block[A_PROGRAM].current_size = last_expression;
+                break;
+            default:
+                ins_f_code(F_POP_VALUE);
+                break;
+        }
+    }
+    else if (last_expression == CURRENT_PROGRAM_SIZE - 3)
+    {
+        /* The following ops are followed by 2 chars of data in the bytecode. */
+        if ( mem_block[A_PROGRAM].block[last_expression] == F_STRING)
+            mem_block[A_PROGRAM].current_size = last_expression;
+        else
+            ins_f_code(F_POP_VALUE);            
+    }
+    else if (last_expression == CURRENT_PROGRAM_SIZE - sizeof(p_int))
+    {
+        /* The following ops are followed by sizeof(p_int) chars of data in 
+         * the bytecode. */
+        if ( mem_block[A_PROGRAM].block[last_expression] == F_NUMBER)
+            mem_block[A_PROGRAM].current_size = last_expression;
+        else
+            ins_f_code(F_POP_VALUE);            
     }
     else
-        /* The last expression is too long ago: just pop whatever there
-         * is on the stack.
-         */
+    {
+        /* last expression unknown or too long ago - just pop whatever there
+         * is on the stack */
         ins_f_code(F_POP_VALUE);
+    }
+    
+    last_expression = -1;
 } /* insert_pop_value() */
 
 /*-------------------------------------------------------------------------*/
-- 
1.6.1

zesstra

2009-05-05 14:50

administrator   ~0001079

Gnomi noticed, that insert_pop_value() is not working properly, because it did not take into account, that the operations have different payloads of data following. I attached a patch which should rectify this. Could someone have a short look? BTW: insert_pop_value() would now always set last_expression to -1, this may have side effects, I didn't notice. If this is the case, it has to be changed.

zesstra

2009-05-05 15:27

administrator   ~0001080

Ok, great. This issue should then be fixed by r2562 and r2563.

Issue History

Date Modified Username Field Change
2009-04-19 15:34 zesstra New Issue
2009-04-19 15:34 zesstra Status new => assigned
2009-04-19 15:34 zesstra Assigned To => zesstra
2009-04-19 16:17 zesstra File Added: 0001-Fix-F_NUMBER-generation.patch
2009-04-19 16:18 zesstra Note Added: 0001059
2009-05-05 14:46 zesstra File Added: 0002-Fixes-insert_pop_value.patch
2009-05-05 14:50 zesstra Note Added: 0001079
2009-05-05 15:27 zesstra Note Added: 0001080
2009-05-05 15:27 zesstra Status assigned => resolved
2009-05-05 15:27 zesstra Fixed in Version => 3.3.719
2009-05-05 15:27 zesstra Resolution open => fixed