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

[CoreCLR] Avoid having any internal calls in managed code for CoreCLR. Fixes #15343. #15358

Merged
merged 2 commits into from
Jul 11, 2022

Conversation

rolfbjarne
Copy link
Member

CoreCLR does not like running into methods declared as internal calls:

[MethodImplAttribute (MethodImplOptions.InternalCall)]

and will fail with an error, even if the method is never executed, just encountered by the JIT:

ECall methods must be packaged into a system module

In the early days of .NET support we solved this by adding an indirection for
CoreCLR, where the internal call was hidden inside a managed call that would
never be executed at runtime. This seems to work fine in most cases, except
when the debugger attached, when I'm guessing the JIT is more aggressive, and
looks further ahead (and fails).

On the other hand, in the early days of .NET support on macOS the managed code
had to work for both Mono and CoreCLR without modification (since the same
implementation assembly would be picked by the build independent on the actual
runtime). This is no longer the case, since we only support CoreCLR on macOS
now, which allows us to put any Mono-specific managed code (such as the
internal call) in !macOS conditions.

Fixes #15343.

Fixes dotnet#15343.

CoreCLR does not like running into methods declared as internal calls:

    [MethodImplAttribute (MethodImplOptions.InternalCall)]

and will fail with an error, even if the method is never executed, just encountered by the JIT:

    ECall methods must be packaged into a system module

In the early days of .NET support we solved this by adding an indirection for
CoreCLR, where the internal call was hidden inside a managed call that would
never be executed at runtime. This seems to work fine in most cases, except
when the debugger attached, when I'm guessing the JIT is more aggressive, and
looks further ahead (and fails).

On the other hand, in the early days of .NET support on macOS the managed code
had to work for both Mono and CoreCLR without modification (since the same
implementation assembly would be picked by the build independent on the actual
runtime). This is no longer the case, since we only support CoreCLR on macOS
now, which allows us to put any Mono-specific managed code (such as the
internal call) in !macOS conditions.

Fixes dotnet#15343.
@rolfbjarne rolfbjarne requested a review from chamons as a code owner June 27, 2022 15:58
@rolfbjarne rolfbjarne added the bug If an issue is a bug or a pull request a bug fix label Jun 27, 2022
Copy link
Contributor

@spouliot spouliot left a comment

Choose a reason for hiding this comment

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

It might not likely happen again (new icalls) but it would be easy to write a Cecil-based test to detect such methods ;-)

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@rolfbjarne
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@dalexsoto
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2
Copy link
Collaborator

🔥 [PR Build] Build failed 🔥

Build failed for the job 'Detect API changes'

Pipeline on Agent
Hash: [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

🔥 Unable to find the contents for the comment: D:\a\1\s\change-detection\results\gh-comment.md does not exist :fire

Pipeline on Agent
Hash: b59c82c7e9ce7cd955301e75f9453e9f5359a1bb [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻

All tests on macOS Mac Catalina (10.15) passed.

Pipeline on Agent
Hash: b59c82c7e9ce7cd955301e75f9453e9f5359a1bb [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent XAMBOT-1099.Monterey'
Hash: b59c82c7e9ce7cd955301e75f9453e9f5359a1bb [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

❌ [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) failed ❌

Failed tests are:

  • introspection
  • monotouch-test

Pipeline on Agent
Hash: b59c82c7e9ce7cd955301e75f9453e9f5359a1bb [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

🔥 [CI Build] Test results 🔥

Test results

❌ Tests failed on VSTS: simulator tests

0 tests crashed, 5 tests failed, 218 tests passed.

Failures

❌ introspection tests

1 tests failed, 7 tests passed.
  • introspection/tvOS - simulator/Debug [dotnet]: TimedOut

Html Report (VSDrops) Download

❌ monotouch tests

3 tests failed, 20 tests passed.
  • monotouch-test/iOS Unified 64-bits - simulator/Debug (LinkSdk): Failed
  • monotouch-test/iOS Unified 64-bits - simulator/Debug (static registrar): Failed
  • monotouch-test/iOS Unified 64-bits - simulator/Release (all optimizations): Failed

Html Report (VSDrops) Download

❌ mtouch tests

1 tests failed, 0 tests passed.
  • MTouch tests/NUnit: Failed (Execution failed with exit code 19)

Html Report (VSDrops) Download

Successes

✅ bcl: All 69 tests passed. Html Report (VSDrops) Download
✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests: All 1 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 7 tests passed. Html Report (VSDrops) Download
✅ framework: All 8 tests passed. Html Report (VSDrops) Download
✅ generator: All 2 tests passed. Html Report (VSDrops) Download
✅ interdependent_binding_projects: All 7 tests passed. Html Report (VSDrops) Download
✅ install_source: All 1 tests passed. Html Report (VSDrops) Download
✅ linker: All 65 tests passed. Html Report (VSDrops) Download
✅ mac_binding_project: All 1 tests passed. Html Report (VSDrops) Download
✅ mmp: All 2 tests passed. Html Report (VSDrops) Download
✅ mononative: All 12 tests passed. Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ xammac: All 3 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 8 tests passed. Html Report (VSDrops) Download
✅ xtro: All 2 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: [PR build]

@rolfbjarne
Copy link
Member Author

Test failures are unrelated (#15422, #15423 and https://github.com/xamarin/maccore/issues/2597)

@rolfbjarne rolfbjarne merged commit 978835a into dotnet:main Jul 11, 2022
@rolfbjarne rolfbjarne deleted the issue-15343-coreclr-ecall branch July 11, 2022 18:09
@rolfbjarne
Copy link
Member Author

/sudo backport release/6.0.3xx

@rolfbjarne
Copy link
Member Author

/sudo backport release/6.0.4xx

@vs-mobiletools-engineering-service2
Copy link
Collaborator

Backport Job to branch release/6.0.3xx Created! The magic is happening here

@vs-mobiletools-engineering-service2
Copy link
Collaborator

Backport Job to branch release/6.0.4xx Created! The magic is happening here

@vs-mobiletools-engineering-service2
Copy link
Collaborator

Hooray! Backport succeeded! Please see https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=6396818 for more details.

@vs-mobiletools-engineering-service2
Copy link
Collaborator

Hooray! Backport succeeded! Please see https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=6396819 for more details.

rolfbjarne added a commit that referenced this pull request Jul 14, 2022
…d code for CoreCLR. Fixes #15343. (#15458)

CoreCLR does not like running into methods declared as internal calls:

    [MethodImplAttribute (MethodImplOptions.InternalCall)]

and will fail with an error, even if the method is never executed, just encountered by the JIT:

    ECall methods must be packaged into a system module

In the early days of .NET support we solved this by adding an indirection for
CoreCLR, where the internal call was hidden inside a managed call that would
never be executed at runtime. This seems to work fine in most cases, except
when the debugger attached, when I'm guessing the JIT is more aggressive, and
looks further ahead (and fails).

On the other hand, in the early days of .NET support on macOS the managed code
had to work for both Mono and CoreCLR without modification (since the same
implementation assembly would be picked by the build independent on the actual
runtime). This is no longer the case, since we only support CoreCLR on macOS
now, which allows us to put any Mono-specific managed code (such as the
internal call) in !macOS conditions.

Fixes #15343.


Backport of #15358

Co-authored-by: Rolf Bjarne Kvinge <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug If an issue is a bug or a pull request a bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[macOS] ECall methods must be packaged into a system module
8 participants