Various tweaks for Android, plus an Alpine fix #2917

joakim-noah · 2018-11-17T17:18:49Z

Android/x64 mostly works, but has some issues because of their unusual choice of having long double be a __float128, likely to match AArch64.

I'm getting this up early to get feedback. There's also two issues with Phobos I'll look into: std.math.copysign won't compile with llvm_copysign on both x86 and x64 and many of the std.variant tests segfault on x64.

Otherwise, only a few tests fail, mostly the same ones as on AArch64 because of a couple trailing bits or missing IEEE Quadruple support:

diff --git a/std/complex.d b/std/complex.d
index eef57f864..8a90a98ce 100644
--- a/std/complex.d
+++ b/std/complex.d
@@ -939,7 +939,7 @@ deprecated
     }
     else
     {
-        assert(z1.re == z2.re && z1.im == z2.im);
+        //assert(z1.re == z2.re && z1.im == z2.im);
     }
 }
 
diff --git a/std/math.d b/std/math.d
index 6742ae105..3e5897b1e 100644
--- a/std/math.d
+++ b/std/math.d
@@ -4211,7 +4211,7 @@ real log(real x) @safe pure nothrow @nogc
 ///
 @safe pure nothrow @nogc unittest
 {
-    assert(log(E) == 1);
+    //assert(log(E) == 1);
 }
 
 /**************************************
diff --git a/std/numeric.d b/std/numeric.d
index 40f80440e..2c91ef0c1 100644
--- a/std/numeric.d
+++ b/std/numeric.d
@@ -610,7 +610,7 @@ public:
     }
 }
 
-@safe unittest
+version(none) @safe unittest
 {
     import std.meta;
     alias FPTypes =
@@ -651,7 +651,7 @@ public:
     // @system due to to!string(CustomFloat)
     import std.conv;
     CustomFloat!(5, 10) y = CustomFloat!(5, 10)(0.125);
-    assert(y.to!string == "0.125");
+    //assert(y.to!string == "0.125");
 }
 
 /**

joakim-noah · 2018-11-17T17:31:01Z

dmd/compiler.d

@@ -107,7 +107,7 @@ else
 }
        Identifier id = Id.entrypoint;
        auto m = new Module("__entrypoint.d", id, 0, 0);
-        scope p = new Parser!ASTCodegen(m, cmaincode, false);
+        auto p = new Parser!ASTCodegen(m, cmaincode, false);


The reason for all these changes from stack-allocating the parser to GC allocation is that the "bump-the-pointer" dmdfe GC aligns to 128-bit by default, whereas stack allocation aligns to 64-bit.

The problem is that when CTFloat.parse is called in Lexer.inreal, it segfaults when trying to assign to t.floatvalue, because it expects that union in the token to be 128-bit aligned and uses a MOVAPS instruction. If the parser is 128-bit aligned, then so is the token and the union and everything works. I tried putting an align(16) on the Parser or Lexer, but it makes no difference. Maybe getting scope to use those alignments is the real fix.

I checked why this doesn't hit on linux/x64, where the stack-allocated parser isn't 128-bit aligned either, and it's because LLVM generates a FSTPT when setting t.floatvalue there, which doesn't care about alignment.

So the C++ code assumes a 16-bytes alignment for 128-bit long double, which seems to make perfect sense. Judging by your comment and a superficial quick glance, the issue seems to be that the Token (or its nested union's) alignment on the D side doesn't match (as the Tokens seem to be allocated in D IIRC). And I would guess that that's due to the used host compiler's real.alignof not returning 16, which should be trivial to check. You can also just extract the Token declaration and check its alignment (and the union's offset) with the host compiler.

I played around with run.dlang.io, and it looks as if Token is 16-bytes aligned, and the union's offset 64 bytes: https://run.dlang.io/is/WnzZVI

If so, then these Tokens must be somehow wrongly allocated (in IR, we obviously use align 16 for locals on the stack).

After some more tests, I think it's scope; the Parser instance allocated on the stack only has an 8-bytes alignment (corresponding to the class reference alignment).

Edit: Ah yeah,

whereas stack allocation aligns to 64-bit [...] Maybe getting scope to use those alignments is the real fix.

that's exactly it, I should have read carefully.

The misalignment of scope class objects is a pretty big bug imo! Nice find.

[related: #1692]

OK, given the long-standing issue noting this misalignment, how should we deal with this? I can move this and the sprintf segfault workarounds to local patches in the Termux repo, as they only affect native Android/x64 builds, not cross-compiling. Don't want to clutter LDC up with hacks for such a niche platform as Android/x64.

Seems like a good reason to finally fix it, at least partially. As making sure scope allocations use the actual class alignment seems to be enough in this case, that'd be a good start.

joakim-noah · 2018-11-17T17:38:09Z

dmd/cppmangle.d

@@ -1196,7 +1196,7 @@ extern(C++):
            case Tint128:                c = 'n';       break;
            case Tuns128:                c = 'o';       break;
            case Tfloat64:               c = 'd';       break;
-            case Tfloat80:               c = 'e';       break;
+            case Tfloat80:               c = 'g';       break;


Along with using __float128 comes its own mangle, as can be seen in the comment block above, but only for Android/x64, llvm-mirror/clang@309b490. I guess I'll have to add a global.params.isAndroid and change this for the Android/x64 target alone.

joakim-noah · 2018-11-17T17:46:58Z

dmd/dmangle.d

@@ -913,7 +913,7 @@ public:
        {
            enum BUFFER_LEN = 36;
            char[BUFFER_LEN] buffer;
-            const n = CTFloat.sprint(buffer.ptr, 'A', value);
+            const n = CTFloat.sprint(buffer.ptr, 'g', value);


sprintf/printf from Bionic/x64 segfault for certain long double values passed to them with the hex format, at least in the Anbox Android/x64 environment I'm testing in, termux/termux-packages#3058. Maybe they gave up on implementing hex formatting in Bionic, as they simply closed this issue, android/ndk#437, and the Termux issue shows the garbled output on AArch64 too.

At least it doesn't segfault on AArch64, so the tests still pass, whereas a few tests in std.conv and std.algorithm.iteration get ldc to segfault in this sprintf when it's passed some floating-point numbers, such as 1.414. Simply using decimal formatting works around the Android/x64 bug.

No TLS support, no/buggy hex string <-> FP conversion... seriously? And then 128-bit software long double - I find Bionic pretty ridiculous as C runtime in 2018 created by one of the biggest software companies.

joakim-noah · 2018-11-20T03:32:16Z

dmd/cppmangle.d

-            case Tfloat80:               c = 'e';       break;
+            case Tfloat80:
+                // LDC: `long double` on Android/x64 is __float128 and mangled as `g`.
+                c = global.params.isAndroid && isArchx86_64() ? 'g' : 'e';


Apparently Kai added this before but Iain removed it, dlang/dmd#6887, because GDC probably doesn't reuse Tfloat80 for 128-bit floats, so this doesn't hit and goes to the Target.cppTypeMangle in the default case below instead.

That is probably preferable; the C++ mangling of Tfloat80 representing D real / C++ long double is target-specific with this now too (and most likely would be for proper PowerPC support too).

Updated, let me know if this new commit is what you had in mind, passes the tests.

kinke · 2018-11-21T19:23:10Z

I'd prefer Target::cppTypeMangle() to be implemented in our gen/target.cpp, where all or most of our Target specifics reside. [In there, it probably doesn't pay off to forward to a new TargetABI method yet for this one special case.] You have full access to the target triple on the C++ side and can get rid of most of the front-end adaptations (isAndroid etc., which all need to be guarded by version (IN_LLVM) btw, so pls don't modify upstream comments etc.).
You can version-out the case Tfloat80 line, simply fall through to the default case, and rely on our Target::cppTypeMangle() to return the correct string, with no further upstream mods (except for fwd-declaring the C++ implementation in the IN_LLVM block further up).

joakim-noah · 2018-12-02T16:59:58Z

Alright, changed the mangling in the file you wanted.

Ignore the Phobos commits, that's not going to be in the final version of this pull. I simply figured that if this pull is going to kick off a Shippable build, might as well take advantage and check some AArch64 stuff too.

kinke · 2018-12-02T18:54:32Z

Thx; I re-added the (versioned-out) upstream Target.cppTypeMangle(), slightly modified the cppmangle.d diff and reorganized a few (unrelated) things in gen/target.cpp/dmd/target.d for improved consistency.

kinke · 2018-12-03T20:04:30Z

So this is still working as intended with Target.cppTypeMangle(), right? Did you check const and indirections (ref real, real*) too? Then feel free to merge after removing the Phobos advancement.

joakim-noah · 2018-12-04T03:53:22Z

Haven't checked on Android/x64 yet with your changes or those additional cases, also want to get the std.variant/math issues ironed out. Need to look into those.

joakim-noah · 2018-12-10T09:40:12Z

Alright, looked into the std.variant failures more, two tests trip because of alignment issues when trying to access 128-bit reals. That first one trips because it tries comparing the struct members stored in MyVariant one by one, and segfaults when trying to load the 128-bit real at S.d with a MOVAPS instruction, which requires 128-bit alignment but the real is only 64-bit aligned. Checking the alignment and offset by inserting a pragma(msg, "V alignment is ", v.alignof, " and offset of struct is ", v.store.offsetof); gives 8LU for both.

OK, that's expected given the union it's using, so I tried sticking an align(16) on the union in VariantN just to see if it worked around the issue. After that, all the tests pass if I compile that module alone, but not if I build the full phobos2-test-runner. I start getting other tests segfaulting then, such as this block and this one, presumably other alignment issues for the codegen.

Not unexpected given the union and all the casts I see scattered around, but not sure the best way to fix this. I guess std.variant happens to work with 64-bit alignment and AArch64 128-bit reals that don't require 128-bit alignment, but not on Android/x64.

joakim-noah · 2018-12-10T10:22:12Z

Regarding the last std.math issue, ldc itself errors out in the backend when trying to use llvm_copysign in std.math.copysign on Android/x86 or x64:

LLVM ERROR: Cannot select: 0x7f56d69fac98: ch = store<(store 8 into %stack.1), trunc to f64> 0x7f56d2dd3758, 0x7f56d637b6e8, FrameIndex:i64<1>, undef:i64
0x7f56d637b6e8: f128,ch = CopyFromReg 0x7f56d2dd3758, Register:f128 %0
    0x7f56d0ff8270: f128 = Register %0             
0x7f56d637b270: i64 = FrameIndex<1>                                                               
0x7f56d571d270: i64 = undef
In function: _D3std4math5asinhFNaNbNiNfdZd

It works if I use the straight D bit-casting version just below or replace llvm_copysign with core.stdc.math.copysignl instead, which I believe is all the frontend is trying to do. I can dump the IR when using llvm_copysign just fine with --output-ll, implying the problem is with the IR we're sending to LLVM.

@kinke, you know this stuff best, what do you suggest?

kinke · 2018-12-10T11:25:32Z

OK, that's expected given the union it's using, so I tried sticking an align(16) on the union in VariantN just to see if it worked around the issue.

Yeah, that's definitely a bug in that VariantN template. The union is only pointer-aligned (if it's at least the size of a pointer, otherwise alignment = 1). It clearly needs to use at least the max alignment of AllowedTypes.

Regarding the last std.math issue, ldc itself errors out in the backend when trying to use llvm_copysign in std.math.copysign on Android/x86 or x64

Seems like LLVM reasonably doesn't expect an f128 type for that intrinsic for an x86 target. So std.math.copysign most likely needs a special case for that. There'll probably be more intrinsics like this.

joakim-noah · 2018-12-10T18:21:29Z

It clearly needs to use at least the max alignment of AllowedTypes.

Like this? All std.variant tests pass with that patch on Android/x64, and seem to be working most everywhere else too, only one third-party static assert in dyaml.

…re.sys.posix.sys.stat.stat_t.

joakim-noah · 2018-12-12T06:56:02Z

Pulled in the upstream Phobos commits too, will check those Android/x64 ABI cases you mentioned and this should be ready to go.

Once LLVM 7.0.1 is out, release 1.13 this weekend?

kinke · 2018-12-12T10:45:57Z

Yeah, that'd be the current plan.

joakim-noah · 2018-12-13T17:04:47Z

Alright, looked over your commit and checked those other Android/x64 mangles: const does nothing and ref/* mangle to Rg and Pg, just like clang does. Tests all pass: I'm done, so merge when ready.

kinke · 2018-12-23T14:05:31Z

The mangling adaptation introduced a regression wrt. backreferences (see https://forum.dlang.org/thread/[email protected]):

extern(C++) real _modulo(real x, real y);
pragma(msg, _modulo.mangleof);

_Z7_moduloee for LDC < 1.13 and DMD 2.083, _Z7_moduloeS_ for LDC 1.13.

JohanEngelen · 2018-12-23T14:13:19Z

--> #2953

joakim-noah commented Nov 17, 2018

View reviewed changes

joakim-noah commented Nov 20, 2018

View reviewed changes

joakim-noah mentioned this pull request Dec 9, 2018

Merge v2.083.1 #2933

Merged

joakim-noah mentioned this pull request Dec 11, 2018

Android and AArch64: tweak tests that trip dlang/phobos#6790

Merged

joakim-noah and others added 5 commits December 12, 2018 12:19

Android: change long double mangling on Android/x64 to g

1c82d12

Resync gen/target.cpp with dmd/target.d

8a87cad

Android: make x86/x64 std.math work and cherrypick other upstream fixes.

32805f6

Android: don't use ld.gold by default yet.

8eee08b

Alpine: get Phobos building again by adding attributes it needs to co…

026e912

…re.sys.posix.sys.stat.stat_t.

joakim-noah changed the title ~~[WIP] Android: get x64 working~~ Various tweaks for Android, plus an Alpine fix Dec 13, 2018

kinke merged commit dbd2ba4 into ldc-developers:master Dec 14, 2018

joakim-noah deleted the x64 branch December 15, 2018 18:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various tweaks for Android, plus an Alpine fix #2917

Various tweaks for Android, plus an Alpine fix #2917

joakim-noah commented Nov 17, 2018 •

edited

Loading

joakim-noah Nov 17, 2018 •

edited

Loading

kinke Nov 17, 2018

kinke Nov 17, 2018

kinke Nov 17, 2018 •

edited

Loading

JohanEngelen Nov 17, 2018

kinke Nov 17, 2018 •

edited

Loading

joakim-noah Nov 19, 2018

kinke Nov 19, 2018

joakim-noah Nov 17, 2018 •

edited

Loading

kinke Nov 17, 2018

joakim-noah Nov 17, 2018 •

edited

Loading

kinke Nov 17, 2018

joakim-noah Nov 20, 2018

kinke Nov 20, 2018

joakim-noah Nov 21, 2018

kinke commented Nov 21, 2018 •

edited

Loading

joakim-noah commented Dec 2, 2018

kinke commented Dec 2, 2018 •

edited

Loading

kinke commented Dec 3, 2018

joakim-noah commented Dec 4, 2018

joakim-noah commented Dec 10, 2018

joakim-noah commented Dec 10, 2018

kinke commented Dec 10, 2018 •

edited

Loading

joakim-noah commented Dec 10, 2018

joakim-noah commented Dec 12, 2018

kinke commented Dec 12, 2018

joakim-noah commented Dec 13, 2018 •

edited

Loading

kinke commented Dec 23, 2018

JohanEngelen commented Dec 23, 2018

Various tweaks for Android, plus an Alpine fix #2917

Various tweaks for Android, plus an Alpine fix #2917

Conversation

joakim-noah commented Nov 17, 2018 • edited Loading

joakim-noah Nov 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinke Nov 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinke Nov 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakim-noah Nov 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakim-noah Nov 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinke commented Nov 21, 2018 • edited Loading

joakim-noah commented Dec 2, 2018

kinke commented Dec 2, 2018 • edited Loading

kinke commented Dec 3, 2018

joakim-noah commented Dec 4, 2018

joakim-noah commented Dec 10, 2018

joakim-noah commented Dec 10, 2018

kinke commented Dec 10, 2018 • edited Loading

joakim-noah commented Dec 10, 2018

joakim-noah commented Dec 12, 2018

kinke commented Dec 12, 2018

joakim-noah commented Dec 13, 2018 • edited Loading

kinke commented Dec 23, 2018

JohanEngelen commented Dec 23, 2018

joakim-noah commented Nov 17, 2018 •

edited

Loading

joakim-noah Nov 17, 2018 •

edited

Loading

kinke Nov 17, 2018 •

edited

Loading

kinke Nov 17, 2018 •

edited

Loading

joakim-noah Nov 17, 2018 •

edited

Loading

joakim-noah Nov 17, 2018 •

edited

Loading

kinke commented Nov 21, 2018 •

edited

Loading

kinke commented Dec 2, 2018 •

edited

Loading

kinke commented Dec 10, 2018 •

edited

Loading

joakim-noah commented Dec 13, 2018 •

edited

Loading