Add step to run linker on entire runtime pack during libraries build #40172
Conversation
|
Tagging subscribers to this area: @safern, @ViktorHofer |
layomia
force-pushed
the
link_runtimepack
branch
from
2a7c268
to
b473edb
Compare
layomia
force-pushed
the
link_runtimepack
branch
from
2666100
to
5cf9bd2
Compare
|
Will this obsolete linker run on individual libraries? |
No, it doesn't. We still run the linker on individual libraries, and that is what ships in the runtimepack. See #39133 (comment). And the reasoning why we still run the linker on the individual libraries is listed in #31712. |
Does it mean we won't get correct warnings for the scenarios which use linker by default? |
I'm not sure I follow this question. Can you elaborate? The scenarios which use linker by default will use the assemblies that are contained in the runtimepack. This change is to pass those same libraries into the linker, now with warnings enabled. So we are running the linker in 2 different locations during the dotnet/runtime build. Once when building the individual library, like we have always been - warnings disabled. Once at the end of the build against the whole runtimepack - warnings enabled. |
layomia
force-pushed
the
link_runtimepack
branch
from
c06ec1d
to
aa9cd3a
Compare
| --> | ||
| <ILLinkArgs>$(ILLinkArgs) --nowarn IL2006;IL2008;IL2009;IL2012;IL2025;IL2026;IL2035;IL2041</ILLinkArgs> | ||
| <ILLinkArgs>$(ILLinkArgs) --nowarn IL2006;IL2008;IL2009;IL2012;IL2025;IL2026;IL2035;IL2050</ILLinkArgs> |
There was a problem hiding this comment.
Why the change from IL2041 to IL2050? Nothing in this change should have changed what warnings are produced for the individual library build. So I think both 2041 and 2050 can be removed here.
There was a problem hiding this comment.
I started getting 2050 after syncing with master at the beginning of this PR. Some new code since the initial suppressions check in must have been added which causes these errors:
ILLink : error IL2050: System.Runtime.InteropServices.Marshal.CreateBindCtx(UInt32,IBindCtx&): P/invoke method 'System.Runtime.InteropServices.Marshal.CreateBindCtx(UInt32,IBindCtx&)' declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed. [D:\repos\dotnet_runtime_2\src\coreclr\src\System.Private.CoreLib\System.Private.CoreLib.csproj]
ILLink : error IL2050: System.Runtime.InteropServices.Marshal.CreateBindCtx(UInt32,IBindCtx&): P/invoke method 'System.Runtime.InteropServices.Marshal.CreateBindCtx(UInt32,IBindCtx&)' declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed. [D:\repos\dotnet_runtime_2\src\coreclr\src\System.Private.CoreLib\System.Private.CoreLib.csproj]
ILLink : error IL2050: System.Runtime.InteropServices.Marshal.MkParseDisplayName(IBindCtx,String,UInt32&,IMoniker&): P/invoke method 'System.Runtime.InteropServices.Marshal.MkParseDisplayName(IBindCtx,String,UInt32&,IMoniker&)' declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed. [D:\repos\dotnet_runtime_2\src\coreclr\src\System.Private.CoreLib\System.Private.CoreLib.csproj]
ILLink : error IL2050: System.Runtime.InteropServices.Marshal.MkParseDisplayName(IBindCtx,String,UInt32&,IMoniker&): P/invoke method 'System.Runtime.InteropServices.Marshal.MkParseDisplayName(IBindCtx,String,UInt32&,IMoniker&)' declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed. [D:\repos\dotnet_runtime_2\src\coreclr\src\System.Private.CoreLib\System.Private.CoreLib.csproj]
ILLink : error IL2050: System.Runtime.InteropServices.Marshal.BindMoniker(IMoniker,UInt32,Guid&,Object&): P/invoke method 'System.Runtime.InteropServices.Marshal.BindMoniker(IMoniker,UInt32,Guid&,Object&)' declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed. [D:\repos\dotnet_runtime_2\src\coreclr\src\System.Private.CoreLib\System.Private.CoreLib.csproj]
ILLink : error IL2050: System.Runtime.InteropServices.Marshal.BindMoniker(IMoniker,UInt32,Guid&,Object&): P/invoke method 'System.Runtime.InteropServices.Marshal.BindMoniker(IMoniker,UInt32,Guid&,Object&)' declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed. [D:\repos\dotnet_runtime_2\src\coreclr\src\System.Private.CoreLib\System.Private.CoreLib.csproj]
ILLink : error IL2050: Microsoft.VisualBasic.CompilerServices.UnsafeNativeMethods.VarNumFromParseNum(Byte[],Byte[],Int32): P/invoke method 'Microsoft.VisualBasic.CompilerServices.UnsafeNativeMethods.VarNumFromParseNum(Byte[],Byte[],Int32)' declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed. [D:\repos\dotnet_runtime_2\src\libraries\Microsoft.VisualBasic.Core\src\Microsoft.VisualBasic.Core.vbproj]
ILLink : error IL2050: Microsoft.VisualBasic.CompilerServices.UnsafeNativeMethods.VarNumFromParseNum(Byte[],Byte[],Int32): P/invoke method 'Microsoft.VisualBasic.CompilerServices.UnsafeNativeMethods.VarNumFromParseNum(Byte[],Byte[],Int32)' declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed. [D:\repos\dotnet_runtime_2\src\libraries\Microsoft.VisualBasic.Core\src\Microsoft.VisualBasic.Core.vbproj]
ILLink : error IL2050: Microsoft.VisualBasic.CompilerServices.UnsafeNativeMethods.VariantChangeType(Object&,Object&,Int16,Int16): P/invoke method 'Microsoft.VisualBasic.CompilerServices.UnsafeNativeMethods.VariantChangeType(Object&,Object&,Int16,Int16)' declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed. [D:\repos\dotnet_runtime_2\src\libraries\Microsoft.VisualBasic.Core\src\Microsoft.VisualBasic.Core.vbproj]
ILLink : error IL2050: Microsoft.VisualBasic.CompilerServices.UnsafeNativeMethods.VariantChangeType(Object&,Object&,Int16,Int16): P/invoke method 'Microsoft.VisualBasic.CompilerServices.UnsafeNativeMethods.VariantChangeType(Object&,Object&,Int16,Int16)' declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed. [D:\repos\dotnet_runtime_2\src\libraries\Microsoft.VisualBasic.Core\src\Microsoft.VisualBasic.Core.vbproj]
0 Warning(s)
10 Error(s)
2041 doesn't show up anymore following the sync hence removed.
There was a problem hiding this comment.
Looks like that came in last week with dotnet/linker#1382.
There was a problem hiding this comment.
Yes, we now detect COM usage and mark it as unsafe. Apps that end up referencing these methods after trimming will always warn.
Running linker against the whole runtime pack will produce different output than running linker when building individual libraries? Why are we doing the first run which is now redundant and only slows down the build? |
| <ILLinkArgs>$(ILLinkArgs) --nowarn IL2006;IL2009;IL2025;IL2026;IL2035;IL2050</ILLinkArgs> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- Retrieve CoreLib's targetpath via GetTargetPath as it isn't binplaced yet. --> |
There was a problem hiding this comment.
I'm not sure I understand this. Why can't we use $(MicrosoftNetCoreAppRuntimePackNativeDir) instead?
There was a problem hiding this comment.
$(MicrosoftNetCoreAppRuntimePackNativeDir) does not have S.P.CoreLib present when the new target is running. @safern's thought on it was that the directory gets populated in pretest.proj as a preparation for being able to run tests.
fwiw, wasm.targets takes a similar approach -
runtime/src/mono/wasm/wasm.targets
Lines 26 to 30 in b77b242
There was a problem hiding this comment.
My naive thinking is that there should be somewhere in the build (that doesn't rely on tests) that we can hook in where the entire runtimepack is built. But I'm not seeing where that place is. Maybe @safern @ViktorHofer @joperezr or @ericstj might know.
But for now, what you have works so let's go with that.
There was a problem hiding this comment.
SPC and the runtime gets binplaced into libraries runtime pack and testhost as part of runtime.depproj.
Runtime.depproj is built as part of pretest.proj which is the last step of the libs subset. Which happens after libs.src that is the subset that builds src.proj. That is why wasm.targets also follows that approach.
FWIW we could introduce a common target to get SPC target path instead of duplicating the logic to call MSBuild on it with GetTargetPath target.
There was a problem hiding this comment.
FWIW we could introduce a common target to get SPC target path instead of duplicating the logic to call MSBuild on it with GetTargetPath target.
It feels to me that the correct fix here would be to fully populate the runtimepack earlier, and not make it dependent on testing.
There was a problem hiding this comment.
It feels to me that the correct fix here would be to fully populate the runtimepack earlier, and not make it dependent on testing.
I agree with @eerhardt, we have talked about this couple times already. I believe we should binplace the runtime artifacts incrementally after they are built (CoreLib, coreclr.dll, etc). For CI we just need a target that re-binplaces the runtime artifacts (to preserve the existing behavior which allows to build coreclr and libraries in parallel).
The reason why having the runtime binplace at pretest is handy, is for inner loop development. For example if I update code in SPC after a vertical build, in order to run tests against the new SPC, I don't have to do a full libraries vertical, I can just do: build libs.pretest -rc
I don't understand why binplacing the runtime artifacts in pretest is handy for inner loop development instead of binplacing the runtime assets at the time they are (re-)built, which doesn't require a libraries rebuild.
There was a problem hiding this comment.
I don't understand why binplacing the runtime artifacts in pretest is handy for inner loop development instead of binplacing the runtime assets at the time they are (re-)built, which doesn't require a libraries rebuild.
We've also talked about this a couple of times. If you build for example System.Runtime which has a P2P to SPC, and don't specify RuntimeConfiguration as an argument you would potentially be binplacing a Debug build of SPC when probably you built the runtime for release, this could cause a mismatch in between the native bits and the managed bits of coreclr which is not recommended:
#38885 (comment)
So we should be careful on how we do that.
There was a problem hiding this comment.
Btw, I agree we should change this if possible, I'm just saying we should be really careful how and where we put this binplacing to happen.
Also, I wouldn't like removing the build.pretest binplacing the runtime as it is handy for CI as well as I explained above... if we remove that, then provide another hook to binplace the runtime as an individual step.
There was a problem hiding this comment.
We've also talked about this a couple of times. If you build for example System.Runtime which has a P2P to SPC, and don't specify RuntimeConfiguration as an argument you would potentially be binplacing a Debug build of SPC when probably you built the runtime for release, this could cause a mismatch in between the native bits and the managed bits of coreclr which is not recommended:
#38885 (comment)
Btw, I agree we should change this if possible, I'm just saying we should be really careful how and where we put this binplacing to happen.
I think we all agree that we should change when we binplace the runtime artifacts and I agree that it should be done with the highest diligence. We should do this as part of #38034.
Also, I wouldn't like removing the build.pretest binplacing the runtime as it is handy for CI as well as I explained above... if we remove that, then provide another hook to binplace the runtime as an individual step.
That's what I meant with:
For CI we just need a target that re-binplaces the runtime artifacts (to preserve the existing behavior which allows to build coreclr and libraries in parallel).
There was a problem hiding this comment.
That's what I meant with:
I somehow missed that part 🤦
We should do this as part of #38034.
Sounds good.
Yes, it will produce different warnings. Please read the discussion here #39133 (comment).
It is not redundant. Please see read the discussion in #31712 for why we are doing the first run. |
|
Curious, how much time does this additional step take? |
| @@ -49,6 +49,9 @@ | |||
| Properties="$(TraversalGlobalProperties)" /> | |||
| </Target> | |||
|
|
|||
| <Import Condition="'$(BuildTargetFramework)' == '$(NetCoreAppCurrent)'" | |||
There was a problem hiding this comment.
nit: "$(BuildNetCoreAppVertical) == true"
There was a problem hiding this comment.
The BuildNetCoreAppVertical property does not appear to be set at this point in the build.
For now, we throw them away - they aren't used. We are only running the linker to get the warnings/analysis. |
|
@ViktorHofer, thanks for the feedback. I'll address them shortly in a follow up PR. |
A local run took 13.366s. |
…otnet#40172) * Add step to run linker on entire runtime pack during libraries build * Address feedback * Only run for NetCoreAppCurrent * Include appropriate S.P.CoreLib * Address review feedback * Add common property group for shared linker args; remove unneeded changes from PR * Rename illink-sharedframeworks.targets to illink-sharedframework.targets * Address review feedback
ghost
locked as resolved and limited conversation to collaborators
Contributes to #38033.
The next step following this PR is to suppress individual warnings using baseline xml files. This can be done following dotnet/linker#1386.
cc @mateoatr @vitek-karas @MichalStrehovsky @sbomer