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

FFI support for packed structs #38158

Closed
ds84182 opened this issue Sep 1, 2019 · 8 comments
Closed

FFI support for packed structs #38158

ds84182 opened this issue Sep 1, 2019 · 8 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi

Comments

@ds84182
Copy link
Contributor

ds84182 commented Sep 1, 2019

Amazingly, it turns out that some Win32 structs are packed. (In my case, OPENFILENAMEW). I don't know how many Win32 structs are packed, but the lack of struct packing makes it difficult to interface with Win32.

Example:

class OpenFileNameW extends Struct<OpenFileNameW> {
  @Int32()
  int lStructSize; // DWORD         lStructSize;

  @IntPtr()
  int hwndOwner; // HWND          hwndOwner;

  @IntPtr()
  int hInstance; // HINSTANCE     hInstance;

  Pointer<Uint16 /* CWSTR */ > lpstrFilter; // LPCWSTR       lpstrFilter;

  Pointer<Uint16 /* WSTR */ > lpstrCustomFilter; // LPWSTR        lpstrCustomFilter;

  @Int32()
  int nMaxCustFilter; // DWORD         nMaxCustFilter;

  @Int32()
  int nFilterIndex; // DWORD         nFilterIndex;

  Pointer<Uint16 /* WSTR */ > lpstrFile; // LPWSTR        lpstrFile;

  @Int32()
  int nMaxFile; // DWORD         nMaxFile;

  Pointer<Uint16 /* WSTR */ > lpstrFileTitle; // LPWSTR        lpstrFileTitle;

  @Int32()
  int nMaxFileTitle; // DWORD         nMaxFileTitle;

  Pointer<Uint16 /* CWSTR */ > lpstrInitialDir; // LPCWSTR       lpstrInitialDir;

  Pointer<Uint16 /* CWSTR */ > lpstrTitle; // LPCWSTR       lpstrTitle;

  @Int32()
  int Flags; // DWORD         Flags;

  @Int16()
  int nFileOffset; // WORD          nFileOffset;

  @Int16()
  int nFileExtension; // WORD          nFileExtension;

  Pointer<Uint16 /* CWSTR */ > lpstrDefExt; // LPCWSTR       lpstrDefExt;

  @IntPtr()
  int lCustData; // LPARAM        lCustData;

  Pointer<Void /* OFNHOOKPROC */ > lpfnHook; // LPOFNHOOKPROC lpfnHook;

  Pointer<Uint16 /* CWSTR */ > lpTemplateName; // LPCWSTR       lpTemplateName;

  Pointer<Void /* EditMenu */ > lpEditInfo; // LPEDITMENU    lpEditInfo;

  Pointer<CString> lpstrPrompt; // LPCSTR        lpstrPrompt;

  Pointer<Void> pvReserved; // void          *pvReserved;

  @Int32()
  int dwReserved; // DWORD         dwReserved;

  @Int32()
  int FlagsEx; // DWORD         FlagsEx;
}

sizeOf returns 168, but in Godbolt with the actual comdlg headers, the result is 152. https://godbolt.org/z/6Tyt3h

@sjindel-google sjindel-google added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi labels Sep 1, 2019
@sjindel-google
Copy link
Contributor

I don't see any indication in the commdlg.h that the struct is not using the normal alignment rules.

However, I did notice that some of the fields are conditionally included:

#ifdef _MAC
   LPEDITMENU   lpEditInfo;
   LPCSTR       lpstrPrompt;
#endif
#if (_WIN32_WINNT >= 0x0500)
   void *        pvReserved;
   DWORD        dwReserved;
   DWORD        FlagsEx;
#endif // (_WIN32_WINNT >= 0x0500)

Are you sure you were correct to include these?

@ds84182
Copy link
Contributor Author

ds84182 commented Sep 1, 2019

Ah sorry, my struct definition is incorrect, since the one in Win32 API docs don't match up. Thanks Microsoft. https://docs.microsoft.com/en-us/windows/win32/api/commdlg/ns-commdlg-openfilenamew

commdlg does include pshpack1.h which turns on byte packing on 32-bit targets, so 32-bit platforms are affected by it. https://godbolt.org/z/JAHfQ7

@dcharkes
Copy link
Contributor

@ds84182 did using the correct struct definition solve your problem?

@dcharkes
Copy link
Contributor

dcharkes commented Oct 25, 2019

For reference: most compilers support a form of struct packing with pragmas or attributes.

If we want to support those pragma's or attributes, then we should also think about supporting alignment attributes/pragmas on individual members of structs.

cc @mkustermann

@ds84182
Copy link
Contributor Author

ds84182 commented Oct 27, 2019

@dcharkes Yep, it solved my problem on 64-bit Windows, but I know on 32-bit Windows it'll be an issue still. I can't find any open-source C libraries that use packed structs via github search right now,

@dcharkes
Copy link
Contributor

@ds84182 Do you need packed structs in Win32, or is it just that the struct is different on Win32? You could work around the struct being different on 64 and 32 bit Windows by introducing a second struct just for win32, and duplicating the function signatures that use that struct. If that is really really un-ergonomic, that gives us a reason to address #35768.

@dcharkes
Copy link
Contributor

Adding support for packed structs should be relatively easy. (1) We need to specify a way that a struct is packed, and (2) our offset and size calculations need to take this into account.

Possible syntax

@Packed()
class MyStruct extends Struct {
  @Int8()
  int x;

  @Int64()
  int y;  // Is 1-byte aligned rather than 8-byte aligned.
} // Has a size of 9 bytes, rather than 12 or 16 bytes.

However, we might want to add .valueUnaligned and .valueUnaligned = to support unaligned reads and writes. Otherwise these packed structs might be unusable on 32-bit arm.

@timsneath
Copy link
Contributor

An example of a packed struct that I've just hit on Win32 is TASKDIALOGCONFIG, and I'm blocked because of this issue.

Per the declaration in CommCtrl.h:

#include <pshpack1.h> // declares #pragma pack(1)

typedef struct _TASKDIALOGCONFIG
{
    UINT        cbSize;     // 4 bytes (Uint32)
    HWND        hwndParent; // 8 bytes on 64-bit (IntPtr)
...
}

#include <poppack.h> // declares #pragma pack()

The sizeOf() for this struct is ultimately 16 bytes too long because of alignment issues.

@dcharkes dcharkes added this to the February Beta Release milestone Feb 11, 2021
dart-bot pushed a commit that referenced this issue Feb 11, 2021
This CL changes `@pragma('vm:ffi:struct-fields', [...])` to
`@pragma('vm:ffi:struct-fields', _FfiStructLayout([...]))` which makes
it easier to add more data in subsequent CLs.

Extends `FindPragma` to allow returning multiple matched pragma's, so
that we can filter them. (In this case to avoid matching user-defined
pragma's that do not have an instance of the private class.)

Separated out from https://dart-review.googlesource.com/c/sdk/+/183640
because of the extra constant in existing expectation files.

Bug: #35763
Bug: #38158

TEST=tests/ffi(_2)/*_by_value_*_test.dart

Change-Id: Idef9f82e9b53c2a32dffabcec19669eae550fe2f
Cq-Include-Trybots: luci.dart.try:front-end-nnbd-mac-release-x64-try,front-end-linux-release-x64-try,vm-precomp-ffi-qemu-linux-release-arm-try,vm-kernel-nnbd-linux-debug-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/184181
Reviewed-by: Martin Kustermann <kustermann@google.com>
Reviewed-by: Clement Skau <cskau@google.com>
dart-bot pushed a commit that referenced this issue Feb 11, 2021
Introduces the `NativeTypeCfe` type hierarchy for cleaner calculation
of sizes and offsets of native types and cleaner code generation.

The transformation ensures we're only visiting the structs in
topological order, which means the struct type can always look up its
(indirectly) nested structs in the `structCache`.

This simplifies adding inline arrays
(https://dart-review.googlesource.com/c/sdk/+/183640), packed structs,
and unions to this transformation.

`typedDataBaseOffset` is moved to the `FfiTransformer` because the
dependent CL uses it in the `FfiUseSiteTransformer`.

Bug: #35763
Bug: #38158
Bug: #38491

TEST=tests/ffi(_2)/(.*)by_value(.*)_test.dart

Change-Id: I345e02c48844ca795f9137a5addd5ba89992e1c9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/184421
Reviewed-by: Dmitry Stefantsov <dmitryas@google.com>
Reviewed-by: Clement Skau <cskau@google.com>
Commit-Queue: Daco Harkes <dacoharkes@google.com>
dart-bot pushed a commit that referenced this issue Feb 22, 2021
With packed structs, the x64 non-Windows ABI only put structs in CPU/FPU
registers when all it fields happen to be aligned.

This CL introduces the `NativeType::ContainsUnalignedMembers` query
for calling convention calculations.

Bug: #38158

tools/build.py run_ffi_unit_tests && tools/test.py ffi_unit
TEST=runtime/vm/compiler/ffi/native_calling_convention_test.cc
TEST=runtime/vm/compiler/ffi/native_type_test.cc
These tests are exercised on vm-precomp-ffi-qemu-linux-release-arm-try.

Change-Id: I504f67ae22a6af375d5eb57ff26b58d340099df8
Cq-Include-Trybots: luci.dart.try:vm-precomp-ffi-qemu-linux-release-arm-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/186142
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Clement Skau <cskau@google.com>
dart-bot pushed a commit that referenced this issue Mar 17, 2021
This CL does not add packed structs to `dart:ffi` yet, only to the mock
SDK in the analyzer for testing the analyzer. This means we can review
and land it separately.

Bug: #38158

Split off https://dart-review.googlesource.com/c/sdk/+/186143.

Change-Id: I9c9ac9154e3dec0e05242f57cf7dddf8406df5fb
Cq-Include-Trybots: luci.dart.try:analyzer-analysis-server-linux-try,analyzer-linux-release-try,analyzer-nnbd-linux-release-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/191700
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Daco Harkes <dacoharkes@google.com>
dart-bot pushed a commit that referenced this issue Mar 19, 2021
With packed structs, the x64 non-Windows ABI only put structs in CPU/FPU
registers when all it fields happen to be aligned. The previous CL did
not check the alignment of nested structs and structs in inline arrays
based on their offset in an outer struct.

Follow up of: https://dart-review.googlesource.com/c/sdk/+/186142.
Split off: https://dart-review.googlesource.com/c/sdk/+/186143.

Bug: #38158

tools/build.py run_ffi_unit_tests && tools/test.py ffi_unit
TEST=runtime/vm/compiler/ffi/native_type_test.cc
These tests are exercised on vm-precomp-ffi-qemu-linux-release-arm-try.

Change-Id: Ic64bdef2c13ac737dbf58864911f043fc7a3d831
Cq-Include-Trybots: luci.dart.try:vm-precomp-ffi-qemu-linux-release-arm-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/191720
Reviewed-by: Aske Simon Christensen <askesc@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi
Projects
None yet
Development

No branches or pull requests

5 participants