Skip to content

Conversation

@jandupej
Copy link
Contributor

When AOTing, type load checks no longer produce a compilation error. When a CIL instruction is known at compile time to fail because of invalid type, it is replaced with throwing a TypeLoadException. JIT behavior is not affected.

A new icall is added to facilitate throwing TypeLoadException.

Additionally, the unit tests Loader/classloader/explicitlayout/NestedStructs/... are reenabled as they test working with edge case types.

The PR also addresses #62311.

@jandupej jandupej added this to the 9.0.0 milestone Aug 29, 2023
@jandupej jandupej self-assigned this Aug 29, 2023
Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

  1. I strongly feel we shoudl include the name of the class that failed to load in the exception, whenever possible.
  2. I'm not sure about clearing the cfg exception field between phases - it feels like we might silently ignore an unhandled problem in the future

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

LGTM. If possible try to pass the MonoClass* to mono_throw_type_load by encoding the class constant in the AOT image.

…untime errors that were not necessary for the unit tests.
@jandupej
Copy link
Contributor Author

jandupej commented Sep 1, 2023

The LLVM AOT CI lane failures are related. It seems that there is stack corruption somewhere along the way.

@jandupej
Copy link
Contributor Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

}
}

// Maintain linked list consistency. This BB should have been added as the last,
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to remove dead blocks between the bb_init and bb_exit which may not have a direct link to them?

Copy link
Contributor Author

@jandupej jandupej Sep 20, 2023

Choose a reason for hiding this comment

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

Possibly not; it seems that unreachable blocks are detected (and removed) without my explicitly doing it. This is the first iteration that seems to work locally, waiting for CI. I may be able to simplify it if CI turns out favorable.

EDIT: Are you aware of any unlinked blocks that should be kept?

Copy link
Member

Choose a reason for hiding this comment

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

This change looks good to me. I was just curious whether the unreachable blocks are detected and removed.

Is it possible to skip the bb_init and link the bb_entry with the throw block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had control flow issues when generating LLVM IR without the init block (which was empty no less). I'll see if it can be done.

Copy link
Contributor

Choose a reason for hiding this comment

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

An alternative is to restart the compilation, there is already a mechanism to do that if llvm fails, etc. and emit the exception without compiling the rest of the IL.

@jandupej
Copy link
Contributor Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jandupej
Copy link
Contributor Author

Merging, as the changes seem to work OK. Changing the emission to the compilation restart approach is tracked here: #92688

@jandupej jandupej merged commit ca8d6f0 into dotnet:main Sep 27, 2023
@jandupej jandupej deleted the issue62311 branch September 27, 2023 08:25
@ghost ghost locked as resolved and limited conversation to collaborators Oct 27, 2023
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.

5 participants