List:Commits« Previous MessageNext Message »
From:pem Date:January 26 2006 4:25pm
Subject:bk commit into 5.0 tree (pem:1.2009) BUG#16568
View as plain text  
Below is the list of changes that have just been committed into a local
5.0 repository of pem. When pem does a push these changes will
be propagated to the main repository and, within 24 hours after the
push, to the public repository.
For information on how to access the public repository
see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html

ChangeSet
  1.2009 06/01/26 17:26:25 pem@stripped +6 -0
   Fixed on BUG#16568: Continue handler with simple CASE not working correctly
    After trying multiple inheritance (to messy and hard make it work) and
    sublassing jump_if_not (worked, but ugly), decided to on this solution
    instead:
    Inserting an abstract sp_instr_opt_meta class as parent for all instructions
    with destinations makes it possible to handle a continuation pointer for
    sp_instr_set_case_expr too.
    Note: No special test case; the fix is captured by the changed behaviour of
    bug14643_2, and bug14498_4 (formerly disabled), in sp.test.

  sql/sql_yacc.yy
    1.450 06/01/26 17:26:22 pem@stripped +9 -8
    sp_instr_set_case_expr is now added to backpatch list (for the new cont. destination).

  sql/sql_parse.cc
    1.524 06/01/26 17:26:21 pem@stripped +1 -1
    Fixed small bug at show_routine_code() call (tested return value the wrong way).

  sql/sp_head.h
    1.78 06/01/26 17:26:21 pem@stripped +69 -27
    New abstract class between sp_instr and instructions with destinations, in particular
    sp_instr_set_case_expr, for continuation destination handling.
    Changed type of some parameters and variables (sp_instr_opt_meta instead of sp_instr_jump*).
    Added opt_mark(), opt_move() and set_destination() methods to
    sp_instr_set_case_expr.

  sql/sp_head.cc
    1.199 06/01/26 17:26:21 pem@stripped +64 -21
    Changed type of some parameters and variables (sp_instr_opt_meta instead of sp_instr_jump*).
    Added consistency check of m_ip member in instructions in sp_head::show_routine_code().
    Updated print() method of, and added opt_mark() and opt_move() methods to
    sp_instr_set_case_expr, to handle the new continuation destination.

  mysql-test/t/sp.test
    1.173 06/01/26 17:26:21 pem@stripped +1 -4
    Enabled test bug14498_4 for BUG#16568.

  mysql-test/r/sp.result
    1.184 06/01/26 17:26:21 pem@stripped +5 -2
    Updated results for BUG#16568 (affects results for bug14643_2 and bug14498_4)

# This is a BitKeeper patch.  What follows are the unified diffs for the
# set of deltas contained in the patch.  The rest of the patch, the part
# that BitKeeper cares about, is below these diffs.
# User:	pem
# Host:	pem.mysql.com
# Root:	/extern/mysql/work/bug16568/mysql-5.0

--- 1.523/sql/sql_parse.cc	2006-01-23 17:03:03 +01:00
+++ 1.524/sql/sql_parse.cc	2006-01-26 17:26:21 +01:00
@@ -4599,7 +4599,7 @@
       else
         sp= sp_find_routine(thd, TYPE_ENUM_FUNCTION, lex->spname,
                             &thd->sp_func_cache, FALSE);
-      if (!sp || !sp->show_routine_code(thd))
+      if (!sp || sp->show_routine_code(thd))
       {
         /* We don't distinguish between errors for now */
         my_error(ER_SP_DOES_NOT_EXIST, MYF(0),

--- 1.449/sql/sql_yacc.yy	2006-01-24 18:15:09 +01:00
+++ 1.450/sql/sql_yacc.yy	2006-01-26 17:26:22 +01:00
@@ -2018,17 +2018,18 @@
 	    sp_head *sp= lex->sphead;
 	    sp_pcontext *parsing_ctx= lex->spcont;
 	    int case_expr_id= parsing_ctx->register_case_expr();
+            sp_instr_set_case_expr *i;
 	    
 	    if (parsing_ctx->push_case_expr_id(case_expr_id))
               YYABORT;
-	    
-	    sp->add_instr(
-	      new sp_instr_set_case_expr(sp->instructions(),
-	                                 parsing_ctx,
-	                                 case_expr_id,
-	                                 $3,
-	                                 lex));
-	    
+
+            i= new sp_instr_set_case_expr(sp->instructions(),
+                                          parsing_ctx,
+                                          case_expr_id,
+                                          $3,
+                                          lex);
+            sp->add_cont_backpatch(i);
+            sp->add_instr(i);
 	    sp->m_flags|= sp_head::IN_SIMPLE_CASE;
 	    sp->restore_lex(YYTHD);
 	  }

--- 1.183/mysql-test/r/sp.result	2006-01-20 08:32:18 +01:00
+++ 1.184/mysql-test/r/sp.result	2006-01-26 17:26:21 +01:00
@@ -4089,8 +4089,6 @@
 call bug14643_2()|
 Handler
 boo
-2
-2
 Handler
 boo
 drop procedure bug14643_1|
@@ -4428,6 +4426,11 @@
 call bug14498_3()|
 v
 maybe
+Handler
+error
+End
+done
+call bug14498_4()|
 Handler
 error
 End

--- 1.172/mysql-test/t/sp.test	2006-01-20 08:32:18 +01:00
+++ 1.173/mysql-test/t/sp.test	2006-01-26 17:26:21 +01:00
@@ -5210,10 +5210,7 @@
 call bug14498_1()|
 call bug14498_2()|
 call bug14498_3()|
-# We couldn't call this before, due to a known bug (BUG#14643)
-# QQ We still can't since the new set_case_expr instruction breaks
-#    the semantics of case; it won't crash, but will get the wrong result.
-#call bug14498_4()|
+call bug14498_4()|
 call bug14498_5()|
 
 drop procedure bug14498_1|

--- 1.198/sql/sp_head.cc	2006-01-20 13:59:18 +01:00
+++ 1.199/sql/sp_head.cc	2006-01-26 17:26:21 +01:00
@@ -1761,7 +1761,7 @@
 
 
 void
-sp_head::new_cont_backpatch(sp_instr_jump_if_not *i)
+sp_head::new_cont_backpatch(sp_instr_opt_meta *i)
 {
   m_cont_level+= 1;
   if (i)
@@ -1773,7 +1773,7 @@
 }
 
 void
-sp_head::add_cont_backpatch(sp_instr_jump_if_not *i)
+sp_head::add_cont_backpatch(sp_instr_opt_meta *i)
 {
   i->m_cont_dest= m_cont_level;
   (void)m_cont_backpatch.push_front(i);
@@ -1784,7 +1784,7 @@
 {
   uint dest= instructions();
   uint lev= m_cont_level--;
-  sp_instr_jump_if_not *i;
+  sp_instr_opt_meta *i;
 
   while ((i= m_cont_backpatch.head()) && i->m_cont_dest == lev)
   {
@@ -2039,10 +2039,10 @@
 
 	set_dynamic(&m_instr, (gptr)&i, dst);
 	while ((ibp= li++))
-	{
-	  sp_instr_jump *ji= static_cast<sp_instr_jump *>(ibp);
-          ji->set_destination(src, dst);
-	}
+        {
+          sp_instr_opt_meta *im= static_cast<sp_instr_opt_meta *>(ibp);
+          im->set_destination(src, dst);
+        }
       }
       i->opt_move(dst, &bp);
       src+= 1;
@@ -2064,6 +2064,10 @@
 
 
 #ifndef DBUG_OFF
+/*
+  Return the routine instructions as a result set.
+  Returns 0 if ok, !=0 on error.
+*/
 int
 sp_head::show_routine_code(THD *thd)
 {
@@ -2091,6 +2095,22 @@
 
   for (ip= 0; (i = get_instr(ip)) ; ip++)
   {
+    /* 
+      Consistency check. If these are different something went wrong
+      during optimization.
+    */
+    if (ip != i->m_ip)
+    {
+      const char *format= "Instruction at position %u has m_ip=%u";
+      char tmp[sizeof(format) + 2*SP_INSTR_UINT_MAXLEN + 1];
+
+      sprintf(tmp, format, ip, i->m_ip);
+      /*
+        Since this is for debugging purposes only, we don't bother to
+        introduce a special error code for it.
+      */
+      push_warning(thd, MYSQL_ERROR::WARN_LEVEL_WARN, ER_UNKNOWN_ERROR, tmp);
+    }
     protocol->prepare_for_resend();
     protocol->store((longlong)ip);
 
@@ -2515,14 +2535,14 @@
 void
 sp_instr_jump_if_not::print(String *str)
 {
-  /* jump_if_not dest ... */
+  /* jump_if_not dest(cont) ... */
   if (str->reserve(2*SP_INSTR_UINT_MAXLEN+14+32)) // Add some for the expr. too
     return;
   str->qs_append(STRING_WITH_LEN("jump_if_not "));
   str->qs_append(m_dest);
-  str->append('(');
+  str->qs_append('(');
   str->qs_append(m_cont_dest);
-  str->append(") ");
+  str->qs_append(STRING_WITH_LEN(") "));
   m_expr->print(str);
 }
 
@@ -3080,28 +3100,51 @@
       spcont->clear_handler();
       thd->spcont= spcont;
     }
+    *nextp= m_cont_dest;        /* For continue handler */
   }
+  else
+    *nextp= m_ip+1;
 
-  *nextp = m_ip+1;
-
-  return res; /* no error */
+  return res;
 }
 
 
 void
 sp_instr_set_case_expr::print(String *str)
 {
-  const char CASE_EXPR_TAG[]= "set_case_expr ";
-  const int CASE_EXPR_TAG_LEN= sizeof(CASE_EXPR_TAG) - 1;
-  const int INT_STRING_MAX_LEN= 10;
-
-  /* We must call reserve(), because qs_append() doesn't care about memory. */
-  str->reserve(CASE_EXPR_TAG_LEN + INT_STRING_MAX_LEN + 2);
-
-  str->qs_append(CASE_EXPR_TAG, CASE_EXPR_TAG_LEN);
+  /* set_case_expr (cont) id ... */
+  str->reserve(2*SP_INSTR_UINT_MAXLEN+18+32); // Add some extra for expr too
+  str->qs_append(STRING_WITH_LEN("set_case_expr ("));
+  str->qs_append(m_cont_dest);
+  str->qs_append(STRING_WITH_LEN(") "));
   str->qs_append(m_case_expr_id);
   str->qs_append(' ');
   m_case_expr->print(str);
+}
+
+uint
+sp_instr_set_case_expr::opt_mark(sp_head *sp)
+{
+  sp_instr *i;
+
+  marked= 1;
+  if ((i= sp->get_instr(m_cont_dest)))
+  {
+    m_cont_dest= i->opt_shortcut_jump(sp, this);
+    m_cont_optdest= sp->get_instr(m_cont_dest);
+  }
+  sp->opt_mark(m_cont_dest);
+  return m_ip+1;
+}
+
+void
+sp_instr_set_case_expr::opt_move(uint dst, List<sp_instr> *bp)
+{
+  if (m_cont_dest > m_ip)
+    bp->push_back(this);        // Forward
+  else if (m_cont_optdest)
+    m_cont_dest= m_cont_optdest->m_ip; // Backward
+  m_ip= dst;
 }
 
 

--- 1.77/sql/sp_head.h	2006-01-19 16:13:00 +01:00
+++ 1.78/sql/sp_head.h	2006-01-26 17:26:21 +01:00
@@ -41,6 +41,7 @@
 
 struct sp_label;
 class sp_instr;
+class sp_instr_opt_meta;
 class sp_instr_jump_if_not;
 struct sp_cond_type;
 struct sp_pvar;
@@ -271,11 +272,11 @@
 
   // Start a new cont. backpatch level. If 'i' is NULL, the level is just incr.
   void
-  new_cont_backpatch(sp_instr_jump_if_not *i);
+  new_cont_backpatch(sp_instr_opt_meta *i);
 
   // Add an instruction to the current level
   void
-  add_cont_backpatch(sp_instr_jump_if_not *i);
+  add_cont_backpatch(sp_instr_opt_meta *i);
 
   // Backpatch (and pop) the current level to the current position.
   void
@@ -372,15 +373,15 @@
   } bp_t;
   List<bp_t> m_backpatch;	// Instructions needing backpatching
   /*
-    We need a special list for backpatching of conditional jump's continue
+    We need a special list for backpatching of instructions with a continue
     destination (in the case of a continue handler catching an error in
     the test), since it would otherwise interfere with the normal backpatch
-    mechanism - jump_if_not instructions have two different destination
+    mechanism - e.g. jump_if_not instructions have two different destinations
     which are to be patched differently.
     Since these occur in a more restricted way (always the same "level" in
     the code), we don't need the label.
    */
-  List<sp_instr_jump_if_not> m_cont_backpatch;
+  List<sp_instr_opt_meta> m_cont_backpatch;
   uint m_cont_level;            // The current cont. backpatch level
 
   /*
@@ -660,21 +661,55 @@
 }; // class sp_instr_trigger_field : public sp_instr
 
 
-class sp_instr_jump : public sp_instr
+/*
+  An abstract class for all instructions with destinations that
+  needs to be updated by the optimizer.
+  Even if not all subclasses will use both the normal destination and
+  the continuation destination, we put them both here for simplicity.
+ */
+class sp_instr_opt_meta : public sp_instr
+{
+public:
+
+  uint m_dest;			// Where we will go
+  uint m_cont_dest;             // Where continue handlers will go
+
+  sp_instr_opt_meta(uint ip, sp_pcontext *ctx)
+    : sp_instr(ip, ctx),
+      m_dest(0), m_cont_dest(0), m_optdest(0), m_cont_optdest(0)
+  {}
+
+  sp_instr_opt_meta(uint ip, sp_pcontext *ctx, uint dest)
+    : sp_instr(ip, ctx),
+      m_dest(dest), m_cont_dest(0), m_optdest(0), m_cont_optdest(0)
+  {}
+
+  virtual ~sp_instr_opt_meta()
+  {}
+
+  virtual void set_destination(uint old_dest, uint new_dest)
+    = 0;
+
+protected:
+
+  sp_instr *m_optdest;		// Used during optimization
+  sp_instr *m_cont_optdest;     // Used during optimization
+
+}; // class sp_instr_opt_meta : public sp_instr
+
+class sp_instr_jump : public sp_instr_opt_meta
 {
   sp_instr_jump(const sp_instr_jump &);	/* Prevent use of these */
   void operator=(sp_instr_jump &);
 
 public:
 
-  uint m_dest;			// Where we will go
-
   sp_instr_jump(uint ip, sp_pcontext *ctx)
-    : sp_instr(ip, ctx), m_dest(0), m_optdest(0)
+    : sp_instr_opt_meta(ip, ctx)
   {}
 
   sp_instr_jump(uint ip, sp_pcontext *ctx, uint dest)
-    : sp_instr(ip, ctx), m_dest(dest), m_optdest(0)
+    : sp_instr_opt_meta(ip, ctx, dest)
   {}
 
   virtual ~sp_instr_jump()
@@ -702,11 +737,7 @@
       m_dest= new_dest;
   }
 
-protected:
-
-  sp_instr *m_optdest;		// Used during optimization
-
-}; // class sp_instr_jump : public sp_instr
+}; // class sp_instr_jump : public sp_instr_opt_meta
 
 
 class sp_instr_jump_if_not : public sp_instr_jump
@@ -716,16 +747,14 @@
 
 public:
 
-  uint m_cont_dest;             // Where continue handlers will go
-
   sp_instr_jump_if_not(uint ip, sp_pcontext *ctx, Item *i, LEX *lex)
-    : sp_instr_jump(ip, ctx), m_cont_dest(0), m_expr(i),
-      m_lex_keeper(lex, TRUE), m_cont_optdest(0)
+    : sp_instr_jump(ip, ctx), m_expr(i),
+      m_lex_keeper(lex, TRUE)
   {}
 
   sp_instr_jump_if_not(uint ip, sp_pcontext *ctx, Item *i, uint dest, LEX *lex)
-    : sp_instr_jump(ip, ctx, dest), m_cont_dest(0), m_expr(i),
-      m_lex_keeper(lex, TRUE), m_cont_optdest(0)
+    : sp_instr_jump(ip, ctx, dest), m_expr(i),
+      m_lex_keeper(lex, TRUE)
   {}
 
   virtual ~sp_instr_jump_if_not()
@@ -757,7 +786,6 @@
 
   Item *m_expr;			// The condition
   sp_lex_keeper m_lex_keeper;
-  sp_instr *m_cont_optdest;     // Used during optimization
 
 }; // class sp_instr_jump_if_not : public sp_instr_jump
 
@@ -899,7 +927,7 @@
 
   uint m_frame;
 
-}; // class sp_instr_hreturn : public sp_instr
+}; // class sp_instr_hreturn : public sp_instr_jump
 
 
 /* This is DECLARE CURSOR */
@@ -1085,14 +1113,18 @@
 }; // class sp_instr_error : public sp_instr
 
 
-class sp_instr_set_case_expr :public sp_instr
+class sp_instr_set_case_expr : public sp_instr_opt_meta
 {
 public:
 
   sp_instr_set_case_expr(uint ip, sp_pcontext *ctx, uint case_expr_id,
                          Item *case_expr, LEX *lex)
-    :sp_instr(ip, ctx), m_case_expr_id(case_expr_id), m_case_expr(case_expr),
-     m_lex_keeper(lex, TRUE)
+    : sp_instr_opt_meta(ip, ctx),
+      m_case_expr_id(case_expr_id), m_case_expr(case_expr),
+      m_lex_keeper(lex, TRUE)
+  {}
+
+  virtual ~sp_instr_set_case_expr()
   {}
 
   virtual int execute(THD *thd, uint *nextp);
@@ -1101,13 +1133,23 @@
 
   virtual void print(String *str);
 
+  virtual uint opt_mark(sp_head *sp);
+
+  virtual void opt_move(uint dst, List<sp_instr> *ibp);
+
+  virtual void set_destination(uint old_dest, uint new_dest)
+  {
+    if (m_cont_dest == old_dest)
+      m_cont_dest= new_dest;
+  }
+
 private:
 
   uint m_case_expr_id;
   Item *m_case_expr;
   sp_lex_keeper m_lex_keeper;
 
-}; // class sp_instr_set_case_expr : public sp_instr
+}; // class sp_instr_set_case_expr : public sp_instr_opt_meta
 
 
 #ifndef NO_EMBEDDED_ACCESS_CHECKS
Thread
bk commit into 5.0 tree (pem:1.2009) BUG#16568pem26 Jan