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

Add runtime module address to SpecialDiagInfo block in createdump and fix missing DotNetRuntimeDebugHeader export #96121

Merged
merged 5 commits into from
Jan 2, 2024

Conversation

mikem8361
Copy link
Member

@mikem8361 mikem8361 commented Dec 18, 2023

Customer Impact

Without this PR the Native AOT SOS when it is released will not work for Linux and MacOS apps/crash dumps. This hasn’t been reported (but will be) by any customers yet since the Native AOT specific (closed source) SOS hasn’t been released yet. This is a regression from .NET 7. The DCR symbol exists in .NET 7 Native AOT apps. This change DOES NOT remove any ILC command line options only adds 2 new ones.

Part of this PR is single-file and Native AOT app-model SOS performance which for single-file has been reported by customers (I forgot to put issue #3737 in the PR text). The reason these are two are in the same PR is the missing DCR export symbol part is required for the performance half (for Native AOT).

This will allow the Native AOT SOS to find the module containing the runtime without searching every module in the system.

Testing

Manual testing with Native AOT SOS.

Risk

Medium. This is changing the compiler and linker command lines. It has potential to break people with custom build integration. We do not document the AOT compiler command line args, but this is the first time we are changing the AOT compiler command line options in servicing.

Details

Fix missing DotNetRuntimeDebugHeader export:

Add ILC options:
--export-dynamic-symbol - Add dynamic export symbol to export file. Used to add DotNetRuntimeDebugHeader export.
--export-unmanaged-entrypoints - controls whether the exported method definitions are exported

Change Native AOT build integration to always pass an export file to ILC and linker.

Bump the Native AOT data contract sizes

@mikem8361 mikem8361 added the Servicing-consider Issue for next servicing release review label Dec 18, 2023
@mikem8361 mikem8361 added this to the 8.0.x milestone Dec 18, 2023
@mikem8361 mikem8361 self-assigned this Dec 18, 2023
@ghost
Copy link

ghost commented Dec 18, 2023

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

Issue Details

Customer Impact

The missing DotNetRuntimeDebugHeader export break createdump finding the runtime module and the Native SOS finding the data contract table.

This will allow the Native AOT SOS to find the module containing the runtime without searching every module in the system.

Testing

Manual testing with Native AOT SOS.

Risk

Low. Only affects the internal Native AOT runtime thread id which is only used internal in a few places and by the Native AOT SOS.

Details

Fix missing DotNetRuntimeDebugHeader export:

Add ILC options:
--export-dynamic-symbol - Add dynamic export symbol to export file. Used to add DotNetRuntimeDebugHeader export.
--export-unmanaged-entrypoints - controls whether the exported method definitions are exported

Change Native AOT build integration to always pass an export file to ILC and linker.

Bump the Native AOT data contract sizes

Author: mikem8361
Assignees: mikem8361
Labels:

Servicing-consider, area-Diagnostics-coreclr

Milestone: 8.0.x

@ghost
Copy link

ghost commented Dec 18, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Customer Impact

The missing DotNetRuntimeDebugHeader export break createdump finding the runtime module and the Native SOS finding the data contract table.

This will allow the Native AOT SOS to find the module containing the runtime without searching every module in the system.

Testing

Manual testing with Native AOT SOS.

Risk

Low. Only affects the internal Native AOT runtime thread id which is only used internal in a few places and by the Native AOT SOS.

Details

Fix missing DotNetRuntimeDebugHeader export:

Add ILC options:
--export-dynamic-symbol - Add dynamic export symbol to export file. Used to add DotNetRuntimeDebugHeader export.
--export-unmanaged-entrypoints - controls whether the exported method definitions are exported

Change Native AOT build integration to always pass an export file to ILC and linker.

Bump the Native AOT data contract sizes

Author: mikem8361
Assignees: mikem8361
Labels:

Servicing-consider, area-Diagnostics-coreclr, area-NativeAOT-coreclr

Milestone: 8.0.x

@jkotas
Copy link
Member

jkotas commented Dec 18, 2023

Low. Only affects the internal Native AOT runtime thread id which is only used internal in a few places and by the Native AOT SOS.

Copy&paste from the other PR...

This is changing the compiler and linker command lines. It has potential to break people with custom build integration. We do not document the AOT compiler command line args, but this is the first time we are changing the AOT compiler command line options in servicing. I would classify the risk as medium.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you

@MichalStrehovsky
Copy link
Member

The failure looks related:

2023-12-18T21:07:23.5863280Z LINK : fatal error LNK1104: cannot open file 'D:\a\_work\1\s\artifacts\tests\coreclr/obj/windows.x64.Checked/Managed/Loader\classloader\StaticVirtualMethods\Regression\RecursiveConstraintOnDefaultImplementationOfStaticAbstractMember\native\RecursiveConstraintOnDefaultImplementationOfStaticAbstractMember.def' [D:\a\_work\1\s\src\tests\Loader\classloader\StaticVirtualMethods\Regression\RecursiveConstraintOnDefaultImplementationOfStaticAbstractMember.csproj] [D:\a\_work\1\s\src\tests\build.proj]

This might be MAX_PATH related; the path to the def file is 262 characters. Interestingly the path to the object file would also be over MAX_PATH here but it might be it's taking different codepaths within link.exe (just speculating).

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. we will take for consideration in 8.0.x

@mikem8361
Copy link
Member Author

I'm not sure how to fix this failure. It didn't happen in main with this change. This change does create and pass a def file to the linker pretty always now.

@jkotas
Copy link
Member

jkotas commented Dec 21, 2023

The test was renamed to have a shorter name in #92407. Rename it in this PR to fix the failure?.

@jeffschwMSFT jeffschwMSFT added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Dec 24, 2023
Fix missing DotNetRuntimeDebugHeader export:

  Add ILC options:
    --export-dynamic-symbol - Add dynamic export symbol to export file. Used to add DotNetRuntimeDebugHeader export.
    --export-unmanaged-entrypoints - controls whether the exported method definitions are exported

  Change Native AOT build integration to always pass an export file to ILC and linker.

Bump the Native AOT data contract sizes
@mikem8361
Copy link
Member Author

Is there anything else needed for this PR?

@jeffschwMSFT jeffschwMSFT merged commit cf2b9e3 into dotnet:release/8.0-staging Jan 2, 2024
119 checks passed
@mikem8361 mikem8361 deleted the diaginfo8 branch January 9, 2024 19:54
@akoeplinger akoeplinger removed this from the 8.0.x milestone Jan 14, 2024
@akoeplinger akoeplinger added this to the 8.0.2 milestone Jan 14, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Feb 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants