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.

Sign up for GitHub

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

Jump to bottom

Ensure MetadataEnumResult is sufficiently updated by MetaDataImport::… #56756

Merged
merged 1 commit into from
Aug 3, 2021
Merged

Ensure MetadataEnumResult is sufficiently updated by MetaDataImport::… #56756

merged 1 commit into from
Aug 3, 2021

Conversation

AndyAyersMS
Copy link
Member

…Enum

MetadataEnumResult has a fixed inline buffer for returning small results
and a pointer to allow it to return larger ones. The indexer for this
checks the pointer and if non-null assumes that's the current set of
values.

But if a MetadataEnumResult is re-used within a loop, values written to it
by MetaDataImport::Enum may bleed from one loop iteration to the next
if the iterations first get a large result and then a small one.

One case where this could happen was in libraries PGO tests, where PGO data
encouraged the jit to inline MemberInfoCache<T>.PopulateProperties(Filter,...)
into MemberInfoCache<T>.PopulateProperties(Filter).

Note this also is a conseqeunce of skipping zero init locals; without that
the struct would have been zeroed each loop iteration.

Fixes #56655.

@AndyAyersMS
Ensure MetadataEnumResult is sufficiently updated by MetaDataImport::…
482eae0 
…Enum

`MetadataEnumResult` has a fixed inline buffer for returning small results
and a pointer to allow it to return larger ones. The indexer for this
checks the pointer and if non-null assumes that's the current set of
values.

But if a `MetadataEnumResult` is re-used within a loop, values written to it
by `MetaDataImport::Enum` may bleed from one loop iteration to the next
if the iterations first get a large result and then a small one.

One case where this could happen was in libraries PGO tests, where PGO data
encouraged the jit to inline `MemberInfoCache<T>.PopulateProperties(Filter,...)`
into `MemberInfoCache<T>.PopulateProperties(Filter)`.

Note this also is a conseqeunce of skipping zero init locals; without that
the struct would have been zeroed each loop iteration.

Fixes dotnet#56655.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 3, 2021
@AndyAyersMS
Copy link
Member Author

@mangod9 can you suggest somebody to review?

cc @dotnet/jit-contrib

@AndyAyersMS
Copy link
Member Author

/azp run runtime-coreclr libraries-pgo

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS
Copy link
Member Author

Note there may still be some failures in libraries-pgo even with this fix.

@AaronRobinsonMSFT
Copy link
Member

/cc @davidwrighton

@AndyAyersMS
Copy link
Member Author

libraries-pgo failures are expected (eg #56689).

dev-innerloop is hitting another known issue (no issue#, pr to fix is #56767)

##[error]src/libraries/System.Text.Json/gen/System.Text.Json.SourceGeneration.csproj(0,0): error MSB4057: The target "GetTargetPath" does not exist in the project

@AndyAyersMS AndyAyersMS merged commit 394e24f into dotnet:main Aug 3, 2021
thaystg added a commit to thaystg/runtime that referenced this pull request Aug 3, 2021
@thaystg
Merge remote-tracking branch 'origin/main' into thays_fix_56249
c0e7fb2 
* origin/main: (64 commits)
  [wasm][debugger] Create test Inherited Properties (dotnet#56754)
  Mark new test as incompatible with GC Mark4781_1GcStressIncompatible (dotnet#56739)
  Ensure MetadataEnumResult is sufficiently updated by MetaDataImport::Enum (dotnet#56756)
  [mono] Remove gdb xdebug and binary writer support, it hasn't worked in a while. (dotnet#56759)
  Update windows-requirements.md (dotnet#56476)
  Update doc and generic parameter name for JsonValue.GetValue (dotnet#56639)
  [wasm][debugger] Inspect static class (dotnet#56740)
  Fix stack overflow handling issue in GC stress (dotnet#56733)
  Use ReflectionOnly as serialization mode in case dynamic code runtime feature is not supported (dotnet#56604)
  Move Windows Compat pack to NuGet pack task (dotnet#56686)
  Fix build error when building some packages (dotnet#56767)
  Simplify JIT shutdown logic in crossgen2 (dotnet#56687)
  Fix race in crossdac publishing with PGO (dotnet#56762)
  Add DictionaryKeyPolicy support for EnumConverter [dotnet#47765] (dotnet#54429)
  Use ComWrappers in some Marshal unit-tests and update platform metadata  (dotnet#56595)
  Set `DisableImplicitNamespaceImports_Dotnet=true` to workaround sdk issue (dotnet#56744)
  Make sure ServerGCHeapDetails is up to date (dotnet#56056)
  [libraries] Reenable System.Diagnostics.DiagnosticSorce.Switches.Tests on mobile (dotnet#56737)
  Disable failing arm64 win10 Graphics.FromHdc tests  (dotnet#56732)
  Match xplat event source conditions (dotnet#56435)
  ...
thaystg added a commit to thaystg/runtime that referenced this pull request Aug 3, 2021
@thaystg
Merge remote-tracking branch 'origin/main' into thays_fix_debugger_break
ad9e5fb 
* origin/main:
  [wasm][debugger] Create test Inherited Properties (dotnet#56754)
  Mark new test as incompatible with GC Mark4781_1GcStressIncompatible (dotnet#56739)
  Ensure MetadataEnumResult is sufficiently updated by MetaDataImport::Enum (dotnet#56756)
thaystg added a commit to thaystg/runtime that referenced this pull request Aug 4, 2021
@thaystg
Merge remote-tracking branch 'origin/main' into thays_implement_debug…
33652d5 
…ger_proxy_attribute

* origin/main: (340 commits)
  add RID for Debian 11 (dotnet#56789)
  [wasm] [debugger] Skip thread static field (dotnet#56749)
  Fix timeouts in coreroot_determinism test in GC stress mode (dotnet#56770)
  Use File.OpenHandle in Socket.SendFile directly (dotnet#56777)
  accept empty realm for digest auth (dotnet#56369) (dotnet#56455)
  [wasm][debugger] Create test Inherited Properties (dotnet#56754)
  Mark new test as incompatible with GC Mark4781_1GcStressIncompatible (dotnet#56739)
  Ensure MetadataEnumResult is sufficiently updated by MetaDataImport::Enum (dotnet#56756)
  [mono] Remove gdb xdebug and binary writer support, it hasn't worked in a while. (dotnet#56759)
  Update windows-requirements.md (dotnet#56476)
  Update doc and generic parameter name for JsonValue.GetValue (dotnet#56639)
  [wasm][debugger] Inspect static class (dotnet#56740)
  Fix stack overflow handling issue in GC stress (dotnet#56733)
  Use ReflectionOnly as serialization mode in case dynamic code runtime feature is not supported (dotnet#56604)
  Move Windows Compat pack to NuGet pack task (dotnet#56686)
  Fix build error when building some packages (dotnet#56767)
  Simplify JIT shutdown logic in crossgen2 (dotnet#56687)
  Fix race in crossdac publishing with PGO (dotnet#56762)
  Add DictionaryKeyPolicy support for EnumConverter [dotnet#47765] (dotnet#54429)
  Use ComWrappers in some Marshal unit-tests and update platform metadata  (dotnet#56595)
  ...
@ghost ghost locked as resolved and limited conversation to collaborators Sep 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AaronRobinsonMSFT AaronRobinsonMSFT AaronRobinsonMSFT approved these changes

@mangod9 mangod9 mangod9 approved these changes

Assignees
No one assigned
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Libraries PGO tests failing with System.BadImageFormatException
3 participants
@AndyAyersMS @AaronRobinsonMSFT @mangod9