Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation

mikem8361
Copy link

Fix a function that was ifdef'ed needed for the metadata locator callbacks to work.

Fix some not properly DAC'ized code in the type desc and server GC code. Caused an exception during dump generation.

Issue: https://github.com/dotnet/coreclr/issues/26907

Fix a function that was ifdef'ed needed for the metadata locator callbacks to work.

Fix some not properly DAC'ized code in the type desc and server GC code. Caused an exception during dump generation.

Issue: https://github.com/dotnet/coreclr/issues/26907
@mikem8361 mikem8361 added this to the 3.1 milestone Oct 30, 2019
@mikem8361 mikem8361 self-assigned this Oct 30, 2019
@mikem8361 mikem8361 requested a review from noahfalk October 30, 2019 17:21
@mikem8361
Copy link
Author

Issue
VS can not load minidumps generated with a 3.0 runtime. A popup happens with the message "Unable to managed debug this minidump...".

Customer impact
This is completely blocking external customers (see issue https://github.com/dotnet/coreclr/issues/26907) and internal customers (ASP.NET).

Fix description
This problem happened with coreclr switched from fragile NGEN to R2R images. This caused an DAC API to be ifdef'ed out (failing) that VS uses to get the metadata for a module. There were also two fixes in the generation side where the memory enumeration failed prematurely.

To fix this end-to-end it requires a change in VS to understand R2R images and not treat them like NGEN ones.

Risk
Low risk because it only affects the DAC (debugger side) of the runtime and only dump generation in the DAC and one API that is only used by VS for the minidump loading not live debugging.

@danmoseley
Copy link
Member

Please mail .net core tactics alias when this is ready for consideration.

@danmoseley danmoseley changed the title Fix minidumps not loading in VS issue [release/3.1] Fix minidumps not loading in VS issue Oct 30, 2019
@mikem8361 mikem8361 merged commit 934ef61 into dotnet:release/3.1 Oct 30, 2019
@mikem8361 mikem8361 deleted the vsdumpfix31 branch October 30, 2019 23:12
Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

Probably fine though I don't understand one part of the change

DacEnumMemoryRegion(dac_cast<TADDR>(pHeap->finalize_queue), sizeof(dac_finalize_queue));
DacEnumMemoryRegion(dac_cast<TADDR>(pHeap->generation_table), gen_table_size);

TADDR taddrTable = dac_cast<TADDR>(pHeap) + offsetof(dac_gc_heap, generation_table);
Copy link
Member

Choose a reason for hiding this comment

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

What is different about the new line vs. the old one? If they compute different values does that mean the same issue exists in the DacEnumMemoryRegion(pHeap->finalize_queue) call that preceeds this?

Copy link
Author

Choose a reason for hiding this comment

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

The original code was throwing an invalid memory exception because generation_table is an array and the dac_cast macro didn't seem to handle it correctly.

@noahfalk
Copy link
Member

@mikem8361 was this supposed to be merged? I didn't get the impression that tactics had signed off on it yet.

@mikem8361
Copy link
Author

I did get approval from tactics.

@noahfalk
Copy link
Member

Cool, sorry to add noise : )

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants