Bug 28672 - Build fails on s390x due to layout of MonoDecimal
Summary: Build fails on s390x due to layout of MonoDecimal
Status: RESOLVED FIXED
Alias: None
Product: Runtime
Classification: Mono
Component: General ()
Version: unspecified
Hardware: Other Linux
: --- normal
Target Milestone: ---
Assignee: Rodrigo Kumpera
URL:
Depends on:
Blocks:
 
Reported: 2015-04-01 19:02 UTC by Neale Ferguson
Modified: 2016-04-14 15:20 UTC (History)
3 users (show)

Tags:
Is this bug a regression?: ---
Last known good build:


Attachments
Proposed fix for decimal-ms.c to enable building on big endian platforms (1.35 KB, application/octet-stream)
2015-04-01 19:02 UTC, Neale Ferguson
Details


Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report on GitHub or Developer Community with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:
Status:
RESOLVED FIXED

Description Neale Ferguson 2015-04-01 19:02:39 UTC
Created attachment 10595 [details]
Proposed fix for decimal-ms.c to enable building on big endian platforms

The definition of MonoDecimal is as follows:

typedef struct tagDECIMAL {
    // Decimal.cs treats the first two shorts as one long
    // And they seriable the data so we need to little endian
    // seriliazation
    // The wReserved overlaps with Variant's vt member
#if G_BYTE_ORDER != G_LITTLE_ENDIAN
    union {
        struct {
            uint8_t sign;
            uint8_t scale;
        } u;
        uint16_t signscale;
    } u;
    uint16_t reserved;
#else
    uint16_t reserved;
    union {
        struct {
            uint8_t scale;
            uint8_t sign;
        } u;
        uint16_t signscale;
    } u;
#endif
    uint32_t Hi32;
    union {
        struct {
            uint32_t Lo32;
            uint32_t Mid32;
        } v;
        uint64_t Lo64;
    } v;
} MonoDecimal;

On little endian platforms, overlaying Lo64 with Lo32 and Mid32 (in that order) is okay. However, this is not valid for big endian. Just swapping the order of Lo32 and Mid32 is not a fix. References to Lo64 are made through the macros DECIMAL_LO64_GET and DECIMAL_LO64_SET and need to change.

The attached patch is proposed to fix the problem. It builds cleanly on s390x and x864.
Comment 1 Rodrigo Kumpera 2015-04-02 11:32:02 UTC
Why swapping the order of Lo32 and Mid32 is not enough?
It should produce the same results as the change to the macros.
Comment 2 Neale Ferguson 2015-04-07 11:26:04 UTC
For some reason, this reply never made it to bugzilla:

The MonoDecimal structure maps to the C# layout of a decimal variable, so I assume that would also have to change the code in the reference source. Is that something that would be ok? Would that affect serialization between platforms?

I had tried that swap in decimal-ms.h but it did not work presumably because it requires changes to decimal.cs.
Comment 3 Rodrigo Kumpera 2015-04-07 11:46:48 UTC
I didn't know about that issue.

Then your approach makes more sense that what I proposed.

Can you make a PR out of your patch?
Comment 4 Rodrigo Kumpera 2016-04-14 05:56:23 UTC
Did we merge the proposed PR?
Comment 5 Neale Ferguson 2016-04-14 14:31:29 UTC
It appears to have been.
Comment 6 Rodrigo Kumpera 2016-04-14 15:20:58 UTC
Then I'll close this one.