Skip to content

[release/6.0] Build improvements based on feedback - #59671

Merged
steveisok merged 4 commits into
dotnet:release/6.0from
radical:fixes-6.0
Oct 6, 2021
Merged

[release/6.0] Build improvements based on feedback#59671
steveisok merged 4 commits into
dotnet:release/6.0from
radical:fixes-6.0

Conversation

@radical

@radicalradical commented Sep 27, 2021

Copy link
Copy Markdown
Member

Backport of #59391

This contains:

  • Fix to handle pinvokes with function pointers, which resolves issue found testing new customer scenario with sqlite
  • Fixes version of System.Reflection.MetadataLoadContext bundled with WasmBuilderApp task, to use the same as other tasks
  • fix for an incremental build issue where the compiled native .o files would
    incorrectly get deleted after linking. And that would cause them to be
    recompiled, unnecessarily increasing the build time.
  • Change the default optimization flag used when building for Debug config
    from -Oz to -O1 to improve development loop experience.

Customer impact

  • Fixes native library customer scenario for some cases (sqlite)
  • Fixes VS build issue with newer library versions.
  • Improves development loop experience (time spent debug builds for building a blazorwasm template app cut in half).

Testing

Manual testing, existing tests, and specific new tests.

Risk

Medium. Improves build experience.

@radicalradical added arch-wasm WebAssembly architecture area-Build-mono labels Sep 27, 2021
@ghost

Copy link
Copy Markdown

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details
  • Refactor to allow fast-path

  • implement fast-path for MonoAOTCompiler when nothing has changed

  • re-enable some tests that got disabled by mistake

Author:radical
Assignees:-
Labels:

arch-wasm, area-Build-mono

Milestone:-

@radicalradical changed the title [release/6.0] MonoAOTCompiler: detect when nothing has changed, and skip any precompiling (#58979)[release/6.0] Build improvements based on feedbackSep 27, 2021
@radical

Copy link
Copy Markdown
MemberAuthor

The wasm test failures are because of a fix missing in sdk's release/6.0.1xx branch. Opened a PR for that - dotnet/sdk#21453 .

@radical

Copy link
Copy Markdown
MemberAuthor

Build MacCatalyst x64 Release AllSubsets_Mono timing out. Rest are all green.

radicaland others added 3 commits October 2, 2021 05:09
- Fix for a incremental build issue
- Change native build defaults for `Debug` config, to make it faster (thanks @SteveSandersonMS, @mattleibow)
- MonoAOTCompiler:
- Skip unmanaged assemblies, and emit a warning
- PInvokeTableGenerator:
- Fix to not fail in presence of native variadic functions, and pinvokes with function pointers (eg. used by sqlite)
Co-authored-by: Larry Ewing <lewing@microsoft.com>
(cherry picked from commit 80f015d)
On VS2022 Preview 5, building a blazorwasm project which used sqlite
native library crashed:
```
System.BadImageFormatException: Expected signature header for 'Property' or 'Method', but found '9' (0x09).
at System.Reflection.Metadata.Ecma335.SignatureDecoder`2.CheckMethodOrPropertyHeader(SignatureHeader header)
at System.Reflection.Metadata.Ecma335.SignatureDecoder`2.DecodeMethodSignature(BlobReader& blobReader)
at System.Reflection.Metadata.Ecma335.SignatureDecoder`2.DecodeType(BlobReader& blobReader, Boolean allowlypeSpecifications, Int32 typeCode)
at System.Reflection.Metadata.Ecma335.SignatureDecoder`2.DecodeMethodSignature(BlobReader& blobReader)
at System.Reflection.Metadata.MethodDefinition.DecodeSignature[TType, TGenericContext] (ISignatureTypeProvider`2 provider, TGenericContext genericCo
at System.Reflection.TypeLoading.Ecma.EcmaMethodDecoder.SpecializeMethodSigStrings(TypeContext& typeContext)
at System.Reflection.TypeLoading.RoDefinitionMethod`1.ComputeMethodSigStrings()
at System.Reflection.TypeLoading.RoMethod.ToString()
at System.Text.StringBuilder.AppendFormatHelper(IFormatProvider provider, String format, ParamsArray args)
at System.String.FormatHelper(IFormatProvider provider, String format, ParamsArray args)
at System.String.Format(String format, Object arg0, Object arg1, Object arg2)
at PInvokeComparer.GetHashCode(PInvoke pinvoke)
at ...
at System.Ling.Enumerable.<DistinctIterator>d__64`1.MoveNext()
at System.Ling.Buffer`1..ctor(IEnumerable`1 source)
at System.Ling.Enumerable.ToArray [TSource] (IEnumerable`1 source)
at PInvokeTableGenerator.EmitPInvokeTable(StreamWriter w, Dictionary`2 modules, List`1 pinvokes)
at PInvokeTableGenerator.GenPInvokeTable(String[] pinvokeModules, String[] assemblies)
at PInvokeTableGenerator.Execute()
```
- This is crashing in a new code path(`PInvokeComparer.GetHashCode`) that
invokes `methodInfo.ToString()` for methods with `DllImport` attribute.
- It fails for `EnumCalendarInfo`, which has a function pointer parameter,
which `System.Reflection.MetadataLoadContext`(SRMLC) does not support, and is
explicitly worked around in the code.
- It usually fails with on `methodInfo.GetParameters()` with
`NotSupportedException`, and `methodInfo.ToString()` works fine
The difference seems to be because the version of `SRMLC` being bundled
with the task is `1.4.5.0`, which is too old.
The fix is to use the same version of SRMLC, and
`System.Reflection.Metadata`(SRM) used by other tasks, which is `5.0.0`
currently.
Also, this mirrors some other reference changes from dotnet#59720
- don't include msbuild assemblies with the task
- and explicitly reference `System.Text.Json`, and
`System.Threading.Tasks.Extensions` to be sure about the version that
we'll include for net472
The following in a table of the assemblies that are bundled with the task, for `net472`, and `net6.0`.
The `new` has fewer assemblies.
| Path | Old | New |
|---------------------------------------------------------------|-------------|-------------|
| net472/Microsoft.Bcl.AsyncInterfaces.dll | 1.0.0.0 | 5.0.0.0 |
| net472/Microsoft.Build.Framework.dll | 15.1.0.0 | |
| net472/Microsoft.Build.Tasks.Core.dll | 15.1.0.0 | |
| net472/Microsoft.Build.Utilities.Core.dll | 15.1.0.0 | |
| net472/Microsoft.Build.dll | 15.1.0.0 | |
| net472/Microsoft.NET.StringTools.dll | 1.0.0.0 | |
| net472/Microsoft.VisualStudio.Setup.Configuration.Interop.dll | 1.0.0.0 | |
| net472/System.Buffers.dll | 4.0.3.0 | 4.0.3.0 |
| net472/System.Collections.Immutable.dll | 5.0.0.0 | 5.0.0.0 |
| net472/System.Configuration.ConfigurationManager.dll | 4.0.3.0 | |
| net472/System.Memory.dll | 4.0.1.1 | 4.0.1.1 |
| net472/System.Numerics.Vectors.dll | 4.1.4.0 | 4.1.4.0 |
| net472/System.Reflection.Metadata.dll | 1.4.5.0 | 5.0.0.0 |
| net472/System.Reflection.MetadataLoadContext.dll | 4.0.1.1 | 5.0.0.0 |
| net472/System.Resources.Extensions.dll | 4.0.0.0 | |
| net472/System.Runtime.CompilerServices.Unsafe.dll | 5.0.0.0 | 5.0.0.0 |
| net472/System.Security.AccessControl.dll | 4.1.3.0 | |
| net472/System.Security.Permissions.dll | 4.0.3.0 | |
| net472/System.Security.Principal.Windows.dll | 4.1.3.0 | |
| net472/System.Text.Encodings.Web.dll | 4.0.5.0 | 5.0.0.0 |
| net472/System.Text.Json.dll | 4.0.1.0 | 5.0.0.0 |
| net472/System.Threading.Tasks.Dataflow.dll | 4.6.3.0 | |
| net472/System.Threading.Tasks.Extensions.dll | 4.2.0.0 | 4.2.0.1 |
| net472/System.ValueTuple.dll | 4.0.3.0 | 4.0.3.0 |
| net472/WasmAppBuilder.dll | 6.0.0.0 | 6.0.0.0 |
| | | |
| net6.0/System.Reflection.MetadataLoadContext.dll | 4.0.1.1 | 5.0.0.0 |
| net6.0/WasmAppBuilder.dll | 6.0.0.0 | 6.0.0.0 |
@lewinglewing added Servicing-consider Issue for next servicing release review Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Oct 5, 2021
@lewing

Copy link
Copy Markdown
Member

approved in email

@steveisok
steveisok merged commit 9630d45 into dotnet:release/6.0Oct 6, 2021
@radical
radical deleted the fixes-6.0 branch October 6, 2021 22:22
@ghostghost locked as resolved and limited conversation to collaborators Nov 6, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

arch-wasmWebAssembly architecturearea-Build-monoServicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@radical@lewing@steveisok