Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ILLink.Substitutions.xml files for System.Private.CoreLib. #37615

Merged
merged 4 commits into from
Jun 10, 2020

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Jun 8, 2020

Ported the settings from https://github.com/mono/mono/blob/eaa32d13659f0a6b6b5e62ddb49af68b1f9efb6c/sdks/wasm/src/linker-subs.xml and split them out as appropriate to reduce duplication across the different platform builds.

Contributes to #31785

The only API proposed in #31785 that I didn't cover was System.Runtime.InteropService.OSPlatform since there isn't a good way of supporting that in the linker, AFAIK.

For a "Hello, World" wasm app, System.Private.CoreLib.dll sizes go from:

Build Size
master 1,534 KB
PR 1,475 KB
PR + Invariant Mode 1,456 KB

@ghost
Copy link

ghost commented Jun 8, 2020

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

@@ -145,6 +141,19 @@
<DefineConstants Condition="'$(TargetstvOS)' == 'true'">$(DefineConstants);TARGET_TVOS</DefineConstants>
</PropertyGroup>

<!-- ILLinker settings -->
<PropertyGroup>
<ILLinkClearInitLocals>true</ILLinkClearInitLocals>
Copy link
Member

Choose a reason for hiding this comment

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

FYI @layomia since you have a PR out changing this too.

@eerhardt
Copy link
Member Author

eerhardt commented Jun 9, 2020

Note that this work is blocked by the failing CI. The failure occurs when creating a Windows PDB for the portable PDB:

  System.Private.CoreLib -> F:\workspace\_work\1\s\artifacts\bin\coreclr\Windows_NT.x64.Checked\IL\System.Private.CoreLib.dll
  PDB0004: token 0x06001C30: Method containing local variables has no local signature
  PDB0004: token 0x06001C30: Method containing local variables has no local signature
  PDB0004: token 0x06001C31: Method containing local variables has no local signature
  PDB0004: token 0x06001C31: Method containing local variables has no local signature
  Error HRESULT E_FAIL has been returned from a call to a COM component.
F:\workspace\_work\1\s\.packages\microsoft.dotnet.arcade.sdk\5.0.0-beta.20280.1\tools\SymStore.targets(70,5): error MSB3073: The command ""F:\workspace\_work\1\s\.packages\microsoft.diasymreader.pdb2pdb\1.1.0-beta2-19575-01\tools\Pdb2Pdb.exe" "F:\workspace\_work\1\s\artifacts\bin\coreclr\Windows_NT.x64.Checked\IL\System.Private.CoreLib.dll" /out "F:\workspace\_work\1\s\artifacts\SymStore\Checked\System.Private.CoreLib\netcoreapp2.1\x64\System.Private.CoreLib.pdb" /srcsvrvar SRC_INDEX=public" exited with code 2. [F:\workspace\_work\1\s\src\coreclr\src\System.Private.CoreLib\System.Private.CoreLib.csproj]
##[error].packages\microsoft.dotnet.arcade.sdk\5.0.0-beta.20280.1\tools\SymStore.targets(70,5): error MSB3073: (NETCORE_ENGINEERING_TELEMETRY=Build) The command ""F:\workspace\_work\1\s\.packages\microsoft.diasymreader.pdb2pdb\1.1.0-beta2-19575-01\tools\Pdb2Pdb.exe" "F:\workspace\_work\1\s\artifacts\bin\coreclr\Windows_NT.x64.Checked\IL\System.Private.CoreLib.dll" /out "F:\workspace\_work\1\s\artifacts\SymStore\Checked\System.Private.CoreLib\netcoreapp2.1\x64\System.Private.CoreLib.pdb" /srcsvrvar SRC_INDEX=public" exited with code 2.

Debugging into Pdb2Pdb.exe, I see the first problem is that we are trimming away locals from this method:

