Merge from dotnet/runtime #1737
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ardless of what they point at (#61538)
Implement method name wildcard matching for method descriptions Globbing doesn't work because we don't have g_pattern_match_simple in eglib. But a plain '*' wildcard does work.
When both OSR and PGO are enabled: * Enable instrumenting OSR methods, so that the combined profile data from Tier0 plus any OSR variants provide a full picture for subsequent Tier1 optimization. * Use block profiles for both Tier0 methods that are likely to have patchpoints and OSR methods. * Fix phase ordering so partially jitted methods don't lose probes. * A few more fixes for partial compilation, because the number of things we think we might instrument and the number of things we end up instrumenting can differ. * Also improve the DumpJittedMethod output for OSR, and allow selective dumping of a particular OSR variant by specifying its IL offset. The updates on the runtime side are to pass BBINSTR to OSR methods, and to handle the (typical) case where the OSR method instrumentation schema is a subset of the Tier0 method schema. We are still allowing OSR methods to read the profile data. So they are both profile instrumented and profile optimized. Not clear if this is going to work well as the Tier0 data will be incomplete and optimization quality may be poor. Something to revisit down the road.
Emit it in the interpreter when a method is inlined or replaced with an intrinsic. This is needed so the AOT profiler can track these methods.
…traints. (#59437) If a generic argument is a primitive type, and it has an interface constraint that enums don't implement, then partial sharing for that instance is not useful, since only the specific primitive type instance will be able to use the shared version. Fixes dotnet/runtime#54910.
* Enable new analyzers in global configs * Address PR feedback
For now there's no functional change to the behavior of the tests, I have just copied the bits to inject from Jeremy's example in his pending PR. Thanks Tomas
* embedded size * next array * read consistency while rehashing * comments * remove CRST_UNSAFE_ANYMODE and COOP mode in the callers * typo * fix 32bit builds * couple changes from review. * Walk the buckets in resize. * remove a `REVIEW:` comment. * Update src/coreclr/vm/dacenumerablehash.inl PR review suggestion Co-authored-by: Jan Kotas <jkotas@microsoft.com> * remove use of `auto` * DAC stuff * Constructor and GrowTable are not called by DAC, no need for DPTR, added a comment about a cast. * SKIP_SPECIAL_SLOTS * More compact dac_cast in GetNext Co-authored-by: Jan Kotas <jkotas@microsoft.com>
For now there's no functional change to the behavior of the tests, I have just copied the bits to inject from Jeremy's example in his pending PR. Thanks Tomas Contributes to: dotnet/runtime#54512
For now there's no functional change to the behavior of the tests, I have just copied the bits to inject from Jeremy's example in his pending PR. I have spot-checked that some of the tests use Main with the command-line args argument. I'm not changing them in this PR, the signature only becomes important once we start actually merging the IL tests and I presume we'll clean that up at that point. Thanks Tomas Contributes to: dotnet/runtime#54512
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
* Add JsonSerializerOptions.Default * address feedback * address feedback
`__DoCrossgen2` appears to be redundant with `__TestBuildMode=crossgen2`.
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
The issue was that VNApplySelectors uses the types of fields when selecting, but that's not valid for "the first field" maps. Mirror how the stores to fields are numbered.
For very large structs (> 64K in size) poisoning could end up generating instructions requiring larger local var offsets than we can handle. This hits IMPL_LIMIT that throws InvalidProgramException. Turn off poisoning for larger structs that require more than 16 movs to also avoid the significant code bloat by the singular movs. This is a less risky version of #61521 for backporting to .NET 6. Fix #60852
…tValueAtIndex (#60520) * SortedList<TKey, TValue> added GetKeyAtIndex, GetValueAtIndex, and SetValueAtIndex * Update src/libraries/System.Collections/src/System/Collections/Generic/SortedList.cs Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>
This optimization is only legal if: 1) "Anything" is a sufficiently small constant itself. 2) We are in a context where we know the address will in fact be used for an indirection. It is the second point that is problematic - one would like to use MorphAddrContext, but it is not suitable for this purpose, as an unknown context is counted as an indirecting one. Additionally, the value of this optimization is rather low. I am guessing it was meant to support the legacy nullchecks, before GT_NULLCHECK was introduced, and had higher impact then. So, just remove the optimization and leave the 5 small regressions across all of SPMI be.
* Apply suggestions from code review Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com> Co-authored-by: Jeremy Koritzinsky <jkoritzinsky@gmail.com>
…Process, System.Diagnostics.Process, System.Diagnostics.FileVersionInfo, System.Runtime.InteropServices.RuntimeInformation (#61532)
* FileSystem.Unix: improve CopyFile. Like the upcoming version of GNU coreutils 'cp' prefer a copy-on-write clone. This shares the physical storage between files, which means no data needs to copied. CoW-clones are supported by a number of Linux file systems, like Btrfs, XFS, and overlayfs. Eliminate a 'stat' call that is always performed for checking if the target is a directory by only performing the check when the 'open' syscall reports an error. Eliminate a 'stat' call for retrieving the file size of the source by passing through the length that was retrieved when checking the opened file is not a directory. Create the destination with file permissions that match the source. We still need to fchmod due to umask being applied to the open mode. When performing a manual copy, limit the allocated buffer for small files. And, avoid the last 'read' call by checking when we've copied the expected nr of bytes. * Don't FICLONE for zero sourceLength * PR feedback * When using sendfile, don't loop when source file gets truncated. * Fall through when FICLONE fails. * Don't stop CopyFile_ReadWrite until read returns zero. * Revert all changes to CopyFile_ReadWrite * Move comment a few lines up. * Fix unused error.
Since libproc is a private Apple API, it is not available on iOS/tvOS and should be excluded (see #61265 (comment) and above for more details). This PR excludes $(CommonPath)Interop\OSX\Interop.libproc.cs on the iOS/tvOS as well as makes some methods in Process, ProcessManager, and ProcessThread classes calling that API throw PNSE so that for iOS/tvOS it's possible to re-use the respective *.UnknownUnix.cs parts.
We've tried to consistently use global:: whenever referring to core library types in the regex generator emitted code. I'd missed these two. (That said, these make the code a lot harder to read, especially in places where we're unable to use extension methods as extensions, so we'll want to revisit this policy.)
… (#52639) * Implement most of the fix for #38824 Reverts the changes in the seperate PR dotnet/runtime@a617a01 to fix #38824. Does not re-enable the test because that relies on #49555, will add a seperate commit to whichever is merged last to enable the SettingUpdatesPropertiesOnSymlink test. * Most of the code for PR #52639 to fix #38824 • Deal with merge conflicts • Add FSOPT_NOFOLLOW for macOS and use it in setattrlist • Use AT_SYMLINK_NOFOLLOW with utimensat (note: there doesn't seem to be an equivalent for utimes) • Add SettingUpdatesPropertiesOnSymlink test to test if it actually sets it on the symlink itself (to the best that we can anyway ie. write + read the times) • Specify FILE_FLAG_OPEN_REPARSE_POINT for CreateFile in FileSystem.Windows.cs (is there any other CreateFile calls that need this?) * Remove additional FILE_FLAG_OPEN_REPARSE_POINT I added FILE_FLAG_OPEN_REPARSE_POINT before and it was then added in another PR, this removes the duplicate entry. * Add missing override keywords Add missing override keywords for abstract members in the tests. * Fix access modifiers Fix access modifiers for tests - were meant to be protected rather than public * Add more symlink tests, rearrange files • Move symlink creation api to better spot in non-base files • Add IsDirectory property for some of the new tests • Change abstract symlink api to CreateSymlink that accepts path and target • Create a CreateSymlinkToItem method that creates a symlink to an item that may be relative that uses the new/modified abstract CreateSymlink method • Add SettingUpdatesPropertiesCore to avoid code duplication • Add tests for the following variants: normal/symlink, target exists/doesn't exist, absolute/relative target • Exclude nonexistent symlink target tests on Unix for Directories since they are counted as files * Fix return type of CreateSymlink in File/GetSetTimes.cs * Remove browser from new symlink tests as it doesn't support creation of symlinks Remove browser from new symlink tests as it doesn't support creation of symlinks * Use lutimes, improve code readability, simplify tests • Use lutimes when it's available • Extract dwFlagsAndAttributes to a variable • Use same year for all tests • Checking to delete old symlink is unnecessary, so don't • Replace var with explicit type * Change year in test to 2014 to reduce diff * Rename symlink tests, use 1 core symlink times function, and check that target times don't change Rename symlink tests, use 1 core symlink times function, and check that target times don't change * Inline RunSymlinkTestPart 'function' Inline RunSymlinkTestPart 'function' so that the code can be reordered so the access time test can be valid. * Share CreateSymlinkToItem call in tests and update comment for clarity * Update symlink time tests • Make SettingUpdatesPropertiesOnSymlink a theory • Remove special case for Unix due to dotnet/runtime#52639 (comment) (will revert if fails) • Rename isRelative to targetIsRelative for clarity * Remove unnecessary Assert.All * Changes to SettingUpdatesPropertiesOnSymlink test • Rename item field to link field • Don't use if one-liner • Use all time functions since only using UTC isn't necessary • Remove the now-defunct IsDirectory property since we aren't checking it anymore * Remove unnecessary fsi.Refresh() • Remove unnecessary fsi.Refresh() since atime is only updated when reading a file * Updates to test and pal_time.c • Remove targetIsRelative cases • Multi-line if statement • Combine HAVE_LUTIMES and #else conditions to allow more code charing * Remove trailing space
by removing obvious warnings
* Delete regArgList Unused. * Delete CountSharedStaticHelper Unused. * Delete GTF_RELOP_QMARK Unused. * Delete lvaPromotedStructAssemblyScratchVar Unused. * Delete dead code from fgMorphSmpOp The conditions tested constitute invalid IR. * Delete gtCompareTree Unused. * Delete gtAssertColonCond Unused. * Delete IsSuperPMIException Unused.
OSR methods share the "monitor acquired" flag with the original method. The monitor acquired flag is s bit of non-IL live state that must be recorded in the patchpoint. Also, OSR methods only need to release the monitor as acquisition can only happen in the original method.
* Apply BuiltInComInterop feature switch to managed code * Feedback * Remove two more
…guration.EnvironmentVariables` (#57433) * Annotate * Add net6 to parent projects * Prefix can be null in AddEnvironmentVariables * DisableImplicitAssemblyReferences * Address PR feedback - Add necessary TFMs - Make configureSource action nullable Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Add COMPlus_JitDisasmWithDebugInfo. When set and in verbose/disasm mode, JIT will display inline comments with debug information before the instructions that debug info applies to. Change superpmi with --debuginfo to use this mode. Also small change to dotnet-pgo flow graph dump to write offsets in the same format.
* Improve superpmi-asmdiffs AzDO pipeline robustness 1. When git fetching origin/main, use `--depth=500` to try to ensure there is enough context to allow finding a JIT change in the history. 2. Add some error checking in pipeline setup so failures in setting up the pipeline should fail the jobs early. * Remove unneeded `success` variable from jitrollingbuild.py
…stenerTests on iOS/tvOS (#61807) This marks System.Diagnostics.TextWriterTraceListenerTests.XmlWriterTraceListenerTests withSkipOnPlatform attribute for iOS/tvOS as those tests try to create a process info, which throws PNSE after S.D.Process API's around libproc have been excluded in #61590.
Change CMake build for JIT to include headers and arch-specific headers in the same way as sources. This makes arch-specific headers appear in the correct standalone jit (clrjit_universel_arm64_x64 etc.) projects.
* Disable several failing tests on iOSSimulator arm64 A few tests popped up as failures on the rolling build due to parts of System.Diagnostics.Process throwing PNSE. Disabled the functional tests from running on arm64 as mlaunch can't detect the return code. * Update src/libraries/System.Runtime.Extensions/tests/System/EnvironmentTests.cs Co-authored-by: Steve Pfister <steve.pfister@microsoft.com> Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
* introduce an overload that accepts ROS<char> instead of a string * remove dead code * avoid string allocations * remove List<int> allocation * Apply suggestions from code review Co-authored-by: Stephen Toub <stoub@microsoft.com>
The logic added in a recent PR to support simplified code generation for a Regex with {One/Notone/Set}lazy nodes is almost sufficient to support Lazy as well. This fills the gap. It also deletes an erroneous optimization added in .NET 5 that removed top-level Atomic nodes; the idea behind it was that such nodes are meaningless as, at the top-level, nothing can backtrack in anyway, but it then means that any node which is paying attention to whether its parent is Atomic may no longer find it (and that's needed by Lazy).
We may want to change the logic around how IP mappings are reported to the EE, and the manually maintained singly-linked lists made it hard to understand the logic that goes on here. Switch to jitstd::list which allows us to simplify the logic by directly removing the mappings we do not want to emit. There are two small behavior changes here: 1. The previous logic would record superfluous IL offset 0 mappings under the assumption that the previous mapping was the prolog; this is not always the case, so check this explicitly 2. The previous logic would record _all_ CALL_INSTRUCTION mappings and would not use these to check whether to remove NO_MAP mappings. This is superfluous as well, if we have a regular mapping at a native offset it does not give any information to add a NO_MAP mapping at the same native offset. All diffs therefore look like the following. Case 1: -IP mapping count : 7 +IP mapping count : 6 IL offs PROLOG : 0x00000000 ( STACK_EMPTY ) IL offs 0x0000 : 0x00000009 ( STACK_EMPTY ) IL offs 0x0000 : 0x000000F4 ( STACK_EMPTY ) -IL offs 0x0000 : 0x000000F4 ( STACK_EMPTY ) IL offs NO_MAP : 0x00000114 ( STACK_EMPTY ) IL offs EPILOG : 0x0000011D ( STACK_EMPTY ) IL offs 0x0000 : 0x00000124 ( STACK_EMPTY ) Case 2: IL offs 0x0000 : 0x000000D7 ( STACK_EMPTY ) IL offs 0x0001 : 0x000000DE ( CALL_INSTRUCTION ) -IL offs NO_MAP : 0x000000E9 ( STACK_EMPTY ) IL offs 0x000B : 0x000000E9 ( CALL_INSTRUCTION ) IL offs NO_MAP : 0x000000F7 ( STACK_EMPTY )
Caught my attention because this is disabled outside issues.targets. This is apparently blocked on dotnet/runtime#5907 which is a crossgen bug. I tried to /p:PublishReadyToRun=true this test locally and it seems to work fine with crossgen2.
|
@MichalStrehovsky Is there any known adjustments that has to made after the integration of the shared files to fix the failing tests? It looks like we are not marking methods as reflectable in some cases, e.g. "Unhandled Exception: System.Exception: Method 'Frob' not found on type ReflectionTest+TestInterfaceMethod+IFoo". |
RyuJIT apparently devirtualized the interface method call in Debug builds and we no longer see the interface method as used. This is also why it work in Release builds (where the scanner makes sure the interface method is seen as used). I'll look into this later. Possibly tomorrow or day after. We could also just disable the Reflection test for now to unblock integration. I don't think this is super critical. We have coverage through the Reflection_ReflectedOnly test. But it's surprising to see optimizations kicking in with optimizations disabled. |
Thanks! I think we should wait with the merge until the problem is understood. |
dotnet/runtime#61868 should fix this. |
This test is pretty fragile but I couldn't come up with anything better. RyuJIT generates a new pattern and that broke the test.
We started doing devir in unoptimized builds. I assume that was an unintended consequence of #61453.
|
There are still a few reflection tests failing in release builds. @MichalStrehovsky Could you please take a look when you get a chance? |
Based on what the test says, I'm pretty sure this is dotnet/runtime#54056. I wonder if we should just delete the attribute stripping from the AOT compiler. Attribute stripping currently removes attributes from the assembly, no questions asked, breaking user code in the process. But I believe pretty strongly that attribute stripping should only happen to things not visible from reflection (dotnet/runtime#103934). If we go that route, it's basically what the compiler does in |
|
I am disabling the tests for now. I agree that the current behavior does not seem to be ideal. At the same time, I would like to avoid introducing differences between trimming done by the IL linker and trimming done by the NativeAOT compiler. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.