-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Fix numeric IntPtr behavioral changes analyzer findings #75557
Conversation
Tagging subscribers to this area: @dotnet/area-meta Issue DetailsWith #74022 we are adding an analyzer that detects behavioral changes in built operators of numeric IntPtr feature. Some built in operators added added with the feature behave differently than the old user defined operators. Some operators that used to throw in unchecked context while overflowing will not throw anymore unless wrapped within checked context, and some operators that not used to throw in checked context now would unless wrapped within unchecked context. Normally we would merge the analyzer to the roslyn-analyzers repo first then update the dogfooding version and along with code fixes, but as the analyzer still not merged yet but it still planned to be merged into 7.0 raising this PR without the analyzer dogfood version updates Warnings found:
How the warnings handled?
|
CC. @jkotas, you were particularly interested in the how some of this was changed. |
...coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs
Outdated
Show resolved
Hide resolved
src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs
Outdated
Show resolved
Hide resolved
src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Timer.Windows.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/Interop/Windows/NtDll/Interop.SYSTEM_PROCESS_INFORMATION.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Win32.SystemEvents/src/Microsoft/Win32/SystemEvents.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Win32.Registry/src/Microsoft/Win32/RegistryKey.Windows.cs
Show resolved
Hide resolved
src/libraries/System.Data.Odbc/src/Common/System/Data/Common/AdapterUtil.Odbc.cs
Outdated
Show resolved
Hide resolved
@@ -1762,7 +1762,7 @@ private void GetRowHandles(/*int skipCount*/) | |||
ProcessResults(hr); | |||
} | |||
_isRead = ((OleDbHResult.DB_S_ENDOFROWSET != hr) || (0 < (int)_rowFetchedCount)); | |||
_rowFetchedCount = (IntPtr)Math.Max((int)_rowFetchedCount, 0); | |||
_rowFetchedCount = Math.Max((int)_rowFetchedCount, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if nint.Max(_rowFetchedCount, 0)
would better match the intent or if explicitly truncating down to int
was the intent...
src/libraries/System.Data.OleDb/src/System/Data/Common/AdapterUtil.cs
Outdated
Show resolved
Hide resolved
src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Tanner Gooding <tagoo@outlook.com>
...ies/System.Diagnostics.PerformanceCounter/src/System/Diagnostics/SharedPerformanceCounter.cs
Outdated
Show resolved
Hide resolved
...em.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/common/BerConverter.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs
Outdated
Show resolved
Hide resolved
I have not found any place in the delta where checked arithmetic is desired. Many places can be simplified now that unchecked arithmetic is the default. |
Update: The PR is almost ready, just waiting the analyzer PR got merged and built for dogfood, then I will add the update the runtime analyzer version with that build |
src/libraries/Common/src/Interop/FreeBSD/Interop.Process.GetProcInfo.cs
Outdated
Show resolved
Hide resolved
…ocInfo.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com>
src/libraries/Common/src/Interop/BSD/System.Native/Interop.Sysctl.cs
Outdated
Show resolved
Hide resolved
This reverts commit 74c580f.
src/libraries/System.Data.OleDb/src/System/Data/Common/AdapterUtil.cs
Outdated
Show resolved
Hide resolved
2 of the failures related to #76280 and #76617
Not sure were to report that, anyway don't think its related, merging |
With #74022 we are adding an analyzer that detects behavioral changes in built operators of numeric IntPtr feature. Some built in operators added added with the feature behave differently than the old user defined operators. Some operators that used to throw in unchecked context while overflowing will not throw anymore unless wrapped within checked context, and some operators that not used to throw in checked context now would unless wrapped within unchecked context.
Normally we would merge the analyzer to the roslyn-analyzers repo first then update the dogfooding version and along with code fixes, but as the analyzer still not merged yet but it still planned to be merged into 7.0 raising this PR without the analyzer dogfood version updates
Warnings found:
How the warnings handled?
(IntPtr)(((long)pNative) - sizeof(uint))