[Libraries][Android] Fix alignment for padding System.Decimal by mdh1418 · Pull Request #57978 · dotnet/runtime

mdh1418 · 2021-08-23T22:02:04Z

There is a difference in the current alignment of a decimal type on Android x86 (32bit) MONO_ABI_ALIGNOF (gpointer) which evaluates to 4 bytes and whats expected, which is 8 bytes. The behavior of MONO_ABI_ALIGNOF (gpointer) seems correct as gpointer is void* and 32-bit aligned pointers on a 32-bit platform makes sense. Instead, it's suspected that using MONO_ABI_ALIGNOF (gpointer) as the alignment is not correct for Android x86 if we are expecting an align of 8 bytes.

Attempting to remove the entire special casing for System Decimal in marshal.c didn't seem to work as noted in #49872 (comment)

This PR will adhere to the alignment value of 4 for Android x86, remove the special casing for System.Decimal because CoreCLR had removed their special casing, and modifies the expected values in the tests. https://github.com/dotnet/corefx/pull/30690/files?file-filters%5B%5D=.cs#r199212623 makes me suspect these tests have yet to be modified to fully accomodate Linux platforms.

lambdageek · 2021-08-24T01:28:43Z

Looks like x86 Windows and x86 Linux have different alignment for a struct like this https://godbolt.org/z/GvnosE9MK
So in this case we are bumping up Decimal so it has the same alignment on 32-bit Linux as on Windows.

Looks like wasm was special-cased in the test and doesn't work now.

mdh1418 · 2021-08-24T15:29:57Z

Looks like x86 Windows and x86 Linux have different alignment for a struct like this

It seems like then the expected behavior should be an align of 4, so instead, modified the expectations for the tests for Android x86.

lambdageek · 2021-08-24T16:42:26Z

@jkotas what's the right interop behavior for System.Decimal on non-Windows platforms? Should it match Windows padding/alignment or the host platform? Specifically, should we make it match Windows x86 when the host is Linux x86? Does CoreCLR make any guarantees here? I saw #38603 so it seems like on coreclr you just follow the normal host ABI, right - and it happens that on Windows it ends up matching the layout of the unmanaged DECIMAL struct?

jkotas · 2021-08-24T18:40:59Z

I saw #38603 so it seems like on coreclr you just follow the normal host ABI, right

Right, Decimal should be treated as ordinary blittable struct. It should not need any special alignment quirks anymore. "Remove Decimal struct special casing in marshal" change looks right to me.

mdh1418 · 2021-08-30T16:24:32Z

/backport to release/6.0

github-actions · 2021-08-30T16:24:46Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1182913073

mdh1418 · 2021-08-23T22:02:04Z

Fixes#49872

There is a difference in the current alignment of a decimal type on Android x86 (32bit) MONO_ABI_ALIGNOF (gpointer) which evaluates to 4 bytes and whats expected, which is 8 bytes. The behavior of MONO_ABI_ALIGNOF (gpointer) seems correct as gpointer is void* and 32-bit aligned pointers on a 32-bit platform makes sense. Instead, it's suspected that using MONO_ABI_ALIGNOF (gpointer) as the alignment is not correct for Android x86 if we are expecting an align of 8 bytes.

Attempting to remove the entire special casing for System Decimal in marshal.c didn't seem to work as noted in #49872 (comment)

This PR will adhere to the alignment value of 4 for Android x86, remove the special casing for System.Decimal because CoreCLR had removed their special casing, and modifies the expected values in the tests. https://github.com/dotnet/corefx/pull/30690/files?file-filters%5B%5D=.cs#r199212623 makes me suspect these tests have yet to be modified to fully accomodate Linux platforms.

lambdageek · 2021-08-24T01:28:43Z

Looks like x86 Windows and x86 Linux have different alignment for a struct like this https://godbolt.org/z/GvnosE9MK
So in this case we are bumping up Decimal so it has the same alignment on 32-bit Linux as on Windows.

Looks like wasm was special-cased in the test and doesn't work now.

mdh1418 · 2021-08-24T15:29:57Z

Looks like x86 Windows and x86 Linux have different alignment for a struct like this

It seems like then the expected behavior should be an align of 4, so instead, modified the expectations for the tests for Android x86.

lambdageek · 2021-08-24T16:42:26Z

@jkotas what's the right interop behavior for System.Decimal on non-Windows platforms? Should it match Windows padding/alignment or the host platform? Specifically, should we make it match Windows x86 when the host is Linux x86? Does CoreCLR make any guarantees here? I saw #38603 so it seems like on coreclr you just follow the normal host ABI, right - and it happens that on Windows it ends up matching the layout of the unmanaged DECIMAL struct?

jkotas · 2021-08-24T18:40:59Z

I saw #38603 so it seems like on coreclr you just follow the normal host ABI, right

Right, Decimal should be treated as ordinary blittable struct. It should not need any special alignment quirks anymore. "Remove Decimal struct special casing in marshal" change looks right to me.

mdh1418 · 2021-08-30T16:24:32Z

/backport to release/6.0

github-actions · 2021-08-30T16:24:46Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1182913073

mdh1418 · 2021-08-23T22:02:04Z

Fixes#49872

There is a difference in the current alignment of a decimal type on Android x86 (32bit) MONO_ABI_ALIGNOF (gpointer) which evaluates to 4 bytes and whats expected, which is 8 bytes. The behavior of MONO_ABI_ALIGNOF (gpointer) seems correct as gpointer is void* and 32-bit aligned pointers on a 32-bit platform makes sense. Instead, it's suspected that using MONO_ABI_ALIGNOF (gpointer) as the alignment is not correct for Android x86 if we are expecting an align of 8 bytes.

Attempting to remove the entire special casing for System Decimal in marshal.c didn't seem to work as noted in #49872 (comment)

This PR will adhere to the alignment value of 4 for Android x86, remove the special casing for System.Decimal because CoreCLR had removed their special casing, and modifies the expected values in the tests. https://github.com/dotnet/corefx/pull/30690/files?file-filters%5B%5D=.cs#r199212623 makes me suspect these tests have yet to be modified to fully accomodate Linux platforms.

lambdageek · 2021-08-24T01:28:43Z

Looks like x86 Windows and x86 Linux have different alignment for a struct like this https://godbolt.org/z/GvnosE9MK
So in this case we are bumping up Decimal so it has the same alignment on 32-bit Linux as on Windows.

Looks like wasm was special-cased in the test and doesn't work now.

mdh1418 · 2021-08-24T15:29:57Z

Looks like x86 Windows and x86 Linux have different alignment for a struct like this

It seems like then the expected behavior should be an align of 4, so instead, modified the expectations for the tests for Android x86.

lambdageek · 2021-08-24T16:42:26Z

@jkotas what's the right interop behavior for System.Decimal on non-Windows platforms? Should it match Windows padding/alignment or the host platform? Specifically, should we make it match Windows x86 when the host is Linux x86? Does CoreCLR make any guarantees here? I saw #38603 so it seems like on coreclr you just follow the normal host ABI, right - and it happens that on Windows it ends up matching the layout of the unmanaged DECIMAL struct?

jkotas · 2021-08-24T18:40:59Z

I saw #38603 so it seems like on coreclr you just follow the normal host ABI, right

Right, Decimal should be treated as ordinary blittable struct. It should not need any special alignment quirks anymore. "Remove Decimal struct special casing in marshal" change looks right to me.

mdh1418 · 2021-08-30T16:24:32Z

/backport to release/6.0

github-actions · 2021-08-30T16:24:46Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1182913073

mdh1418 · 2021-08-23T22:02:04Z

Fixes#49872

There is a difference in the current alignment of a decimal type on Android x86 (32bit) MONO_ABI_ALIGNOF (gpointer) which evaluates to 4 bytes and whats expected, which is 8 bytes. The behavior of MONO_ABI_ALIGNOF (gpointer) seems correct as gpointer is void* and 32-bit aligned pointers on a 32-bit platform makes sense. Instead, it's suspected that using MONO_ABI_ALIGNOF (gpointer) as the alignment is not correct for Android x86 if we are expecting an align of 8 bytes.

Attempting to remove the entire special casing for System Decimal in marshal.c didn't seem to work as noted in #49872 (comment)

This PR will adhere to the alignment value of 4 for Android x86, remove the special casing for System.Decimal because CoreCLR had removed their special casing, and modifies the expected values in the tests. https://github.com/dotnet/corefx/pull/30690/files?file-filters%5B%5D=.cs#r199212623 makes me suspect these tests have yet to be modified to fully accomodate Linux platforms.

lambdageek · 2021-08-24T01:28:43Z

Looks like x86 Windows and x86 Linux have different alignment for a struct like this https://godbolt.org/z/GvnosE9MK
So in this case we are bumping up Decimal so it has the same alignment on 32-bit Linux as on Windows.

Looks like wasm was special-cased in the test and doesn't work now.

mdh1418 · 2021-08-24T15:29:57Z

Looks like x86 Windows and x86 Linux have different alignment for a struct like this

It seems like then the expected behavior should be an align of 4, so instead, modified the expectations for the tests for Android x86.

lambdageek · 2021-08-24T16:42:26Z

@jkotas what's the right interop behavior for System.Decimal on non-Windows platforms? Should it match Windows padding/alignment or the host platform? Specifically, should we make it match Windows x86 when the host is Linux x86? Does CoreCLR make any guarantees here? I saw #38603 so it seems like on coreclr you just follow the normal host ABI, right - and it happens that on Windows it ends up matching the layout of the unmanaged DECIMAL struct?

jkotas · 2021-08-24T18:40:59Z

I saw #38603 so it seems like on coreclr you just follow the normal host ABI, right

Right, Decimal should be treated as ordinary blittable struct. It should not need any special alignment quirks anymore. "Remove Decimal struct special casing in marshal" change looks right to me.

mdh1418 · 2021-08-30T16:24:32Z

/backport to release/6.0

github-actions · 2021-08-30T16:24:46Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1182913073

mdh1418 · 2021-08-23T22:02:04Z

Fixes#49872

There is a difference in the current alignment of a decimal type on Android x86 (32bit) MONO_ABI_ALIGNOF (gpointer) which evaluates to 4 bytes and whats expected, which is 8 bytes. The behavior of MONO_ABI_ALIGNOF (gpointer) seems correct as gpointer is void* and 32-bit aligned pointers on a 32-bit platform makes sense. Instead, it's suspected that using MONO_ABI_ALIGNOF (gpointer) as the alignment is not correct for Android x86 if we are expecting an align of 8 bytes.

Attempting to remove the entire special casing for System Decimal in marshal.c didn't seem to work as noted in #49872 (comment)

This PR will adhere to the alignment value of 4 for Android x86, remove the special casing for System.Decimal because CoreCLR had removed their special casing, and modifies the expected values in the tests. https://github.com/dotnet/corefx/pull/30690/files?file-filters%5B%5D=.cs#r199212623 makes me suspect these tests have yet to be modified to fully accomodate Linux platforms.

lambdageek · 2021-08-24T01:28:43Z

Looks like x86 Windows and x86 Linux have different alignment for a struct like this https://godbolt.org/z/GvnosE9MK
So in this case we are bumping up Decimal so it has the same alignment on 32-bit Linux as on Windows.

Looks like wasm was special-cased in the test and doesn't work now.

mdh1418 · 2021-08-24T15:29:57Z

Looks like x86 Windows and x86 Linux have different alignment for a struct like this

It seems like then the expected behavior should be an align of 4, so instead, modified the expectations for the tests for Android x86.

lambdageek · 2021-08-24T16:42:26Z

@jkotas what's the right interop behavior for System.Decimal on non-Windows platforms? Should it match Windows padding/alignment or the host platform? Specifically, should we make it match Windows x86 when the host is Linux x86? Does CoreCLR make any guarantees here? I saw #38603 so it seems like on coreclr you just follow the normal host ABI, right - and it happens that on Windows it ends up matching the layout of the unmanaged DECIMAL struct?

jkotas · 2021-08-24T18:40:59Z

I saw #38603 so it seems like on coreclr you just follow the normal host ABI, right

Right, Decimal should be treated as ordinary blittable struct. It should not need any special alignment quirks anymore. "Remove Decimal struct special casing in marshal" change looks right to me.

mdh1418 · 2021-08-30T16:24:32Z

/backport to release/6.0

github-actions · 2021-08-30T16:24:46Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1182913073

mdh1418 · 2021-08-23T22:02:04Z

Fixes#49872

There is a difference in the current alignment of a decimal type on Android x86 (32bit) MONO_ABI_ALIGNOF (gpointer) which evaluates to 4 bytes and whats expected, which is 8 bytes. The behavior of MONO_ABI_ALIGNOF (gpointer) seems correct as gpointer is void* and 32-bit aligned pointers on a 32-bit platform makes sense. Instead, it's suspected that using MONO_ABI_ALIGNOF (gpointer) as the alignment is not correct for Android x86 if we are expecting an align of 8 bytes.

Attempting to remove the entire special casing for System Decimal in marshal.c didn't seem to work as noted in #49872 (comment)

This PR will adhere to the alignment value of 4 for Android x86, remove the special casing for System.Decimal because CoreCLR had removed their special casing, and modifies the expected values in the tests. https://github.com/dotnet/corefx/pull/30690/files?file-filters%5B%5D=.cs#r199212623 makes me suspect these tests have yet to be modified to fully accomodate Linux platforms.

lambdageek · 2021-08-24T01:28:43Z

Looks like x86 Windows and x86 Linux have different alignment for a struct like this https://godbolt.org/z/GvnosE9MK
So in this case we are bumping up Decimal so it has the same alignment on 32-bit Linux as on Windows.

Looks like wasm was special-cased in the test and doesn't work now.

mdh1418 · 2021-08-24T15:29:57Z

Looks like x86 Windows and x86 Linux have different alignment for a struct like this

It seems like then the expected behavior should be an align of 4, so instead, modified the expectations for the tests for Android x86.

lambdageek · 2021-08-24T16:42:26Z

@jkotas what's the right interop behavior for System.Decimal on non-Windows platforms? Should it match Windows padding/alignment or the host platform? Specifically, should we make it match Windows x86 when the host is Linux x86? Does CoreCLR make any guarantees here? I saw #38603 so it seems like on coreclr you just follow the normal host ABI, right - and it happens that on Windows it ends up matching the layout of the unmanaged DECIMAL struct?

jkotas · 2021-08-24T18:40:59Z

I saw #38603 so it seems like on coreclr you just follow the normal host ABI, right

Right, Decimal should be treated as ordinary blittable struct. It should not need any special alignment quirks anymore. "Remove Decimal struct special casing in marshal" change looks right to me.

mdh1418 · 2021-08-30T16:24:32Z

/backport to release/6.0

github-actions · 2021-08-30T16:24:46Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1182913073

mdh1418 · 2021-08-23T22:02:04Z

Fixes#49872

There is a difference in the current alignment of a decimal type on Android x86 (32bit) MONO_ABI_ALIGNOF (gpointer) which evaluates to 4 bytes and whats expected, which is 8 bytes. The behavior of MONO_ABI_ALIGNOF (gpointer) seems correct as gpointer is void* and 32-bit aligned pointers on a 32-bit platform makes sense. Instead, it's suspected that using MONO_ABI_ALIGNOF (gpointer) as the alignment is not correct for Android x86 if we are expecting an align of 8 bytes.

Attempting to remove the entire special casing for System Decimal in marshal.c didn't seem to work as noted in #49872 (comment)

This PR will adhere to the alignment value of 4 for Android x86, remove the special casing for System.Decimal because CoreCLR had removed their special casing, and modifies the expected values in the tests. https://github.com/dotnet/corefx/pull/30690/files?file-filters%5B%5D=.cs#r199212623 makes me suspect these tests have yet to be modified to fully accomodate Linux platforms.

lambdageek · 2021-08-24T01:28:43Z

Looks like x86 Windows and x86 Linux have different alignment for a struct like this https://godbolt.org/z/GvnosE9MK
So in this case we are bumping up Decimal so it has the same alignment on 32-bit Linux as on Windows.

Looks like wasm was special-cased in the test and doesn't work now.

mdh1418 · 2021-08-24T15:29:57Z

Looks like x86 Windows and x86 Linux have different alignment for a struct like this

It seems like then the expected behavior should be an align of 4, so instead, modified the expectations for the tests for Android x86.

lambdageek · 2021-08-24T16:42:26Z

@jkotas what's the right interop behavior for System.Decimal on non-Windows platforms? Should it match Windows padding/alignment or the host platform? Specifically, should we make it match Windows x86 when the host is Linux x86? Does CoreCLR make any guarantees here? I saw #38603 so it seems like on coreclr you just follow the normal host ABI, right - and it happens that on Windows it ends up matching the layout of the unmanaged DECIMAL struct?

