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

Move linker to runtime #77149

Closed
wants to merge 2,565 commits into from
Closed

Move linker to runtime #77149

wants to merge 2,565 commits into from

Conversation

tlakollo
Copy link
Contributor

@tlakollo tlakollo commented Oct 18, 2022

This is a test-only draft PR and is not planned to be merged for more information about these changes please refer to #75278.

tlakollo and others added 30 commits February 24, 2022 17:18
Add EnableAotAnalyzer as compiler visible

Commit migrated from dotnet/linker@24373af
Changes "ExpectedWarning" to "ExpectedNoWarnings"

Commit migrated from dotnet/linker@45b481f
…#2639)

Other than sharing more code and adapting it so that it works on both linker and analyzer, this change brings simple analysis of constant integer values in the analyzer. This is necessary to make most reflection API calls recognize binding flags. For example `GetMethods(BindingFlags.Public | BindingFlags.Static)`. So this change adds analysis of constant values (as recognized by the Roslyn's operation tree) and the OR binary operator for integers and enums.

Added some new tests for the binding flags handling.

Reenabled some disabled tests for analyzer.
Moved the main affected tests from the generated source files to the hardcoded one and force them to exact match of warnings for both linker and analyzer.

Commit migrated from dotnet/linker@f0bd2ae
…224.4 (dotnet/linker#2663)

[main] Update dependencies from dotnet/arcade


Commit migrated from dotnet/linker@54c94cb
…0227.1 (dotnet/linker#2664)

[main] Update dependencies from dotnet/runtime


Commit migrated from dotnet/linker@396c37d
* 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
Shares two more intrinsics, with supporting infra.
Fixed a bug in the intrinsics - passing null to the name of a property/field will throw at runtime, so no need to validate anything.

Modifies the existing tests to add warnings, since that is the only verifyable behavior for the analyzer.

Commit migrated from dotnet/linker@8c0df91
…304.3 (dotnet/linker#2676)

[main] Update dependencies from dotnet/arcade


Commit migrated from dotnet/linker@14b3d62
…0307.1 (dotnet/linker#2677)

[main] Update dependencies from dotnet/runtime


Commit migrated from dotnet/linker@26e0c5c
* Use attributes on lambdas in tests

* Fix formatting

* Unindent attributes

Commit migrated from dotnet/linker@93eac59
* Warn on DAM mismatch between overrides

Commit migrated from dotnet/linker@ac6cfb3
…307.6 (dotnet/linker#2684)

[main] Update dependencies from dotnet/arcade


Commit migrated from dotnet/linker@cbcfcfc
…0313.2 (dotnet/linker#2685)

[main] Update dependencies from dotnet/runtime


Commit migrated from dotnet/linker@ae1f280
* Remove SuppressionContextMember

Instead of computing the suppression context member when we push to the scope stack, it is now computed on demand when we need to know whether a warning is suppressed. The suppression context should be entirely determined by the static scopes, so there isn't any need to track it dynamically.

Commit migrated from dotnet/linker@2303da0
…tnet/linker#2675)

Add intrinsic support for Nullable.GetUnderlyingType and support for MakeGenericType with Nullables

Adds ArrayCreationOperation visitors to create ArrayValue's in the analyzer, and adds start of dataflow analysis for array values.

Adds tests to validate dataflow in Arrays.

Co-authored-by: vitek-karas <10670590+vitek-karas@users.noreply.github.com>

Commit migrated from dotnet/linker@ed8b22a
* Add test which mimics what is done in Type.ImplementInterface helper in runtime

There's nothing new in this test, but it's better to have coverage for the pattern as it's used in runtime.

* Add tests for properties

* Simplify

Co-authored-by: vitek-karas <10670590+vitek-karas@users.noreply.github.com>

Commit migrated from dotnet/linker@5929598
…318.2 (dotnet/linker#2697)

[main] Update dependencies from dotnet/arcade


Commit migrated from dotnet/linker@9f890c6
…0321.1 (dotnet/linker#2698)

[main] Update dependencies from dotnet/runtime


Commit migrated from dotnet/linker@64ad0ff
@am11
Copy link
Member

am11 commented Oct 19, 2022

Out of curiosity: What is the goal of this PR please?

@MartyIX, the goal is to move linker code to runtime repository and abandon dotnet/linker repository. For more details, see: #75278.

@joperezr
Copy link
Member

tagging @ViktorHofer and @dotnet/area-infrastructure-libraries so that somebody on the infrastructure side can take a peek at some of the infra changes to make sure they are all right. @tlakollo is this code ready for review?

@ViktorHofer
Copy link
Member

This is draft so I guess you don't want a review yet? If you want one, which commits would you want to be reviewed? Presumably, not the ones that are cherry-picked from dotnet/linker.

@tlakollo tlakollo added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Oct 19, 2022
@tlakollo
Copy link
Contributor Author

At this moment I'm just experimenting with how the infrastructure behaves by inserting the linker repo. Basically, make sure that none of the changes breaks the runtime in any way even if the repo is not running in CI. Then how it will behave after the testing subset is enabled, and so on.

@joperezr
Copy link
Member

I'm so sorry, I missed that it was a draft PR. I have pinged @tlakollo and indeed I don't think a review here is required as this is mostly to iterate on CI for testing.

@tlakollo
Copy link
Contributor Author

Out of curiosity: What is the goal of this PR please?

I added a motivation section on #75278 to give more info about why this is being prototyped

Tlakollo and others added 5 commits October 19, 2022 17:41
Change source lines since now we are using spaces instead of tabs (pending issue with pdbs and symbols on assemblies)
Workaround the deletion of RefSafetyRulesAttribute and their associated types injected by the compiler
For now add extra ExpectedWarnings on warnings being duplicated by the analyzer (pending investigation)
…related tests will fail

Upgrade to MicrosoftCodeAnalysisVersion 4.5+ generates a different type of analysis callbacks, the CheckAttributeInstantiation method is no longer needed. And a bug in which a warning was printed twice by the analyzer is fixed.
Typo on Cecil.Pdb package
Update names in csproj's
Add warning for file header mismatch/missing
@agocke agocke self-assigned this Oct 25, 2022
</ItemGroup>

<ItemGroup Condition="$(_subset.Contains('+tools.linkertests+'))">
<ProjectToBuild Include="$(ToolsProjectRoot)linker\test\Mono.Linker.Tests\Mono.Linker.Tests.csproj"
Copy link
Member

Choose a reason for hiding this comment

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

These should be built even in source build.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, never mind, I guess we don't need tests for source build.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is meant by "source build" please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a nutshell source-build is a requirement from some Linux distributions which require source code to build. So basically when they get dotnet they actually get all the source code (one of those pieces is the runtime, but they are more pieces) and build it. The source-build repository is in charge of taking all of these pieces and glue them together, it also has an explanation of what source build is -> /~https://github.com/dotnet/source-build#source-build-goals
We want all the source code to be included, but the tests are not going to be executed once it's an app like dotnet so we exclude them using the DotNetBuildFromSource variable.

global.json Outdated Show resolved Hide resolved
tlakollo and others added 3 commits October 25, 2022 15:37
Add condition for test group to any change in src/tools/linker
Remove extra tool in global.json
Add a dummy line change to test if CI triggers
…er changes

Exclude building the clr in linker testing
@tlakollo
Copy link
Contributor Author

Closing this in favor of getting a review into an updated and easier to read PR in #77569

@tlakollo tlakollo closed this Oct 27, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 27, 2022
@tlakollo tlakollo deleted the MoveLinkerToRuntime branch February 7, 2023 21:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
linkable-framework Issues associated with delivering a linker friendly framework NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it
Projects
None yet
Development

Successfully merging this pull request may close these issues.