[Bug d/103040] New: gdc.dg/torture/pr101273.d FAILs

[Bug d/103040] New: gdc.dg/torture/pr101273.d FAILs

[ro at gcc dot gnu.org]

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

            Bug ID: 103040
           Summary: gdc.dg/torture/pr101273.d FAILs
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: d
          Assignee: ibuclaw at gdcproject dot org
          Reporter: ro at gcc dot gnu.org
  Target Milestone: ---
            Target: i?86-pc-solaris2.11, sparc-sun-solaris2.11,
                    aarch64-suse-linux-gnu

Between 20211031 (0ef944629aebaa4ab2ca306762a9ff0e8537b597) and 20211101
(a11c53985a7080f9bf6143788ccb455dc9b0da21),
gdc.dg/torture/pr101273.d began to FAIL on a couple of targets:

+FAIL: gdc.dg/torture/pr101273.d   -O1  execution test
+FAIL: gdc.dg/torture/pr101273.d   -O1 -g  execution test

With -g3 added, one sees

core.exception.AssertError@/vol/gcc/src/hg/master/local/gcc/testsuite/gdc.dg/torture/pr101273.d(35):
Assertion failure
----------------
/vol/gcc/src/hg/master/local/libphobos/libdruntime/core/exception.d:441
onAssertError [0x80b44df]
/vol/gcc/src/hg/master/local/libphobos/libdruntime/core/exception.d:631
_d_assertp [0x80b46b8]
/vol/gcc/src/hg/master/local/gcc/testsuite/gdc.dg/torture/pr101273.d:35 _Dmain
[0x80afcff]

[Bug d/103040] gdc.dg/torture/pr101273.d FAILs