jkotas · 2021-08-24T18:40:59Z

I saw #38603 so it seems like on coreclr you just follow the normal host ABI, right

Right, Decimal should be treated as ordinary blittable struct. It should not need any special alignment quirks anymore. "Remove Decimal struct special casing in marshal" change looks right to me.

mdh1418 · 2021-08-30T16:24:32Z

/backport to release/6.0

github-actions · 2021-08-30T16:24:46Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1182913073

mdh1418 · 2021-08-23T22:02:04Z

Fixes#49872

There is a difference in the current alignment of a decimal type on Android x86 (32bit) MONO_ABI_ALIGNOF (gpointer) which evaluates to 4 bytes and whats expected, which is 8 bytes. The behavior of MONO_ABI_ALIGNOF (gpointer) seems correct as gpointer is void* and 32-bit aligned pointers on a 32-bit platform makes sense. Instead, it's suspected that using MONO_ABI_ALIGNOF (gpointer) as the alignment is not correct for Android x86 if we are expecting an align of 8 bytes.

Attempting to remove the entire special casing for System Decimal in marshal.c didn't seem to work as noted in #49872 (comment)

This PR will adhere to the alignment value of 4 for Android x86, remove the special casing for System.Decimal because CoreCLR had removed their special casing, and modifies the expected values in the tests. https://github.com/dotnet/corefx/pull/30690/files?file-filters%5B%5D=.cs#r199212623 makes me suspect these tests have yet to be modified to fully accomodate Linux platforms.

lambdageek · 2021-08-24T01:28:43Z

Looks like x86 Windows and x86 Linux have different alignment for a struct like this https://godbolt.org/z/GvnosE9MK
So in this case we are bumping up Decimal so it has the same alignment on 32-bit Linux as on Windows.

Looks like wasm was special-cased in the test and doesn't work now.

mdh1418 · 2021-08-24T15:29:57Z

Looks like x86 Windows and x86 Linux have different alignment for a struct like this

It seems like then the expected behavior should be an align of 4, so instead, modified the expectations for the tests for Android x86.

lambdageek · 2021-08-24T16:42:26Z

@jkotas what's the right interop behavior for System.Decimal on non-Windows platforms? Should it match Windows padding/alignment or the host platform? Specifically, should we make it match Windows x86 when the host is Linux x86? Does CoreCLR make any guarantees here? I saw #38603 so it seems like on coreclr you just follow the normal host ABI, right - and it happens that on Windows it ends up matching the layout of the unmanaged DECIMAL struct?

jkotas · 2021-08-24T18:40:59Z

I saw #38603 so it seems like on coreclr you just follow the normal host ABI, right

Right, Decimal should be treated as ordinary blittable struct. It should not need any special alignment quirks anymore. "Remove Decimal struct special casing in marshal" change looks right to me.

mdh1418 · 2021-08-30T16:24:32Z

/backport to release/6.0

github-actions · 2021-08-30T16:24:46Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1182913073

[Libraries][Android] Fix alignment for padding System.Decimal
aa88ef3

mdh1418 requested review from EgorBo, jkotas and lambdageek August 23, 2021 22:02

mdh1418 requested review from thaystg and vargaz as code owners August 23, 2021 22:02

ghost added the area-System.Runtime.InteropServices label Aug 23, 2021

Remove other active issue
62415ee

Mitchell Hwang added 2 commits August 24, 2021 10:47

Remove trailing whitespaces
588bb49

Modify tests instead of the align behavior
228b823

Remove Decimal struct special casing in marshal
ddbe7c3

Remove wasm special case tests
babdf4e

lambdageek approved these changes Aug 25, 2021
View reviewed changes

mdh1418 merged commit 5c04363 into dotnet:mainAug 25, 2021

mdh1418 deleted the fix_android_x86_decimal_padding_alignment branch August 25, 2021 16:10

github-actionsBot mentioned this pull request Aug 30, 2021
[release/6.0] [Libraries][Android] Fix alignment for padding System.Decimal #58380
Merged

ghost locked as resolved and limited conversation to collaborators Sep 29, 2021