ConstantWriter emits unchecked integers in unsigned integer enumerations #1369
Assignees
Comments
|
You can find a repro in microsoft/DirectX-Headers#82 😬 (EDIT: Note that the offending enum variants have been commented out to get past the resulting |
|
I ran into this trying to fix #1401 as well but haven't been able to track down where this is coming from. @riverar, I don't believe the line you linked is the culprit. That is for constants that we scrape and turn into enums not enums that are scraped directly from the headers. +@tannergooding @sotteson1 @chenss3 for additional eyes on this. |
|
It's possible ClangSharp has some incorrect handling in That being said, why is this typed as The sign of types is important and there are platforms (such as RISC-V) where using the incorrect type will result in incorrect data. This may be impactful to future platforms that Windows could theoretically support. |
|
Are these enums supposed to be int? What does it mean for a flags enum to be int vs. uint? Is this D3D12_BARRIER_SYNC enum supposed to be used in places that require int or uint? The other flags enums that are scanned seem to be correctly identified as uint without issue. If I add --with-type D3D12_BARRIER_SYNC=uint then below is what clangsharp produces. You can see it's natively scanning it as an int given the NativeTypeName attribute. Though with --with-type, there's no more unchecked issue for this enum, and the enum is typed as a uint. |
Not much in .NET since we don't have the same implicit conversions that C/C++ has. That being said, the win32metadata repo is meant to be usable from many languages where such implicit conversions might exist. In such scenarios, comparisons such as There are also some target platforms (such as RISC-V and I believe MIPS as well) where the sign of the type matters to the calling convention. On 64-bit platforms, a 32-bit value is sign or zero-extended to fill the register and so if typed as In general we should aim to exactly match the signedness and other behaviors as exhibited by C/C++. Likewise, for cases like |
damyanp
added a commit
to damyanp/win32metadata
that referenced
this issue
Jan 25, 2023
damyanp
added a commit
to damyanp/win32metadata
that referenced
this issue
Jan 25, 2023
damyanp
added a commit
to damyanp/win32metadata
that referenced
this issue
Jan 25, 2023
|
I found the place where flags enums were being marked as uint. win32metadata/sources/ClangSharpSourceToWinmd/MetadataSyntaxTreeCleaner.cs Lines 451 to 460 in 992091d By commenting out this section, I get a pretty large diff below. @damyanp can you sanity check the D3D changes? This diff also included the fix for #1401 so there should be (1) more enums tagged with the Flags attribute as well as (2) enums that were previously forced to be uint are now being scanned as int. Are these changes all expected? |
|
Also when I remove your previous workaround: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Example invalid generated output:
Seems to be hardcoded here:
win32metadata/sources/MetadataUtils/ConstantWriter.cs
Lines 188 to 191 in 6627267
Repro: TBD
The text was updated successfully, but these errors were encountered: