[Mono-bugs] [Bug 319930] Interface re-implementations lost in inheritance
bugzilla_noreply at novell.com
bugzilla_noreply at novell.com
Wed Jan 9 06:38:43 EST 2008
https://bugzilla.novell.com/show_bug.cgi?id=319930
User massi at ximian.com added comment
https://bugzilla.novell.com/show_bug.cgi?id=319930#c1
--- Comment #1 from Massimiliano Mantione <massi at ximian.com> 2008-01-09 04:38:42 MST ---
The test "bug-77127.exe" is failing again since r92011.
Strictly speaking, the bug is not the same, it is a new one triggred by the C#
compiler change and luckily discovered by the same test.
But since it is related to "vtable inheritance", it can be handled here...
Tracking the problem down, we see that the compiler change uses different names
for property methods in the interface and in the class that implements it.
The correct override is then specified in the ".override" section.
In the test, Mono handles it correctly in the B class.
However, when building the C vtable, the ".override" data is not visible
anymore, and since the IA interface ends up in a different interface offset,
its vtable section must be rebuilt. This rebuild does not reuse the correct
method from the B vtable, so the wrong one is taken.
IMHO, the obvious fix is to "inherit" all the interface vtable sections
(copying them from the parent vtable to the new interface offset in the current
vtable) before attempting to rebuild them: the rebuild should just add
overrides.
This would be the only way to get the correct method, because the ".override"
data for class B is simply not visible anymore in the code when we build the
vtable for class C.
In doing so, I begun fighting a bit with the current code.
My main issue is that the vtable building loop iterations are "guarded" by "if
(vtable [io + l]) continue;" statements, and it definitely does not play well
with the case when the method slots are pre-initialized.
The current code has a very convoluted algorithm, like this:
[1] Find the max vtable size, and init interface offsets.
[2] Copy over (inherit) the parent vtable.
[3] Foreach method in the ".override" section, if it is an
interface method put it in the right slot *and* in
the override hash table (only for the current class).
[4] Foreach class "k", starting from the current towards its
superclasses:
[4.1] Foreach interface "ic" implemented by "k" (even if
implemented indirectly, but *not* through class
inheritance, only through interface inheritance):
[4.1.1] If "k" is still the starting class:
- Foreach method "im" in "ic":
- Foreach method "cm" in "k":
If the method names and signatures match,
initialize the right slot.
Else, if "ic" is outside of the "k" vtable,
continue loop [4.1].
[4.1.2] Foreach method "im" in "ic":
[4.1.2.1] If its slot is full, continue [4.1.2].
[4.1.2.2] Foreach class "k1" from "k" through
all its superclasses:
- Foreach method "cm" in "k1":
If the fully qualified names and
signatures of "im" and "cm" match,
initialize the slot.
[4.1.3] Foreach method "im" in "ic":
[4.1.3.1] If its slot is full, continue [4.1.3].
[4.1.3.2] Foreach class "k1" like in [4.1.2.2]:
- Foreach method "cm" in "k1":
If the names-signatures of "im" and
"cm" match, initialize the slot.
[4.1.4] If class is not abstract,
foreach method "im" in "ic":
[4.1.4.1] If its slot is empty,
foreach class "parent" from "class"
through all its superclasses:
[4.1.4.1.1] If its corresponding slot
is full, copy it here.
[4.1.4.1.2] IMHO, reading the code, a
break statement (from loop
[4.1.4.1]) is missing!
After loop [4.1.4.1], if the slot is
still empty, raise the loading error.
[4.1.5] Foreach method "im" in "ic":
If it's slot is -1, fix it (nobody knows why
this is needed, or even why it can happen).
[5] Foreach method "cm" in the current class:
- If it in not "newslot", find its slot to override.
- Else, give it a new slot.
[6] A loop like [3], but for non-interface methods.
[7] Apply the override hash table to the vtable.
[8] Put the vtable in place (or, if it's identical, use the parent).
The bad part, IMO, is section [4].
Perhaps it has been written in such a convoluted way because we did not have an
array like "MonoClass.interfaces_packed", which collects all the implemented
intefaces.
The checks at [4.1.2.1] and [4.1.3.1] are the ones that cause me problems, but
I think that the whole logic should be rewritten like this:
[2] In section [2], besides copying the parent vtable, foreach
interface "ic" implemented by "parent", if it has an interface
offset larger than "parent->vtable_size", copy its vtable
section at the correct interface offset inside the current
vtable (this "inherits" the vtable section).
[3] ...
[4] Foreach interface "ic" implemented by the current class:
[4.1] Foreach method "im" in "ic":
[4.1.1] Foreach method "cm" in the current class:
- If "cm" is a suitable override for "im"
(by signature, fully qualified name, name
or ".override" section), use it (potentially
overwriting the slot written at [2]).
[4.1.2] If the class is not abstract, check that the
"im" slot is full (it must, because of [2]),
otherwise raise the loading error.
The simplicity comes mostly from the fact that in [2] and [4] we would loop on
"MonoClass.interfaces_packed", which the previous code could not do.
And this rewrite, beside making the code much more maintainable, would fix the
bug.
However, this is a tricky and risky change, even if we've got plenty of
regression tests to check it...
Comments?
Am I missing something?
--
Configure bugmail: https://bugzilla.novell.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
More information about the mono-bugs
mailing list