Use DllImportGenerator in System.Diagnostics.PerformanceCounter #61389
Conversation
| @@ -32,7 +32,7 @@ | |||
| and '$(IsFrameworkSupportFacade)' != 'true' | |||
| and '$(IsSourceProject)' == 'true' | |||
| and '$(MSBuildProjectExtension)' == '.csproj' | |||
| and '$(TargetFrameworkIdentifier)' == '.NETStandard'" /> | |||
| and ('$(TargetFrameworkIdentifier)' == '.NETStandard' or '$(TargetFrameworkIdentifier)' == '.NETFramework')" /> | |||
There was a problem hiding this comment.
Bah! Sorry @jkoritzinsky looks like I did miss that case <shame />
src/libraries/Common/src/System/Runtime/InteropServices/ArrayMarshaller.cs
Outdated
Show resolved
Hide resolved
elinor-fung
force-pushed
the
convertSystemDiagnosticsPerformanceCounter
branch
from
fa86905
to
93a7166
Compare
| <!-- Suppressions to avoid ifdefs: | ||
| CA1845: Use span-based 'string.Concat' and 'AsSpan' instead of 'Substring' | ||
| CA1846: Prefer 'AsSpan' over 'Substring' when span-based overloads are available --> | ||
| <NoWarn>$(NoWarn);CA1845;CA1846</NoWarn> |
There was a problem hiding this comment.
What's causing these analyzers to fire now and they didn't before?
There was a problem hiding this comment.
The reference to System.Memory (used by some parts of the generated p/invoke stubs).
There was a problem hiding this comment.
I see; then the analyzer sees that these APIs exist and the analyzers start running. Ok, thanks.
| @@ -13,8 +13,8 @@ internal static partial class Interop | |||
| { | |||
| internal static partial class Advapi32 | |||
| { | |||
| [DllImport(Libraries.Advapi32, CharSet = CharSet.Unicode, BestFitMapping = false, EntryPoint = "RegEnumValueW", ExactSpelling = true)] | |||
| internal static extern int RegEnumValue( | |||
| [GeneratedDllImport(Libraries.Advapi32, EntryPoint = "RegEnumValueW", CharSet = CharSet.Unicode, ExactSpelling = true)] | |||
There was a problem hiding this comment.
are we assuming BestFitMapping = true is fine in these?
There was a problem hiding this comment.
Yeah, these are all using CharSet.Unicode, so BestFitMapping shouldn't matter.
There was a problem hiding this comment.
It seems like there are several variations of DllImport/GeneratedDllImport properties that are not meaningful or invalid but just ignored. I wonder whether it would make sense for the generator to flag/reject those so the ycan be cleaned up during conversion? I guess it is a minor thing.
There was a problem hiding this comment.
Certainly something we should keep in mind. We've been trying to remove support for sometimes-non-meaningful things (like BestFitMapping) from GeneratedDllImport, so hopefully it will be in a much better replace than DllImport in terms of weird properties and combinations.
| [DllImport(Interop.Libraries.Advapi32, EntryPoint = "GetSecurityDescriptorLength", CallingConvention = CallingConvention.Winapi, | ||
| CharSet = CharSet.Unicode, ExactSpelling = true)] | ||
| internal static extern /*DWORD*/ uint GetSecurityDescriptorLength(IntPtr byteArray); | ||
| [GeneratedDllImport(Libraries.Advapi32, EntryPoint = "GetSecurityDescriptorLength", CharSet = CharSet.Unicode, ExactSpelling = true)] |
There was a problem hiding this comment.
I guess we are just removing CallingConvention = CallingConvention.Winapi from everywhere as it's default and archaic.
| IntPtr TokenHandle, | ||
| uint TokenInformationClass, | ||
| SafeLocalAllocHandle TokenInformation, | ||
| uint TokenInformationLength, | ||
| out uint ReturnLength); | ||
|
|
||
| #if DLLIMPORTGENERATOR_ENABLED | ||
| [GeneratedDllImport(Interop.Libraries.Advapi32, CharSet = CharSet.Unicode, SetLastError = true)] | ||
| [GeneratedDllImport(Interop.Libraries.Advapi32, SetLastError = true)] |
There was a problem hiding this comment.
why remove CharSet = CharSet.Unicode ? is it the default now?
There was a problem hiding this comment.
It is not the default. I removed it because it is not necessary for this p/invoke - no string data being marshalled, no A/W suffix variants - and having it set to Unicode in this case makes it so that we do an extra probe for an entry point named GetTokenInformationW, since when ExactSpelling is false, we check for the W suffix first for Unicode (but no suffix first for Ansi).
There was a problem hiding this comment.
this seems like a great thing for the generator to warn on (I say the generator since it's new so it wouldn't be a breaking change, and folks are touching their sources anyway)
There was a problem hiding this comment.
We're currently discussing how we can make the whole character set situation better with the generator approach: #61326.
One thing we are considering as part of that is removing ExactSpelling (so weird/negative implications of char set like this wouldn't be a thing) and effectively requiring it to always be true.
elinor-fung
added
area-System.Runtime.InteropServices
and removed
area-Interop-coreclr
labels
ghost
locked as resolved and limited conversation to collaborators
Since we forwarding on down-level, this also enables
DllImportGeneratorby default when targeting Framework (asSystem.Diagnostics.PerformanceCounterdoes).cc @AaronRobinsonMSFT @jkoritzinsky