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

Some enums have different values between x64 and arm64 on macOS and Mac Catalyst #12111

Closed
rolfbjarne opened this issue Jul 14, 2021 · 5 comments · Fixed by #12488
Closed

Some enums have different values between x64 and arm64 on macOS and Mac Catalyst #12111

rolfbjarne opened this issue Jul 14, 2021 · 5 comments · Fixed by #12488
Assignees
Labels
bug If an issue is a bug or a pull request a bug fix dotnet An issue or pull request related to .NET (6) dotnet-pri0 .NET 6: required for stable release
Milestone

Comments

@rolfbjarne
Copy link
Member

rolfbjarne commented Jul 14, 2021

Apple has defined a TARGET_ABI_USES_IOS_VALUES constant in their headers:

#define TARGET_ABI_USES_IOS_VALUES  (!TARGET_CPU_X86_64 || (TARGET_OS_IPHONE && !TARGET_OS_MACCATALYST))

which means TARGET_ABI_USES_IOS_VALUES is true on:

  • iOS, tvOS, watchOS (all abis): because TARGET_OS_IPHONE is 1 on these platforms
  • Mac Catalyst: arm64 (because !TARGET_CPU_X86_64)
  • macOS: arm64 (because !TARGET_CPU_X86_64)

and TARGET_ABI_USES_IOS_VALUES is false on:

  • Mac Catalyst: x64
  • macOS: x64

Searching Apple's headers, this shows up for the following enums:

typedef NS_ENUM(NSInteger, NSTextAlignment) {
    NSTextAlignmentLeft      = 0,    // Visually left aligned
#if TARGET_ABI_USES_IOS_VALUES
    NSTextAlignmentCenter    = 1,    // Visually centered
    NSTextAlignmentRight     = 2,    // Visually right aligned
#else /* !TARGET_ABI_USES_IOS_VALUES */
    NSTextAlignmentRight     = 1,    // Visually right aligned
    NSTextAlignmentCenter    = 2,    // Visually centered
#endif
    NSTextAlignmentJustified = 3,    // Fully-justified. The last line in a paragraph is natural-aligned.
    NSTextAlignmentNatural   = 4     // Indicates the default alignment for script
} API_AVAILABLE(macos(10.0), ios(6.0), watchos(2.0), tvos(9.0));
typedef NS_ENUM(NSInteger, NSImageResizingMode) {
#if !TARGET_ABI_USES_IOS_VALUES
    NSImageResizingModeStretch = 0,
    NSImageResizingModeTile = 1,
#else /* !TARGET_ABI_USES_IOS_VALUES */
    NSImageResizingModeTile = 0,
    NSImageResizingModeStretch = 1,
#endif /* !TARGET_ABI_USES_IOS_VALUES */
} API_AVAILABLE(macos(10.10));
typedef NS_ENUM(NSInteger, UIImageResizingMode) {
#if TARGET_ABI_USES_IOS_VALUES
    UIImageResizingModeTile = 0,
    UIImageResizingModeStretch = 1,
#else /* TARGET_ABI_USES_IOS_VALUES */
    UIImageResizingModeStretch = 0,
    UIImageResizingModeTile = 1,
#endif /* TARGET_ABI_USES_IOS_VALUES */
};
@rolfbjarne rolfbjarne added bug If an issue is a bug or a pull request a bug fix dotnet An issue or pull request related to .NET (6) dotnet-pri0 .NET 6: required for stable release labels Jul 14, 2021
@rolfbjarne rolfbjarne added this to the .NET 6 milestone Jul 14, 2021
@akoeplinger
Copy link
Member

I assume this is a variation of #11498 ?

@filipnavara
Copy link
Contributor

It is a similar issue. The difference with SSLCipherSuite is that it's a deprecated API and thus not very performance sensitive. Some of the same constraints apply though:

  • Existing code and libraries embed the (managed) enum values and thus it's less likely to break existing code if the old values are kept
  • For macOS the ARM64 is new platform so there's no existing code for it and the "reference" values are the x64 ones
  • Mac Catalyst is technically also new platform on the managed side but there's compatibility story with iOS and thus the enums would have to follow the iOS numbers

@rolfbjarne rolfbjarne changed the title Some enums have different values between x64 and arm64 on macOS Some enums have different values between x64 and arm64 on macOS and Mac Catalyst Jul 14, 2021
@spouliot
Copy link
Contributor

It's a breaking change to modify the values - and they differ between Xamarin.[iOS|tvOS|watchOS].dll and Xamarin.Mac.dll assemblies.

Xamarin.MacCatalyst.dll could be set to anything (not a breaking change yet) but it makes sense to keep it like Xamarin.iOS.dll. However the values needs to differ by architecture :-(

The values are also baked directly into (3rd parties) assemblies that refers to them... so that limit what we can do to fix this.

Proposal

I suggest adding extension methods, on all affected enum types, that gives the correct value. They can be optimized/removed later (e.g. by the linker) so they won't represent a large runtime cost.

The generator will also need to be aware of those types, i.e. to use the extension methods, not the enum values, e.g.

void SetAlignment (NSTextAlignment alignment)
{
   long value = alignment.GetArchSpecificValue ();
   Pinvoke (value);
}

Also manual p/invokes will need to use the (public) extensions methods to work properly. That will need to be documented.

Adding a new analyzer could also help in spotting incorrect usage inside user projects.

@rolfbjarne
Copy link
Member Author

I have a slightly different proposal :)

Proposal #2

We already have some code to convert values for these enums, because they're [Native] enums. We could augment that code so that we can do something like this:

[Native (ConvertToNative="NSTextAlignmentExtensions.ConvertToNative", ConvertToManaged="NSTextAlignmentExtension.ConvertToManaged")]

public static NSTextAlignment ConvertToNative (NSTextAlignment value)
{
    if (Runtime.IsARM64CallingConvention) {
       switch (value) {
       case NSTextAlignment.Center: return NSTextAlignment.Right;
       case NSTextAlignment.Right: return NSTextAlignment.Center;
       }
    }

    return value;
}

We'd have a managed enum that would define the values for managed code, and then we convert to the corresponding value whenever we enter/exit native code (exactly like we already do for the existing [Native] support).

The advantage would be that we'd not have to remember to use the extension functions, it would all be automatic (the only manual code would be the extension methods themselves).

It would likely be a bit more work to implement though.

@spouliot
Copy link
Contributor

Love it! The only downside is that [Native] already has some semantics wrt [u]int-[u]long and would not work for SslCipherSuite enum (or similar cases). Hopefully the list is complete and this is not something that should happen anymore...

@rolfbjarne rolfbjarne self-assigned this Aug 17, 2021
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Aug 19, 2021
…ions. Fixes xamarin#12111.

Augment the Native attribute for enums to support custom conversion functions between
native values and managed values for enums. This makes it possible to have different
values in managed code for an enum compared to native code.

This is necessary to support different native enum values based on the architecture,
because in a few cases Apple has different enum values between x86_64 and ARM64.
Enum values are constants in managed code, and without this support it would be impossible
to translate these correctly to native code.

The updated Native attribute supports two new fields: ConvertToNative and ConvertToManaged,
which are managed functions of a specific signature that the generator will emit
calls to whenever needed to do the appropriate conversion.

Fixes xamarin#12111.
rolfbjarne added a commit that referenced this issue Aug 20, 2021
…ions. Fixes #12111. (#12488)

Augment the Native attribute for enums to support custom conversion functions between
native values and managed values for enums. This makes it possible to have different
values in managed code for an enum compared to native code.

This is necessary to support different native enum values based on the architecture,
because in a few cases Apple has different enum values between x86_64 and ARM64.
Enum values are constants in managed code, and without this support it would be impossible
to translate these correctly to native code.

The updated Native attribute supports two new fields: ConvertToNative and ConvertToManaged,
which are managed functions of a specific signature that the generator will emit
calls to whenever needed to do the appropriate conversion.

Fixes #12111.
@ghost ghost locked as resolved and limited conversation to collaborators Apr 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug If an issue is a bug or a pull request a bug fix dotnet An issue or pull request related to .NET (6) dotnet-pri0 .NET 6: required for stable release
Projects
None yet
4 participants