Skip to content

Conversation

@cshung
Copy link
Contributor

@cshung cshung commented Aug 3, 2021

Fixes #56653

This change fixes two issues:

The first issue is that we generation_allocation_start is no longer defined in the generation class.

class generation
{
public:
    // Don't move these first two fields without adjusting the references
    // from the __asm in jitinterface.cpp.
    alloc_context   allocation_context;
    PTR_heap_segment start_segment;
#ifndef USE_REGIONS
    uint8_t*        allocation_start;
#endif //!USE_REGIONS
...

The fix here is completely analogus to #56056. It is a lot of code, but it is all boilerplate.

The second issue is that unlike in the segment case, gen1 and gen0 have their own linked list of regions.

The removal of the allocation_start pointer means we no longer have a pointer for the beginning of a generation, and it is easily remedied because the start of the generation is the beginning of the first region in that linked list.

This fix is sufficient to get heap enumeration working, but it is probably not addressing all uses of the generation_allocation_start field (and potentially also fields whose values are derived from it). That would be out of the scope of the current PR, the remaining work are tracked as issues and they can wait because they are shipped out of band.

The work to fix SOS is tracked here and the work to fix clrmd is tracked here.

@ghost
Copy link

ghost commented Aug 3, 2021

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

Issue Details

Fixes #56653

There are two key changes in the data structure with respect to heap enumeration.

  1. The field generation_allocation_start is no longer valid. Although it is still on the dac_generation structure, it is no longer defined in the underlying runtime data structure, so the value read there is something else (that we should not depend on [*]). and

  2. Unlike in the segment case, gen1 and gen0 have their own linked list of regions.

This fix is sufficient to get heap enumeration working, but it is probably not addressing all uses of the generation_allocation_start field (and potentially also fields whose values are derived from it). That would be out of the scope of the current PR.

[*], that something else is the next field in the structure, which is a pointer to the allocation segment, so no worries about reading unmapped memory. In #56056, I do have a mechanism to skip optional fields for the gc_heap type. I could have applied the same trick for dac_generation, but it just feels like overkill for this case. That's why I simply fixed this bug by tactically avoiding usage of the field instead.

Author: cshung
Assignees: -
Labels:

area-Diagnostics-coreclr

Milestone: -

@Maoni0
Copy link
Member

Maoni0 commented Aug 4, 2021

why are we not doing the same thing as we did for the gc_heap/dac_gc_heap? seems like the same type of problem.

I would also not use something like saved_sweep_ephemeral_start to identify if it's using regions or not.. we may not even have this field at all anymore in the future. would be a shame to keep it just for this purpose. we do have these fields -

    gcDacVars->major_version_number = 1;
    gcDacVars->minor_version_number = 0;

can these actually be put to use?

@cshung cshung requested review from hoyosjs and removed request for noahfalk August 11, 2021 18:39
Copy link
Member

@Maoni0 Maoni0 left a comment

Choose a reason for hiding this comment

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

the rest LGTM

@Maoni0
Copy link
Member

Maoni0 commented Aug 17, 2021

other than those comments above the rest LGTM

@cshung cshung merged commit b493278 into dotnet:main Aug 17, 2021
@cshung cshung deleted the public/dac-walk branch August 17, 2021 14:18
@ghost ghost locked as resolved and limited conversation to collaborators Sep 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.

ICorDebugProcess5::EnumerateHeapRegions returned a COR_SEGMENT that grow backwards under USE_REGIONS

3 participants