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

Fix behavior of intrinsics with empty inputs #2652

Merged
merged 6 commits into from
Feb 28, 2022

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented Feb 24, 2022

The latest analyzer is producing warnings in runtime for this pattern:
/~https://github.com/dotnet/runtime/blob/959a13fa0d1822dc4c7bfc99a9d5e7a0c50d6306/src/libraries/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs#L129

This fixes that by ensuring that AssemblyQualifiedName and other similar intrinsics pass through a TopValue input instead of falling back on the annotations when there is no input. (The analyzer intentionally returns TopValue for unimplemented intrinsics, and the idea is that these should not produce any warnings.)

@sbomer sbomer requested a review from marek-safar as a code owner February 24, 2022 22:44
@sbomer sbomer requested a review from vitek-karas February 24, 2022 22:59
@vitek-karas
Copy link
Member

This applies to basically any intrinsic which returns a value of interest. It's also a bug in both linker and analyzer. In the analyzer it's more common, because analyzer doesn't use UnknownValue, it uses the "empty value" instead (to avoid producing warnings where it doesn't understand, unlike linker). But it's possible to get here with linker as well - null values on input will produce "empty value" on output for lot of intrinsics (intentionally).

I wonder if there's a way to somehow make this a bit more general solution. But it's tricky.
At the very least, we should add tests for this for every intrinsic which returns a value of interest (so Type or string or MethodInfo). Type.GetType is another one which might hit this. And also Type.GetMethod since we're tracking MethodInfo values because of MakeGenericMethod. Could you please go over all intrinsics (not just those already shared) and add tests if appropriate?

@sbomer sbomer force-pushed the fixIntrinsicsWithEmptyInputs branch from 71a60e4 to ad184a5 Compare February 25, 2022 23:52
@sbomer
Copy link
Member Author

sbomer commented Feb 25, 2022

Thanks for the suggestion - I added tests for null/no-value flowing into intrinsics where it made sense and it caught a few more bugs, including a crash in the linker. :) I left in some warnings that we're currently producing in the linker, so please let me know if you think any of these should be changed:

I also didn't add tests for AppDomain.CreateInstance* which currently only has basic testing:

// Just a basic test that these are all recognized, we're not testing that it marks correctly as it has the exact same implementation
// as the above tested Activator.CreateInstance overloads

@sbomer
Copy link
Member Author

sbomer commented Feb 26, 2022

I wonder if there's a way to somehow make this a bit more general solution.

Not really a general solution, but I did at least add an exception here to force all of the shared intrinsics to set a return value (when they have return types): /~https://github.com/dotnet/linker/pull/2652/files#diff-4d295034befb7f16882c6686e3bc9dcc12ce427af6af6cef53ebc0ca3a7562baR186-R187

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.

Thanks a lot for all of the new tests!

if (returnValue == null)
returnValue = calledMethod.ReturnsVoid () ? MultiValueLattice.Top : GetMethodReturnValue (calledMethod, returnValueDynamicallyAccessedMemberTypes);
if (returnValue == null && !calledMethod.ReturnsVoid ())
throw new InvalidOperationException ("No return value set for intrinsic");
Copy link
Member

Choose a reason for hiding this comment

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

Given how many times we've been struggling with this (and thus the implied lack of test coverage) - I'm a bit nervous making it throw here (if linker throws, there's basically no workaround for trimming the app). Maybe we should assert and fallback to returning the annotated value - that should in theory lead to warnings which people report to us as noise (and we still catch the most glaring holes in our tests if we hit them).

On a related note (as this has been a recurring topic recently) - maybe we should use Debug builds of linker in our repos (runtime, MAUI, ...) when they run their Debug CI legs - that way we get better test coverage of the linker without the need to break app trimming with exceptions (we could basically use asserts more frequently without losing that much data).

Copy link
Member Author

@sbomer sbomer Feb 28, 2022

Choose a reason for hiding this comment

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

I left this as before, because some of the newly shared intrinsics rely on the fall-back. Even though they don't return types, they currently get are tracked as ValueWithDynamicallyAccessedMembers, and then cause warnings in some cases, like MakeGenericMethod. I think these should be changed to track UnknownValue.

- Fix test
- Replace exception with an assert
Some of the new shared intrinsics that don't produce type values need
to have some tracked value. Presumably this should really
be Unknown, but currently they fall back on the shared logic
to track a value with annotations (which for these intrinsics
will be None).
@sbomer sbomer merged commit 6aa9837 into dotnet:main Feb 28, 2022
agocke pushed a commit to dotnet/runtime that referenced this pull request Nov 16, 2022
* Fix behavior of intrinsics with null or empty inputs

Add tests for intrinsics receiving null or empty values, and tweak a few
intrinsics to avoid warnings for these cases. This fixes some unnecessary
warnings, and also fixes a crash in the linker.

Some of the new shared intrinsics that don't produce type values need
to have some tracked value. Presumably this should really
be Unknown, but currently they fall back on the shared logic
to track a value with annotations (which for these intrinsics
will be None).

A few unnecessary warnings are left as-is to avoid changing the linker
behavior.

Commit migrated from dotnet/linker@6aa9837
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants