[Bug ipa/106389] New: IPA modref breaks Safe Bitfields in C++

[Bug ipa/106389] New: IPA modref breaks Safe Bitfields in C++

[mpolacek at gcc dot gnu.org]

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

            Bug ID: 106389
           Summary: IPA modref breaks Safe Bitfields in C++
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: ipa
          Assignee: unassigned at gcc dot gnu.org
          Reporter: mpolacek at gcc dot gnu.org
                CC: marxin at gcc dot gnu.org
  Target Milestone: ---

The following test is miscompiled since r11-3308-gd119f34c952f87 (so the
problem goes away with -fno-ipa-modref).

void __assert_fail(const char *, const char *, int, const char *) {
__builtin_abort(); }
typedef int uint32_t;
template <int Offset> struct BitFieldMember {
  int value;
  operator int() { return value & 1; }
  BitFieldMember operator=(int v) {
    value = value | v << Offset;
    return *this;
  }
  BitFieldMember operator-=(int v) {
    *this ? void() : __assert_fail("", "", 9, __PRETTY_FUNCTION__);
    value -= v << Offset;
    return *this;
  }
};
union Status {
  struct {
    uint32_t value;
  } wrapper;
  Status(uint32_t v = 0) { wrapper.value = v; }
  operator uint32_t() { return wrapper.value; }
  BitFieldMember<0> readers;
  BitFieldMember<10> waitToRead;
  BitFieldMember<20> writers;
};
uint32_t u;
struct N {
  uint32_t m_status;
  void foo () {
    Status oldStatus = m_status, newStatus = oldStatus;
    u = oldStatus.waitToRead;
    newStatus.writers -= u;
    newStatus.readers = u;
    m_status = newStatus;
  }
};
int main() {
  Status status;
  status.writers = status = 1;
  N lock;
  lock.m_status = status;
  lock.foo ();
  if (lock.m_status != 1)
    __builtin_abort();
}

[Bug ipa/106389] [11/12/13 Regression] IPA modref breaks Safe Bitfields in C++

[mpolacek at gcc dot gnu.org]

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

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hubicka at gcc dot gnu.org
           Keywords|                            |wrong-code
           Priority|P3                          |P2
   Target Milestone|---                         |11.4
            Summary|IPA modref breaks Safe      |[11/12/13 Regression] IPA
                   |Bitfields in C++            |modref breaks Safe
                   |                            |Bitfields in C++

--- Comment #1 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
[class.union] says that "At most one of the
non-static data members of an object of union type can be active at any time,
that is, the value of at most one of the non-static data members can be stored
in a union at any time.", however, it also has an exception:

"One special guarantee is made in order to simplify the use of unions: If a
standard-layout union contains several standard-layout structs that share a
common initial sequence ([class.mem]), and if a non-static data member of an
object of this standard-layout union type is active and is one of the
standard-layout structs, it is permitted to inspect the common initial sequence
of any of the standard-layout struct members"

so in this test, using the non-active union member doesn't trigger UB.  More
info at <https://preshing.com/20150324/safe-bitfields-in-cpp/>.

[Bug ipa/106389] [11/12/13 Regression] IPA modref breaks Safe Bitfields in C++

[mpolacek at gcc dot gnu.org]

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

--- Comment #2 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Ooops, forgot to say: use -O2 to trigger the bug.

[Bug c++/106389] [11/12/13 Regression] IPA modref breaks Safe Bitfields in C++

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu.org
          Component|ipa                         |c++

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
There's a similar rule in C and we simply don't implement that - it's a "stupid
rule" given there's no good way to implement it.  Well.  -fno-strict-aliasing.

For GCC the allowed way is to access the common initial sequence through the
_union_ type.  Thus

union U { struct A { int i; } a; struct B { int i; float f; } b; };

union U *p;

p->a.i = 1;
.. = p->b.i;  // OK
B *b = &p->b;
... = b->i; // not OK

IPA modref simply makes this issue more visible (this across function
boundary).

Note the C rule doesn't involve unions but consider

A *a = &p->a;

  a->i = 1;
  ... = b->i;

I read the C++ rule as if that were allowed as well (*a could be inside a
union,
who knows - the union declaration might not even be visible in the TU!).

For the rule to take effect the frontend needs to mark each access that
falls under the rule as to have alias-set zero.

[Bug c++/106389] [11/12/13 Regression] IPA modref breaks Safe Bitfields in C++

[mpolacek at gcc dot gnu.org]

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

--- Comment #4 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #3)
> For the rule to take effect the frontend needs to mark each access that
> falls under the rule as to have alias-set zero.

Something like: make c_common_get_alias_set return -1 for those COMPONENT_REFs
in a standard-layout union that share a common initial sequence, and are
standard-layout structs?

[Bug c++/106389] [11/12/13 Regression] IPA modref breaks Safe Bitfields in C++

[hubicka at gcc dot gnu.org]

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

--- Comment #5 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
I agree with Richi that this is not modref bug. It merely exposes the fact that
we intentionally ignore this rule.

For single compilation unit this rule is already problematic since one can not
determine alias sets incrementally. If struct A and struct B share same initial
segment one needs to delay any TBAA decisions about them until the end of
translation unit when one knows if union of A and B exists and then one needs
to pesimize the optimization.

However standard does not seem to require union to be in the given translation
unit (think of comdat inlines where aliasing of A/B happens in one translation
unit and function is called from other). Moreover with LTO we do not stream
types not needed for a given partition of program and the union may thus be
lost on the way.

So I do not see how to implement this correctly short of
 1) giving up on base alias set disambiguations
 2) delaying all alias set constructions after finalizing all user defined
types, look for all possible initial segments and hack alias oracle to know
about this.  Even this is hard since we currently do disambiguations based on
pairs base_set, ref_set without knowing the offset of ref_set inside base_set
so given two pairs
 base_set1, ref_set1
 base_set2, ref_set2
we would not be able to disambiguate accesses with same ref sets that occurs
after the common initial part of the type.

So indeed I would prefer this to stay undefined and require may_alias attribute
in the user code...

[Bug c++/106389] [11/12/13 Regression] IPA modref breaks Safe Bitfields in C++

[rguenth at gcc dot gnu.org]

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

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Marek Polacek from comment #4)
> (In reply to Richard Biener from comment #3)
> > For the rule to take effect the frontend needs to mark each access that
> > falls under the rule as to have alias-set zero.
> 
> Something like: make c_common_get_alias_set return -1 for those
> COMPONENT_REFs in a standard-layout union that share a common initial
> sequence, and are standard-layout structs?

return 0, not -1 for them.  But note to be reliable you'd have to do this
for the actual aggregate type, not just COMPONENT_REFs since we have to
be able to re-query TBAA from the types (and COMPONENT_REFs might get lost).
See also the response from Honza.

union U { struct A { int i; } a; struct B { int i; int f; } b; } u;

is 'u.a' accessing the common initial sequence of A and B btw.?

[Bug c++/106389] [11/12/13/14 Regression] IPA modref breaks Safe Bitfields in C++

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|11.4                        |11.5

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 11.4 is being released, retargeting bugs to GCC 11.5.

[Bug c++/106389] [11/12/13/14 Regression] IPA modref breaks Safe Bitfields in C++

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2023-07-08
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW

--- Comment #8 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
.

[Bug c++/106389] [11/12/13/14 Regression] IPA modref breaks Safe Bitfields in C++

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |barryns86 at gmail dot com

--- Comment #9 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
*** Bug 112302 has been marked as a duplicate of this bug. ***