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

Validate hardcoded offsets to PROFILE_PLATFORM_SPECIFIC_DATA struct #91595

Merged

Conversation

tomeksowi
Copy link
Contributor

These offsets are now in asmconstants.h validated against the C struct with static asserts.

This refactoring is a follow-up to discussion under PR #91313.

Note: I didn't define offset constants for i386 because its asm profile stubs are based on a sequence of pushes rather than field offsets so they wouldn't use them.

Part of #84834
cc @wscho77 @HJLeee @JongHeonChoi @t-mustafin @alpencolt @gbalykov @clamp03

These offsets are now in asmconstants.h validated against the C struct with static asserts.
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 5, 2023
@tomeksowi tomeksowi marked this pull request as ready for review September 6, 2023 06:17
@tomeksowi
Copy link
Contributor Author

@davmason Can you have a look please?
+ @jkotas @shushanhf

@shushanhf
Copy link
Contributor

@davmason Can you have a look please? + @jkotas @shushanhf

Thanks
I will upload the LA's patch.

@shushanhf
Copy link
Contributor

shushanhf commented Sep 6, 2023

@davmason Can you have a look please? + @jkotas @shushanhf

Thanks I will upload the LA's patch.

I think I need some time to refactor some function to fix some errors for LoongArch. Maybe tomorrow is ok.

Or you just merge this PR liking your headline said this is just a refacting struct _PROFILE_PLATFORM_SPECIFIC_DATA.

I will fix some errors within the profiler.cpp and the struct fields later by a new PR.
Is this OK?

@tomeksowi
Copy link
Contributor Author

I think I need some time to refactor some function to fix some errors for LoongArch. Maybe tomorrow is ok.

Or you just merge this PR liking your headline said this is just a refacting struct _PROFILE_PLATFORM_SPECIFIC_DATA.

I will fix some errors within the profiler.cpp and the struct fields later by a new PR. Is this OK?

It's OK with me to merge this as-is and fix LoongArch specifics later.

BTW, from what I noticed ELT profiler on LoongArch64 isn't called at all, genProfiling(Enter|Leave)Callback are a no-op. Given the arch similarity, I think you can base your implementation off of mine in PR #91313.

@tomeksowi
Copy link
Contributor Author

tomeksowi commented Sep 6, 2023

cc @yurai007 @sirntar

@ghost
Copy link

ghost commented Sep 6, 2023

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

Issue Details

These offsets are now in asmconstants.h validated against the C struct with static asserts.

This refactoring is a follow-up to discussion under PR #91313.

Note: I didn't define offset constants for i386 because its asm profile stubs are based on a sequence of pushes rather than field offsets so they wouldn't use them.

Part of #84834
cc @wscho77 @HJLeee @JongHeonChoi @t-mustafin @alpencolt @gbalykov @clamp03

Author: tomeksowi
Assignees: tomeksowi
Labels:

area-Diagnostics-coreclr, area-VM-coreclr, community-contribution

Milestone: -

@jkotas jkotas requested a review from davmason September 6, 2023 18:54
@jkotas jkotas merged commit 7f191ad into dotnet:main Sep 7, 2023
105 of 109 checks passed
@jkotas
Copy link
Member

jkotas commented Sep 7, 2023

Thank you!

@ghost ghost locked as resolved and limited conversation to collaborators Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Diagnostics-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants