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
Regression: Flags are not being marked as unsigned #1502
Comments
Any other information (e.g. what version this was working in)? |
The fact that all flags enums were being forced to uint was a bug. We fixed that with #1478. Enum types now match their native types as scraped by ClangSharp. Are you just pointing out a documentation discrepancy here? Or are you not able to use the APIs? |
Or maybe just be a few releases behind? 🙃 |
Closing as by design. I updated the docs. Reactivate if you run into usability issues. |
|
@mikebattista Looks like I don't have permissions to reopen an issue after you've closed it. |
@tannergooding I thought we should trust ClangSharp's types here per #1369 (comment). Is ClangSharp getting this wrong? |
The The actual |
@timsneath are you able to pass |
C#/.NET won't let you for sure nor will some other languages, such as Rust. They are both strongly typed and won't let you implicit convert from something that has a more constrained domain (such as an There are in fact CPU architectures, such as RISC-V, where the sign of types is important. On a 64-bit RISC-V system, a signed value is passed differently from an unsigned value. So preserving this information correct/accurately is a core requirement, particularly if Windows supports such architectures in the future. |
Below is our metadata definition. Will that be usable as expected without casting? [DllImport("OLE32.dll", ExactSpelling = true, PreserveSig = false)]
[SupportedOSPlatform("windows5.0")]
public unsafe static extern HRESULT CoInitializeEx([Optional][In] void* pvReserved, [In] COINIT dwCoInit);
[Flags]
public enum COINIT
{
COINIT_APARTMENTTHREADED = 2,
COINIT_MULTITHREADED = 0,
COINIT_DISABLE_OLE1DDE = 4,
COINIT_SPEED_OVER_MEMORY = 8
} |
Yes. But the definition of We really shouldn't be changing signatures anywhere, even for convenience. We should directly map to the C/C++ ABI and should only provide convenience in the form of additional metadata -- e.g. We should have |
That means that by default users will have to do However, it will guarantee correctness and tooling, such as CsWin32, can provide a convenience wrapper which handles the casting for them while still preserving ABI correctness. |
Feels like we're off track. The problem, as I see it, is the |
This is an incorrect change as I've already iterated above. |
Are you saying the implementation expects a signed integer and that the headers are wrong? |
No. There is an API defined in _Check_return_
WINOLEAPI
CoInitializeEx(
_In_opt_ LPVOID pvReserved,
_In_ DWORD dwCoInit
); There is then an enum defined in typedef enum tagCOINIT
{
COINIT_APARTMENTTHREADED = 0x2, // Apartment model
#if (_WIN32_WINNT >= 0x0400 ) || defined(_WIN32_DCOM) // DCOM
// These constants are only valid on Windows NT 4.0
COINIT_MULTITHREADED = COINITBASE_MULTITHREADED,
COINIT_DISABLE_OLE1DDE = 0x4, // Don't use DDE for Ole1 support.
COINIT_SPEED_OVER_MEMORY = 0x8, // Trade memory for speed.
#endif // DCOM
} COINIT; There is no API |
So your feedback is in cases where the types differ, we should not encode the friendly enum in the parameter type in the metadata. Users of the API will need to cast. Is that right? |
I suspect the COINIT definition (with signed integers) is more of a human error or legacy artifact resulting from old ISO C standards/compilers. The author clearly intended to pass in flags. And there's no ABI difference on x86, x64, or arm64.
It may be a good point for RISC-V but by the time we have to worry about that, metadata may look very different. |
Yes. That would be required for ABI correctness. I do think providing additional metadata, in the form of attributes, so that tooling can help provide safe wrapper APIs is fine. For example, providing some public static HRESULT CoInitializeEx(void* pvReserved, COINIT dwCoInit)
{
return (HRESULT)CoInitializeEx(pvReserved, (uint)dwCoInit);
}
[DllImport("ole32", ExactSpelling = true)]
private static extern int CoInitializeEx(void* pvReserved, uint dwCoInit); |
Intent does not matter here. The reality is, for better or worse, that these differ.
There is also a lot more than just ABI consideration to take into account. There is also the impact of other code that may be ported or involved, including code outside the realm of the Simply comparing a As does upcasting it to |
The repo can and likely should still provide all the necessary information such that tooling can generate friendly bindings still. It just requires representing that information via additional metadata to ensure correctness by default and niceness by extension if the tooling decides it wants to utilize what information is available. |
Thanks for the insight Tanner. We have a test that warns for enum remaps that change the size of the parameter/field (from uint to ushort, for example), but it doesn't yet warn against sign changes like this. I'm going to explore updating that test to detect sign changes to see how widespread the issue is now that we honor the native types of the enums. Then we can decide how to handle. |
The main issue that I have right now is that Like @riverar, I see it as merely an idiosyncrasy that If I understand the header file correctly, the enum is not defined as either a signed or an unsigned integer: C does not distinguish itself between the two, and therefore the API should be normative in terms of establishing this? Or am I misunderstanding things? I'm trying to understand why you believe " |
Tanner has a fair point regarding the ABI issue when you start to think about win32metadata/non-win32metadata code co-mingling). @timsneath I believe the proposal on the table is to:
|
Yes that is the proposal. The other ask is to update our enum remapping test to identify sign changes like this. |
There is far more to be considerate of than simply what APIs the headers expose as there are millions of lines of user code that use such types and which do get ported or converted to other languages. A simple example of where a negative value may be encountered is one such as using it as a sentinel to represent an operation that hasn't been completed yet:
Changing the sign of the enum changes the meaning of this code and therefore what happens. This is a case where it would probably get caught relatively quickly, but it is a deviation from how equivalent code functioned in C/C++.
C++ explicitly defines it to be You're correct that C does not explicitly distinguish between them. It is partially implementation defined as to the size and sign of an arbitrary However, Windows has always treated them as
That matches my expectations. In the case of Dart, it should then be able to continue seeing that |
"C++ explicitly defines it to be int if not otherwise specified." -- ah, thank you! That's the subtlety I was missing out on. Your motivating example is good too (even though it seems like a poor choice to set an enum to a deliberately invalid value, I'm sure I've written worse code myself in the past!) But yeah, so long as we take care of 1 on the above list, I'll be a very happy camper! I can't currently roll out the latest metadata file because I'd break too many users, so fixing this would be much appreciated. |
Was mostly giving a representative example of what someone might do. Notably some languages, like rust, may allow this as part of |
Below are the sign mismatch issues after fixing what could be fixed. These should mostly be scraped enums of one type that are assigned to a parameter or field of another type. Manual enums defined in enums.json that have issues are usually the result of being assigned to a mismatch of types. All these assignments should be removed and a new attribute should be added instead to allow projections to improve the developer experience if they can. I'll remove the assignments to close this issue and will create a separate issue to track a new attribute.
|
|
|
|
|
Judging by Mike's scans so far, this change is going to create a lot of churn downstream and anger lots of folks. Probably obvious but will say it anyway: we need to work closely with all the projection authors to make sure what we do here makes sense to everyone and minimize impact. |
The test keeps revealing new failures with every pass. And we haven't quite figured out how to reference types properly in attributes. See #389. I'm reluctant to remove all these enum applications until we have the E2E lined up. Is fixing only the Preserving the types of scraped enums was a good change that unblocked other issues so I don't want to revert that. |
…s or removing enum mappings for #1502. The test seems to give up within a given family of issues after the first reported issue. Fixing issues will often reveal other related issues.
It's your project, not mine, so obviously I don't have a casting vote!
But it feels like this change is a significant regression (since it has
incorrectly changed the projections). And your (wonderful) tests are
unfortunately revealing quite how broad the regression is. If this were a
Google project, we'd revert the change, and introduce it again later when
we'd figured out how to avoid the regression.
Unfortunately, unless I manually introduced my own table of overrides, I
wouldn't be comfortable to move forward from the earlier metadata checked
into the current Dart Win32 projection, because it would cause cascading
breaking changes to apps using it. COINIT is particularly prominent, but I
think I'd cause a ton of downstream breakage if I adopted the current
metadata.
I guess I still feel that the real anomaly is that the untyped enum being
passed into a DWORD really is a DWORD, and I'm skeptical that we should
hold all other languages hostage to a C++ idiosyncrasy (when there are so
many of them!)
Sorry -- this is very unfortunate, I know!
…On Tue, Mar 21, 2023 at 12:35 PM Mike Battista ***@***.***> wrote:
The test keeps revealing new failures with every pass. And we haven't
quite figured out how to reference types properly in attributes. See #389
<#389>.
I'm reluctant to remove all these enum applications until we have the E2E
lined up.
Is fixing only the COINIT issue enough to unblock you @timsneath
<https://github.com/timsneath>? Or are you just going to run into all of
these other issues? Are there workarounds in impacted projections that can
be applied in the meantime?
Preserving the types of scraped enums was a good change that unblocked
other issues so I don't want to revert that.
—
Reply to this email directly, view it on GitHub
<#1502 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AARWL63KBSCO7P3RZB3TOMDW5H7I7ANCNFSM6AAAAAAV7FMPSY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
The best way, IMO, to avoid the regression is to ensure that there is metadata annotating the "associated enum type" so that tooling can pick that up and continue exposing the "friendly" overload. Tooling has to minorly churn to process the metadata differently but if it does that it can in turn result in no change to what bindings they're exposing.
The headers are written in C/C++ and compiled in C/C++ exposing an You could of course make the "break" in the original C exports included in the Windows SDK, but that risks causing problems for C++ code that is consuming the headers directly. You could move such breaks under some It's the same reason why you can't trivially go and replace those |
I'm curious to hear from @AArnott and @kennykerr since I believe they updated to the latest metadata. Have you heard customer complaints? Even though the types of the enums changed, if where they are applied expects the same type (since they're remapped to expect the enum), the majority usage scenario of passing in values from the enum should still work right? The tests have shown that we have several enums applied today that have mismatched types from the original definitions, but that really hasn't impacted anyone. So while the type in the API definition is different from the original header, callers haven't seemed to care. We could do the work proposed here to preserve the original types from the headers while still allowing projections to present the friendly enums, but I'm still a bit unclear how broken things are today in practice. |
No complaints from my customers. |
No complaints so far from users of the latest CsWin32 either. |
…'t align (#1572) * Added test for enum size mismatches and started fixing reported issues or removing enum mappings for #1502. The test seems to give up within a given family of issues after the first reported issue. Fixing issues will often reveal other related issues. * Removed or corrected enums with mismatched sizes. * Added AssociatedEnum attributes. * Documented AssociatedEnum. * Fixed bad merge.
Per generationOptions.md,
--enumMakeFlags
tells the emitter to add the[Flags]
attribute and also to make the enum unsigned.In generation\WinSDK\emitter.settings.rsp,
COINIT
is listed under the--enumMakeFlags
section, but is now converted to int32 in the metadata:Others are also incorrectly shown as signed, for example Windows.Win32.Storage.Vhd.COMPACT_VIRTUAL_DISK_FLAG.
The text was updated successfully, but these errors were encountered: