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

[cdac] Implement NibbleMap lookup and tests #108403

Merged
merged 10 commits into from
Oct 8, 2024

Conversation

lambdageek
Copy link
Member

@lambdageek lambdageek commented Sep 30, 2024

The execution manager uses a nibble map to quickly map program counter pointers to the beginnings of the native code for the managed method.

Implement the lookup algorithm for a nibble map.

Start adding unit tests for the nibble map

Also for testing in MockMemorySpace simplify ReaderContext, there's nothing special about the descriptor HeapFragments anymore. We can use a uniform reader.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 30, 2024
@lambdageek lambdageek added area-Diagnostics-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Sep 30, 2024
Copy link
Contributor

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

The execution manager uses a nibble map to quickly map program counter
pointers to the beginnings of the native code for the managed method.

Implement the lookup algorithm for a nibble map.

Start adding unit tests for the nibble map

Also for testing in MockMemorySpace simplify ReaderContext, there's nothing special about the descriptor HeapFragments anymore.  We can use a uniform reader.
@lambdageek lambdageek force-pushed the cdac-nibblemap-standalone branch from 5c161e9 to 51d7696 Compare October 7, 2024 23:40
@lambdageek lambdageek force-pushed the cdac-nibblemap-standalone branch from 51d7696 to 4182125 Compare October 7, 2024 23:51
Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

I have two design questions, but other than that it looks good.

// We will then align the map index to the start of the current map unit (map index 8) and move back to the previous map unit (map index 7)
// At that point, we scan backwards for non-zero map units. Since there are none, we return null.

internal class NibbleMap
Copy link
Member

Choose a reason for hiding this comment

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

Should we think about putting version numbers on these classes. For instance, we know that there are more optimal forms of the NibbleMap that we might implement in the future, and I don't want to get rid of the existing code here, as it could be used to support an old version of the runtime. Do we want to think about calling these things NibbleMap_1 or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep. we could do it now, or we could do it when we have a second algorithm. NibbleMap doesn't leak out of the ExecutionManager contract, so we can cross that bridge when we get to it (also maybe we'll have better names than _1 and _2)

public bool Equals(TargetCodePointer x, TargetCodePointer y) => x.Value == y.Value;
public int GetHashCode(TargetCodePointer obj) => obj.Value.GetHashCode();

public TargetPointer AsTargetPointer => new(Value);
Copy link
Member

Choose a reason for hiding this comment

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

On ARM32 platforms should this strip off the thumb bit? For reference, on ARM32 Thumb2 targets, the lowest bit is typically set on a code address, to indicate that the pointer refers to a code using the Thumb2 instruction set instead of the ARM instruction set.

I see this as a potential problem around the conversion to ulong here, as well as the AsTargetPointer api.

Copy link
Member Author

Choose a reason for hiding this comment

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

yea that's a good idea. Elsewhere (in the PrecodeStubs contract) I have an explicit helper that strips off the thumb bit:

    internal TargetPointer CodePointerReadableInstrPointer(TargetCodePointer codePointer)
    {
        // Mask off the thumb bit, if we're on arm32, to get the actual instruction pointer
        ulong instrPointer = (ulong)codePointer.AsTargetPointer & MachineDescriptor.CodePointerToInstrPointerMask.Value;
        return new TargetPointer(instrPointer);
    }

I couldn't decide if that's something we want on the TargetCodePointer or on the Target (or on a contract, as I've prototyped it so far)

I think on TargetCodePointer makes the most sense, but then i'll need to store the mask in the code pointer instance at creation time (or make the conversion to a TargetPointer depend on the current target) - and i wasn't sure about the usability of that approach

@lambdageek lambdageek merged commit 9d923b8 into dotnet:main Oct 8, 2024
148 of 151 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 8, 2024
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.

4 participants