Mark pvBuffer as In/Out #1247
Mark pvBuffer as In/Out #1247
Conversation
This parameter is actually used as in/out. Per https://docs.microsoft.com/en-us/dotnet/framework/interop/blittable-and-non-blittable-types they must be marked as such ("you must apply the InAttribute and OutAttribute attributes if you want to marshal the argument as an In/Out parameter"). The code currently works, because it's taking advantage of the pinning optimization. It will keep working the same, but the metadata will be correct after the change.
|
@JeremyKuhne could you please have a look? |
Yep, looks better. I also fixed up the |
| NativeMethods.USEROBJECTFLAGS flags = new NativeMethods.USEROBJECTFLAGS(); | ||
|
|
||
| if (UnsafeNativeMethods.GetUserObjectInformation(new HandleRef(null, hwinsta), NativeMethods.UOI_FLAGS, flags, Marshal.SizeOf(flags), ref lengthNeeded)) | ||
| if (UnsafeNativeMethods.GetUserObjectInformation(new HandleRef(null, hwinsta), NativeMethods.UOI_FLAGS, ref flags, Marshal.SizeOf<NativeMethods.USEROBJECTFLAGS>(), ref lengthNeeded)) |
There was a problem hiding this comment.
Can you turn this to sizeof(USEROBJECTFLAGS)? There's no need to hit the marshaller if it's blittable.
There was a problem hiding this comment.
Sure, done. But it requires making the method unsafe because of dumb C# rules.
There was a problem hiding this comment.
method unsafe because of dumb C# rules.
Yeah, that's one I hope to get fixed eventually. :)
src/Common/src/NativeMethods.cs
Outdated
| } | ||
| } | ||
|
|
||
| [StructLayout(LayoutKind.Sequential)] |
There was a problem hiding this comment.
nit: the [StructLayout] isn't needed anymore
There was a problem hiding this comment.
I tend to not touch these, because this attribute has a side effect of shutting up compiler warnings about unused fields. We'll see what the CI says (I didn't try to build any of this locally because these are all de minimis changes).
There was a problem hiding this comment.
@JeremyKuhne Interesting, I was under the assumption that structs passed through the interop layer always needed this. Is it documented somewhere when it can be left off?
There was a problem hiding this comment.
It's documented somewhere in the C# language spec. The C# compiler marks all structures as sequential by default.
There was a problem hiding this comment.
I was under the assumption that structs passed through the interop layer always needed this.
StructLayoutAttribute is a pseudo attribute (see ECMA 335: I I.21.2.1) which means what you're typing in code isn't actually stored as a normal attribute. The IL flag it relates to is always set one way or another, which you can see in IL with the auto, sequential, and explicit keywords on the type. When reflecting the attribute is always manually created for you if you try and look for it.
Our general interop guidelines are to not add attributes that are default behavior, which is what I'm calling out here. As @MichalStrehovsky calls out, in this particular case C# might complain about unused fields without it being explicitly put in code. I think the rule for that one is constrained to public types, and it might have an out for things in "NativeMethods" classes, I'm not entirely sure. You also have to have this explicitly defined if you have char fields so you can specify that they're Unicode rather than ANSI (the default) for marshalling (i.e. [StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)])
There was a problem hiding this comment.
Yeah I knew about all that but I was thinking the default was auto not sequential, interesting that it isn't.
ghost
locked as resolved and limited conversation to collaborators
4 participants
This parameter is actually used as out. Per https://docs.microsoft.com/en-us/dotnet/framework/interop/blittable-and-non-blittable-types these parameters must be marked as such ("you must apply the InAttribute and OutAttribute attributes if you want to marshal the argument as an In/Out parameter"). The code currently works, because it's taking advantage of the pinning optimization that the CLR does, otherwise it would be treated as "in" only. It will keep working the same with the extra metadata, but the metadata will be correct.
Hit this while running on a runtime that doesn't do the pinning optimization (and this annotation is a fix for that).