Skip to content

Add profiler ELT test - #39550

Merged
davmason merged 4 commits into
dotnet:masterfrom
davmason:elt_tests
Jul 29, 2020
Merged

Add profiler ELT test#39550
davmason merged 4 commits into
dotnet:masterfrom
davmason:elt_tests

Conversation

@davmason

@davmasondavmason commented Jul 17, 2020

Copy link
Copy Markdown
Contributor

As mentioned in #39335 we currently have no ELT hook coverage in the runtime repo.

cc @AndyAyersMS

@davmason
davmason requested review from a team and noahfalkJuly 17, 2020 21:29
@davmasondavmason self-assigned this Jul 17, 2020
@ghost

ghost commented Jul 17, 2020

Copy link
Copy Markdown

Tagging subscribers to this area: @tommcdon
Notify danmosemsft if you want to be subscribed.

@AndyAyersMS

AndyAyersMS commented Jul 17, 2020

Copy link
Copy Markdown
Member

Thanks! This will be interesting when run conjunction with gc stress. Should happen in our overnight testing, once this is merged.

@davmason
davmasonforce-pushed the elt_tests branch 2 times, most recently from 2c192c2 to d005b1cCompareJuly 18, 2020 22:23
@davmason

davmason commented Jul 18, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

There are some bugs in the runtime's slow path ELT assembly stubs on linux. Not surprising given that we haven't ever been testing them. I just pushed some changes to add more diagnostics, I am going to go through and root cause them.

@davmason
davmasonforce-pushed the elt_tests branch 2 times, most recently from d874005 to bdd8471CompareJuly 19, 2020 20:38
This was referenced Jul 20, 2020
Closed
Closed
@davmasondavmason reopened this Jul 22, 2020
@davmason
davmasonforce-pushed the elt_tests branch 2 times, most recently from a83e7b1 to 4714690CompareJuly 23, 2020 11:04
@davmason

davmason commented Jul 23, 2020

Copy link
Copy Markdown
ContributorAuthor

@noahfalk the remaining failures are an infrastructure issue, this is ready to review now

@AndyAyersMS do you or anyone else from the jit team want to take a look at the assembly stub changes I made?

I found the following issues:

  • On amd64 linux we didn't save and restore the xmm registers, and didn't handle enregistered 16 bytes structs as return values
  • On arm we didn't save and restore the floating point registers (I made the linux arm helpers match the windows arm helpers)
  • On arm64 we didn't handle 16 byte enregistered structs as return values

If there are other interesting corner cases to test, please let me know.

@AndyAyersMS

AndyAyersMS commented Jul 23, 2020

Copy link
Copy Markdown
Member

cc @dotnet/jit-contrib

@davmason

davmason commented Jul 23, 2020

Copy link
Copy Markdown
ContributorAuthor

@jashook this is the PR we talked about in standup, didn't realize you weren't part of the dotnet-diag group

@noahfalk

noahfalk commented Jul 28, 2020

Copy link
Copy Markdown
Member

@jashook this is the PR we talked about in standup, didn't realize you weren't part of the dotnet-diag group

That is fixed now right or is there still an issue?

noahfalk
noahfalk approved these changes Jul 28, 2020
View reviewed changes

@noahfalknoahfalk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, though mostly I am relying on your test cases rather than my ability spot suspicious assembly code : )

As a broader issue if we've had these long standing bugs and nobody is complaining we should keep an eye out for feedback whether these APIs matter to the profiler authors. Its not clear to me that they do.

Comment threadsrc/coreclr/src/vm/arm/profiler.cpp

// add the prespill register(r0-r3) size to get the stack pointer of previous function
_ASSERTE(pData->profiledSp == (void*)(ctx.Sp - 4*4));
_ASSERTE(pData->profiledSp == (void*)(ctx.Sp - 4*4) || pData->profiledSp == (void*)(ctx.Sp - 6*4));

@noahfalknoahfalkJul 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The comment above the assert doesn't lead me to expect the 6 dword case, update the comment?

@davmason

davmason commented Jul 29, 2020

Copy link
Copy Markdown
ContributorAuthor

The CI legs marked as in progress are actually complete, going to merge.

@davmason
davmason merged commit f0ede2b into dotnet:masterJul 29, 2020
Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
@davmason@Jacksondr5
Add profiler ELT test (dotnet#39550)
2712076
Fix the following issues:
On amd64 linux we didn't save and restore the xmm registers, and didn't handle enregistered 16 bytes structs as return values
On arm we didn't save and restore the floating point registers (I made the linux arm helpers match the windows arm helpers)
On arm64 we didn't handle 16 byte enregistered structs as return values
And add tests
@karelzkarelz added this to the 5.0.0 milestone Aug 18, 2020
@davmasondavmason mentioned this pull request Nov 13, 2020
Closed
@ghostghost locked as resolved and limited conversation to collaborators Dec 8, 2020
@davmason
davmason deleted the elt_tests branch January 20, 2021 08:59
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@noahfalknoahfalknoahfalk approved these changes

Assignees

@davmasondavmason

Labels

area-Diagnostics-coreclr

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@davmason@AndyAyersMS@noahfalk@karelz