Move linker to runtime - #77149

Closed
tlakollo wants to merge 2565 commits into
dotnet:mainfrom
tlakollo:MoveLinkerToRuntime
Closed

Move linker to runtime#77149
tlakollo wants to merge 2565 commits into
dotnet:mainfrom
tlakollo:MoveLinkerToRuntime

Conversation

@tlakollo

@tlakollotlakollo commented Oct 18, 2022

Copy link
Copy Markdown
Contributor

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

tlakolloand 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
Comment threadsrc/tools/linker/src/tlens/TLens/LensesCollection.cs Outdated
@am11

ghost commented Oct 19, 2022

Copy link
Copy Markdown
Member

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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
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.

@tlakollotlakollo 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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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

Tlakolloand 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
@agockeagocke self-assigned this Oct 25, 2022
Comment threadeng/Subsets.props
</ItemGroup>

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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.

Comment threadglobal.json Outdated
tlakolloand 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

ghost commented Oct 27, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@ghostghost 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 freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

linkable-frameworkIssues associated with delivering a linker friendly frameworkNO-MERGEThe PR is not ready for merge yet (see discussion for detailed reasons)NO-REVIEWExperimental/testing PR, do NOT review it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

20 participants

@tlakollo@MartyIX@am11@joperezr@ViktorHofer@agocke@sbomer@jtschuster@vitek-karas@mrvoorhe@marek-safar@MichaelSimons@akoeplinger@iserrato@jkurdek@lbussell@Youssef1313@MichalStrehovsky@alexperovich@dkurepa
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Move linker to runtime - #77149

Closed
tlakollo wants to merge 2565 commits into
dotnet:mainfrom
tlakollo:MoveLinkerToRuntime
Closed

Move linker to runtime#77149
tlakollo wants to merge 2565 commits into
dotnet:mainfrom
tlakollo:MoveLinkerToRuntime

Conversation

@tlakollo

@tlakollotlakollo commented Oct 18, 2022

Copy link
Copy Markdown
Contributor

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

tlakolloand 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
Comment threadsrc/tools/linker/src/tlens/TLens/LensesCollection.cs Outdated
@am11

ghost commented Oct 19, 2022

Copy link
Copy Markdown
Member

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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
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.

@tlakollotlakollo 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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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

Tlakolloand 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
@agockeagocke self-assigned this Oct 25, 2022
Comment threadeng/Subsets.props
</ItemGroup>

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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.

Comment threadglobal.json Outdated
tlakolloand 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

ghost commented Oct 27, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@ghostghost 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 freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

linkable-frameworkIssues associated with delivering a linker friendly frameworkNO-MERGEThe PR is not ready for merge yet (see discussion for detailed reasons)NO-REVIEWExperimental/testing PR, do NOT review it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

20 participants

@tlakollo@MartyIX@am11@joperezr@ViktorHofer@agocke@sbomer@jtschuster@vitek-karas@mrvoorhe@marek-safar@MichaelSimons@akoeplinger@iserrato@jkurdek@lbussell@Youssef1313@MichalStrehovsky@alexperovich@dkurepa
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Move linker to runtime - #77149

Closed
tlakollo wants to merge 2565 commits into
dotnet:mainfrom
tlakollo:MoveLinkerToRuntime
Closed

Move linker to runtime#77149
tlakollo wants to merge 2565 commits into
dotnet:mainfrom
tlakollo:MoveLinkerToRuntime

Conversation

@tlakollo

@tlakollotlakollo commented Oct 18, 2022

Copy link
Copy Markdown
Contributor

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

tlakolloand 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
Comment threadsrc/tools/linker/src/tlens/TLens/LensesCollection.cs Outdated
@am11

ghost commented Oct 19, 2022

Copy link
Copy Markdown
Member

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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
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.

@tlakollotlakollo 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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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

Tlakolloand 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
@agockeagocke self-assigned this Oct 25, 2022
Comment threadeng/Subsets.props
</ItemGroup>

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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.

Comment threadglobal.json Outdated
tlakolloand 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

ghost commented Oct 27, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@ghostghost 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 freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

linkable-frameworkIssues associated with delivering a linker friendly frameworkNO-MERGEThe PR is not ready for merge yet (see discussion for detailed reasons)NO-REVIEWExperimental/testing PR, do NOT review it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

20 participants

@tlakollo@MartyIX@am11@joperezr@ViktorHofer@agocke@sbomer@jtschuster@vitek-karas@mrvoorhe@marek-safar@MichaelSimons@akoeplinger@iserrato@jkurdek@lbussell@Youssef1313@MichalStrehovsky@alexperovich@dkurepa
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Move linker to runtime - #77149

Closed
tlakollo wants to merge 2565 commits into
dotnet:mainfrom
tlakollo:MoveLinkerToRuntime
Closed

Move linker to runtime#77149
tlakollo wants to merge 2565 commits into
dotnet:mainfrom
tlakollo:MoveLinkerToRuntime

Conversation

@tlakollo

@tlakollotlakollo commented Oct 18, 2022

Copy link
Copy Markdown
Contributor

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

tlakolloand 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
Comment threadsrc/tools/linker/src/tlens/TLens/LensesCollection.cs Outdated
@am11

ghost commented Oct 19, 2022

Copy link
Copy Markdown
Member

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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
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.

@tlakollotlakollo 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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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

Tlakolloand 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
@agockeagocke self-assigned this Oct 25, 2022
Comment threadeng/Subsets.props
</ItemGroup>

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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.

Comment threadglobal.json Outdated
tlakolloand 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

ghost commented Oct 27, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@ghostghost 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 freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

linkable-frameworkIssues associated with delivering a linker friendly frameworkNO-MERGEThe PR is not ready for merge yet (see discussion for detailed reasons)NO-REVIEWExperimental/testing PR, do NOT review it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

20 participants

@tlakollo@MartyIX@am11@joperezr@ViktorHofer@agocke@sbomer@jtschuster@vitek-karas@mrvoorhe@marek-safar@MichaelSimons@akoeplinger@iserrato@jkurdek@lbussell@Youssef1313@MichalStrehovsky@alexperovich@dkurepa
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Move linker to runtime - #77149

Closed
tlakollo wants to merge 2565 commits into
dotnet:mainfrom
tlakollo:MoveLinkerToRuntime
Closed

Move linker to runtime#77149
tlakollo wants to merge 2565 commits into
dotnet:mainfrom
tlakollo:MoveLinkerToRuntime

Conversation

@tlakollo

@tlakollotlakollo commented Oct 18, 2022

Copy link
Copy Markdown
Contributor

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

tlakolloand 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
Comment threadsrc/tools/linker/src/tlens/TLens/LensesCollection.cs Outdated
@am11

ghost commented Oct 19, 2022

Copy link
Copy Markdown
Member

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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
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.

@tlakollotlakollo 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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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

Tlakolloand 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
@agockeagocke self-assigned this Oct 25, 2022
Comment threadeng/Subsets.props
</ItemGroup>

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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.

Comment threadglobal.json Outdated
tlakolloand 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

ghost commented Oct 27, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@ghostghost 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 freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

linkable-frameworkIssues associated with delivering a linker friendly frameworkNO-MERGEThe PR is not ready for merge yet (see discussion for detailed reasons)NO-REVIEWExperimental/testing PR, do NOT review it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

20 participants

@tlakollo@MartyIX@am11@joperezr@ViktorHofer@agocke@sbomer@jtschuster@vitek-karas@mrvoorhe@marek-safar@MichaelSimons@akoeplinger@iserrato@jkurdek@lbussell@Youssef1313@MichalStrehovsky@alexperovich@dkurepa
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Move linker to runtime - #77149

Closed
tlakollo wants to merge 2565 commits into
dotnet:mainfrom
tlakollo:MoveLinkerToRuntime
Closed

Move linker to runtime#77149
tlakollo wants to merge 2565 commits into
dotnet:mainfrom
tlakollo:MoveLinkerToRuntime

Conversation

@tlakollo

@tlakollotlakollo commented Oct 18, 2022

Copy link
Copy Markdown
Contributor

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

tlakolloand 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
Comment threadsrc/tools/linker/src/tlens/TLens/LensesCollection.cs Outdated
@am11

