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

Add compiler-generated code tests for NativeAot #77457

Merged
merged 7 commits into from
Nov 28, 2022
Merged

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented Oct 25, 2022

This makes progress towards #68786. It is still hitting issues such as #68688 and #77455, which are referenced in the test comments.

Also fixes an issue where warnings were not produced for ldftn. The test changes are a modified copy of tests from the linker codebase. See dotnet/linker#3085 for the included changes.

Also fixes an issue where warnings were not produced for ldftn.
Copy link
Contributor

@tlakollo tlakollo left a comment

Choose a reason for hiding this comment

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

Looks good to me. Nice catch on the Release vs Debug releases. Did you try the checked version too? I'm guessing it behaves like the Debug one

@sbomer
Copy link
Member Author

sbomer commented Oct 25, 2022

I don't think there is a checked configuration for managed code by default - only for native code in dotnet/runtime. Your comment made me realize I didn't add the release mode tests to this PR yet, thanks!

@MichalStrehovsky
Copy link
Member

I don't think there is a checked configuration for managed code by default

We do support building the tools directory as checked:

<!-- MSBuild doesn't understand the Checked configuration -->
<PropertyGroup Condition="'$(Configuration)' == 'Checked'">
<Optimize Condition="'$(Optimize)' == ''">true</Optimize>
<DefineConstants>DEBUG;$(DefineConstants)</DefineConstants>
</PropertyGroup>

It may become less relevant if linker tests get moved out of the coreclr test tree in the future (we don't need to support checked outside the coreclr tree).

@sbomer
Copy link
Member Author

sbomer commented Oct 26, 2022

We do support building the tools directory as checked

Good to know, thanks! For the tests I added, there should be no difference since we compile the testcase in-proc in a way that doesn't depend on the tools config.

Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

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

Couple of comments, but nothing blocking.

- Remove TODO comments
- Use built-in NATIVEAOT define
- Use display name helpers
@sbomer sbomer merged commit ebb461a into dotnet:main Nov 28, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 29, 2022
@sbomer sbomer deleted the cgcFixes branch November 3, 2023 18:37
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