View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0000537LDMud 3.3LPC Compiler/Preprocessorpublic2008-05-04 11:562018-01-29 22:57
Reporterzesstra Assigned ToGnomi  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Platformx86_64OSMacOS XOS Version10.5.x
Product Version3.3 
Fixed in Version3.3.717 
Summary0000537: Missing initialization of local variables in some cases
DescriptionThere seems to be an issue with the initialization of local variables.
Assume the following code:

void test() {
  mapping m=(["bla": 100]);
  printf("Anfang: %O\n",m);
  object key;
  foreach (key, int val: &m) {
    val--;
  }
  printf("Nach FE: %O\n", m);
  int tmp=-1;
  printf("Ende: %O\n", m);
}

It produces the following output:
Anfang: ([ /* 0000001 */
  "bla": 100
])
Nach FE: ([ /* 0000001 */
  "bla": 99
])
Ende: ([ /* 0000001 */
  "bla": -1
])

The driver allocates the same space to variables val and tmp, because val does
not exist after the foreach() block. Unfortunately the svalue for the new tmp
is obviously not properly initialized. It is still of type T_LVALUE and the pointer u.lvalue in it still points to the svalue in the mapping which was assigned in the previous code block.
It does not happen if the declaration of the new variable is in another code
block, upon entering a code block the local variables are zeroed, because of
exactly this problem (prolang.<, line 6854.
Luckily, the bug only shows up in rare circumstances (at least in our lib), but it can be quite nasty to track.
TagsNo tags attached.

Relationships

related to 0000553 resolvedGnomi LDMud 3.2 Backports of 3.3 fixes for 3.2.16 

Activities

2008-05-07 14:41

 

prolang.clearlocals.diff (7,820 bytes)    
Index: trunk/src/prolang.y
===================================================================
--- trunk/src/prolang.y	(Revision 2364)
+++ trunk/src/prolang.y	(Arbeitskopie)
@@ -576,6 +576,10 @@
 {
     int     first_local;  /* Number of first local defined in this scope */
     int     num_locals;   /* Number of locals defined in this scope */
+    int     num_cleared;
+      /* Number of locals that have been cleared by earlier CLEAR_LOCALS */
+    Bool    clobbered;
+      /* Local variables beyond num_locals may be clobbered */
     mp_uint addr;
       /* Address of CLEAR_LOCALS instruction, needed for backpatching */
 };
@@ -2772,6 +2776,8 @@
 {
     block_scope[depth-1].num_locals = 0;
     block_scope[depth-1].first_local = current_number_of_locals;
+    block_scope[depth-1].num_cleared = 0;
+    block_scope[depth-1].clobbered = MY_FALSE;
     block_scope[depth-1].addr = 0;
 } /* init_scope() */
 
@@ -2795,10 +2801,13 @@
 
 /*-------------------------------------------------------------------------*/
 static void
-leave_block_scope (void)
+leave_block_scope (Bool dontclobber)
 
 /* Leave the current scope (if use_local_scopes requires it), freeing
  * all local names defined in that scope.
+ *
+ * <dontclobber> should be MY_TRUE if the stack of the to-be-left scope
+ * is independent of the outer scope (i.e. the scope of closures).
  */
 
 {
@@ -2806,6 +2815,13 @@
     {
         free_local_names(block_depth);
         block_depth--;
+        if (block_depth && !dontclobber
+         && (block_scope[block_depth].num_locals
+          || block_scope[block_depth].clobbered))
+        {
+            /* the block we just left may have clobbered local variables */
+            block_scope[block_depth-1].clobbered = MY_TRUE;
+        }
     }
 } /* leave_block_scope() */
 
@@ -5142,8 +5158,8 @@
         yyerror("Implementation restriction: Inline closure must not span "
                 "include file limits");
         /* Clean up */
-        leave_block_scope();  /* Argument scope */
-        leave_block_scope();  /* Context scope */
+        leave_block_scope(MY_TRUE);  /* Argument scope */
+        leave_block_scope(MY_TRUE);  /* Context scope */
         finish_inline_closure(MY_TRUE);
         return;
     }
@@ -5248,8 +5264,8 @@
 
 
     /* Clean up */
-    leave_block_scope();  /* Argument scope */
-    leave_block_scope();  /* Context scope */
+    leave_block_scope(MY_TRUE);  /* Argument scope */
+    leave_block_scope(MY_TRUE);  /* Context scope */
     finish_inline_closure(MY_FALSE);
 } /* complete_inline_closure() */
 #endif /* USE_NEW_INLINES */
@@ -5969,6 +5985,12 @@
 
           if (!inline_closure_prototype(9))
               YYACCEPT;
+
+          /* Put the code block in its own scope apart from the
+           * parameters, so that define_local_variable doesn't
+           * assume that there are already 9 Variables.
+           */
+          enter_block_scope();
 #ifdef DEBUG_INLINES
 printf("DEBUG: Before comma_expr: program size %ld\n", CURRENT_PROGRAM_SIZE);
 #endif /* DEBUG_INLINES */
@@ -5982,6 +6004,8 @@
 #ifdef DEBUG_INLINES
 printf("DEBUG: After L_END_INLINE: program size %ld\n", CURRENT_PROGRAM_SIZE);
 #endif /* DEBUG_INLINES */
+         leave_block_scope(MY_FALSE);
+        
          $$.start = current_inline->end;
          $$.code = -1;
          $$.type = Type_Closure;
@@ -6860,17 +6884,17 @@
           {
               block_scope_t *scope = block_scope + block_depth - 1;
 
-              if (use_local_scopes && scope->num_locals)
+              if (use_local_scopes && scope->num_locals > scope->num_cleared)
               {
                   mem_block[A_PROGRAM].block[scope->addr+2]
-                    = (char)scope->num_locals;
+                    = (char)(scope->num_locals - scope->num_cleared);
               }
           }
       }
 
       '}'
 
-      { leave_block_scope(); }
+      { leave_block_scope(MY_FALSE); }
 ; /* block */
 
 
@@ -7491,10 +7515,10 @@
           {
               block_scope_t *scope = block_scope + block_depth - 1;
 
-              if (use_local_scopes && scope->num_locals)
+              if (use_local_scopes && scope->num_locals > scope->num_cleared)
               {
                   mem_block[A_PROGRAM].block[scope->addr+2]
-                    = (char)scope->num_locals;
+                    = (char)(scope->num_locals - scope->num_cleared);
               }
           }
       }
@@ -7583,7 +7607,7 @@
           current_break_address    = $<numbers>3[1];
 
           /* and leave the for scope */
-          leave_block_scope();
+          leave_block_scope(MY_FALSE);
       }
 ; /* for */
 
@@ -7725,10 +7749,10 @@
           {
               block_scope_t *scope = block_scope + block_depth - 1;
 
-              if (use_local_scopes && scope->num_locals)
+              if (use_local_scopes && scope->num_locals > scope->num_cleared)
               {
                   mem_block[A_PROGRAM].block[scope->addr+2]
-                    = (char)scope->num_locals;
+                    = (char)(scope->num_locals - scope->num_cleared);
               }
           }
 
@@ -7842,7 +7866,7 @@
           current_break_address    = $<numbers>3[1];
 
           /* and leave the scope */
-          leave_block_scope();
+          leave_block_scope(MY_FALSE);
       }
 ; /* foreach */
 
@@ -13226,7 +13250,7 @@
                                     );
 
            /* Restore the old locals information */
-           leave_block_scope();
+           leave_block_scope(MY_TRUE);
            use_local_scopes = pragma_use_local_scopes;
            all_locals = save_all_locals;
            current_number_of_locals = save_current_number_of_locals;
@@ -13806,14 +13830,24 @@
             q = redeclare_local(name, actual_type, block_depth);
         else
             q = add_local_name(name, actual_type, block_depth);
-        if (use_local_scopes && scope->num_locals == 1)
+
+        if (use_local_scopes && scope->clobbered)
         {
+            /* finish the previous CLEAR_LOCALS, if any */
+            if (scope->num_locals - 1 > scope->num_cleared)
+                mem_block[A_PROGRAM].block[scope->addr+2]
+                  = (char)(scope->num_locals - 1 - scope->num_cleared);
+            scope->clobbered = MY_FALSE;
+            scope->num_cleared = scope->num_locals - 1;
+        }
+        if (use_local_scopes && scope->num_locals == scope->num_cleared + 1)
+        {
             /* First definition of a local, so insert the
              * clear_locals bytecode and remember its position
              */
             scope->addr = mem_block[A_PROGRAM].current_size;
             ins_f_code(F_CLEAR_LOCALS);
-            ins_byte(scope->first_local);
+            ins_byte(scope->first_local + scope->num_cleared);
             ins_byte(0);
         }
 
@@ -13826,14 +13860,24 @@
     else
         q = add_local_name(name, actual_type, block_depth, MY_FALSE);
 
-    if (use_local_scopes && scope->num_locals == 1)
+    if (use_local_scopes && scope->clobbered)
     {
+        /* finish the previous CLEAR_LOCALS, if any */
+        if (scope->num_locals - 1 > scope->num_cleared)
+            mem_block[A_PROGRAM].block[scope->addr+2]
+              = (char)(scope->num_locals - 1 - scope->num_cleared);
+        scope->clobbered = MY_FALSE;
+        scope->num_cleared = scope->num_locals - 1;
+    }
+
+    if (use_local_scopes && scope->num_locals == scope->num_cleared + 1)
+    {
         /* First definition of a local, so insert the
          * clear_locals bytecode and remember its position
          */
         scope->addr = mem_block[A_PROGRAM].current_size;
         ins_f_code(F_CLEAR_LOCALS);
-        ins_byte(scope->first_local);
+        ins_byte(scope->first_local + scope->num_cleared);
         ins_byte(0);
     }
prolang.clearlocals.diff (7,820 bytes)   

Gnomi

2008-05-07 14:46

manager   ~0000616

I uploaded a patch from Fuchur@Wunderland for this bug. It keeps track of these local variables that have to be initialized again and inserts additional F_CLEAR_LOCAL commands where appropriate.

Coogan

2008-05-08 09:10

reporter   ~0000618

This issue also affects driver version 3.2.15.

2008-05-09 15:34

 

fix-clear-locals.patch (8,459 bytes)    
commit 85a39653c8099faad49a56b3b8c03deb621f267e
Author: Bertram Felgenhauer <int-e@gmx.de>
Date:   Fri May 9 21:30:33 2008 +0200

    Fix a bug in the initialisation of local variables.
    The code was overly eager in combining several variable initialisations into
    a single F_CLEAR_LOCALS statement. This patch fixes that.
    
    This patch also includes a bugfix by Gnomi related to the contexts for
    inline closures, and an optimization to the clobbered flag handling, also
    by Gnomi.

diff --git a/src/prolang.y b/src/prolang.y
index 87be9fd..bc2a116 100644
--- a/src/prolang.y
+++ b/src/prolang.y
@@ -576,6 +576,10 @@ struct block_scope_s
 {
     int     first_local;  /* Number of first local defined in this scope */
     int     num_locals;   /* Number of locals defined in this scope */
+    int     num_cleared;
+      /* Number of locals that have been cleared by earlier CLEAR_LOCALS */
+    Bool    clobbered;
+      /* Local variables beyond num_locals may be clobbered */
     mp_uint addr;
       /* Address of CLEAR_LOCALS instruction, needed for backpatching */
 };
@@ -2772,6 +2776,8 @@ init_scope (int depth)
 {
     block_scope[depth-1].num_locals = 0;
     block_scope[depth-1].first_local = current_number_of_locals;
+    block_scope[depth-1].num_cleared = 0;
+    block_scope[depth-1].clobbered = MY_FALSE;
     block_scope[depth-1].addr = 0;
 } /* init_scope() */
 
@@ -2795,7 +2801,7 @@ enter_block_scope (void)
 
 /*-------------------------------------------------------------------------*/
 static void
-leave_block_scope (void)
+leave_block_scope (Bool dontclobber)
 
 /* Leave the current scope (if use_local_scopes requires it), freeing
  * all local names defined in that scope.
@@ -2806,6 +2812,13 @@ leave_block_scope (void)
     {
         free_local_names(block_depth);
         block_depth--;
+        if (block_depth && !dontclobber
+         && (block_scope[block_depth].num_locals
+          || block_scope[block_depth].clobbered))
+        {
+            /* the block we just left may have clobbered local variables */
+            block_scope[block_depth-1].clobbered = MY_TRUE;
+        }
     }
 } /* leave_block_scope() */
 
@@ -5142,8 +5155,8 @@ printf("DEBUG: Generate inline closure function:\n");
         yyerror("Implementation restriction: Inline closure must not span "
                 "include file limits");
         /* Clean up */
-        leave_block_scope();  /* Argument scope */
-        leave_block_scope();  /* Context scope */
+        leave_block_scope(MY_TRUE);  /* Argument scope */
+        leave_block_scope(MY_TRUE);  /* Context scope */
         finish_inline_closure(MY_TRUE);
         return;
     }
@@ -5248,8 +5261,8 @@ printf("DEBUG:     -> F_CONTEXT_CLOSURE %d %d\n", current_inline->function, cont
 
 
     /* Clean up */
-    leave_block_scope();  /* Argument scope */
-    leave_block_scope();  /* Context scope */
+    leave_block_scope(MY_TRUE);  /* Argument scope */
+    leave_block_scope(MY_TRUE);  /* Context scope */
     finish_inline_closure(MY_FALSE);
 } /* complete_inline_closure() */
 #endif /* USE_NEW_INLINES */
@@ -5969,6 +5982,8 @@ printf("DEBUG: After L_BEGIN_INLINE: program size %ld\n", CURRENT_PROGRAM_SIZE);
 
           if (!inline_closure_prototype(9))
               YYACCEPT;
+
+          enter_block_scope();
 #ifdef DEBUG_INLINES
 printf("DEBUG: Before comma_expr: program size %ld\n", CURRENT_PROGRAM_SIZE);
 #endif /* DEBUG_INLINES */
@@ -5982,6 +5997,8 @@ printf("DEBUG: Before comma_expr: program size %ld\n", CURRENT_PROGRAM_SIZE);
 #ifdef DEBUG_INLINES
 printf("DEBUG: After L_END_INLINE: program size %ld\n", CURRENT_PROGRAM_SIZE);
 #endif /* DEBUG_INLINES */
+         leave_block_scope(MY_FALSE);
+        
          $$.start = current_inline->end;
          $$.code = -1;
          $$.type = Type_Closure;
@@ -6860,17 +6877,17 @@ block:
           {
               block_scope_t *scope = block_scope + block_depth - 1;
 
-              if (use_local_scopes && scope->num_locals)
+              if (use_local_scopes && scope->num_locals > scope->num_cleared)
               {
                   mem_block[A_PROGRAM].block[scope->addr+2]
-                    = (char)scope->num_locals;
+                    = (char)(scope->num_locals - scope->num_cleared);
               }
           }
       }
 
       '}'
 
-      { leave_block_scope(); }
+      { leave_block_scope(MY_FALSE); }
 ; /* block */
 
 
@@ -7491,10 +7508,10 @@ for:
           {
               block_scope_t *scope = block_scope + block_depth - 1;
 
-              if (use_local_scopes && scope->num_locals)
+              if (use_local_scopes && scope->num_locals > scope->num_cleared)
               {
                   mem_block[A_PROGRAM].block[scope->addr+2]
-                    = (char)scope->num_locals;
+                    = (char)(scope->num_locals - scope->num_cleared);
               }
           }
       }
@@ -7583,7 +7600,7 @@ for:
           current_break_address    = $<numbers>3[1];
 
           /* and leave the for scope */
-          leave_block_scope();
+          leave_block_scope(MY_FALSE);
       }
 ; /* for */
 
@@ -7725,10 +7742,10 @@ foreach:
           {
               block_scope_t *scope = block_scope + block_depth - 1;
 
-              if (use_local_scopes && scope->num_locals)
+              if (use_local_scopes && scope->num_locals > scope->num_cleared)
               {
                   mem_block[A_PROGRAM].block[scope->addr+2]
-                    = (char)scope->num_locals;
+                    = (char)(scope->num_locals - scope->num_cleared);
               }
           }
 
@@ -7842,7 +7859,7 @@ foreach:
           current_break_address    = $<numbers>3[1];
 
           /* and leave the scope */
-          leave_block_scope();
+          leave_block_scope(MY_FALSE);
       }
 ; /* foreach */
 
@@ -13226,7 +13243,7 @@ inline_fun:
                                     );
 
            /* Restore the old locals information */
-           leave_block_scope();
+           leave_block_scope(MY_TRUE);
            use_local_scopes = pragma_use_local_scopes;
            all_locals = save_all_locals;
            current_number_of_locals = save_current_number_of_locals;
@@ -13806,14 +13823,24 @@ printf("DEBUG:   context name '%s'\n", get_txt(name->name));
             q = redeclare_local(name, actual_type, block_depth);
         else
             q = add_local_name(name, actual_type, block_depth);
-        if (use_local_scopes && scope->num_locals == 1)
+
+        if (use_local_scopes && scope->clobbered)
+        {
+            /* finish the previous CLEAR_LOCALS, if any */
+            if (scope->num_locals - 1 > scope->num_cleared)
+                mem_block[A_PROGRAM].block[scope->addr+2]
+                  = (char)(scope->num_locals - 1 - scope->num_cleared);
+            scope->clobbered = MY_FALSE;
+            scope->num_cleared = scope->num_locals - 1;
+        }
+        if (use_local_scopes && scope->num_locals == scope->num_cleared + 1)
         {
             /* First definition of a local, so insert the
              * clear_locals bytecode and remember its position
              */
             scope->addr = mem_block[A_PROGRAM].current_size;
             ins_f_code(F_CLEAR_LOCALS);
-            ins_byte(scope->first_local);
+            ins_byte(scope->first_local + scope->num_cleared);
             ins_byte(0);
         }
 
@@ -13826,14 +13853,24 @@ printf("DEBUG:   context name '%s'\n", get_txt(name->name));
     else
         q = add_local_name(name, actual_type, block_depth, MY_FALSE);
 
-    if (use_local_scopes && scope->num_locals == 1)
+    if (use_local_scopes && scope->clobbered)
+    {
+        /* finish the previous CLEAR_LOCALS, if any */
+        if (scope->num_locals - 1 > scope->num_cleared)
+            mem_block[A_PROGRAM].block[scope->addr+2]
+              = (char)(scope->num_locals - 1 - scope->num_cleared);
+        scope->clobbered = MY_FALSE;
+        scope->num_cleared = scope->num_locals - 1;
+    }
+
+    if (use_local_scopes && scope->num_locals == scope->num_cleared + 1)
     {
         /* First definition of a local, so insert the
          * clear_locals bytecode and remember its position
          */
         scope->addr = mem_block[A_PROGRAM].current_size;
         ins_f_code(F_CLEAR_LOCALS);
-        ins_byte(scope->first_local);
+        ins_byte(scope->first_local + scope->num_cleared);
         ins_byte(0);
     }
fix-clear-locals.patch (8,459 bytes)   

fufu

2008-05-09 15:36

manager   ~0000619

Last edited: 2008-05-09 15:37

 I've just uploaded my patch for this problem with Gnomi's changes included.
(The patch Gnomi uploaded applied on top of my initial patch.)

Gnomi

2008-07-10 05:04

manager   ~0000715

Fixed in 3.3.717 (r2379).

Issue History

Date Modified Username Field Change
2008-05-04 11:56 zesstra New Issue
2008-05-07 14:41 Gnomi File Added: prolang.clearlocals.diff
2008-05-07 14:46 Gnomi Note Added: 0000616
2008-05-08 09:10 Coogan Note Added: 0000618
2008-05-09 15:34 fufu File Added: fix-clear-locals.patch
2008-05-09 15:36 fufu Note Added: 0000619
2008-05-09 15:37 fufu Note Edited: 0000619
2008-07-01 10:06 Gnomi Status new => confirmed
2008-07-10 05:04 Gnomi Status confirmed => resolved
2008-07-10 05:04 Gnomi Fixed in Version => 3.3.717
2008-07-10 05:04 Gnomi Resolution open => fixed
2008-07-10 05:04 Gnomi Assigned To => Gnomi
2008-07-10 05:04 Gnomi Note Added: 0000715
2008-07-10 09:36 Gnomi Relationship added related to 0000553
2010-11-16 10:42 Gnomi Source_changeset_attached => ldmud.git master c038c51a
2010-11-16 10:42 Gnomi Source_changeset_attached => ldmud.git master 7c7d10be
2018-01-29 19:59 Gnomi Source_changeset_attached => ldmud.git master c038c51a
2018-01-29 19:59 Gnomi Source_changeset_attached => ldmud.git master 7c7d10be
2018-01-29 22:57 Gnomi Source_changeset_attached => ldmud.git master c038c51a
2018-01-29 22:57 Gnomi Source_changeset_attached => ldmud.git master 7c7d10be