[wasm][R2R] Request flat layout for composite image on WebAssembly - #131354
Conversation
OpenR2RFromPE requested PEImageLayout::LAYOUT_LOADED for the composite R2R container. On WebAssembly (PEIMAGE_FLAT_LAYOUT_ONLY) there is no OS-loadable mapped layout, so the loaded-layout branch in GetOrCreateLayoutInternal is compiled out and the request degrades to nothing, tripping the _ASSERTE(bIsFlatLayoutSuitable) consistency check in Debug builds (and only silently degrading in Release). Every other wasm image open uses LAYOUT_ANY. Request LAYOUT_FLAT explicitly on TARGET_WASM so a webcil composite loads via the flat layout, matching the rest of the wasm image-load paths. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 50ddfb2c-b6f8-4847-81e7-44d37d44a175
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Updates CoreCLR’s composite R2R loading path on WebAssembly to request a flat PE layout explicitly when opening the composite image, avoiding a debug assert caused by requesting a loaded/mapped layout on a flat-only platform.
Changes:
- Under
TARGET_WASM, callPEImage::GetOrCreateLayout(PEImageLayout::LAYOUT_FLAT)instead ofLAYOUT_LOADEDwhen opening the composite image. - Add in-code rationale explaining why the flat layout is required on WASM for webcil composites.
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
jkoritzinsky
left a comment
There was a problem hiding this comment.
Is there a reason we aren't using the "native format composite image" path we added for Mach-O for WASM?
Use flat PE image layout on flat-layout-only targets and record the NativeImageEagerFixups to PregeneratedStringThunks lock order exposed by composite eager fixups. The flat-layout correction is based on dotnet#131354. Co-authored-by: Larry Ewing <lewing@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 44f4b4ea-409c-4a52-89a0-f488a89488b9
Right now wasm composite mode is being used as a minimum delta between the browser (webcil) target and wasi which effectively requires composite mode. Once wasm r2r is running tests in CI we can revisit the format. |
|
@lewing, have you validated that a composite image on the browser path works with this change as well as your wasi approach? |
yes, it is both necessary and sufficient to wasm webcil r2r composite mode working on browser right now (just double checked) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/coreclr/vm/nativeimage.cpp:145
- The
#endiftrailing comment doesn’t match the corresponding#ifdef(PEIMAGE_FLAT_LAYOUT_ONLY). The repo’s native-code conventions call out matching#endifcomments exactly to avoid confusion when refactoring preprocessor blocks.
#endif // TARGET_WASM
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
…-1 tests (#131883) ## Summary PR #131110 enabled CoreCLR browser-WASM out-of-process runtime tests. The newly enabled priority-1 coverage exposed eight unique failures. Four were composite WebCIL/ReadyToRun failures tracked by #131767 and are fixed on `main` by #131354. This change removes their temporary `ActiveIssue` annotations and Crossgen2 bypasses, restoring the intended R2R coverage. The unrelated field-layout suppression for #131421 remains. The other four tests are narrowly gated: - `GC/API/GC/GetTotalAllocatedBytesServerGC` is excluded on CoreCLR browser, which does not provide Server GC. - `GC/API/GC/GetTotalMemoryConcurrent` uses `PlatformDetection.IsMultithreadingSupported` through `ConditionalFact`. - `baseservices/threading/regressions/2164/foreground-shutdown` retains its explicit `Main` and returns success before creating a thread when multithreading is unsupported. - `JIT/jit64/regress/ddb/113574` is excluded only on CoreCLR browser because its multi-billion-iteration optimizing-JIT workload exceeds the browser interpreter's CI timeout. The runtime capability checks skip single-threaded browser and WASI while preserving desktop and threaded-browser coverage. The Server GC and interpreter-workload exclusions are tracked in #131321. ## Testing Built the Checked browser runtime from `main`, including #131354, then built and ran the complete priority-1 browser-WASM Node suite: | Scope | Total | Passed | Failed | Skipped | | --- | ---: | ---: | ---: | ---: | | Full suite | 14,745 | 13,823 | 0 | 922 | | Out-of-process subset | 672 | 560 | 0 | 112 | The four former WebCIL/R2R failures now pass: - `Regressions/coreclr/GitHub_49826/test49826` - `Regressions/coreclr/GitHub_49982/test49982` - `readytorun/tests/genericsload/callgenericctor` - `readytorun/tests/genericsload/usegenericfield` Additional focused validation for the runtime multithreading gates: - 8 MSBuild evaluations across desktop, threaded browser, single-threaded browser, and single-threaded WASI; neither project is permanently marked unsupported. - 2 targeted Checked browser builds, both with 0 warnings and 0 errors. - 2 single-threaded browser wrapper runs, both passing with expected/actual exit code 100. - Native `foreground-shutdown` execution exited with 100 after 2.28 seconds, confirming that its foreground thread still keeps the process alive after `Main` returns. > [!NOTE] > This pull request was created with assistance from GitHub Copilot. Copilot-Session: 19e61792-38f9-4f80-8d38-3e3e12d1275e
…otnet#131354) # Honour ErrorOnUnknownConfiguration on value conversion fail Fixes dotnet#98231 ## Problem `BinderOptions.ErrorOnUnknownConfiguration` is documented as controlling whether the binder throws "when converting a value", but it was only honoured on some of the paths that can fail: | Failure | Flag honoured? | |---|---| | Unknown configuration key | Yes | | Leaf value with no `TypeConverter` at all | Yes | | Collection item that fails to convert | Yes | | **Leaf value whose `TypeConverter` throws** | **No, always threw** | So a single malformed value in an otherwise valid configuration source would tear down binding even for callers that had deliberately left the flag at its default: ```csharp // appsettings.json: { "Timeout": "not-a-number" } var options = config.Get<MyOptions>(); // threw InvalidOperationException ``` This is inconsistent both with the documented contract on the property and with the sibling "no converter" case a few lines away in the same method, which already returned quietly. ## Fix Gate the rethrow on the flag, in both the reflection binder and the source generator. ### Reflection binder `BindInstance` no longer rethrows the conversion error unconditionally; it returns and leaves the binding point untouched, so the member keeps whatever value it already had. ### Source generator The emitted `ParseX(value, path)` helpers become `TryParseX(value, path, errorOnFailedBinding, out result)`: ```csharp public static bool TryParseInt(string value, string? path, bool errorOnFailedBinding, out int result) { try { result = int.Parse(value, NumberStyles.Integer, CultureInfo.InvariantCulture); return true; } catch when (!errorOnFailedBinding) { result = default; return false; } catch (Exception exception) { throw new InvalidOperationException($"Failed to convert configuration value '{value ?? "null"}' at '{path}' to type '{typeof(int)}'.", exception); } } ``` Call sites pass `binderOptions?.ErrorOnUnknownConfiguration is true`, except for two cases that must always report the failure: - `GetValueCore`, since `GetValue<T>` has no `BinderOptions` overload. - Constructor parameters with no declared default, which are unsatisfiable either way. These keep throwing the accurate `Failed to convert configuration value ...` rather than degrading to `... has no matching config`. A try/catch at each call site was considered instead, but it would also have swallowed nested binding errors in the dictionary case; a `Func<>`-based helper would have allocated. ## Behaviour change This is a breaking change and will need a breaking-change doc. | Scenario | Before | After | |---|---|---| | `Bind` / `Get<T>` with an unconvertible scalar, default options | throws | member keeps its existing value | | `GetSection("x").Get<int>()` with a bad value | throws | `default(int)` | | Same, with `ErrorOnUnknownConfiguration = true` | throws | throws (unchanged) | | `GetValue<T>` with a bad value | throws | throws (unchanged) | | Required constructor parameter with a bad value | throws | throws (unchanged) | | Constructor parameter with a declared default and a bad value | throws | falls back to the default | The `Get`/`Bind` versus `GetValue` asymmetry is deliberate and follows the design agreed in the issue, but it is user-visible. ## Also in this change - Fixes a latent generator bug: a primitive section with an empty value used to fall through `GetCore` to `throw new NotSupportedException("Unable to bind to type ...: generator did not detect the type as input.")`. It now returns `null`, matching the reflection binder. - Fixes a hard-coded path separator in `GeneratorTests.Helpers.cs` that stopped `/p:UpdateBaselines=true` working on non-Windows hosts. - Un-gates two `CollectionsBindingWithErrorOnUnknownConfiguration` tests that were restricted to the reflection binder; they now pass under source generation too. ## Testing - 4 existing tests that asserted the old unconditional throw now opt in with `ErrorOnUnknownConfiguration = true`. - New regression tests cover both flag states, the `GetValue` carve-out, and constructor parameters with and without declared defaults. All run in both binder modes. - 111 of the 120 generator baselines regenerated (`netcoreapp` and `net462`, `Version0` and `Version1`). Some `net462` `Version0` files additionally pick up unrelated drift corrections that had accumulated on `main`. ``` Microsoft.Extensions.Configuration.Binder.Tests 356 passed, 0 failed Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests 433 passed, 0 failed, 26 skipped ```
On WebAssembly, PE images are flat-layout only (
PEIMAGE_FLAT_LAYOUT_ONLY). A webcil composite R2R image has no OS-loadable/mapped layout, so requestingLAYOUT_LOADEDyields no layout and trips the flat-suitability assert inGetOrCreateLayoutInternal. Request the flat layout explicitly underTARGET_WASMwhen obtaining the composite image layout inNativeImage::Open.Guarded by
#ifdef TARGET_WASM, so no behavior change on any other target.Note
This pull request was authored with assistance from GitHub Copilot.