search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.96k stars 4.65k forks source link

Different behaviour of default interface impelmentation in monoVM vs CoreCLR #81882

Closed michaldobrodenka closed 1 year ago

michaldobrodenka commented 1 year ago

Description

In net7 when using monoVM different method is called.

Reproduction Steps

This small example:

    interface IDefault
    {
        public void Method1()
        {
            Console.WriteLine("Interface Method1");
        }

        public void Method2()
        {
            Console.WriteLine("Interface Method2");
        }
    }

    abstract class ClassA : IDefault
    {
        virtual public void Method1()
        {
            Console.WriteLine("ClassA Method 1");
        }
    }

    class ClassB : ClassA
    {
        virtual public void Method2()
        {
            Console.WriteLine("ClassB Method2");
        }
    }

    internal class Program
    {
        static void Main(string[] args)
        {
            IDefault c = new ClassB();

            c.Method1();
            c.Method2();
        }
    }

Expected behavior

the same methods to be called

Actual behavior

For CoreCLR outputs: ClassA Method 1 ClassB Method2

When using monoVM: ClassA Method 1 Interface Method2

Regression?

no

Known Workarounds

not to use default interface implementatinos

Configuration

Tested on:

net7/linux/armv6 (only supports monoVM so this is where I found it) net7/win/x64 when publishing app with dotnet publish --self-contained true -p:UseMonoRuntime=true net7-android

Other information

No response

dotnet-issue-labeler[bot] commented 1 year ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

michaldobrodenka commented 1 year ago

smallest example csproj

ConsoleApp4(1).zip

michaldobrodenka commented 1 year ago

Only when ClassA is abstract!

janvorli commented 1 year ago

cc: @trylek - maybe that's what you've fixed recently?

akoeplinger commented 1 year ago

/cc @lambdageek @vargaz

lambdageek commented 1 year ago
vtable debug log 
*** Vtable for class 'ClassB' at "AFTER INHERITING PARENT VTABLE" (size 7)
Packed interface table for class ClassB has size 1
  [000][UUID 622][SLOT 004][SIZE  002] interface IDefault
