Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mono] Fix StackTrace from a dim and Vtable offsets for static interface method #60770

Merged
merged 5 commits into from
Dec 2, 2021

Conversation

thaystg
Copy link
Member

@thaystg thaystg commented Oct 22, 2021

  • Fix StackTrace when called from a DIM.
  • Fix the other test case that was added for @bholmes, and this case when the method TestMethod5 was being called it was executing TestMethod10, and this was fixed skipping static interface methods when was calculating vtable offsets.

The fix was completely done by @vargaz, I just opened the PR.
Thanks @vargaz .

Fix #60486

@ghost
Copy link

ghost commented Oct 22, 2021

Tagging subscribers to this area:
See info in area-owners.md if you want to be subscribed.

Issue Details

I'm not sure if this is the right fix, but with it I can see the callstack correctly as I can see if I run using CoreCLR.

Output running on mono:

at System.Environment.get_StackTrace()
   at DefItf.ISubscriber`1.InternalSubscribe(ISubscriber`1 subscriber, IPublisher`1 publisher) in /Users/thaysgrazia/runtime/src/mono/sample/HelloWorld/Program.cs:line 27
   at DefItf.Program.Subscribe(IPublisher`1 publisher) in /Users/thaysgrazia/runtime/src/mono/sample/HelloWorld/Program.cs:line 68
   at DefItf.Program.Start() in /Users/thaysgrazia/runtime/src/mono/sample/HelloWorld/Program.cs:line 61
   at DefItf.Program.Main(String[] args) in /Users/thaysgrazia/runtime/src/mono/sample/HelloWorld/Program.cs:line 53
do something
   at System.Environment.get_StackTrace()
   at DefItf.ISubscriber`1.InternalUnsubscribe(ISubscriber`1 subscriber, IPublisher`1 publisher) in /Users/thaysgrazia/runtime/src/mono/sample/HelloWorld/Program.cs:line 33
   at DefItf.ISubscriber`1.Unsubscribe(IPublisher`1 publisher) in /Users/thaysgrazia/runtime/src/mono/sample/HelloWorld/Program.cs:line 21
   at DefItf.Program.Start() in /Users/thaysgrazia/runtime/src/mono/sample/HelloWorld/Program.cs:line 63
   at DefItf.Program.Main(String[] args) in /Users/thaysgrazia/runtime/src/mono/sample/HelloWorld/Program.cs:line 53
Author: thaystg
Assignees: -
Labels:

area-VM-meta-mono

Milestone: -

@lambdageek
Copy link
Member

@vargaz
Copy link
Contributor

vargaz commented Oct 22, 2021

The stacktrace is supposed to contain the exact generic instance not DefItf.ISubscriber`1.

@vargaz
Copy link
Contributor

vargaz commented Oct 22, 2021

@thaystg
Copy link
Member Author

thaystg commented Oct 22, 2021

@vargaz , your fix looks like it works the result running on mono with your fix would be:

   at System.Environment.get_StackTrace()
   at DefItf.ISubscriber`1[[DefItf.InputData, HelloWorld, Version=7.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35]].InternalSubscribe(ISubscriber`1 subscriber, IPublisher`1 publisher) in /Users/thaysgrazia/runtime/src/mono/sample/HelloWorld/Program.cs:line 27
   at DefItf.Program.Subscribe(IPublisher`1 publisher) in /Users/thaysgrazia/runtime/src/mono/sample/HelloWorld/Program.cs:line 68
   at DefItf.Program.Start() in /Users/thaysgrazia/runtime/src/mono/sample/HelloWorld/Program.cs:line 61
   at DefItf.Program.Main(String[] args) in /Users/thaysgrazia/runtime/src/mono/sample/HelloWorld/Program.cs:line 53
do something
   at System.Environment.get_StackTrace()
   at DefItf.ISubscriber`1[[DefItf.InputData, HelloWorld, Version=7.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35]].InternalUnsubscribe(ISubscriber`1 subscriber, IPublisher`1 publisher) in /Users/thaysgrazia/runtime/src/mono/sample/HelloWorld/Program.cs:line 33
   at DefItf.ISubscriber`1.Unsubscribe(IPublisher`1 publisher) in /Users/thaysgrazia/runtime/src/mono/sample/HelloWorld/Program.cs:line 21
   at DefItf.Program.Start() in /Users/thaysgrazia/runtime/src/mono/sample/HelloWorld/Program.cs:line 63
   at DefItf.Program.Main(String[] args) in /Users/thaysgrazia/runtime/src/mono/sample/HelloWorld/Program.cs:line 53

But it's different of what I get when I run on CoreCLR where I have:
image

@vargaz
Copy link
Contributor

vargaz commented Oct 24, 2021

Its not defined what the result should be, we try to obtain the exact generic instance if possible.

@bholmes
Copy link
Contributor

bholmes commented Nov 5, 2021

I created a new program with more cases.

https://gist.github.com/bholmes/e3ccaf9d044b7261d6865c8f0173dc64

Method5 appears to be broken but I think this patch is good.

@thaystg thaystg changed the title [mono] Fix mono_get_generic_context_from_stack_frame when it's from a dim. [mono] Fix StackTrace from a dim and Vtable offsets for static interface method Nov 26, 2021
@thaystg
Copy link
Member Author

thaystg commented Nov 26, 2021

/azp run runtime-manual

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Comment on lines +1521 to +1522
if (m_method_is_static (method))
continue;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're going to need to revisit this (and line 1535, below) when non-abstract static virtual methods are going to be supported

@bholmes
Copy link
Contributor

bholmes commented Dec 1, 2021

If this is approved, I want this to go back to mono/mono. Let me know if and when you want me to back port these changes to that repo. (assuming that automation will fail.)

@lambdageek
Copy link
Member

/azp run sync-runtime-to-mono

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lambdageek
Copy link
Member

There are CoreCLR and Mono test failures from the new test.

  1. CoreCLR failures are because the new test methods should all be marked NoInlining
  2. Mono failure is because the validation for
       t1.TestMethod5(null, new [] {new StackFrame ("TestItf1`1", "TestMethod3"), new StackFrame ("TestItf2`1.TestItf1", "TestMethod5")})
    
    is too strict - the actual string in Mono is TestItf2`1[[InputData, github60486, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null]].TestItf1<TT>. So we should make something like new StackFrame (new [] {"TestItf2`1", "TestItf1"}, "TestMethod5") so that the class name can be in multiple parts.

In both cases, it's an issue with the testcase, not with the implementation. (So the mono backport is still ok)

@thaystg thaystg merged commit 5789792 into dotnet:main Dec 2, 2021
agocke added a commit that referenced this pull request Dec 3, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jan 1, 2022
@thaystg
Copy link
Member Author

thaystg commented Feb 17, 2022

/backport to release/6.0

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[mono] Generic Default Interface Method crash with Environment.StackTrace
4 participants