Android and AArch64: tweak tests that trip #6790

joakim-noah · 2018-12-04T11:50:52Z

I'm still looking at some alignment issues with std.variant because Android/x64 uses an IEEE Quad 128-bit long double, will be finished once I track that down.

I'm targeting stable because we'll just pull these into the final release of ldc 1.13 when the final point release of dmd 2.083 is out. Only the /proc/cpuinfo change has the potential to affect the dmd release, since most everything else is versioned out for the official dmd release.

…is still readable.

dlang-bot · 2018-12-04T11:50:54Z

Thanks for your pull request, @joakim-noah!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "stable + phobos#6790"

thewilsonator

Alright, let me know when this is good to go.

thewilsonator · 2018-12-04T11:54:04Z

Also did you mean to create a branch for this pr rather that from your stable?

joakim-noah · 2018-12-04T11:54:11Z

std/math.d

@@ -4110,7 +4110,7 @@ real log(real x) @safe pure nothrow @nogc
 ///
 @safe pure nothrow @nogc unittest
 {
-    assert(log(E) == 1);
+    assert(feqrel(log(E), 1) >= real.mant_dig - 1);


The assert for equality only trips on Android with Quad reals, but not on linux/glibc/AArch64. Since it's a documented test, I can version this only for IEEE Quadruples like the std.complex test above and keep this simpler assert for readability if wanted.

joakim-noah · 2018-12-04T11:59:06Z

std/math.d

+    static if (floatTraits!(real).realFormat == RealFormat.ieeeQuadruple)
+    {
+        // This static assert overflows when cross-compiling from lower-precision
+        // floating-point reals at compile-time to higher precision at runtime.


As noted, this isn't an issue just for Quadruples, but also hits when say the compile-time real is 64-bit and runtime is 80-bit, as I just tested with ldc on Android/ARM cross-compiling to linux/x64 (a Win/MSVC target works however, as it also uses 64-bit reals).

I'm unsure how to check the compile-time real's precision however, so I just settled for disabling this test whenever compiling to a Quad runtime real. Anyone know how to get the compile-time real's precision?

@ibuclaw or @jpf91, any input?

As noted in the forum, there's no way of detecting the host's compile-time precision. Compiled code shouldn't have to know, and an assert makes perfect sense to guarantee the precision is sufficiently high. It clearly isn't with current LDC in all cross-compilation scenarios, as we don't have a software real_t yet.

So maybe weakening the static assert to a runtime enum <actual-ct-value> = ...; assert(...) would be a solution for this problem, ~~potentially~~ probably restricted to LDC only (and so not here upstream).

Doesn't affect gdc, as we have used soft float since the start.

These static asserts assume IEEE though. Non-IEEE reals could (and infact do in the case of ibm real) throw a compile time assert.

Alright, since this doesn't affect dmd, because it only supports Intel, or gdc, because it uses a wider-precision soft-float at compile-time, I'll keep this in ldc: removed here.

joakim-noah · 2018-12-04T16:20:21Z

I don't have a stable branch on my Phobos fork, deleted it a while back and just pushed this now for this pull, after which I'll delete it again. 😉

thewilsonator · 2018-12-09T13:01:27Z

This good to go?

joakim-noah · 2018-12-09T13:10:00Z

Almost, but no hurry now that the point release shipped.

kinke · 2018-12-10T18:54:05Z

std/variant.d

@@ -102,6 +102,23 @@ struct This;

 private alias This2Variant(V, T...) = AliasSeq!(ReplaceType!(This, V, T));

+// copied from std.traits and modified the base case


You could reduce the code duplication by simply checking for an empty tuple (and returning the pointer-alignment then, which seems like a valid guess, although max(size_t.alignof, real.alignof) would be safer), and otherwise use the std.traits version.

Made the std.traits version package, so I can reuse it like you said.

As for setting the maximum alignment of real or size_t, that hits the Android/x64 segfaults I mentioned in the ldc pull, when using align(16) for the union. It's using 128-bit alignment then, as the real alignment is always larger than 64-bit size_t there, and then segfaults when trying to MOVAPS a 64-bit aligned location into the FP register $xmm0. With size_t alignment, it uses a MOVUPS instead.

Another case where LLVM isn't expecting that __float128 alignment?

and then segfaults when trying to MOVAPS a 64-bit aligned location into the FP register $xmm0

That's exactly what shouldn't happen when aligning the union at 16 bytes. What that does though is change the min size of the union to 16 bytes (and move its offset in VariantN from 8 to 16), which might be unexpected for some test.

I mentioned max(size_t.alignof, real.alignof) for an empty AllowedTypes tuples (=> arbitrary types) as that should make sure it can be used as real too on platforms like Android x64. I'm pretty sure you'll hit alignment issues otherwise, just like the ones you currently get when using that max, for some bizarre reason.

So we're clear, the original failing test passes now: it's the other tests that start to fail if I use real.alignof when no types are passed to Variant, as I mentioned before. Looking into it more in a debugger and after hacking that first test block down to just two lines, Variant b = 42; assert(b.type == typeid(int));, it loads a 64-bit-aligned _D3std7variant__T8VariantNVmi32ZQp6__initZ and tries to MOVAPS the first 128-bit word and fails.

If I use this pull with size_t.alignof instead, all the std.variant tests pass, and it loads the FP registers from that same 64-bit aligned initializer with MOVUPS instructions instead.

I honestly don't care how this stuff is aligned, as long as the tests pass. 😉

it loads a 64-bit-aligned _D3std7variant__T8VariantNVmi32ZQp6__initZ

That seems to be the issue then - our (LDC) init symbols seem to violate the alignment.

Sorry, I wasn't really focused - movups is of course used for align(8). movaps should be fine, and the reduced testcase suggests LDC does the right thing by requesting 16-bytes alignment for the init symbol -> I guess it's really a linker issue, wouldn't be the first of this kind.

Should be easily testable; real.alignof is 16 for x86_64 Linux and macOS too, so if CI passes there, it's Android specific.

Alright, trying it.

Works on the auto-tester, could the Android/x64 linker be why std.variant passed its tests when I used align(16) and linked that module alone, but failed when I linked it into the test runner? If you think it's more correct this way, fine with me.

Wiped my build directory and rebuilt the phobos test runner from scratch, all the std.variant tests pass now. Maybe that 64-bit-aligned variant initializer was lying around in another object file and then getting picked up first by the linker every time I incrementally compiled std.variant alone. Sorry about the confusion, everything's working with this pull now, using the scheme you suggested.

Ah, very nice then.

kinke · 2018-12-10T19:01:08Z

std/variant.d

@@ -167,7 +184,7 @@ private:
    // state
    ptrdiff_t function(OpID selector, ubyte[size]* store, void* data) fptr
        = &handler!(void);
-    union
+    align(maxAlignment!(AllowedTypes)) union
    {
        ubyte[size] store;


If you move the align() down here, you won't change the alignment for AllowedTypes = { int[2], short[4] } for 64-bit targets (so that it would still be 8-bytes pointer-aligned due to the pointers array). It doesn't really matter, as the function pointer field makes sure the whole VariantN struct is still at least pointer-aligned and that the union's offset is 8; it'd just be good practice.

Don't really want to think about these alignment issues, just moved it down like you said: works on Android/x64.

joakim-noah · 2018-12-11T08:42:41Z

std/traits.d

@@ -4759,7 +4759,7 @@ template TemplateArgsOf(T : Base!Args, alias Base, Args...)
 }


-private template maxAlignment(U...)
+package template maxAlignment(U...)


This protection plus the modification was why I copied it before.

joakim-noah · 2018-12-11T09:07:22Z

Single dmd testsuite failure on linux seems spurious, since it passes everywhere else. Once the yaml-d issue is resolved, should be ready to go: removed the WIP title.

thewilsonator · 2018-12-11T11:04:34Z

This trips an assert in dyaml
../.dub/packages/dyaml-0.7.0/dyaml/source/dyaml/node.d(188,9): Error: static assert: "Unexpected YAML value size"

struct Node
{
    ...
    alias Value = Algebraic!(YAMLNull, YAMLMerge, bool, long, real, ubyte[], SysTime, string,
                         Node.Pair[], Node[], YAMLObject);
    ...
    Value value_;
    ...
    static assert(Value.sizeof <= 24, "Unexpected YAML value size");
}

because value is, with this change effectively

struct 
{
    void* functionPointer;
    align(16) ubyte[16];
}

for 32 bytes.

kinke · 2018-12-11T11:10:51Z

That's a bug in dyaml then. ;) - The layout can be optimized to struct VariantN { union { ... }; void* functionPointer }, that'll save padding.

Edit: Well, in this case it would just move the inner padding to trailing padding, the VariantN struct would still be 32-bytes large.

kinke · 2018-12-11T18:21:01Z

std/variant.d

+        enum maxVariantAlignment = size_t.alignof;
+    else
+    {
+        import std.traits : maxAlignment;


There's a global import std.traits at the top of the file.

joakim-noah · 2018-12-12T05:22:57Z

@bbasile and @wilzbach, thanks for updating dyaml and dlang-tour for this pull. However, dlang-tour appears to have broken when checking out now, which is stalling buildkite.

ghost · 2018-12-12T06:01:08Z

The problem is really related to dlang-tour, forced push won't help.

thewilsonator · 2018-12-12T06:08:24Z

Yeah dlang/tour fails on other build as well. Is this still WIP?

joakim-noah · 2018-12-12T06:18:10Z

Looks like the problem is the shallow clone, strange that that would affect the submodule. Just tried it and not reproducible for me on my Android tablet, running the same two commands for dlang-tour plus its English version.

joakim-noah · 2018-12-12T06:31:59Z

Is this still WIP?

Looks done to me.

wilzbach · 2018-12-12T14:14:36Z

Looks like the problem is the shallow clone, strange that that would affect the submodule. Just tried it and not reproducible for me on my Android tablet, running the same two commands for dlang-tour plus its English version.

See dlang/ci#363 which should hopefully fix Buildkite.
Anyhow, the failure is unrelated.

joakim-noah · 2018-12-17T06:40:06Z

Alright, fixed the Buildkite issue, but the build agents seem flakier and slow now: some hardware changes going on right now?

thewilsonator · 2018-12-17T06:41:06Z

Yeah I noticed that on a few dmd PRs recently, no idea.

wilzbach · 2018-12-17T11:31:14Z

@MartinNowak recently changed all agents to be auto-provisioned depending on the current load.

…nt.VariantN is properly aligned for Quadruples.

joakim-noah · 2018-12-18T00:58:21Z

OK, ready to merge, CI passes.

Android: much of /proc was made inaccessible since Oreo, but cpuinfo …

a3e12db

…is still readable.

joakim-noah requested review from CyberShadow, ibuclaw, dnadlinger and kyllingstad as code owners December 4, 2018 11:50

dlang-bot added the Review:WIP Work In Progress - not ready for review or pulling label Dec 4, 2018

thewilsonator approved these changes Dec 4, 2018

View reviewed changes

joakim-noah commented Dec 4, 2018

View reviewed changes

joakim-noah mentioned this pull request Dec 9, 2018

Merge v2.083.1 ldc-developers/ldc#2933

Merged

joakim-noah requested a review from andralex as a code owner December 10, 2018 17:15

kinke reviewed Dec 10, 2018

View reviewed changes

joakim-noah requested a review from PetarKirov as a code owner December 11, 2018 08:41

joakim-noah commented Dec 11, 2018

View reviewed changes

joakim-noah changed the title ~~[WIP] Android and AArch64: tweak tests that trip~~ Android and AArch64: tweak tests that trip Dec 11, 2018

thewilsonator mentioned this pull request Dec 11, 2018

Overzealous size assert in node dlang-community/D-YAML#205

Closed

ghost mentioned this pull request Dec 11, 2018

update dyaml to 0.7.1 dlang-tour/core#718

Merged

kinke reviewed Dec 11, 2018

View reviewed changes

thewilsonator added Merge:72h no objection -> merge The PR will be merged if there are no objections raised. and removed Review:WIP Work In Progress - not ready for review or pulling labels Dec 12, 2018

Loosen two tests for IEEE Quadruple precision and make sure std.varia…

073ea40

…nt.VariantN is properly aligned for Quadruples.

thewilsonator added Merge:auto-merge and removed Merge:72h no objection -> merge The PR will be merged if there are no objections raised. labels Dec 18, 2018

dlang-bot merged commit 43be36c into dlang:stable Dec 18, 2018

joakim-noah deleted the stable branch December 18, 2018 15:39

		@@ -102,6 +102,23 @@ struct This;

		private alias This2Variant(V, T...) = AliasSeq!(ReplaceType!(This, V, T));

		// copied from std.traits and modified the base case

Android and AArch64: tweak tests that trip #6790

Android and AArch64: tweak tests that trip #6790

Conversation

joakim-noah commented Dec 4, 2018 • edited Loading

dlang-bot commented Dec 4, 2018

Bugzilla references

Testing this PR locally

thewilsonator left a comment

Choose a reason for hiding this comment

thewilsonator commented Dec 4, 2018

joakim-noah Dec 4, 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 Dec 8, 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

joakim-noah commented Dec 4, 2018

thewilsonator commented Dec 9, 2018

joakim-noah commented Dec 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinke Dec 11, 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 Dec 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakim-noah commented Dec 11, 2018

thewilsonator commented Dec 11, 2018

kinke commented Dec 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakim-noah commented Dec 12, 2018

ghost commented Dec 12, 2018

thewilsonator commented Dec 12, 2018

joakim-noah commented Dec 12, 2018

joakim-noah commented Dec 12, 2018

wilzbach commented Dec 12, 2018

joakim-noah commented Dec 17, 2018

thewilsonator commented Dec 17, 2018

wilzbach commented Dec 17, 2018

joakim-noah commented Dec 18, 2018

joakim-noah commented Dec 4, 2018 •

edited

Loading

joakim-noah Dec 4, 2018 •

edited

Loading

kinke Dec 8, 2018 •

edited

Loading

kinke Dec 11, 2018 •

edited

Loading

kinke Dec 11, 2018 •

edited

Loading

kinke commented Dec 11, 2018 •

edited

Loading