Interface flags: (0,F)(1,F)(2,F)(3,F)(4,F)(5,F)(6,F)(7,F)(8,F)(9,F)(10,F)(11,F)(12,F)(13,F)(14,F)(15,F)(16,F)(17,F)(18,F)(19,F)(20,F)(21,F)(22,F)(23,F)(24,F)(25,F)(26,F)(27,F)(28,F)(29,F)(30,F)(31,F)(32,F)(33,F)(34,F)(35,F)(36,F)(37,F)(38,F)(39,F)(40,F)(41,F)(42,F)(43,F)(44,F)(45,F)(46,F)(47,F)(48,F)(49,F)(50,F)(51,F)(52,F)(53,F)(54,F)(55,F)(56,F)(57,F)(58,F)(59,F)(60,F)(61,F)(62,F)(63,F)(64,F)(65,F)(66,F)(67,F)(68,F)(69,F)(70,F)(71,F)(72,F)(73,F)(74,F)(75,F)(76,F)(77,F)(78,F)(79,F)(80,F)(81,F)(82,F)(83,F)(84,F)(85,F)(86,F)(87,F)(88,F)(89,F)(90,F)(91,F)(92,F)(93,F)(94,F)(95,F)(96,F)(97,F)(98,F)(99,F)(100,F)(101,F)(102,F)(103,F)(104,F)(105,F)(106,F)(107,F)(108,F)(109,F)(110,F)(111,F)(112,F)(113,F)(114,F)(115,F)(116,F)(117,F)(118,F)(119,F)(120,F)(121,F)(122,F)(123,F)(124,F)(125,F)(126,F)(127,F)(128,F)(129,F)(130,F)(131,F)(132,F)(133,F)(134,F)(135,F)(136,F)(137,F)(138,F)(139,F)(140,F)(141,F)(142,F)(143,F)(144,F)(145,F)(146,F)(147,F)(148,F)(149,F)(150,F)(151,F)(152,F)(153,F)(154,F)(155,F)(156,F)(157,F)(158,F)(159,F)(160,F)(161,F)(162,F)(163,F)(164,F)(165,F)(166,F)(167,F)(168,F)(169,F)(170,F)(171,F)(172,F)(173,F)(174,F)(175,F)(176,F)(177,F)(178,F)(179,F)(180,F)(181,F)(182,F)(183,F)(184,F)(185,F)(186,F)(187,F)(188,F)(189,F)(190,F)(191,F)(192,F)(193,F)(194,F)(195,F)(196,F)(197,F)(198,F)(199,F)(200,F)(201,F)(202,F)(203,F)(204,F)(205,F)(206,F)(207,F)(208,F)(209,F)(210,F)(211,F)(212,F)(213,F)(214,F)(215,F)(216,F)(217,F)(218,F)(219,F)(220,F)(221,F)(222,F)(223,F)(224,F)(225,F)(226,F)(227,F)(228,F)(229,F)(230,F)(231,F)(232,F)(233,F)(234,F)(235,F)(236,F)(237,F)(238,F)(239,F)(240,F)(241,F)(242,F)(243,F)(244,F)(245,F)(246,F)(247,F)(248,F)(249,F)(250,F)(251,F)(252,F)(253,F)(254,F)(255,F)(256,F)(257,F)(258,F)(259,F)(260,F)(261,F)(262,F)(263,F)(264,F)(265,F)(266,F)(267,F)(268,F)(269,F)(270,F)(271,F)(272,F)(273,F)(274,F)(275,F)(276,F)(277,F)(278,F)(279,F)(280,F)(281,F)(282,F)(283,F)(284,F)(285,F)(286,F)(287,F)(288,F)(289,F)(290,F)(291,F)(292,F)(293,F)(294,F)(295,F)(296,F)(297,F)(298,F)(299,F)(300,F)(301,F)(302,F)(303,F)(304,F)(305,F)(306,F)(307,F)(308,F)(309,F)(310,F)(311,F)(312,F)(313,F)(314,F)(315,F)(316,F)(317,F)(318,F)(319,F)(320,F)(321,F)(322,F)(323,F)(324,F)(325,F)(326,F)(327,F)(328,F)(329,F)(330,F)(331,F)(332,F)(333,F)(334,F)(335,F)(336,F)(337,F)(338,F)(339,F)(340,F)(341,F)(342,F)(343,F)(344,F)(345,F)(346,F)(347,F)(348,F)(349,F)(350,F)(351,F)(352,F)(353,F)(354,F)(355,F)(356,F)(357,F)(358,F)(359,F)(360,F)(361,F)(362,F)(363,F)(364,F)(365,F)(366,F)(367,F)(368,F)(369,F)(370,F)(371,F)(372,F)(373,F)(374,F)(375,F)(376,F)(377,F)(378,F)(379,F)(380,F)(381,F)(382,F)(383,F)(384,F)(385,F)(386,F)(387,F)(388,F)(389,F)(390,F)(391,F)(392,F)(393,F)(394,F)(395,F)(396,F)(397,F)(398,F)(399,F)(400,F)(401,F)(402,F)(403,F)(404,F)(405,F)(406,F)(407,F)(408,F)(409,F)(410,F)(411,F)(412,F)(413,F)(414,F)(415,F)(416,F)(417,F)(418,F)(419,F)(420,F)(421,F)(422,F)(423,F)(424,F)(425,F)(426,F)(427,F)(428,F)(429,F)(430,F)(431,F)(432,F)(433,F)(434,F)(435,F)(436,F)(437,F)(438,F)(439,F)(440,F)(441,F)(442,F)(443,F)(444,F)(445,F)(446,F)(447,F)(448,F)(449,F)(450,F)(451,F)(452,F)(453,F)(454,F)(455,F)(456,F)(457,F)(458,F)(459,F)(460,F)(461,F)(462,F)(463,F)(464,F)(465,F)(466,F)(467,F)(468,F)(469,F)(470,F)(471,F)(472,F)(473,F)(474,F)(475,F)(476,F)(477,F)(478,F)(479,F)(480,F)(481,F)(482,F)(483,F)(484,F)(485,F)(486,F)(487,F)(488,F)(489,F)(490,F)(491,F)(492,F)(493,F)(494,F)(495,F)(496,F)(497,F)(498,F)(499,F)(500,F)(501,F)(502,F)(503,F)(504,F)(505,F)(506,F)(507,F)(508,F)(509,F)(510,F)(511,F)(512,F)(513,F)(514,F)(515,F)(516,F)(517,F)(518,F)(519,F)(520,F)(521,F)(522,F)(523,F)(524,F)(525,F)(526,F)(527,F)(528,F)(529,F)(530,F)(531,F)(532,F)(533,F)(534,F)(535,F)(536,F)(537,F)(538,F)(539,F)(540,F)(541,F)(542,F)(543,F)(544,F)(545,F)(546,F)(547,F)(548,F)(549,F)(550,F)(551,F)(552,F)(553,F)(554,F)(555,F)(556,F)(557,F)(558,F)(559,F)(560,F)(561,F)(562,F)(563,F)(564,F)(565,F)(566,F)(567,F)(568,F)(569,F)(570,F)(571,F)(572,F)(573,F)(574,F)(575,F)(576,F)(577,F)(578,F)(579,F)(580,F)(581,F)(582,F)(583,F)(584,F)(585,F)(586,F)(587,F)(588,F)(589,F)(590,F)(591,F)(592,F)(593,F)(594,F)(595,F)(596,F)(597,F)(598,F)(599,F)(600,F)(601,F)(602,F)(603,F)(604,F)(605,F)(606,F)(607,F)(608,F)(609,F)(610,F)(611,F)(612,F)(613,F)(614,F)(615,F)(616,F)(617,F)(618,F)(619,F)(620,F)(621,F)(622,T)
Dump interface flags: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 40
[LEVEL 0] Implemented interfaces by class ClassB:
[LEVEL 1] Implemented interfaces by class ClassA:
  [UIID 622] interface IDefault
  [000][UUID 622][SLOT 004][SIZE  002] interface .IDefault
[LEVEL 2] Implemented interfaces by class Object:
* Interfaces for class 'ClassB' done.
Starting vtable (size 7):
  [O][000][INDEX 000] object:GetHashCode () [0x12701ac30]
  [O][001][INDEX 001] object:Equals (object) [0x12701ac08]
  [O][002][INDEX 002] object:ToString () [0x12701abe0]
  [O][003][INDEX 003] object:Finalize () [0x12701abb8]
  [O][004][INDEX 006] ClassA:Method1 () [0x1370678b8]
  [O][005][INDEX 001] IDefault:Method2 () [0x137067890]
  [O][006][INDEX 006] ClassA:Method1 () [0x1370678b8]
Override map "AFTER OVERRIDING INTERFACE METHODS" EMPTY.
*** Vtable for class 'ClassB' at "AFTER OVERRIDING INTERFACE METHODS" (size 7)
  [O][000][INDEX 000] object:GetHashCode () [0x12701ac30]
  [O][001][INDEX 001] object:Equals (object) [0x12701ac08]
  [O][002][INDEX 002] object:ToString () [0x12701abe0]
  [O][003][INDEX 003] object:Finalize () [0x12701abb8]
  [O][004][INDEX 006] ClassA:Method1 () [0x1370678b8]
  [O][005][INDEX 001] IDefault:Method2 () [0x137067890]
  [O][006][INDEX 006] ClassA:Method1 () [0x1370678b8]
    checking iface method IDefault:Method1 ()
    For slot 4 (''.'IDefault':'Method1'), trying method ''.'ClassB':'Method2'... [EXPLICIT IMPLEMENTATION = 0][SLOT IS NULL = 0][RANK CHECK FAILED]
    checking iface method IDefault:Method2 ()
    For slot 5 (''.'IDefault':'Method2'), trying method ''.'ClassB':'Method2'... [EXPLICIT IMPLEMENTATION = 0][SLOT IS NULL = 0][NOT EXPLICIT IMPLEMENTATION IN FULL SLOT REFUSED]
      [class is not abstract, checking slot 4 for interface ''.'IDefault', method Method1, slot check is 0]
      [class is not abstract, checking slot 5 for interface ''.'IDefault', method Method2, slot check is 0]
*** Vtable for class 'ClassB' at "AFTER SETTING UP INTERFACE METHODS" (size 7)
  [O][000][INDEX 000] object:GetHashCode () [0x12701ac30]
  [O][001][INDEX 001] object:Equals (object) [0x12701ac08]
  [O][002][INDEX 002] object:ToString () [0x12701abe0]
  [O][003][INDEX 003] object:Finalize () [0x12701abb8]
  [O][004][INDEX 006] ClassA:Method1 () [0x1370678b8]
  [O][005][INDEX 001] IDefault:Method2 () [0x137067890]
  [O][006][INDEX 006] ClassA:Method1 () [0x1370678b8]
*** Vtable for class 'ClassB' at "AFTER OVERRIDING NON-INTERFACE METHODS" (size 8)
  [O][000][INDEX 000] object:GetHashCode () [0x12701ac30]
  [O][001][INDEX 001] object:Equals (object) [0x12701ac08]
  [O][002][INDEX 002] object:ToString () [0x12701abe0]
  [O][003][INDEX 003] object:Finalize () [0x12701abb8]
  [O][004][INDEX 006] ClassA:Method1 () [0x1370678b8]
  [O][005][INDEX 001] IDefault:Method2 () [0x137067890]
  [O][006][INDEX 006] ClassA:Method1 () [0x1370678b8]
  [N][007][INDEX 007] ClassB:Method2 () [0x1370678e0]
*** Vtable for class 'ClassB' at "FINALLY" (size 8)
  [O][000][INDEX 000] object:GetHashCode () [0x12701ac30]
  [O][001][INDEX 001] object:Equals (object) [0x12701ac08]
  [O][002][INDEX 002] object:ToString () [0x12701abe0]
  [O][003][INDEX 003] object:Finalize () [0x12701abb8]
  [O][004][INDEX 006] ClassA:Method1 () [0x1370678b8]
  [O][005][INDEX 001] IDefault:Method2 () [0x137067890]
  [O][006][INDEX 006] ClassA:Method1 () [0x1370678b8]
  [N][007][INDEX 007] ClassB:Method2 () [0x1370678e0]

The relevant bit is:

    For slot 5 (''.'IDefault':'Method2'), trying method ''.'ClassB':'Method2'... [EXPLICIT IMPLEMENTATION = 0][SLOT IS NULL = 0][NOT EXPLICIT IMPLEMENTATION IN FULL SLOT REFUSED]

So we didn't like ClassB.Method2 because IDefault is inherited from ClassA, not explicitly implemented by ClassB.

ClassB.Method2 is always .method public hidebysig newslot virtual instance. So when ClassA is not abstract, or when DIMs are not in the picture at all, this makes sense to me... we have a newslot method that happens to have the same name as something that our parent already implemented. We can ignore it.

But I guess if the parent (ancestor?) is abstract then we do want to override the DIM? I think I need to read the ecma augments...

trylek commented 1 year ago

@janvorli - I don't think I've been fixing anything in this area, please note this issue deals with instance, not static virtual methods, I've only been making changes to the static path. One way or another, the reabstraction fix you reviewed about a week or two ago still hasn't been merged in because it fails in several Mono legs, I'm working with the Mono team on figuring out the fix.

FWIW, I'm somewhat surprised by the fact that the method overrides in ClassA and ClassB are marked as virtual, not as override as I'd expect, but perhaps that's just my incomplete knowledge of the C# syntax.

lambdageek commented 1 year ago

I think the issue is we only want to fill the vtable slot if the current class is not abstract. That is, classA's Method2 slot should be empty, and class B should fill it in with the DIM method as the last step.

https://github.com/dotnet/runtime/blob/d067431ed81ab4a2068863474ab9a284cb9ccf93/src/mono/mono/metadata/class-setup-vtable.c#L1920-L1924

PR shortly, after I validate locally that this doesn't break some other cases, to avoid public embarrassment.

lambdageek commented 1 year ago

Fixed in https://github.com/dotnet/runtime/pull/82556