public static int PopCount(uint value)
{
if (Popcnt.IsSupported)
{
return (int)Popcnt.PopCount(value);
}
if (AdvSimd.Arm64.IsSupported)
{
// PopCount works on vector so convert input value to vector first.
// Vector64.CreateScalar(uint) generates suboptimal code by storing and
// loading the result to memory.
// See https://github.com/dotnet/runtime/issues/35976 for details.
// Hence use Vector64.Create(ulong) to create Vector64<ulong> and operate on that.
Vector64<ulong> input = Vector64.Create((ulong)value);
Vector64<byte> aggregated = AdvSimd.Arm64.AddAcross(AdvSimd.PopCount(input.AsByte()));
return aggregated.ToScalar();
}

The if (AdvSimd.Arm64.IsSupported) branch is getting trimmed, but it looks like the local variables are not getting removed from the portable .pdb. Here is the code after trimming:

        public static int PopCount(uint value)
        {
            if (Popcnt.IsSupported)
            {
                return (int)Popcnt.PopCount(value);
            }
            !AdvSimd.Arm64.IsSupported;
            return BitOperations.<PopCount>g__SoftwareFallback|9_0(value);
        }

@vitek-karas @marek-safar - I logged dotnet/linker#1260 for this.

After that, Pdb2Pdb is trying to work on this method:

private static double GetNumericValueNoBoundsCheck(uint codePoint)
{
nuint offset = GetNumericGraphemeTableOffsetNoBoundsChecks(codePoint);
ref byte refToValue = ref Unsafe.AddByteOffset(ref MemoryMarshal.GetReference(NumericValues), offset * 8 /* sizeof(double) */);
// 'refToValue' points to a little-endian 64-bit double.
if (BitConverter.IsLittleEndian)
{
return Unsafe.ReadUnaligned<double>(ref refToValue);
}
else
{
ulong temp = Unsafe.ReadUnaligned<ulong>(ref refToValue);
temp = BinaryPrimitives.ReverseEndianness(temp);
return Unsafe.As<ulong, double>(ref temp);
}
}

But it is throwing a COMException when calling OpenScope:

>	Microsoft.DiaSymReader.Native.amd64.dll!SymWriter::OpenScope(unsigned int,unsigned int *)	Unknown
 	[Managed to Native Transition]	
 	Microsoft.DiaSymReader.dll!Microsoft.DiaSymReader.SymUnmanagedWriterImpl.OpenScope(int startOffset) Line 261	C#
 	Microsoft.DiaSymReader.Converter.dll!Microsoft.DiaSymReader.Tools.PdbConverterPortableToWindows.Convert.__OpenScope|6_6(int startOffset, int endOffset, ref Microsoft.DiaSymReader.Tools.PdbConverterPortableToWindows.<>c__DisplayClass6_0 value, ref Microsoft.DiaSymReader.Tools.PdbConverterPortableToWindows.<>c__DisplayClass6_1 value) Line 295	C#
 	Microsoft.DiaSymReader.Converter.dll!Microsoft.DiaSymReader.Tools.PdbConverterPortableToWindows.Convert(System.Reflection.PortableExecutable.PEReader peReader, System.Reflection.Metadata.MetadataReader pdbReader, Microsoft.DiaSymReader.SymUnmanagedWriter pdbWriter, Microsoft.DiaSymReader.Tools.PortablePdbConversionOptions options) Line 322	C#
 	Microsoft.DiaSymReader.Converter.dll!Microsoft.DiaSymReader.Tools.PdbConverter.ConvertPortableToWindows(System.Reflection.PortableExecutable.PEReader peReader, System.Reflection.Metadata.MetadataReader pdbReader, Microsoft.DiaSymReader.SymUnmanagedWriter pdbWriter, Microsoft.DiaSymReader.Tools.PortablePdbConversionOptions options) Line 189	C#
 	Microsoft.DiaSymReader.Converter.dll!Microsoft.DiaSymReader.Tools.PdbConverter.ConvertPortableToWindows(System.Reflection.PortableExecutable.PEReader peReader, System.Reflection.Metadata.MetadataReader pdbReader, System.IO.Stream targetPdbStream, Microsoft.DiaSymReader.Tools.PortablePdbConversionOptions options) Line 171	C#
 	Pdb2Pdb.exe!Microsoft.DiaSymReader.Tools.Pdb2Pdb.Convert(Microsoft.DiaSymReader.Tools.Pdb2Pdb.Args args) Line 268	C#
 	Pdb2Pdb.exe!Microsoft.DiaSymReader.Tools.Pdb2Pdb.Main(string[] args) Line 59	C#

From doing a little debugging, it appears that the top scope is getting closed in Microsoft.DiaSymReader.Native.amd64.dll, and the native code is returning E_FAIL when trying to open the scope again. It seems that Pdb2Pdb is getting in some invalid state right before this.

I've opened dotnet/symreader-converter#174 for the Pdb2Pdb crash, which is blocking clean CI.

@eerhardt eerhardt force-pushed the CoreLibLinkerSubstitutions branch from d4c4c8a to 923a634 Compare June 9, 2020 22:11
@eerhardt
Copy link
Member Author

eerhardt commented Jun 9, 2020

I've removed the $(Platform)s that aren't wasm for now, until dotnet/linker#1260 is addressed. This should allow this change to move forward.

@eerhardt eerhardt merged commit ee12b54 into dotnet:master Jun 10, 2020
@eerhardt eerhardt deleted the CoreLibLinkerSubstitutions branch June 10, 2020 16:24
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants