[dart:ffi] Unnecessary constraints on packed nested structs

[timsneath]

Take the following example:

class RECT extends Struct {
  @Int32()
  external int left;
  @Int32()
  external int top;
  @Int32()
  external int right;
  @Int32()
  external int bottom;
}

@Packed(1)
class MCI_DGV_CAPTURE_PARMS extends Struct {
  @IntPtr()
  external int dwCallback;
  external Pointer<Utf16> lpstrFileName;
  external RECT rc;
}

FFI complains with the error:

Nesting the non-packed or less tightly packed struct 'RECT' in a packed struct 'MCI_DGV_CAPTURE_PARMS' is not supported.
Try packing the nested struct or packing the nested struct more tightly. dart(packed_nesting_non_packed)

This is unnecessarily restrictive. The equivalent C code is fine, as best as I can infer (here's the original that I'm mapping):

#pragma pack(1)
typedef struct {
    DWORD_PTR   dwCallback;
    LPWSTR  lpstrFileName;
    RECT    rc;
} MCI_DGV_CAPTURE_PARMS;
...

and in a separate file without a #pragma directive:

typedef struct tagRECT
{
    LONG    left;
    LONG    top;
    LONG    right;
    LONG    bottom;
} RECT, *PRECT, NEAR *NPRECT, FAR *LPRECT;

Any thoughts, @dcharkes? Marking RECT with @Packed(1) is not an option, since this is being auto-generated from the original header file.

[a-siva]

/cc @dcharkes

[dcharkes]

@timsneath, in this case it works because tagRECT would have exactly the same layout with and without packing.

However, if tagRECT would have been the following:

typedef struct tagRECT
{
    LONG    left;
    LONG    top;
    uint8_t    right;
    LONG    bottom;
} RECT, *PRECT, NEAR *NPRECT, FAR *LPRECT;

Then its layout would be different packed or non-packed.

Code generated for bottom would not expect to be unaliged, but my_ MCI_DGV_CAPTURE_PARMS.rc.bottom would be an unaligned access causing segfaults on Arm 32 bit.

The current analysis does not check whether any of the members would be unaligned if packing is enabled, so it is too conservative.

I think we have multiple options here:

1. Make the analysis smarter, and keep it an error, preventing compilation. (safety-first approach.)
2. Remove the analysis and let users run into wrong reads on Arm v7. (C compiler approach, with pass by reference semantics.)

Illustration of C compiler when using pointers: https://godbolt.org/z/9vz9W8EEa

uint32_t UseRect(RECT* rect){
    // Loads from offset 12 for bottom: ldr r0, [r0, #12]
    return rect->bottom;
}

UseRect(tagRECT*):
        sub     sp, sp, #4
        str     r0, [sp]
        ldr     r0, [sp]
        ldr     r0, [r0, #12]  // Loads from offset 12 for bottom, but should have loaded with offset 9.
        add     sp, sp, #4
        bx      lr

Any preference for one of the options @mkustermann @cskau-g ?

[mkustermann]

There's a few things here:

• Compared to predecessors, ARMv7 does allow unaligned access - depending on the instruction (see A3.2.1 Unaligned data access in the ISA). Normal loads/stores allow unaligned access - for loads/stores of floating point values one has to be careful by using the right instructions (see Issue 45009).

  Right now our LoadIndexedInstr/StoreIndexedInstr handle integer loads/stores differently for unaligned access by using byte-based loads.
  => We should fix this.

• Be less strict about nesting unpacked inside packed structs:

  a) We can allow usage of non-packed structs in packed structs as long as the non-packed ones don't contain floating point values.
  => That would be safe with no penalty in code-size, performance or safety.

  b) We could allow usage of all non-packed structs in packed structs by making all floating point load/stores use safe instructions.
  => That could come with a performance penalty but makes us stay in the safe world. If the penalty is negligible, it might be a good choice.

  c) We could allow usage of all non-packed structs in packed structs if the non-packed members naturally have the right alignment (no padding is inserted) - as mentioned by @dcharkes .
  => The issue with that approach is that it is ABI-specific whether padding is needed or not. So one couldn't make it a compile-time error in general without knowing the ABI.

  We could allow usage of all non-packed structs in packed structs without penalty - as mentioned by @dcharkes
  => This can cause app crashes that are hard to debug - especially for users.

I suggest to implement option a) above - which should fix @timsneath 's issue without downsides. This seems to be a quite rare case, so we can leave it there for now (until we actually have user demand for nesting with floating point types).

[ds84182]

Changing all loads to unaligned loads to fit a single usecase is quite destructive (performance-wise). Outside of FP, LDRD/STRD on ARMv7 requires 4-byte alignment. Otherwise 64-bit load/stores would need two instructions.

The way Rust handles this is by restricting the creation of references to unaligned fields. This allows code to still rely on alignment. In the above example, if you needed to pass a reference to the RECT to some other function for mutation, you'd have to copy it to stack first. https://rust.godbolt.org/z/1EGnK48hT

[sensuikan1973]

Marking RECT with @PACKED(1) is not an option, since this is being auto-generated from the original header file.

If we use ffigen, As a workaround, we can set structs->pack config.

structs:
  pack:
    'RECT': 1

[aeb-dev]

I am trying to write a generator for steamworks-sdk. I am not expert in this topic, but I think I have an error related to this issue.

I am not using ffigen btw.

Error:

Nesting the non-packed or less tightly packed struct 'SteamNetConnectionInfo' in a packed struct 'SteamNetworkingMessagesSessionFailed' is not supported.
Try packing the nested struct or packing the nested struct more tightly.dart(packed_nesting_non_packed)

C++ code:

// in isteamnetworkingmessage.h

#pragma pack( push, 1 )

struct SteamNetworkingMessagesSessionFailed_t
{ 
	SteamNetConnectionInfo_t m_info;
};

#pragma pack(pop)

// in steamnetworkingtypes.h

#pragma pack( push, 4 )

struct SteamNetConnectionInfo_t
{
	SteamNetworkingIdentity m_identityRemote;
	int64 m_nUserData;
	HSteamListenSocket m_hListenSocket;
	uint16 m__pad1;
	SteamNetworkingPOPID m_idPOPRemote;
	SteamNetworkingPOPID m_idPOPRelay;
	ESteamNetworkingConnectionState m_eState;
	int m_eEndReason;
	char m_szEndDebug[ k_cchSteamNetworkingMaxConnectionCloseReason ];
	char m_szConnectionDescription[ k_cchSteamNetworkingMaxConnectionDescription ];
	int m_nFlags;
	uint32 reserved[63];
};

#pragma pack(pop)

Generated dart code:

@Packed(1)
class SteamNetworkingMessagesSessionFailed extends Struct {
  external SteamNetConnectionInfo info;
}

@Packed(4)
class SteamNetConnectionInfo extends Struct {
  external SteamNetworkingIdentity identityRemote;

  @LongLong()
  external int userData;

  @UnsignedInt()
  external HSteamListenSocket listenSocket;

  external SteamNetworkingIpAddr addrRemote;

  @UnsignedShort()
  external int pad1;

  @UnsignedInt()
  external SteamNetworkingPOPId dPOPRemote;

  @UnsignedInt()
  external SteamNetworkingPOPId dPOPRelay;

  @Int32()
  external ESteamNetworkingConnectionState state;

  @Int()
  external int endReason;

  external Pointer<Utf8> endDebug;

  external Pointer<Utf8> connectionDescription;

  @Int()
  external int flags;

  @Array<UnsignedInt>(63)
  external Array<UnsignedInt> reserved;
}

[timsneath]

Yeah, this is the same issue, @aeb-dev.

What I see in https://stackoverflow.com/a/49132034 is that Dart is stricter than compiler convention (packing instructions are not transitive, therefore Dart should not enforce transitiveness). Given that we now have two common examples of this (Steamworks and Windows), I'd like to advocate for a loosening of our approach.

[dcharkes]

Fix underway: https://dart-review.googlesource.com/c/sdk/+/247382

Should we keep analyzer warnings/hints just in case people do it by accident? These warnings/hints can then be ignored with a // ignore to signal intent.

class Foo extends Struct {
  external Pointer<Uint8> notEmpty;
}

@Packed(1)
class Bar extends Struct {
  external Pointer<Uint8> notEmpty;

  // ignore: packed_nesting_non_packed
  external Foo nestedNotPacked;
}

In package:ffigen people could add // ignore_for_file: packed_nesting_non_packed to the preamble if needed.

[timsneath]

We could add an optional lint, but my understanding is that we try to avoid likely false positive in the lints package recommended list.

…

On Mon, Jun 6, 2022 at 12:32 PM Daco Harkes ***@***.***> wrote: Fix underway: https://dart-review.googlesource.com/c/sdk/+/247382 Should we keep analyzer warnings/hints just in case people do it by accident? These warnings/hints can then be ignored with a // ignore to signal intent. class Foo extends Struct { external Pointer<Uint8> notEmpty; } @PACKED(1)class Bar extends Struct { external Pointer<Uint8> notEmpty; // ignore: packed_nesting_non_packed external Foo nestedNotPacked; } In package:ffigen people could add // ignore_for_file: packed_nesting_non_packed to the preamble if needed. — Reply to this email directly, view it on GitHub <#46644 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARWL66BN6HBAKHCFYU5T2DVNZG27ANCNFSM5AP42GKA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[dcharkes]

@mit-mit who do we cc on lint/hint/warning questions?