Use primitive type

[jmkuhn]

No description provided.

[coveralls]

Pull Request Test Coverage Report for Build 190

• 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.3%) to 63.445%

---

Totals
Change from base Build 185:	-0.3%
Covered Lines:	151
Relevant Lines:	238

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 199

• 4 of 5 (80.0%) changed or added relevant lines in 1 file are covered.
• 50 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+1.05%) to 64.8%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/DecFP.jl	4	5	80.0%

Files with Coverage Reduction	New Missed Lines	%
src/DecFP.jl	50	64.8%

Totals
Change from base Build 185:	1.05%
Covered Lines:	162
Relevant Lines:	250

---

💛 - Coveralls

[stevengj]

Looks good except for the AppVeyor failure…

[stevengj]

Bump — can we get CI working here?

[jmkuhn]

Sorry, I've just recently gotten a local Windows system for testing and I'm still getting used to the working in a Windows environment. Windows is passing Dec32 and Dec64 tests but failing many Dec128 tests. I think we may need to use primitive types for Dec32 and Dec64 while using struct for Dec128. I'll look at this in the next few days to see if this works for both Windows and ARM.

[jmkuhn]

Possibly related JuliaLang/julia#22649

The C typedef for Decimal128 has ALIGN(16)

[stevengj]

We could always remove that alignment in C.

[simonbyrne]

We may be able to do something similar to Quadmath.jl:
https://github.com/JuliaMath/Quadmath.jl/blob/6bf23f464bf7e6996c3d85cdd2186336d24570fb/src/Quadmath.jl#L75-L81
See JuliaMath/Quadmath.jl#4 for more details.

[simonbyrne]

Removing the alignment in C is probably the simplest. The only downside is potential loss of compatibility.

[simonbyrne]

I don't quite understand why this would cause a failure though, since a struct and a primitive type should have the same alignment.

[stevengj]

@yuyichao, do you have any thoughts on the alignment issues here?

[yuyichao]

The commit message and comments above doesn't seem to give much context. From what I can guess from #90 the change in type is to match C ABI and the issue is crashes on windows?

There are many possible issues and it's hard to say what it is without looking at where exactly the crash happens.

1. It is possible that alignment specifier changes the ABI. Such change in ABI usually shouldn't result in a crash and should be just a incorrect result for a non-pointer primitive type but if the change changes the stack alignment assumption it's possible that the code could segfault.
2. Depending on what you use to compile the binary on windows, gcc is known to be bad at dealing with stack alignment on windows. It's mostly for avx2/32byte vector but other bugs are of course possible. It also has disagreement with msvc/clang on ABI in those cases.
3. A struct and a primitive type do have different ABI. (usually not cause crashing)

Instead of guessing, the simplest way is to just check the assembly code and possibly reproducing it with simpler functions.

[stevengj]

@yuyichao, would changing from a struct to a primitive type in Julia (for a 128-bit quantity) change the ABI on the Julia side?

[yuyichao]

Yes. And depending on what the issue/question is, that's the (3) above.

[jmkuhn]

The C typedefs are:

typedef unsigned int Decimal32;
typedef unsigned long long Decimal64;
typedef ALIGN(16) struct { unsigned long long w[2]; } Decimal128;

The definition for ALIGN depends on the compiler. Given that the ABI will vary between the primitives and struct, the PR now uses primitive type for Dec32 and Dec64 and struct for Dec128.

Is

struct Dec128 <: DecimalFloatingPoint
    x::UInt128
    Dec128(x::Number) = convert(Dec128, x)
    Base.reinterpret(::Type{Dec128}, x::UInt128) = new(x)
end

safe to use for Dec128 in place of the struct containing an array from C? @yuyichao is the ALIGN likely to cause problems? Should I remove it from the C lib?

The C compilers are whatever BinaryBuilder uses for cc on Linux, Windows, MacOS, FreeBSD on Intel, ARM, and PowerPC.

[yuyichao]

In general, yes you can have an issue whenever your declaration doesn’t match the c one. The only way to guarantee the abi is correct ignoring compiler bug is to remove the align declaration since Julia does not yet support it. (That is, unless you want to write llvmcall, which is doable....)

It is almost certainly possible you can find a type declaration that works for each arch and you just need to test it to make sure. With clang, you don’t even need to have access to all the environments since you can just cross compile and compare the generated llvm it.

[jmkuhn]

I'll submit a PR against DecFPBuilder to remove the ALIGN from the type declaration.

[stevengj]

Has anything been done in https://github.com/JuliaPackaging/Yggdrasil/blob/master/D/DecFP/build_tarballs.jl?

[jmkuhn]

I have a few more "easy" issues that I want to resolve, then I will look at this with issue #69.

[stevengj]

Rebased.

[giordano]

Looks like this fixed the segmentation faults on 32-bit systems

[stevengj]

But now it's failing on 64-bit ARM?

[stevengj]

I don't understand why it's still failing on the arm64 CI … I thought I reverted the changes on that arch? Oh, it's only failing on Julia 1.3, where the new BinaryPlatform stuff doesn't work.

[codecov]

Codecov Report

Merging #91 (38927bb) into master (a677c85) will decrease coverage by 1.64%.
The diff coverage is 52.94%.

@@            Coverage Diff             @@
##           master      #91      +/-   ##
==========================================
- Coverage   92.38%   90.74%   -1.65%     
==========================================
  Files           1        1              
  Lines         420      432      +12     
==========================================
+ Hits          388      392       +4     
- Misses         32       40       +8     

Impacted Files	Coverage Δ
src/DecFP.jl	90.74% <52.94%> (-1.65%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a677c85...38927bb. Read the comment docs.