ghost commented Oct 19, 2022

Copy link
Copy Markdown
Member

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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
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.

@tlakollotlakollo 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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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

Tlakolloand 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
@agockeagocke self-assigned this Oct 25, 2022
Comment threadeng/Subsets.props
</ItemGroup>

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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.

Comment threadglobal.json Outdated
tlakolloand 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

ghost commented Oct 27, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@ghostghost 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 freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

linkable-frameworkIssues associated with delivering a linker friendly frameworkNO-MERGEThe PR is not ready for merge yet (see discussion for detailed reasons)NO-REVIEWExperimental/testing PR, do NOT review it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

20 participants

@tlakollo@MartyIX@am11@joperezr@ViktorHofer@agocke@sbomer@jtschuster@vitek-karas@mrvoorhe@marek-safar@MichaelSimons@akoeplinger@iserrato@jkurdek@lbussell@Youssef1313@MichalStrehovsky@alexperovich@dkurepa
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Move linker to runtime - #77149

Closed
tlakollo wants to merge 2565 commits into
dotnet:mainfrom
tlakollo:MoveLinkerToRuntime
Closed

Move linker to runtime#77149
tlakollo wants to merge 2565 commits into
dotnet:mainfrom
tlakollo:MoveLinkerToRuntime

Conversation

@tlakollo

@tlakollotlakollo commented Oct 18, 2022

Copy link
Copy Markdown
Contributor

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

tlakolloand 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
Comment threadsrc/tools/linker/src/tlens/TLens/LensesCollection.cs Outdated
@am11

ghost commented Oct 19, 2022

Copy link
Copy Markdown
Member

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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
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.

@tlakollotlakollo 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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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

Tlakolloand 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
@agockeagocke self-assigned this Oct 25, 2022
Comment threadeng/Subsets.props
</ItemGroup>

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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.

Comment threadglobal.json Outdated
tlakolloand 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

ghost commented Oct 27, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@ghostghost 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 freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

linkable-frameworkIssues associated with delivering a linker friendly frameworkNO-MERGEThe PR is not ready for merge yet (see discussion for detailed reasons)NO-REVIEWExperimental/testing PR, do NOT review it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

20 participants

@tlakollo@MartyIX@am11@joperezr@ViktorHofer@agocke@sbomer@jtschuster@vitek-karas@mrvoorhe@marek-safar@MichaelSimons@akoeplinger@iserrato@jkurdek@lbussell@Youssef1313@MichalStrehovsky@alexperovich@dkurepa
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Move linker to runtime - #77149

Closed
tlakollo wants to merge 2565 commits into
dotnet:mainfrom
tlakollo:MoveLinkerToRuntime
Closed

Move linker to runtime#77149
tlakollo wants to merge 2565 commits into
dotnet:mainfrom
tlakollo:MoveLinkerToRuntime

Conversation

@tlakollo

@tlakollotlakollo commented Oct 18, 2022

Copy link
Copy Markdown
Contributor

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

tlakolloand 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
Comment threadsrc/tools/linker/src/tlens/TLens/LensesCollection.cs Outdated
@am11

ghost commented Oct 19, 2022

Copy link
Copy Markdown
Member

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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
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.

@tlakollotlakollo 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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
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

ghost commented Oct 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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

Tlakolloand 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
@agockeagocke self-assigned this Oct 25, 2022
Comment threadeng/Subsets.props
</ItemGroup>

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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.

Comment threadglobal.json Outdated
tlakolloand 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

ghost commented Oct 27, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@ghostghost 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 freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

linkable-frameworkIssues associated with delivering a linker friendly frameworkNO-MERGEThe PR is not ready for merge yet (see discussion for detailed reasons)NO-REVIEWExperimental/testing PR, do NOT review it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

20 participants

@tlakollo@MartyIX@am11@joperezr@ViktorHofer@agocke@sbomer@jtschuster@vitek-karas@mrvoorhe@marek-safar@MichaelSimons@akoeplinger@iserrato@jkurdek@lbussell@Youssef1313@MichalStrehovsky@alexperovich@dkurepa