[Bug d/96153] New: d: struct literals have non-deterministic hash values

[Bug d/96153] New: d: struct literals have non-deterministic hash values

[ibuclaw at gdcproject dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96153

            Bug ID: 96153
           Summary: d: struct literals have non-deterministic hash values
           Product: gcc
           Version: 10.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: d
          Assignee: ibuclaw at gdcproject dot org
          Reporter: ibuclaw at gdcproject dot org
  Target Milestone: ---

struct Checked(T, Hook)
{
    private T payload;
    Hook hook;

    size_t toHash() const nothrow @safe
    {
        return hashOf(payload) ^ hashOf(hook);
    }
}

Checked!(T, Hook) checked(Hook, T)(const T value)
{
    return Checked!(T, Hook)(value);
}

@system unittest
{
    static struct Hook3
    {
        ulong var1 = ulong.max;
        uint var2 = uint.max;
    }

    assert(checked!Hook3(12).toHash() != checked!Hook3(13).toHash());
    assert(checked!Hook3(13).toHash() == checked!Hook3(13).toHash());
}

[Bug d/96153] d: struct literals have non-deterministic hash values

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96153

--- Comment #1 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Iain Buclaw <ibuclaw@gcc.gnu.org>:

https://gcc.gnu.org/g:2ac51bdf63b0e17d1b9974f30303fb24e3cbc83d

commit r11-2548-g2ac51bdf63b0e17d1b9974f30303fb24e3cbc83d
Author: Iain Buclaw <ibuclaw@gdcproject.org>
Date:   Wed Jul 22 09:50:38 2020 +0200

    d: Fix struct literals that have non-deterministic hash values (PR96153)

    Adds code generation for generating a temporary for, and pre-filling
    struct and array literals with zeroes before assigning, so that
    alignment holes don't cause objects to produce a non-deterministic hash
    value.  A new field has been added to the expression visitor to track
    whether the result is being generated for another literal, so that
    memset() is only called once on the top-level literal expression, and
    not for nesting struct or arrays.

    gcc/d/ChangeLog:

            PR d/96153
            * d-tree.h (build_expr): Add literalp argument.
            * expr.cc (ExprVisitor): Add literalp_ field.
            (ExprVisitor::ExprVisitor): Initialize literalp_.
            (ExprVisitor::visit (AssignExp *)): Call memset() on blits where
RHS
            is a struct literal.  Elide assignment if initializer is all
zeroes.
            (ExprVisitor::visit (CastExp *)): Forward literalp_ to generation
of
            subexpression.
            (ExprVisitor::visit (AddrExp *)): Likewise.
            (ExprVisitor::visit (ArrayLiteralExp *)): Use memset() to pre-fill
            object with zeroes.  Set literalp in subexpressions.
            (ExprVisitor::visit (StructLiteralExp *)): Likewise.
            (ExprVisitor::visit (TupleExp *)): Set literalp in subexpressions.
            (ExprVisitor::visit (VectorExp *)): Likewise.
            (ExprVisitor::visit (VectorArrayExp *)): Likewise.
            (build_expr): Forward literal_p to ExprVisitor.

    gcc/testsuite/ChangeLog:

            PR d/96153
            * gdc.dg/pr96153.d: New test.

[Bug d/96153] d: struct literals have non-deterministic hash values

[ro at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96153

Rainer Orth <ro at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ro at gcc dot gnu.org

--- Comment #2 from Rainer Orth <ro at gcc dot gnu.org> ---
The new test FAILs on 64-bit Solaris/SPARC:

+FAIL: gdc.dg/pr96153.d   -O1  execution test
+FAIL: gdc.dg/pr96153.d   -O1 -frelease  execution test
+FAIL: gdc.dg/pr96153.d   -O1 -frelease -g  execution test
+FAIL: gdc.dg/pr96153.d   -O1 -g  execution test
+FAIL: gdc.dg/pr96153.d   -O2  execution test
+FAIL: gdc.dg/pr96153.d   -O2 -frelease  execution test
+FAIL: gdc.dg/pr96153.d   -O2 -frelease -g  execution test
+FAIL: gdc.dg/pr96153.d   -O2 -g  execution test
+FAIL: gdc.dg/pr96153.d   -O3  execution test
+FAIL: gdc.dg/pr96153.d   -O3 -frelease  execution test
+FAIL: gdc.dg/pr96153.d   -O3 -frelease -g  execution test
+FAIL: gdc.dg/pr96153.d   -O3 -g  execution test

core.exception.AssertError@/vol/gcc/src/hg/master/local/gcc/testsuite/gdc.dg/pr96153.d(30):
unittest failure
----------------
/vol/gcc/src/hg/master/local/libphobos/libdruntime/gcc/deh.d:500 _d_throw
[0x10008262b]
/vol/gcc/src/hg/master/local/libphobos/libdruntime/core/exception.d:459
onAssertErrorMsg [0x100063503]
??:? void pr96153.__unittestL21_1() [0x10005c603]
??:? void pr96153.__modtest() [0x10005c613]
/vol/gcc/src/hg/master/local/libphobos/libdruntime/core/runtime.d:561
__foreachbody2 [0x100075167]
/vol/gcc/src/hg/master/local/libphobos/libdruntime/object.d:1599 __lambda2
[0x10006fce7]
/vol/gcc/src/hg/master/local/libphobos/libdruntime/rt/minfo.d:777
__foreachbody2 [0x10006ab07]
/vol/gcc/src/hg/master/local/libphobos/libdruntime/gcc/sections/elf_shared.d:109
int gcc.sections.elf_shared.DSO.opApply(scope int delegate(ref
gcc.sections.elf_shared.DSO)) [0x10005dadf]
/vol/gcc/src/hg/master/local/libphobos/libdruntime/rt/minfo.d:770 int
rt.minfo.moduleinfos_apply(scope int delegate(immutable(object.ModuleInfo*)))
[0x10006c1cf]
/vol/gcc/src/hg/master/local/libphobos/libdruntime/object.d:1598 int
object.ModuleInfo.opApply(scope int delegate(object.ModuleInfo*)) [0x100071bc3]
/vol/gcc/src/hg/master/local/libphobos/libdruntime/core/runtime.d:551
runModuleUnitTests [0x1000754bb]
/vol/gcc/src/hg/master/local/libphobos/libdruntime/rt/dmain2.d:496 runAll
[0x10005d1ff]
/vol/gcc/src/hg/master/local/libphobos/libdruntime/rt/dmain2.d:472 tryExec
[0x10005d3c3]
/vol/gcc/src/hg/master/local/libphobos/libdruntime/rt/dmain2.d:505 _d_run_main
[0x10005d3c3]
??:? main [0x10005c63b]
??:? _start [0x10005be4b]

The test PASSes with -O0 and -Os, as well as with -m32.

[Bug d/96153] d: struct literals have non-deterministic hash values

[ibuclaw at gdcproject dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96153

--- Comment #3 from Iain Buclaw <ibuclaw at gdcproject dot org> ---
(In reply to Rainer Orth from comment #2)
> The new test FAILs on 64-bit Solaris/SPARC:
> 
> The test PASSes with -O0 and -Os, as well as with -m32.

Thanks, I've checked Linux/SPARC64 and see the same thing.

[Bug d/96153] d: struct literals have non-deterministic hash values

[ibuclaw at gdcproject dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96153

--- Comment #4 from Iain Buclaw <ibuclaw at gdcproject dot org> ---
Created attachment 49006
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49006&action=edit
inline memset

(In reply to Iain Buclaw from comment #3)
> (In reply to Rainer Orth from comment #2)
> > The new test FAILs on 64-bit Solaris/SPARC:
> > 
> > The test PASSes with -O0 and -Os, as well as with -m32.
> 
> Thanks, I've checked Linux/SPARC64 and see the same thing.

DSE is removing the call to memset as dead code, so the optimizer is not
understanding the hint.

Inlining the memset using a bunch of MEM_REFs yields the correct result, and
infact the optimizer is able to fully const-fold the hash computation at -O3 so
all assert contracts are removed.

Though not sure about the approach, are there any pitfalls to this?

This is what optimized trees look like at -O1 and -O2 levels, with comments
inserted describing which bits are set.

x86_64 -O1:
  // 0..128 = 0
  MEM <uint128_t *> [(uint128_t *)&D.4244] = 0B;
  // 128..192 = 0
  MEM[(ulong *)&D.4244 + 16B] = 0B;
  // 0..32 = 12
  D.4244.payload = 12;
  // 128..160 = 4294967295
  D.4244.hook.var2 = 4294967295;
  // 64..128 = 18446744073709551615
  D.4244.hook.var1 = 18446744073709551615;

x86_64 -O2:
  // 0..64 = 12
  MEM <unsigned long> [(void *)&D.4244] = 12;
  // 64..128 = 18446744073709551615
  D.4244.hook.var1 = 18446744073709551615;
  // 128..192 = 4294967295
  MEM <unsigned long> [(void *)&D.4244 + 16B] = 4294967295;

SPARC64 -O1:
  // 0..128 = 51539607552
  MEM <uint128_t *> [(void *)&D.1048] = 51539607552B;
  // 64..128 = 18446744073709551615
  MEM <ulong> [(void *)&D.1048 + 8B] = 18446744073709551615;
  // 128..192 = -4294967296
  MEM <ulong *> [(void *)&D.1048 + 16B] = -4294967296B;

SPARC -O2
  // 0..128 = 51539607552
  MEM <uint128_t *> [(void *)&D.1048] = 51539607552B;
  // 64..128 = 18446744073709551615
  MEM <ulong> [(void *)&D.1048 + 8B] = 18446744073709551615;
  // 128..192 = -4294967296
  MEM <ulong *> [(void *)&D.1048 + 16B] = -4294967296B;


Both SPARC and x86_64 yield the same hash values.

  assert(-1444610504 != -1444610503);
  assert(-1444610503 == -1444610503);

[Bug d/96153] d: struct literals have non-deterministic hash values

[ibuclaw at gdcproject dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96153

Iain Buclaw <ibuclaw at gdcproject dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #49006|0                           |1
        is obsolete|                            |

--- Comment #5 from Iain Buclaw <ibuclaw at gdcproject dot org> ---
Created attachment 49010
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49010&action=edit
inline memset v2

Slight tweak to the patch to use indirect_ref instead (as mem_ref wasn't
understood either).

Generated code looks a lot more sensible now.

x86_64 -O1:
  // 0..128 = 0
  MEM[(uint128_t *)&D.4341] = 0;
  // 128..192 = 0
  MEM[(ulong *)&D.4341 + 16B] = 0;
  // 0..32 = 12
  D.4341.payload = 12;
  // 64..128 = 18446744073709551615
  D.4341.hook.var1 = 18446744073709551615;
  // 128..160 = 4294967295
  D.4341.hook.var2 = 4294967295;

x86_64 -O2:
  // 0..64 = 12
  MEM <unsigned long> [(void *)&D.4341] = 12;
  // 64..128 = 18446744073709551615
  MEM <unsigned long> [(void *)&D.4341 + 8B] = 18446744073709551615;
  // 128..192 = 4294967295
  MEM <unsigned long> [(void *)&D.4341 + 16B] = 4294967295;

SPARC64 -O1:
  // 0..128 = 0
  MEM[(uint128_t *)&D.1303] = 0;
  // 128..192 = 0
  MEM[(ulong *)&D.1303 + 16B] = 0;
  // 0..32 = 12
  D.1303.payload = 12;
  // 64..128 = 18446744073709551615
  D.1303.hook.var1 = 18446744073709551615;
  // 128..160 = 4294967295
  D.1303.hook.var2 = 4294967295;

SPARC64 -O2:
  // 0..64 = 12, 64..128 = 18446744073709551615
  MEM <uint128_t> [(void *)&D.1048] = 0xc00000000ffffffffffffffff;
  // 128..192 = 18446744069414584320
  MEM <ulong> [(void *)&D.1048 + 16B] = 18446744069414584320;

[Bug d/96153] d: struct literals have non-deterministic hash values

[ibuclaw at gdcproject dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96153

--- Comment #6 from Iain Buclaw <ibuclaw at gdcproject dot org> ---
The distinction between x86_64 passing and SPARC64 failing seems to be whether
the struct returns in memory, or via registers.  If via registers, alignment
holes that were filled are ignored by the caller (values are copied away from
the returned struct, so holes will not be carried over to the destination).

By that measure then, all initialization of any struct type that doesn't
satisfy identity_compare_p will require pre-filling with zeros.  There are
still some cases where this does not happen, such as in this testcase:
toHash(checked(12));

[Bug d/96153] d: struct literals have non-deterministic hash values

[ibuclaw at gdcproject dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96153

--- Comment #7 from Iain Buclaw <ibuclaw at gdcproject dot org> ---
Fixing the case for SPARC64 triggers the test case in pr96157 to fail on
x86_64.

[Bug d/96153] d: struct literals have non-deterministic hash values

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96153

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Iain Buclaw <ibuclaw@gcc.gnu.org>:

https://gcc.gnu.org/g:1db88844a22f75b13ced224415f645680784d354

commit r11-2864-g1db88844a22f75b13ced224415f645680784d354
Author: Iain Buclaw <ibuclaw@gdcproject.org>
Date:   Tue Aug 25 11:05:57 2020 +0200

    d: Fix small struct literals that have non-deterministic hash values

    Same issue as the initial commit that addressed PR96153, only this time to
fix
    it also for structs that are not returned in memory.  Tests have been added
    that triggered an assertion on x86_64, however the original test was
failing
    on SPARC 64-bit targets.

    gcc/d/ChangeLog:

            PR d/96153
            * d-codegen.cc (build_address): Create a temporary for CALL_EXPRs
            returning trivial aggregates, pre-filling it with zeroes.
            (build_memset_call): Use build_zero_cst if setting the entire
object.

    gcc/testsuite/ChangeLog:

            PR d/96153
            * gdc.dg/pr96153.d: Add new tests.

[Bug d/96153] d: struct literals have non-deterministic hash values

[ibuclaw at gdcproject dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96153
Bug 96153 depends on bug 96157, which changed state.

Bug 96157 Summary: d: No NRVO when returning an array of a non-POD struct
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96157

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED