Skip to content
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

Feature request: use get/set to deal with bitfields. #1392

Closed
zhouhang95 opened this issue Nov 30, 2022 · 9 comments · Fixed by #1620
Closed

Feature request: use get/set to deal with bitfields. #1392

zhouhang95 opened this issue Nov 30, 2022 · 9 comments · Fixed by #1620
Assignees
Labels
enhancement New feature or request

Comments

@zhouhang95
Copy link

zhouhang95 commented Nov 30, 2022

Motivation

struct in cpp:

typedef struct D3D12_RAYTRACING_INSTANCE_DESC {
  FLOAT                     Transform[3][4];
  UINT                      InstanceID : 24;
  UINT                      InstanceMask : 8;
  UINT                      InstanceContributionToHitGroupIndex : 24;
  UINT                      Flags : 8;
  D3D12_GPU_VIRTUAL_ADDRESS AccelerationStructure;
} D3D12_RAYTRACING_INSTANCE_DESC;

struct in rust:

pub struct D3D12_RAYTRACING_INSTANCE_DESC {
    pub Transform: [f32; 12],
    pub _bitfield1: u32,
    pub _bitfield2: u32,
    pub AccelerationStructure: u64,
}

code in cpp

pInstanceDesc->InstanceMask = 0xFF

code in rust

let old_bitfield1 = pInstanceDesc.InstanceMask ;
let new_bitfield1 = (0xFFu32 << 24) | (old_bitfield1 & 0x00FFFFFF);
pInstanceDesc.InstanceMask = new_bitfield1;

I think it is a bug-prone way.

Could we use get/set to deal with bitfields.
accord with rust's safe philosophy.

impl D3D12_RAYTRACING_INSTANCE_DESC {
  pub fn set_InstanceMask(&mut self, value: u32) {
    let old_bitfield1 = pInstanceDesc.InstanceMask ;
    let new_bitfield1 = (value << 24) | (old_bitfield1 & 0x00FFFFFF);
    self.InstanceMask = new_bitfield1;
  }
  pub fn get_InstanceMask(&self) -> u32 {
    let old_bitfield1 = pInstanceDesc.InstanceMask ;
    old_bitfield >> 24
  }
}

Drawbacks

No response

Rationale and alternatives

No response

Additional context

No response

@zhouhang95 zhouhang95 added the enhancement New feature or request label Nov 30, 2022
@kennykerr
Copy link
Contributor

Looks like the Win32 metadata doesn't capture the bit fields. Will transfer for their comment.

@kennykerr kennykerr transferred this issue from microsoft/windows-rs Dec 1, 2022
@kennykerr kennykerr removed the enhancement New feature or request label Dec 1, 2022
@damyanp
Copy link
Member

damyanp commented Dec 3, 2022

+1 on this. Looking at the DirectStorage API we have:

struct DSTORAGE_REQUEST_OPTIONS {
    DSTORAGE_COMPRESSION_FORMAT CompressionFormat : 8;
    DSTORAGE_REQUEST_SOURCE_TYPE SourceType : 1;
    DSTORAGE_REQUEST_DESTINATION_TYPE DestinationType : 7;
    UINT64 Reserved : 48;
};

Which turns into:

// Microsoft.Direct3D.DirectStorage.DSTORAGE_REQUEST_OPTIONS
public struct DSTORAGE_REQUEST_OPTIONS
{
	public byte _bitfield1;
	public ulong _bitfield2;
}

(Yes, sizeof(DSTORAGE_REQUEST_OPTIONS) == 16)

@mikebattista
Copy link
Contributor

@tannergooding / @sotteson1 / @chenss3 is this a clangsharp problem or a win32metadata infrastructure problem?

@tannergooding
Copy link
Member

Looks like a win32metadata problem.

ClangSharp supports generating get/set properties for bitfields, see for example my TerraFX.Interop.Windows project: https://source.terrafx.dev/#TerraFX.Interop.Windows/DirectX/headers/d3d12/D3D12_RAYTRACING_INSTANCE_DESC.cs,636021ab7f8d0bcd

I don't see anything on the ClangSharp site that would be causing this to be filtered out. That being said, it's also worth noting that win32metadata explicitly passes -config exclude-anonymous-field-helpers which does cause it to miss out on cases like https://source.terrafx.dev/#TerraFX.Interop.Windows/DirectX/headers/d3d12/D3D12_UNORDERED_ACCESS_VIEW_DESC.cs,6743366be7be3982

