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

SingleFile diagnostic support - Add export table and DotNetRuntimeInfo to dumps #52731

Merged
merged 2 commits into from
May 17, 2021

Conversation

hoyosjs
Copy link
Member

@hoyosjs hoyosjs commented May 13, 2021

  • DACize PEDecoder::GetExport

DACize the extraction of exports and account for mapped vs non-mapped versions. @trylek, could you please check this first commit.

  • Ensure export table and runtimeinfo export are always present in a minidump

@hoyosjs hoyosjs added this to the 6.0.0 milestone May 13, 2021
@hoyosjs hoyosjs self-assigned this May 13, 2021
@ghost
Copy link

ghost commented May 13, 2021

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

Issue Details
  • DACize PEDecoder::GetExport

DACize the extraction of exports and account for mapped vs non-mapped versions. @trylek, could you please check this first commit.

  • Ensure export table and runtimeinfo export are always present in a minidump
Author: hoyosjs
Assignees: hoyosjs
Labels:

area-Diagnostics-coreclr

Milestone: 6.0.0

@hoyosjs hoyosjs force-pushed the juhoyosa/singlefile-basechange branch from 2e58772 to 443504f Compare May 13, 2021 23:59
@hoyosjs hoyosjs requested a review from mikem8361 May 13, 2021 23:59
@hoyosjs hoyosjs force-pushed the juhoyosa/singlefile-basechange branch from 443504f to 35a2f6b Compare May 14, 2021 07:34
@hoyosjs hoyosjs force-pushed the juhoyosa/singlefile-basechange branch from 35a2f6b to df6d02f Compare May 14, 2021 20:47
@hoyosjs hoyosjs added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label May 14, 2021
@hoyosjs hoyosjs force-pushed the juhoyosa/singlefile-basechange branch from df6d02f to 8fde24e Compare May 15, 2021 04:06
@hoyosjs hoyosjs removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label May 15, 2021
@hoyosjs
Copy link
Member Author

hoyosjs commented May 15, 2021

@trylek, had to fix a bug in the GetExport logic, it was using the index in the name table as an offset to the address table, but according to the spec it is an index into the ordinal table, which will give the offset for the address table. Can you PTAL?

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@hoyosjs
Copy link
Member Author

hoyosjs commented May 15, 2021

/azp list

@hoyosjs
Copy link
Member Author

hoyosjs commented May 15, 2021

/azp run runtime-coreclr crossgen2-composite

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@trylek trylek left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the fix. Can you please double-check that the corresponding code in R2RDump is correct? I think it is but you have the structure in fresh memory now ;-).

@hoyosjs
Copy link
Member Author

hoyosjs commented May 17, 2021

@trylek I've verified both the PEReader extensions and ILCompiler.PEWriter.SectionBuilder.SerializeExportSection, and both do the ordinal table indirection correctly. Are the tests in the composite pipeline expected to fail like this?

@trylek
Copy link
Member

trylek commented May 17, 2021

@hoyoys - Thank you! For the composite runs, all failures I'm seeing correspond to known issues. I have a long-haul PR #51416 to fix part of them but it's taking a bit due to a complex discussion on the proper fix and the nullrefs on OSX arm64 are a known codegen issue unrelated to Crossgen2 I believe.

@hoyosjs hoyosjs merged commit 6f5ab2b into dotnet:main May 17, 2021
@hoyosjs hoyosjs deleted the juhoyosa/singlefile-basechange branch May 17, 2021 20:40
@ghost ghost locked as resolved and limited conversation to collaborators Jun 16, 2021
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