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

Fix published crossgen2 crash #107447

Closed
wants to merge 1 commit into from
Closed

Fix published crossgen2 crash #107447

wants to merge 1 commit into from

Conversation

clamp03
Copy link
Member

@clamp03 clamp03 commented Sep 6, 2024

If test is built as Debug or Checked without LibrariesConfiguration, LibrariesConfiguration is set to default Release configuration.
So when it builds crossgen2, it uses debug c/c++ codes with non-debug libraries. It makes some mismatching and crossgen2 crashes in some tests.
For example, ParentMethodTableOffset in MethodTable has different values in methodtable.h and RuntimeHelpers.CoreCLR.cs because debug mode has debug_m_szClassName field and release mode doesn't have.

cc @dotnet/samsung

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 6, 2024
Copy link
Contributor

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

@clamp03
Copy link
Member Author

clamp03 commented Sep 6, 2024

@jkotas @am11 After #106965, some tests fail when it compiles with crossgen2 in tests on RISC-V. I found reasons and made this PR. However, I do not know about build and I think I did in a wrong way. Could you please help? Thank you.

@clamp03 clamp03 self-assigned this Sep 6, 2024
@clamp03 clamp03 requested review from am11 and jkotas September 6, 2024 07:23
@am11
Copy link
Member

am11 commented Sep 6, 2024

@clamp03, libraries configurations are always default to Release. If you pass LibrariesConfiguraiton from command line it is overridden. That PR did not change that mechanism. Can you share the command which my PR regressed in your workflow? I tested the whole matrix, including riscv64: https://github.com/am11/CrossRepoCITesting/actions/runs/10541101133/workflow#L21.

@clamp03
Copy link
Member Author

clamp03 commented Sep 6, 2024

@clamp03, libraries configurations are always default to Release. If you pass LibrariesConfiguraiton from command line it is overridden. That PR did not change that mechanism. Can you share the command which my PR regressed in your workflow? I tested the whole matrix, including riscv64: https://github.com/am11/CrossRepoCITesting/actions/runs/10541101133/workflow#L21.

Thank you! I checked it doesn't fail if I give LibrariesConfiguation well. However, if I don't give, it builds well and it crashes in tests. I think it is better to make test successfully if it is possible to build without LibraraiesConfiguations. I don't think your PR is wrong. It is just appeared after your PR.

This is how I test (without this PR).

Build

$ ROOTFS_DIR=<PATH TO RISCV64> ./build.sh -cross -arch riscv64 -c Checked -s clr+libs
$ ROOTFS_DIR=<PATH TO RISCV64> ./build.sh -cross -arch riscv64 -c Release -s libs
$ export BuildAllTestsAsStandalone=true
$ ROOTFS_DIR=<PATH TO RISCV64> ./src/tests/build.sh -riscv64 -Checked -priority1 -cross -p:UseLocalAppHostPack=true

Test on RISC-V

$ ./crossgen2
Segmentation fault (core dumped)

@am11
Copy link
Member

am11 commented Sep 6, 2024

Thanks.

$ ./crossgen2
Segmentation fault (core dumped)

Is it the one from Core_Root directory? Before that PR, we were using dotnet crossgen2.dll, PR changed it to use crossgen2(1), so we test what is shipped in crossgen2 nuget package. For non-Release libraries configuration in tests, the supported workflow is pass -p:LibrariesConfiguration from command line: #80154 (comment). So it should work with Release libraries and Checked runtime. If it is not working, that's most likely a product bug?

@clamp03
Copy link
Member Author

clamp03 commented Sep 6, 2024

Is it the one from Core_Root directory? Before that PR, we were using dotnet crossgen2.dll, PR changed it to use crossgen2(1), so we test what is shipped in crossgen2 nuget package.

I used runtime/artifacts/tests/coreclr/linux.riscv64.Checked/Tests/Core_Root/crossgen2/crossgen2 which is built from test build.

For non-Release libraries configuration in tests, the supported workflow is pass -p:LibrariesConfiguration from command line: #80154 (comment). So it should work with Release libraries and Checked runtime. If it is not working, that's most likely a product bug?

I see. You already discussed with @jakobbotsch Thank you so much for the sharing. If we have to pass LibrariesConfiguation for non-Release, it also looks good to me.
Then I just wonder "if we have to pass LibrariesConfiguation for non-Release" and "if it is not passed for non-Release, crossgen2 crashes." then, do we have to set Libraries to Release as a default in test build? And do you know how to change lib configuaration only for crossgen2 in test build?

This is detail about what I investigated.
In Checked runtime, debug_m_szClassesName is included in methodtable.

#ifdef _DEBUG
LPCUTF8 debug_m_szClassName;
#endif //_DEBUG
PTR_MethodTable m_pParentMethodTable;

So offset to m_pParentMethodTable is 0x18 in RISC-V.

In Release lib, ParentMethodTableOffset is 0x10 because lib dosesn't have DEBUG definition and DebugClassNamePtr is 0.

private const int DebugClassNamePtr = // adjust for debug_m_szClassName
#if DEBUG
#if TARGET_64BIT
8
#else
4
#endif
#else
0
#endif
;
private const int ParentMethodTableOffset = 0x10 + DebugClassNamePtr;
#if TARGET_64BIT
private const int AuxiliaryDataOffset = 0x20 + DebugClassNamePtr;
#else
private const int AuxiliaryDataOffset = 0x18 + DebugClassNamePtr;

Thank you!!!

@am11
Copy link
Member

am11 commented Sep 6, 2024

Could you try this patch: main...am11:runtime:patch-11 (delete artifacts/ dir before the rebuild)?

@clamp03
Copy link
Member Author

clamp03 commented Sep 6, 2024

@am11 Thank you. I will try and let you know the result.

@clamp03
Copy link
Member Author

clamp03 commented Sep 6, 2024

I tested with your patch. However, it still fails with the same reason (It crashes at the same machine code location).
I should get off work now. I am so sorry. Have a good weekend.

@clamp03
Copy link
Member Author

clamp03 commented Sep 9, 2024

If it is necessary to give LibrariesConfiguration for non-release test build, I close this PR.
@dotnet/samsung Please give LibrariesConfiguration for non-release test build on your CI as well as your build to prevent crashes.
Thank you.

@clamp03 clamp03 closed this Sep 9, 2024
@am11
Copy link
Member

am11 commented Sep 9, 2024

Yup, this is the right approach. Currently, LibrariesConfiguration=Release is intentional. When/if we integrate runtime/src/tests/build.sh with top-level runtime/build.sh, we may end up announcing/updating the dev workflow.

See #75033; same change which was ultimately rejected due to sensitive nature of these workflows.

@github-actions github-actions bot locked and limited conversation to collaborators Oct 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants