View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0000422LDMud 3.3LPC Compiler/Preprocessorpublic2005-11-30 16:562018-01-29 21:57
ReporterGnomi Assigned Tolars 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Platformi686OSDebian GNU/LinuxOS Version3.1
Product Version3.3.712 
Fixed in Version3.3.714 
Summary0000422: 'virtual inherit' may merge too many functions
DescriptionHi,

Given the following programs:

=== a.c ===
private mapping store = ([]);

mixed change_value(string key, mixed val)
{
    mixed res = store[key];
    store[key] = val;
    return res;
}

mixed get_value(string key)
{
    return store[key];
}

=== b.c ===
private functions inherit "a";

void create()
{
    printf("B: %O\n", change_value("owner","B"));
}

=== c.c ===
private functions inherit "a";

void create()
{
    printf("C: %O\n", change_value("owner", "C"));
}

=== d.c ===
virtual inherit "b";
virtual inherit "c";

void create()
{
    "*"::create();
    "*"::create();
}

loading 'd' results in:

B: 0
C: "B"
B: "C"
C: "B"

That means, 'b' and 'c' are using the same 'a' (with the same mapping), because the driver crossdefines b::a::change_value with c::a::change_value, although 'a' is not inherited virtually. This is, because the driver just checks, that both definitions point to the same functions and that somewhere in the inherit path there is a 'virtual'. But in the above example 'b' and 'c' want to have their own 'a' and don't want to have it merged. (The variable set of 'a' does exist twice in 'd'.)

I have a patch, that changes get_function_id so, that it traverses the inherit path until there is no TYPE_MOD_VIRTUAL flag anymore. This entry should be the least common function entry of both programs and should be equal, if both function definitions should be merged. Otherwise normal function overriding rules should apply. The patch builds upon my patches for bug 0000362. (I don't know whether a similar patch works for plain 3.3.712 because of the obscure overriding handling there.)

Greetings,
Gnomi.
TagsNo tags attached.
Attached Files
prolang.virtinh.diff (2,164 bytes)    
diff -Naur 3.3privinh/src/prolang.y 3.3privvirtinh/src/prolang.y
--- 3.3privinh/src/prolang.y	2005-08-18 15:14:05.000000000 +0200
+++ 3.3privvirtinh/src/prolang.y	2005-11-30 23:21:00.000000000 +0100
@@ -14648,15 +14648,19 @@
 
 /*-------------------------------------------------------------------------*/
 static funflag_t *
-get_function_id (program_t *progp, int fx)
+get_virtual_function_id (program_t *progp, int fx)
 
-/* Return a pointer to the function flags of function <fx> in <progp>.
+/* Return a pointer to the flags of the first entry of function <fx> in <progp>
+ * that was inherited virtual (i.e. the first entry we encounter that doesn't have
+ * TYPE_MOD_VIRTUAL).
+ *
  * This function takes care of resolving cross-definitions and inherits
  * to the real function flag.
  */
 
 {
     funflag_t flags;
+    funflag_t *last;
 
     flags = progp->functions[fx];
 
@@ -14666,9 +14670,12 @@
         fx += CROSSDEF_NAME_OFFSET(flags);
         flags = progp->functions[fx];
     }
+    
+    /* This one is inherited virtual. We wont get called otherwise. */
+    last = &progp->functions[fx];
 
     /* Walk the inherit chain */
-    while(flags & NAME_INHERITED)
+    while((flags & (NAME_INHERITED|TYPE_MOD_VIRTUAL)) == (NAME_INHERITED|TYPE_MOD_VIRTUAL))
     {
         inherit_t *inheritp;
 
@@ -14680,7 +14687,7 @@
 
     /* This is the one */
     return &progp->functions[fx];
-} /* get_function_id() */
+} /* get_virtual_function_id() */
 
 /*-------------------------------------------------------------------------*/
 #ifdef USE_STRUCTS
@@ -14939,8 +14946,8 @@
                         }
                         else if ((fun.flags | type) & TYPE_MOD_VIRTUAL
                               && OldFunction->flags & TYPE_MOD_VIRTUAL
-                          &&    get_function_id(from, i)
-  == get_function_id(INHERIT(OldFunction->offset.inherit).prog
+                          &&    get_virtual_function_id(from, i)
+  == get_virtual_function_id(INHERIT(OldFunction->offset.inherit).prog
                 , n - INHERIT(OldFunction->offset.inherit).function_index_offset
                      )
                                  )
prolang.virtinh.diff (2,164 bytes)   
prolang.virtinh-2.diff (9,381 bytes)    
Index: 3.3virtinh/src/prolang.y
===================================================================
--- 3.3virtinh/src/prolang.y	(Revision 2253)
+++ 3.3virtinh/src/prolang.y	(Arbeitskopie)
@@ -14647,15 +14647,19 @@
 
 /*-------------------------------------------------------------------------*/
 static funflag_t *
-get_function_id (program_t *progp, int fx)
+get_virtual_function_id (program_t *progp, int fx)
 
-/* Return a pointer to the function flags of function <fx> in <progp>.
+/* Return a pointer to the flags of the first entry of function <fx> in <progp>
+ * that is inherited virtual (i.e. the first entry we encounter that doesn't have
+ * TYPE_MOD_VIRTUAL).
+ *
  * This function takes care of resolving cross-definitions and inherits
  * to the real function flag.
  */
 
 {
     funflag_t flags;
+    funflag_t *last;
 
     flags = progp->functions[fx];
 
@@ -14665,9 +14669,12 @@
         fx += CROSSDEF_NAME_OFFSET(flags);
         flags = progp->functions[fx];
     }
+    
+    /* This one is inherited virtual. We wont get called otherwise. */
+    last = &progp->functions[fx];
 
     /* Walk the inherit chain */
-    while(flags & NAME_INHERITED)
+    while((flags & (NAME_INHERITED|TYPE_MOD_VIRTUAL)) == (NAME_INHERITED|TYPE_MOD_VIRTUAL))
     {
         inherit_t *inheritp;
 
@@ -14679,7 +14686,7 @@
 
     /* This is the one */
     return &progp->functions[fx];
-} /* get_function_id() */
+} /* get_virtual_function_id() */
 
 /*-------------------------------------------------------------------------*/
 #ifdef USE_STRUCTS
@@ -14936,25 +14943,6 @@
                                         , current_func_index - n );
                             p->u.global.function = current_func_index;
                         }
-                        else if ((fun.flags | type) & TYPE_MOD_VIRTUAL
-                              && OldFunction->flags & TYPE_MOD_VIRTUAL
-                          &&    get_function_id(from, i)
-  == get_function_id(INHERIT(OldFunction->offset.inherit).prog
-                , n - INHERIT(OldFunction->offset.inherit).function_index_offset
-                     )
-                                 )
-                        {
-                            /* Entries denote the same function. We have to use
-                             * cross_define nonetheless, to get consistant
-                             * redefinition (and we prefer the first one)
-                             */
-                            OldFunction->flags |= fun.flags &
-                                (TYPE_MOD_PUBLIC|TYPE_MOD_NO_MASK);
-                            OldFunction->flags &= fun.flags |
-				~(TYPE_MOD_STATIC|TYPE_MOD_PRIVATE|TYPE_MOD_PROTECTED|NAME_HIDDEN);
-                            cross_define( OldFunction, &fun
-                                        , n - current_func_index );
-                        }
                         else if ( (fun.flags & (TYPE_MOD_PRIVATE|NAME_HIDDEN|NAME_UNDEFINED))
                                     == (TYPE_MOD_PRIVATE|NAME_HIDDEN) )
                         {
@@ -14974,6 +14962,55 @@
 
                             p->u.global.function = current_func_index;
                         }
+                        else if ((fun.flags | type) & TYPE_MOD_VIRTUAL
+                              && OldFunction->flags & TYPE_MOD_VIRTUAL
+                          &&    get_virtual_function_id(from, i)
+  == get_virtual_function_id(INHERIT(OldFunction->offset.inherit).prog
+                , n - INHERIT(OldFunction->offset.inherit).function_index_offset
+                     )
+                                 )
+                        {
+                            /* Entries denote the same function and both
+                             * entries are visible. We have to use
+                             * cross_define nonetheless, to get consistant
+                             * redefinition (and to avoid the nomask
+                             * checking that comes next), and we prefer
+                             * the first one.
+                             *
+                             * It is important, that both entries are
+                             * indeed visible, because otherwise invisible
+                             * (i.e. private) functions would be made
+                             * visible again by another visible occurrence
+                             * of the same function. The originally invisible
+                             * occurrence would then be subject to
+                             * redefinition and nomask checking.
+                             */
+                            OldFunction->flags |= fun.flags &
+                                (TYPE_MOD_PUBLIC|TYPE_MOD_NO_MASK);
+                            OldFunction->flags &= fun.flags |
+				~(TYPE_MOD_STATIC|TYPE_MOD_PRIVATE|TYPE_MOD_PROTECTED|NAME_HIDDEN);
+                            cross_define( OldFunction, &fun
+                                        , n - current_func_index );
+                        }
+                        else if ((fun.flags | type) & TYPE_MOD_VIRTUAL
+                              && OldFunction->flags & TYPE_MOD_VIRTUAL
+                          &&    get_virtual_function_id(from, i)
+  == get_virtual_function_id(INHERIT(OldFunction->offset.inherit).prog
+                , n - INHERIT(OldFunction->offset.inherit).function_index_offset
+                     )
+                                 )
+                        {
+                            /* Entries denote the same function. We have to use
+                             * cross_define nonetheless, to get consistant
+                             * redefinition (and we prefer the first one)
+                             */
+                            OldFunction->flags |= fun.flags &
+                                (TYPE_MOD_PUBLIC|TYPE_MOD_NO_MASK);
+                            OldFunction->flags &= fun.flags |
+				~(TYPE_MOD_STATIC|TYPE_MOD_PRIVATE|TYPE_MOD_PROTECTED|NAME_HIDDEN);
+                            cross_define( OldFunction, &fun
+                                        , n - current_func_index );
+                        }
                         else if ( (fun.flags & OldFunction->flags & TYPE_MOD_NO_MASK)
                              &&  !( (fun.flags|OldFunction->flags) & NAME_UNDEFINED ) )
                         {
@@ -15239,7 +15276,7 @@
                 inherit_t inherit, *inheritp2;
                 int k, inherit_index;
                 funflag_t *flagp;
-                function_t *funp, *funp2;
+                function_t *funp;
 
                 if (variables_initialized)
                     yyerror("illegal to inherit virtually after "
@@ -15280,8 +15317,8 @@
                 else
                     inherit.inherit_type |= INHERIT_TYPE_DUPLICATE;
 
-                inherit_index = (mem_block[A_INHERITS].current_size - j) /
-                   sizeof(inherit_t) - 1;
+                inherit_index = (mem_block[A_INHERITS].current_size) /
+                   sizeof(inherit_t);
                 inherit.function_index_offset += fun_index_offset;
                 add_to_mem_block(A_INHERITS, (char *)&inherit, sizeof inherit);
                   /* If a function is directly inherited from a program that
@@ -15293,36 +15330,20 @@
                    */
 
                 /* Update the offset.inherit in all these functions to point
-                 * to the first (virtual) inherit of the program.
+                 * to the new inherit_t structure. (But only, if it wasn't
+                 * already cross-defined to something else.)
                  */
                 flagp = from->functions + inheritp->function_index_offset;
                 funp = (function_t *)mem_block[A_FUNCTIONS].block +
                     inherit.function_index_offset;
-                funp2 = (function_t *)mem_block[A_FUNCTIONS].block +
-                    inheritp2->function_index_offset;
-                    /* Usually funp2 == funp, but if the program is inherited
-                     * virtually several times with differing visibilities,
-                     * the two pointers differ.
-                     */
-                for (k = inherit.prog->num_functions; --k >= 0; funp++, funp2++)
+                for (k = inherit.prog->num_functions; --k >= 0; funp++)
                 {
                     if ( !(funp->flags & NAME_CROSS_DEFINED)
-                     &&  !(funp2->flags & NAME_CROSS_DEFINED)
                      && (*flagp & (NAME_INHERITED|NAME_CROSS_DEFINED)) ==
                            NAME_INHERITED
                      && (*flagp & INHERIT_MASK) == inheritc )
                     {
                         funp->offset.inherit = inherit_index;
-
-                        if(funp != funp2)
-                        {
-                            /* I don't think, that this is a wise idea.
-                               Because the function index offset of this
-                               inherit is not correct for this function.
-                            */
-                            yywarnf("Adjusting inherit index because of virtual inherit %.*s!\n",
-                                    (int) from->name->size, from->name->txt);
-                        }
                     }
                     flagp++;
                 }
prolang.virtinh-2.diff (9,381 bytes)   

Activities

Gnomi

2005-12-15 17:30

manager   ~0000460

Don't apply this patch. This has to be solved in another way. (The patch works, but there's a second problem, and its solution will hopefully cover both problems.) I will post the reason and another solution within the next few days.

Gnomi

2005-12-21 04:44

manager   ~0000461

My second problem was a crash caused by the following constellation:

a.c: private void fun() {}
b.c: int x; private void fun() {}
c.c: virtual inherit "b";
d.c: virtual inherit "a";
     virtual inherit "b";
     inherit "c";
e.c: inherit "d";

When copying the functions from d to e there is a segfault, because some function index is not computed right.

The cause of this is the adjustment of the inherit index in copy_variables. (I wrote about this dangerous adjustment in my notes to bug 0000362.) This adjustment was necessary, because both occurrences of b::fun were not cross-defined to each other (i.e. the second one to the first), because they didn't see each other, because they're private.

So I thought about another strategy of cross-defining occurrences of the same virtual function, but I came to the conclusion, that invisible occurrences shouldn't be cross-defined, because of the following scenarios:

Scenario 1: (B inherits A virtual and private, C inherits A virtual, D inherits B and C)
    private virtual B::A::fun()
    public virtual C::A::fun()
    public D::fun()
    
    D::fun overwrites C::A::fun() (so C gets D::fun, when it calls fun()),
    but should not overwrite B::A::fun() (because in B A::fun is
    private and thus should be invisible to outsiders).

Scenario 2:
    nomask private virtual B::A::fun
    public virtual C::A::fun
    
    Cross-Defining both together would make A::fun public nomask
    (and thus making the invisible nomask visible).

But on the other side visible occurrences of the same virtual function should be cross-defined, because they should together be subject to redefinition (and nomask checking). So cross-definitions of virtual functions should be done in copy_functions after the visibility was handled (just before nomask handling). My original patch from this bug should still be applied accordingly (because then cross-defining too many functions may still happen).

My second problem has then to be solved another way. These occurrences of the private virtual functions should be different, when it comes to overriding, but they should use the same variables. So I took a deeper look at copy_variables. It does create a new inherit_t structure just for that purpose. But instead of using it, it seaches for the first occurrence of this virtual inherit and adjusts the inherit index of all functions to this inherit (which then leads to a crash, because the function index offset is just wrong - that's why I considered this adjustment dangerous). So the solution is simple: Take the new inherit_t. Use the old variable index, but calculate an own function index offset (which is done already) and adjust the inherit index of all functions to this inherit.

While I was looking at copy_variables, I couldn't figure out, what the check of funp2 (the other occurrence of the same virtual function) should accomplish. I looked at the old bug reports (b-020530, fixed in 3.2dev439/440/442) and I think, it tried the fix this wrong inherit index adjustment but instead just avoided it. So I took it out and tested the bug again my patch and the patch works.

I uploaded a new patch with all that.

Grettings,
Gnomi

lars

2006-03-14 22:57

reporter   ~0000497

The patch has been used successfully in Unitopia for a while now, so I incorporated it.

Issue History

Date Modified Username Field Change
2005-11-30 16:56 Gnomi New Issue
2005-11-30 16:56 Gnomi File Added: prolang.virtinh.diff
2005-12-15 17:30 Gnomi Note Added: 0000460
2005-12-21 04:37 Gnomi File Added: prolang.virtinh-2.diff
2005-12-21 04:44 Gnomi Note Added: 0000461
2006-03-14 22:57 lars Status new => resolved
2006-03-14 22:57 lars Fixed in Version => 3.3.714
2006-03-14 22:57 lars Resolution open => fixed
2006-03-14 22:57 lars Assigned To => lars
2006-03-14 22:57 lars Note Added: 0000497
2007-10-06 19:55 lars Status resolved => closed
2010-11-16 09:42 lars Source_changeset_attached => ldmud.git master b672d310
2018-01-29 18:59 lars Source_changeset_attached => ldmud.git master b672d310
2018-01-29 21:57 lars Source_changeset_attached => ldmud.git master b672d310