In the above you'll note that there is an Anonymous field but then there is an [UnscopedRef] public ref D3D12_BUFFER_UAV Buffer => ref Anonymous.Buffer and related helper properties that help clarify the fact that there was a union and the field can be accessed directly. win32metadata only gets the field, not the helper properties.

@mikebattista
Copy link
Contributor

Below is the output without exclude-anonymous-field-helpers.

The helper properties aren't emitted in the winmd. I'm not sure where in the pipeline it's getting dropped but isn't code like this unsupported in the metadata anyway?

Can ClangSharp do a better job with handling this rather than just emitting _bitfield1 with no other context? The NativeTypeName that indicates this was a bitfield to begin with is on the helper type and not the _bitfield1 that's emitted in the winmd. Not sure how difficult it would be to try and cross-reference that information in the metadata tooling.

public unsafe partial struct D3D12_RAYTRACING_INSTANCE_DESC
{
    [NativeTypeName("FLOAT[3][4]")]
    public fixed float Transform[3 * 4];

    public uint _bitfield1;

    [NativeTypeName("uint : 24")]
    public uint InstanceID
    {
        get
        {
            return _bitfield1 & 0xFFFFFFu;
        }

        set
        {
            _bitfield1 = (_bitfield1 & ~0xFFFFFFu) | (value & 0xFFFFFFu);
        }
    }

    [NativeTypeName("uint : 8")]
    public uint InstanceMask
    {
        get
        {
            return (_bitfield1 >> 24) & 0xFFu;
        }

        set
        {
            _bitfield1 = (_bitfield1 & ~(0xFFu << 24)) | ((value & 0xFFu) << 24);
        }
    }

    public uint _bitfield2;

    [NativeTypeName("uint : 24")]
    public uint InstanceContributionToHitGroupIndex
    {
        get
        {
            return _bitfield2 & 0xFFFFFFu;
        }

        set
        {
            _bitfield2 = (_bitfield2 & ~0xFFFFFFu) | (value & 0xFFFFFFu);
        }
    }

    [NativeTypeName("uint : 8")]
    public uint Flags
    {
        get
        {
            return (_bitfield2 >> 24) & 0xFFu;
        }

        set
        {
            _bitfield2 = (_bitfield2 & ~(0xFFu << 24)) | ((value & 0xFFu) << 24);
        }
    }

    [NativeTypeName("D3D12_GPU_VIRTUAL_ADDRESS")]
    public ulong AccelerationStructure;
}

@tannergooding
Copy link
Member

but isn't code like this unsupported in the metadata anyway?

Unsure myself. Does winmd trim or otherwise drop function bodies?

Can ClangSharp do a better job with handling this rather than just emitting _bitfield1 with no other context? The NativeTypeName that indicates this was a bitfield to begin with is on the helper type and not the _bitfield1 that's emitted in the winmd. Not sure how difficult it would be to try and cross-reference that information in the metadata tooling.

What are you envisioning?

ClangSharp could potentially emit some information such as [NativeBitfield("InstanceId", 24)] on the field (as well as other relevant bitfields).

That would allow this to work without win32metadata needing to expose the properties as well. Tooling would need to understand such attributes and generate properties or equivalent themselves.

@mikebattista
Copy link
Contributor

I see the C# syntax walker visiting the property now in the debugger, but it's not being emitted in the winmd. I'll play with it a bit more but I suspect it's not supported for the same reason inline functions aren't supported in metadata.

ClangSharp could potentially emit some information such as [NativeBitfield("InstanceId", 24)] on the field (as well as other relevant bitfields).

Yes something like that would be ideal. Either we just plumb through your attribute so that projections can implement their own helpers, or we can easily detect that attribute in our tooling and adapt it to our needs without having to try and cross-reference with the helper properties.

@tannergooding
Copy link
Member

If you could log an issue on dotnet/clangsharp, I'll include this in the features I plan on getting done for the v16.0 release.

@mikebattista
Copy link
Contributor

Done. Thanks!

@mikebattista mikebattista removed their assignment Mar 20, 2023
@mikebattista mikebattista added the enhancement New feature or request label Jul 14, 2023
mikebattista added a commit that referenced this issue Jul 16, 2023
* Updated to ClangSharp 16.0.

* Feature request: use get/set to deal with bitfields. Fixed #1392.

* Updated the baseline.

* Documented NativeBitfield.

* Fixed formatting bug.

* Lowercased Name.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants