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 #95816

Merged
merged 5 commits into from
Dec 12, 2023

Conversation

mikem8361
Copy link
Member

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.

@ghost
Copy link

ghost commented Dec 9, 2023

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

Issue 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.

Author: mikem8361
Assignees: mikem8361
Labels:

area-Diagnostics-coreclr

Milestone: -

@mikem8361
Copy link
Member Author

Anybody have time to review and approve this PR? I know a lot of it is createdump, but everybody else is on vacation.

/cc @tommcdon

@@ -36,12 +36,12 @@ struct GlobalValueEntry

// This size should be one bigger than the number of entries since a null entry
// signifies the end of the array.
static constexpr size_t DebugTypeEntriesArraySize = 96;
static constexpr size_t DebugTypeEntriesArraySize = 128;
Copy link
Member

Choose a reason for hiding this comment

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

Did we hit this limit? If yes, anything we can do to avoid hitting it silently?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are asserts but they are only in debug (it would be nice if there was a Release build assert) and are off by one (which I should have and will fixed) because they didn't account for the end/zero entry that terminates the list. The size increase was to allow for that terminating entry. I also got carried away with the size increase; I'll reduced it.

Copy link
Member

Choose a reason for hiding this comment

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

This should better by initialized at compile time that would solve this problem as well. We can make the structure to have the exact right size if it is initialized at compile time. It is wasteful to initialize this at runtime.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I agree, but can we do that in a later .NET 9 PR?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I agree it should be a separate .NET 9 PR.

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.

LGTM otherwise. Thank you!

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.
@mikem8361 mikem8361 merged commit f9529fd into dotnet:main Dec 12, 2023
110 checks passed
@mikem8361 mikem8361 deleted the diaginfo branch December 12, 2023 19:20
@mikem8361
Copy link
Member Author

/backport to release/8.0-staging

Copy link
Contributor

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/7187347712

Copy link
Contributor

@mikem8361 backporting to release/8.0-staging failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Add runtime module address to SpecialDiagInfo block in createdump
Using index info to reconstruct a base tree...
M	src/coreclr/debug/createdump/crashinfo.cpp
M	src/coreclr/debug/createdump/crashinfounix.cpp
M	src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets
M	src/coreclr/nativeaot/Runtime/DebugHeader.cpp
M	src/coreclr/tools/aot/ILCompiler/ILCompilerRootCommand.cs
M	src/coreclr/tools/aot/ILCompiler/Program.cs
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/tools/aot/ILCompiler/Program.cs
Auto-merging src/coreclr/tools/aot/ILCompiler/ILCompilerRootCommand.cs
Auto-merging src/coreclr/nativeaot/Runtime/DebugHeader.cpp
Auto-merging src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets
Auto-merging src/coreclr/debug/createdump/crashinfounix.cpp
Auto-merging src/coreclr/debug/createdump/crashinfo.cpp
Applying: Code review feedback
Using index info to reconstruct a base tree...
M	src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets
Applying: Revert previous ILC/.targets changes
Applying: Bump the Native AOT data contract sizes
Applying: Code review feedback
error: sha1 information is lacking or useless (src/coreclr/nativeaot/Runtime/DebugHeader.cpp).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0005 Code review feedback
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

Copy link
Contributor

@mikem8361 an error occurred while backporting to release/8.0-staging, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants