-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Conversation
Tagging subscribers to this area: @tommcdon Issue DetailsFix missing DotNetRuntimeDebugHeader export: Add ILC options: Change Native AOT build integration to always pass an export file to ILC and linker.
|
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ExportsFileWriter.cs
Outdated
Show resolved
Hide resolved
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
/backport to release/8.0-staging |
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/7187347712 |
@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! |
@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. |
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.