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

[NativeAOT] fix debug assert on large number of virtual methods #77106

Merged
merged 2 commits into from
Oct 18, 2022

Conversation

AustinWise
Copy link
Contributor

According to the ARM docs for this op code:

https://developer.arm.com/documentation/ddi0596/2020-12/Base-Instructions/LDR--immediate---Load-Register--immediate--?lang=en

For the 64-bit variant: is the optional positive immediate byte offset, a multiple of 8 in the range 0 to 32760, defaulting to 0 and encoded in the "imm12" field as /8.

I have a program that reproduces the problem. It is just a class with 1000 virtual properties and a RD.xml file to root them all. The actual program I was trying to compile when I encountered the problem was Microsoft.macos.dll. There are some classes with a large number of virtual methods in that assembly.

According to the ARM docs for this op code:

https://developer.arm.com/documentation/ddi0596/2020-12/Base-Instructions/LDR--immediate---Load-Register--immediate--?lang=en

> For the 64-bit variant: is the optional positive immediate byte offset, a multiple of 8 in the range 0 to 32760, defaulting to 0 and encoded in the "imm12" field as <pimm>/8.

You can see that once the offset has been devided by 8, it will be in the range [0, 4095], which will fit in the 12 bits allocated for the offset.
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 17, 2022
@@ -63,7 +63,7 @@ public void EmitLDR(Register regDst, Register regAddr)

public void EmitLDR(Register regDst, Register regSrc, int offset)
{
Debug.Assert(offset >= -255 && offset <= 4095);
Debug.Assert(offset >= -255 && offset <= 32760);
Copy link
Member

Choose a reason for hiding this comment

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

We may want to throw new NotImplementedException(); when the offset is too big. I think it is possible to construct cases where the offset is too big. It is better to throw exception than emit bad code silently.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks!

@jkotas jkotas merged commit 8393c4e into dotnet:main Oct 18, 2022
@AustinWise AustinWise deleted the austin/FixIlcAssert branch October 29, 2022 19:50
@ghost ghost locked as resolved and limited conversation to collaborators Nov 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-crossgen2-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants