microsoft / CsWin32

A source generator to add a user-defined set of Win32 P/Invoke methods and supporting types to a C# project.
MIT License
1.99k stars 84 forks source link

APPBARDATA should compile on AnyCPU #1195

Closed AffluentOwl closed 4 weeks ago

AffluentOwl commented 1 month ago

Repro

  1. NativeMethods.txt content:
    SHAppBarMessage
  2. Build with Any CPU selected

Actual

Errors

Warning PInvoke005  This API is only available when targeting a specific CPU architecture. AnyCPU cannot generate this API.
Error   CS0246  The type or namespace name 'APPBARDATA' could not be found (are you missing a using directive or an assembly reference?)
Error   CS0103  The name 'SHAppBarMessage' does not exist in the current context

Expected

Build succeeds without failures

Context

Background

See #505 . In particular, the argument against this compiling was that StructLayout was used with the Pack = 1 parameter. However, this definition is semantically equivalent on x86 as leaving off the Pack = 1 parameter. Perhaps this was a machine generated value which doesn't apply to this struct and can be removed from the metadata?

I'd like this bug to track whether or not CsWin32 should check if two alternative definitions are semantically equivalent before throwing an error. For example, by using reflection over the fields and ensuring matching field offsets. I'm unsure of the magnitude of structs affected by this issue, but this approach could help with a number of metadata issues.

  [StructLayout(LayoutKind.Sequential, Pack = 1)]
  public struct APPBARDATA2
  {
    public uint cbSize;
    public HWND hWnd;
    public uint uCallbackMessage;
    public uint uEdge;
    public RECT rc;
    public LPARAM lParam;
  }

  [StructLayout(LayoutKind.Sequential)]
  public struct APPBARDATA3
  {
    public uint cbSize;
    public HWND hWnd;
    public uint uCallbackMessage;
    public uint uEdge;
    public RECT rc;
    public LPARAM lParam;
  }

  {
    APPBARDATA2 a = new();
    byte* addr = (byte*)&a;
    Console.WriteLine($"{(byte*)&a.cbSize - addr} {(byte*)&a.hWnd - addr} {(byte*)&a.uCallbackMessage - addr} {(byte*)&a.uEdge - addr} {(byte*)&a.rc - addr} {(byte*)&a.lParam - addr} {sizeof(APPBARDATA2)}");
  }
  {
    APPBARDATA3 a = new();
    byte* addr = (byte*)&a;
    Console.WriteLine($"{(byte*)&a.cbSize - addr} {(byte*)&a.hWnd - addr} {(byte*)&a.uCallbackMessage - addr} {(byte*)&a.uEdge - addr} {(byte*)&a.rc - addr} {(byte*)&a.lParam - addr} {sizeof(APPBARDATA3)}");
  }
x86 Output
0 4 8 12 16 32 36
0 4 8 12 16 32 36

x64 Output
0 4 12 16 20 36 44
0 8 16 20 24 40 48
AffluentOwl commented 1 month ago

A similar proposal sent to win32metadata: microsoft/win32metadata/issues/1917

However, they may feel that it's CsWin32's job to unify definitions where possible. Even when they over document the metadata differences that make no semantic impact.

AffluentOwl commented 4 weeks ago

Took a look at https://github.com/microsoft/win32metadata/issues/1300#issuecomment-1464715747

Turns out that Packing can leak out beyond just the struct that the attribute is applied to. Therefore, the simplification I proposed will not allow for nested packed structs.

[StructLayout(LayoutKind.Sequential, Pack = 4)]
public struct MINIDUMP_INCLUDE_MODULE_CALLBACK1
{
  public ulong BaseOfImage;
}

public struct MINIDUMP_INCLUDE_MODULE_CALLBACK2
{
  public ulong BaseOfImage;
}

public struct Test1
{
  public byte x;
  public MINIDUMP_INCLUDE_MODULE_CALLBACK1 y;
}

public struct Test2
{
  public byte x;
  public MINIDUMP_INCLUDE_MODULE_CALLBACK2 y;
}

{
  Test1 a = new();
  byte* addr = (byte*)&a;
  Console.WriteLine($"{(byte*)&a.x - addr} {(byte*)&a.y - addr} {sizeof(Test1)}");
}

{
  Test2 a = new();
  byte* addr = (byte*)&a;
  Console.WriteLine($"{(byte*)&a.x - addr} {(byte*)&a.y - addr} {sizeof(Test2)}");
}
0 4 12
0 8 16