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

struct with 3 bytes not marshaled correctly with LLVM on watchOS #8486

Closed
rolfbjarne opened this issue Apr 27, 2018 · 8 comments · Fixed by #12992

Comments

@rolfbjarne
Copy link
Member

@rolfbjarne rolfbjarne commented Apr 27, 2018

Steps to Reproduce

  1. Download test case: struct-bbb-bug.zip
  2. Open solution, select the watch app as the startup project.
  3. Select a Release|Device configuration.
  4. Run.

Current Behavior

Application output:

[...]
Assertion failed: (s.x == 1), function set_s, file set_s.c, line 10.
0 -70 32
Xamarin.Hosting: Process '571' exited.

The code (in InterfaceController.cs) calls a P/Invoke that takes a struct with 3 bytes, managed code passes a struct with the values [1;2;3], native code receives [0;-72;32].

Other failing tests indicate that structs with 3 shorts and structs with 2 bytes have the same problem (I didn't bother creating separate test cases, since it looks like the same issue). Other variations work fine (structs with 1 or 4 bytes, structs with 1, 2 or 4 shorts, structs with 1, 2, 3 or 4 ints, longs, floats or doubles).

Expected Behavior

If running the debug configuration:

[...]
1 2 3
DONE

On which platforms did you notice this

watchOS with LLVM.

Version Used:

xamarin-macios/master (xamarin/xamarin-macios@bf15996) which is mono's 2018-02 branch.

It does not look like a regression (I can reproduce with d15-7, d15-6 and d15-5. I didn't try with earlier versions).

@luhenry

This comment has been minimized.

Copy link
Member

@luhenry luhenry commented Apr 27, 2018

/cc @vargaz

@lewurm

This comment has been minimized.

Copy link
Member

@lewurm lewurm commented May 7, 2018

that looks similar to #7449 which got fixed by #7691

maybe we have a similar issue with the llvm backend

@marek-safar

This comment has been minimized.

Copy link
Member

@marek-safar marek-safar commented May 28, 2018

@vargaz could you confirm that?

@marek-safar

This comment has been minimized.

Copy link
Member

@marek-safar marek-safar commented Jun 19, 2018

@vargaz what's the status of this issue?

@rolfbjarne

This comment has been minimized.

Copy link
Member Author

@rolfbjarne rolfbjarne commented Aug 20, 2018

Updated test case that tests for more signatures (structs with 1/2/3 bytes/shorts):
struct-bbb-bug-v2.zip

Result:

S1: expected <1>, got <52736>
S2: success
S3: expected <1, 2, 3>, got <1, 2, 1>
B1: expected <1>, got <0>
B2: expected <1, 2>, got <0, -50>
B3: expected <1, 2, 3>, got <0, -50, 38>
DONE status: False

This is using mono 2018-06.

@rolfbjarne

This comment has been minimized.

Copy link
Member Author

@rolfbjarne rolfbjarne commented Feb 7, 2019

Still happens with mono 2018-08.

@marek-safar

This comment has been minimized.

Copy link
Member

@marek-safar marek-safar commented Feb 7, 2019

@vargaz could you please address the issue

@marek-safar marek-safar added this to the 2019-02 (6.0.xx) milestone Feb 7, 2019
@lewurm

This comment has been minimized.

Copy link
Member

@lewurm lewurm commented Feb 8, 2019

can reproduce on watch4

S1: expected <1>, got <64000>
S2: success
S3: expected <1, 2, 3>, got <1, 2, 0>
B1: expected <1>, got <0>
B2: expected <1, 2>, got <0, -6>
B3: expected <1, 2, 3>, got <0, -6, 52>
2019-02-08 19:32:30.646296+0100 bug601711wappExtension[411:377807] DONE status: False (XI version: 12.7.0)

will look into it

@lewurm lewurm assigned lewurm and unassigned vargaz Feb 8, 2019
monojenkins added a commit that referenced this issue Feb 14, 2019
[2018-10] [bitcode] round up value type size for slot calculation

consider

```csharp
struct S3
{
    public ushort X;
    public ushort Y;
    public ushort Z;
}
```
this would be 3 * 2 = 6 bytes. Structs, however, are passed around as i32
arrays in LLVM IR between functions. Those, if such a struct is passed, are
split to `int32`s depending on the struct's size. The previous calulation was
wrongly reserving one such i32 slot instead of two for `S3` above.

Additionally increase slot size to i64 so that it aligns with a
slightly different ABI on arm64_32.


Fixes #8486

@rolfbjarne I've tested it on a arm64_32 device, please confirm that this fixes the issue on your armv7k device as well

Backport of #12992.

/cc @lewurm
monojenkins added a commit that referenced this issue Feb 15, 2019
[2018-12] [bitcode] round up value type size for slot calculation

consider

```csharp
struct S3
{
    public ushort X;
    public ushort Y;
    public ushort Z;
}
```
this would be 3 * 2 = 6 bytes. Structs, however, are passed around as i32
arrays in LLVM IR between functions. Those, if such a struct is passed, are
split to `int32`s depending on the struct's size. The previous calulation was
wrongly reserving one such i32 slot instead of two for `S3` above.

Additionally increase slot size to i64 so that it aligns with a
slightly different ABI on arm64_32.


Fixes #8486

@rolfbjarne I've tested it on a arm64_32 device, please confirm that this fixes the issue on your armv7k device as well

Backport of #12992.

/cc @lewurm
lewurm added a commit to lewurm/mono that referenced this issue May 10, 2019
Regression of mono#12992 & mono#14362

In the end, the actual fix boils down to:
```patch
--- a/mono/mini/mini-arm.c
+++ b/mono/mini/mini-arm.c
@@ -2374,7 +2374,7 @@ mono_arch_get_llvm_call_info (MonoCompile *cfg, MonoMethodSignature *sig)
                                lainfo->nslots = ALIGN_TO (ainfo->struct_size, 8) / 8;
                                lainfo->esize = 8;
                        } else {
-                               lainfo->nslots = ainfo->struct_size / sizeof (target_mgreg_t);
+                               lainfo->nslots = ALIGN_TO (ainfo->struct_size, sizeof (target_mgreg_t)) / sizeof (target_mgreg_t);
                                lainfo->esize = 4;
                        }
                        break;
```

Tested on `xamarin-macios/arm64_32-v3` branch:
* mscorlib
* mini
* dont link
* monotouch tests
* and a modified version of mono#8486 (comment) (added larger structs too)

with each device, Watch Series 3 (`armv7k`) and Watch Series 4 (`arm64_32`).

I hope that is the last iteration on this.
vargaz added a commit that referenced this issue May 20, 2019
* [arm] one more attempt to fix slotsize issue on llvmonly

Regression of #12992 & #14362

In the end, the actual fix boils down to:
```patch
--- a/mono/mini/mini-arm.c
+++ b/mono/mini/mini-arm.c
@@ -2374,7 +2374,7 @@ mono_arch_get_llvm_call_info (MonoCompile *cfg, MonoMethodSignature *sig)
                                lainfo->nslots = ALIGN_TO (ainfo->struct_size, 8) / 8;
                                lainfo->esize = 8;
                        } else {
-                               lainfo->nslots = ainfo->struct_size / sizeof (target_mgreg_t);
+                               lainfo->nslots = ALIGN_TO (ainfo->struct_size, sizeof (target_mgreg_t)) / sizeof (target_mgreg_t);
                                lainfo->esize = 4;
                        }
                        break;
```

Tested on `xamarin-macios/arm64_32-v3` branch:
* mscorlib
* mini
* dont link
* monotouch tests
* and a modified version of #8486 (comment) (added larger structs too)

with each device, Watch Series 3 (`armv7k`) and Watch Series 4 (`arm64_32`).

I hope that is the last iteration on this.

* remove arm64_32_abi
lewurm added a commit to lewurm/mono that referenced this issue May 20, 2019
Regression of mono#12992 & mono#14362

In the end, the actual fix boils down to:
https://gist.github.com/lewurm/6de007036b770ce0bdea4f3ae5c42558

Tested on `xamarin-macios/arm64_32-v3` branch:
* mscorlib
* mini
* dont link
* monotouch tests
* and a modified version of mono#8486 (comment) (added larger structs too)

with each device, Watch Series 3 (`armv7k`) and Watch Series 4 (`arm64_32`).

I hope that is the last iteration on this.
lewurm added a commit to lewurm/mono that referenced this issue May 20, 2019
* [arm] one more attempt to fix slotsize issue on llvmonly

Regression of mono#12992 & mono#14362

In the end, the actual fix boils down to:
```patch
--- a/mono/mini/mini-arm.c
+++ b/mono/mini/mini-arm.c
@@ -2374,7 +2374,7 @@ mono_arch_get_llvm_call_info (MonoCompile *cfg, MonoMethodSignature *sig)
                                lainfo->nslots = ALIGN_TO (ainfo->struct_size, 8) / 8;
                                lainfo->esize = 8;
                        } else {
-                               lainfo->nslots = ainfo->struct_size / sizeof (target_mgreg_t);
+                               lainfo->nslots = ALIGN_TO (ainfo->struct_size, sizeof (target_mgreg_t)) / sizeof (target_mgreg_t);
                                lainfo->esize = 4;
                        }
                        break;
```

Tested on `xamarin-macios/arm64_32-v3` branch:
* mscorlib
* mini
* dont link
* monotouch tests
* and a modified version of mono#8486 (comment) (added larger structs too)

with each device, Watch Series 3 (`armv7k`) and Watch Series 4 (`arm64_32`).

I hope that is the last iteration on this.

* remove arm64_32_abi
lewurm added a commit to lewurm/mono that referenced this issue May 20, 2019
* [arm] one more attempt to fix slotsize issue on llvmonly

Regression of mono#12992 & mono#14362

In the end, the actual fix boils down to:
```patch
--- a/mono/mini/mini-arm.c
+++ b/mono/mini/mini-arm.c
@@ -2374,7 +2374,7 @@ mono_arch_get_llvm_call_info (MonoCompile *cfg, MonoMethodSignature *sig)
                                lainfo->nslots = ALIGN_TO (ainfo->struct_size, 8) / 8;
                                lainfo->esize = 8;
                        } else {
-                               lainfo->nslots = ainfo->struct_size / sizeof (target_mgreg_t);
+                               lainfo->nslots = ALIGN_TO (ainfo->struct_size, sizeof (target_mgreg_t)) / sizeof (target_mgreg_t);
                                lainfo->esize = 4;
                        }
                        break;
```

Tested on `xamarin-macios/arm64_32-v3` branch:
* mscorlib
* mini
* dont link
* monotouch tests
* and a modified version of mono#8486 (comment) (added larger structs too)

with each device, Watch Series 3 (`armv7k`) and Watch Series 4 (`arm64_32`).

I hope that is the last iteration on this.

* remove arm64_32_abi
lewurm added a commit to lewurm/mono that referenced this issue May 20, 2019
* [arm] one more attempt to fix slotsize issue on llvmonly

Regression of mono#12992 & mono#14362

In the end, the actual fix boils down to:
```patch
--- a/mono/mini/mini-arm.c
+++ b/mono/mini/mini-arm.c
@@ -2374,7 +2374,7 @@ mono_arch_get_llvm_call_info (MonoCompile *cfg, MonoMethodSignature *sig)
                                lainfo->nslots = ALIGN_TO (ainfo->struct_size, 8) / 8;
                                lainfo->esize = 8;
                        } else {
-                               lainfo->nslots = ainfo->struct_size / sizeof (target_mgreg_t);
+                               lainfo->nslots = ALIGN_TO (ainfo->struct_size, sizeof (target_mgreg_t)) / sizeof (target_mgreg_t);
                                lainfo->esize = 4;
                        }
                        break;
```

Tested on `xamarin-macios/arm64_32-v3` branch:
* mscorlib
* mini
* dont link
* monotouch tests
* and a modified version of mono#8486 (comment) (added larger structs too)

with each device, Watch Series 3 (`armv7k`) and Watch Series 4 (`arm64_32`).

I hope that is the last iteration on this.

* remove arm64_32_abi
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.