[ro at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.0

[Bug d/103040] gdc.dg/torture/pr101273.d FAILs

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Seen on x86_64-linux and i686-linux too.

[Bug d/103040] gdc.dg/torture/pr101273.d FAILs

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Just a wild guess, maybe r12-4814-gca84f39399fda80c770306465276ffd66d3766ed ?
esra dump is still identical, while ealias:
diff -up
/home/jakub/src/gcc/obj{46,48}/gcc/testsuite/gcc/pr101273.d.036t.ealias | grep
'^[+-]'
--- /home/jakub/src/gcc/obj46/gcc/testsuite/gcc/pr101273.d.036t.ealias 
2021-11-02 11:10:56.857782057 +0100
+++ /home/jakub/src/gcc/obj48/gcc/testsuite/gcc/pr101273.d.036t.ealias 
2021-11-02 11:06:24.211654521 +0100
-ESCAPED = ret
-ESCAPED = D.1562
-ESCAPED = &nrvo
-ESCAPED = &rvo
-ESCAPED = { NULL STRING ESCAPED NONLOCAL nrvo rvo }
+ESCAPED = { STRING ESCAPED NONLOCAL }
-nrvo = { NULL ESCAPED NONLOCAL } same as D.1567
-rvo = { NULL ESCAPED NONLOCAL } same as D.1570
+nrvo = { NULL NONLOCAL } same as D.1567
+rvo = { NULL NONLOCAL } same as D.1570
-D.1567 = { NULL ESCAPED NONLOCAL }
-_1 = { NULL ESCAPED NONLOCAL } same as D.1567
+D.1567 = { NULL NONLOCAL }
+_1 = { NULL NONLOCAL } same as D.1567
-D.1570 = { NULL ESCAPED NONLOCAL }
-_5 = { NULL ESCAPED NONLOCAL } same as D.1570
+D.1570 = { NULL NONLOCAL }
+_5 = { NULL NONLOCAL } same as D.1570
-ESCAPED, points-to non-local, points-to NULL, points-to vars: { D.1566 D.1569
} (escaped)
+ESCAPED, points-to non-local, points-to vars: { }
-_1, points-to non-local, points-to escaped, points-to NULL, points-to vars: {
}
-_5, points-to non-local, points-to escaped, points-to NULL, points-to vars: {
}
+_1, points-to non-local, points-to NULL, points-to vars: { }
+_5, points-to non-local, points-to NULL, points-to vars: { }
and fre1 right after that results in unconditional _d_assertp where it wasn't
unconditional Oct 29th.

[Bug d/103040] gdc.dg/torture/pr101273.d FAILs

[ibuclaw at gdcproject dot org]

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

--- Comment #3 from Iain Buclaw <ibuclaw at gdcproject dot org> ---
The test is trivially translatable into C++, though I don't think C++ attempts
to ensure (n)rvo always occurs.

---
#include <assert.h>

struct S101273
{
    int x;
    S101273* impl;
    S101273(int x)
    {
        this->x = x;
        this->impl = this;
    }
    ~S101273() { }
};

S101273 makeS101273()
{
    return S101273(2);
}

S101273 nrvo101273()
{
    S101273 ret = makeS101273();
    return ret;
}

S101273 rvo101273()
{
    return makeS101273();
}

int main()
{
    auto nrvo = nrvo101273();
    assert(&nrvo == nrvo.impl);

    auto rvo = rvo101273();
    assert(&rvo == rvo.impl);

    return 0;
}

[Bug d/103040] gdc.dg/torture/pr101273.d FAILs

[jakub at gcc dot gnu.org]

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Yeah, I've tried that through:
struct S { S () : x (0) { impl = this; } int x; S *impl; ~S () {} };

S
foo ()
{
  S ret;
  return ret;
}

S
bar ()
{
  return S ();
}

int
main ()
{
  S a = foo ();
  S b = bar ();
  if (a.impl != &a || b.impl != &b)
    __builtin_abort ();
}
but for C++ we don't miscompile that, though indeed starting with the above
mentioned commit at -O1 ealias info changes:
-ESCAPED = &a
-ESCAPED = &b
-ESCAPED = { ESCAPED NONLOCAL a b }
+ESCAPED = { }
-a = { ESCAPED NONLOCAL a } same as _1
-b = { ESCAPED NONLOCAL b } same as _2
+a = { NONLOCAL a } same as _1
+b = { NONLOCAL b } same as _2
-_1 = { ESCAPED NONLOCAL a }
-_2 = { ESCAPED NONLOCAL b }
+_1 = { NONLOCAL a }
+_2 = { NONLOCAL b }
-ESCAPED, points-to non-local, points-to vars: { D.2419 D.2421 } (escaped)
+ESCAPED, points-to vars: { }
-_1, points-to non-local, points-to escaped, points-to NULL, points-to vars: {
D.2419 } (escaped)
-_2, points-to non-local, points-to escaped, points-to NULL, points-to vars: {
D.2421 } (escaped)
+_1, points-to non-local, points-to NULL, points-to vars: { D.2419 }
+_2, points-to non-local, points-to NULL, points-to vars: { D.2421 }

[Bug d/103040] gdc.dg/torture/pr101273.d FAILs

[jakub at gcc dot gnu.org]

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
So the question is what is different between the C++ and D version.
In C++17, RVO is mandatory, NRVO is an optimization, but guess one could add a
copy constructor that would set impl to this and copy over x from the
argument's ->x, then regardless of whether NRVO happens or not the observable
result should be the same.
Like:
struct S101273
{
    int x;
    S101273* impl;
    S101273(int x)
    {
        this->x = x;
        this->impl = this;
    }
    S101273(const S101273 &o)
    {
        this->x = o.x;
        this->impl = this;
    }
    ~S101273() { }
};

S101273 makeS101273()
{
    return S101273(2);
}

S101273 nrvo101273()
{
    S101273 ret = makeS101273();
    return ret;
}

S101273 rvo101273()
{
    return makeS101273();
}

int main()
{
    auto nrvo = nrvo101273();
    if(&nrvo != nrvo.impl) __builtin_abort ();

    auto rvo = rvo101273();
    if(&rvo != rvo.impl) __builtin_abort ();

    return 0;
}

But that isn't miscompiled at -O1 either.

[Bug d/103040] gdc.dg/torture/pr101273.d FAILs

[jakub at gcc dot gnu.org]

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
One quite visible change is that D has those stmts that clear the vars before
it is constructed, so e.g. in main:
  SR.11_24 = 0;
  _12 = SR.11_24;
  MEM <uint128_t> [(unsigned char * {ref-all})&nrvo] = _12;
  nrvo = nrvo101273 (); [return slot optimization]
instead of C++
  nrvo = nrvo101273 (); [return slot optimization]

[Bug d/103040] gdc.dg/torture/pr101273.d FAILs

[ibuclaw at gdcproject dot org]

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

--- Comment #7 from Iain Buclaw <ibuclaw at gdcproject dot org> ---
(In reply to Jakub Jelinek from comment #6)
> One quite visible change is that D has those stmts that clear the vars
> before it is constructed, so e.g. in main:
>   SR.11_24 = 0;
>   _12 = SR.11_24;
>   MEM <uint128_t> [(unsigned char * {ref-all})&nrvo] = _12;
>   nrvo = nrvo101273 (); [return slot optimization]
> instead of C++
>   nrvo = nrvo101273 (); [return slot optimization]

The "this" pointer in D constructors are a reference rather than a pointer,
would that be enough to throw it off?

Haven't gotten round to finishing build on
a11c53985a7080f9bf6143788ccb455dc9b0da21 just yet to see.

[Bug d/103040] gdc.dg/torture/pr101273.d FAILs

[ibuclaw at gdcproject dot org]

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

--- Comment #8 from Iain Buclaw <ibuclaw at gdcproject dot org> ---
Still occurs after stubbing out the build_memset_call function in the D
front-end.  Just having a look at the this reference parameter now.

[Bug d/103040] gdc.dg/torture/pr101273.d FAILs

[ibuclaw at gdcproject dot org]

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

--- Comment #9 from Iain Buclaw <ibuclaw at gdcproject dot org> ---
Confirmed that r12-4814 is the first commit when this FAIL started to occur.

[Bug d/103040] gdc.dg/torture/pr101273.d FAILs

[ibuclaw at gdcproject dot org]

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

--- Comment #10 from Iain Buclaw <ibuclaw at gdcproject dot org> ---
(In reply to Iain Buclaw from comment #8)
> Still occurs after stubbing out the build_memset_call function in the D
> front-end.  Just having a look at the this reference parameter now.

Switched the return value of __ctor from S& to void, and changed the ref
parameter to a pointer as well.  FAIL still occurs on the small test, so there
must be some flag C++ front-end is setting that D is not.

Really, the code generated should be functionally identical.

[Bug d/103040] [12 Regression] gdc.dg/torture/pr101273.d FAILs

[hubicka at gcc dot gnu.org]

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

Jan Hubicka <hubicka at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2021-11-02
             Status|UNCONFIRMED                 |NEW

--- Comment #11 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Sorry for the breakage.

Looking at Jakub's comment:
  SR.11_24 = 0;
  _12 = SR.11_24;
  MEM <uint128_t> [(unsigned char * {ref-all})&nrvo] = _12;
  nrvo = nrvo101273 (); [return slot optimization]
instead of C++
  nrvo = nrvo101273 (); [return slot optimization]

I think this may be the problem - we represent the call as full write and thus
we do not expect
  MEM <uint128_t> [(unsigned char * {ref-all})&nrvo] = _12;
to survive.  We also do not expect the return slot argument to be read from -
it should be write only and the write should correspond to the size of
structure being initialized.

Things might have been working before simply because we made nrvo to escape and
this is flow insensitive and therefore we believed that nrvo101273 may need the
value.

[Bug d/103040] [12 Regression] gdc.dg/torture/pr101273.d FAILs

[jakub at gcc dot gnu.org]

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

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Jan Hubicka from comment #11)
> Sorry for the breakage.
> 
> Looking at Jakub's comment:
>   SR.11_24 = 0;
>   _12 = SR.11_24;
>   MEM <uint128_t> [(unsigned char * {ref-all})&nrvo] = _12;
>   nrvo = nrvo101273 (); [return slot optimization]
> instead of C++
>   nrvo = nrvo101273 (); [return slot optimization]
> 
> I think this may be the problem - we represent the call as full write and
> thus we do not expect
>   MEM <uint128_t> [(unsigned char * {ref-all})&nrvo] = _12;
> to survive.  We also do not expect the return slot argument to be read from
> - it should be write only and the write should correspond to the size of
> structure being initialized.

See above comments from Iain, even if that pre-initialization is removed it is
still miscompiled.  And, the testcase fails not because of the padding bits not
being zero, but because the address of self stored into one of the fields isn't
there or modref thinks it can't be changed or set to that.  But for
corresponding C++ it handles it ok.

Re: [Bug d/103040] [12 Regression] gdc.dg/torture/pr101273.d FAILs

[Jan Hubicka]

> See above comments from Iain, even if that pre-initialization is removed it is
> still miscompiled.  And, the testcase fails not because of the padding bits not
> being zero, but because the address of self stored into one of the fields isn't
> there or modref thinks it can't be changed or set to that.  But for
> corresponding C++ it handles it ok.
Perhaps TREE_ADDRESSABLE on the type which is being used to test whether
return slot pointer may escape.

[Bug d/103040] [12 Regression] gdc.dg/torture/pr101273.d FAILs

[hubicka at kam dot mff.cuni.cz]

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

--- Comment #13 from hubicka at kam dot mff.cuni.cz ---
> See above comments from Iain, even if that pre-initialization is removed it is
> still miscompiled.  And, the testcase fails not because of the padding bits not
> being zero, but because the address of self stored into one of the fields isn't
> there or modref thinks it can't be changed or set to that.  But for
> corresponding C++ it handles it ok.
Perhaps TREE_ADDRESSABLE on the type which is being used to test whether
return slot pointer may escape.

[Bug d/103040] [12 Regression] gdc.dg/torture/pr101273.d FAILs

[ibuclaw at gdcproject dot org]

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

--- Comment #14 from Iain Buclaw <ibuclaw at gdcproject dot org> ---
(In reply to hubicka from comment #13)
> > See above comments from Iain, even if that pre-initialization is removed it is
> > still miscompiled.  And, the testcase fails not because of the padding bits not
> > being zero, but because the address of self stored into one of the fields isn't
> > there or modref thinks it can't be changed or set to that.  But for
> > corresponding C++ it handles it ok.
> Perhaps TREE_ADDRESSABLE on the type which is being used to test whether
> return slot pointer may escape.

Non-POD types (all classes, and any struct with a postblit, copyctor, or dtor)
already have TREE_ADDRESSABLE set on the RECORD_TYPE.

[Bug d/103040] [12 Regression] gdc.dg/torture/pr101273.d FAILs

[ibuclaw at gdcproject dot org]

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

--- Comment #15 from Iain Buclaw <ibuclaw at gdcproject dot org> ---
Got it. The difference between D and C++ is a matter of early inlining.

The C++ example Jakub posted fails in the same way that D does if you compile
with: -O1 -fno-inline

Re: [Bug d/103040] [12 Regression] gdc.dg/torture/pr101273.d FAILs

[Jan Hubicka]

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103040
> 
> --- Comment #15 from Iain Buclaw <ibuclaw at gdcproject dot org> ---
> Got it. The difference between D and C++ is a matter of early inlining.
> 
> The C++ example Jakub posted fails in the same way that D does if you compile
> with: -O1 -fno-inline
Great, I will take a look now (I was travelling that is why i did not
started earlier)

Honza

[Bug d/103040] [12 Regression] gdc.dg/torture/pr101273.d FAILs

[hubicka at kam dot mff.cuni.cz]

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

--- Comment #16 from hubicka at kam dot mff.cuni.cz ---
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103040
> 
> --- Comment #15 from Iain Buclaw <ibuclaw at gdcproject dot org> ---
> Got it. The difference between D and C++ is a matter of early inlining.
> 
> The C++ example Jakub posted fails in the same way that D does if you compile
> with: -O1 -fno-inline
Great, I will take a look now (I was travelling that is why i did not
started earlier)

Honza

[Bug d/103040] [12 Regression] gdc.dg/torture/pr101273.d FAILs

[hubicka at kam dot mff.cuni.cz]

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

--- Comment #17 from hubicka at kam dot mff.cuni.cz ---
> Great, I will take a look now (I was travelling that is why i did not
> started earlier)
Found it - there is a thinko in way NOT_RETURNED flag is handled in the
call statement analysis.  I hope that would also explain the omnetpp
miscompile.  Thanks for the analysis!
Honza

[Bug d/103040] [12 Regression] gdc.dg/torture/pr101273.d FAILs

[cvs-commit at gcc dot gnu.org]

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

--- Comment #18 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jan Hubicka <hubicka@gcc.gnu.org>:

https://gcc.gnu.org/g:62af7d9402f551fa708125fafed2950d8912b25e

commit r12-4857-g62af7d9402f551fa708125fafed2950d8912b25e
Author: Jan Hubicka <hubicka@ucw.cz>
Date:   Wed Nov 3 01:45:47 2021 +0100

    Fix wrong code caulsed by retslot EAF flags propagation [PR103040]

    Fixe (quite nasty) thinko in how I propagate EAF flags from callee
    to caller.  In this case some flags needs to be changed.  In particular
      - EAF_NOT_RETURNED in callee does not really mean EAF_NOT_RETURNED in
caller
        since we speak of different return values
      - if callee escapes the parametr, we caller may return it
      - for retslot the rewritting is even bit more funny, since escaping to of
        return slot to return slot is not really an escape, however escape of
        argument to itself is.

    This patch should correct all of the cases above and does fix the testcase
from PR103040.

    Bootstrapped/regtested x86_64 with all languages.  Also lto-bootstrapped.

    gcc/ChangeLog:

            PR ipa/103040
            * ipa-modref.c (callee_to_caller_flags): New function.
            (modref_eaf_analysis::analyze_ssa_name): Use it.
            (ipa_merge_modref_summary_after_inlining): Fix whitespace.

    gcc/testsuite/ChangeLog:

            * g++.dg/torture/pr103040.C: New test.

[Bug d/103040] [12 Regression] gdc.dg/torture/pr101273.d FAILs

[marxin at gcc dot gnu.org]

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

Martin Liška <marxin at gcc dot gnu.org> changed:

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

--- Comment #19 from Martin Liška <marxin at gcc dot gnu.org> ---
I guess, it's fixed, Honza?

[Bug d/103040] [12 Regression] gdc.dg/torture/pr101273.d FAILs

[ibuclaw at gdcproject dot org]

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

--- Comment #20 from Iain Buclaw <ibuclaw at gdcproject dot org> ---
I don't see any failures for this test anymore, so inclined to say yes.

[Bug d/103040] [12 Regression] gdc.dg/torture/pr101273.d FAILs

[marxin at gcc dot gnu.org]

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

Martin Liška <marxin at gcc dot gnu.org> changed:

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

--- Comment #21 from Martin Liška <marxin at gcc dot gnu.org> ---
Fixed then.