[Bug c++/109495] New: Stack is used (unexpectedly) for copying on-heap objects (while clang doesn't)

[Bug c++/109495] New: Stack is used (unexpectedly) for copying on-heap objects (while clang doesn't)

[gcc-bug-reports at xhtml dot guru]

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

            Bug ID: 109495
           Summary: Stack is used (unexpectedly) for copying on-heap
                    objects (while clang doesn't)
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: gcc-bug-reports at xhtml dot guru
  Target Milestone: ---

Could be a bug or could be a feature -- can't tell. Please help understand why
GCC involves stack in certain conditions when copying on-heap objects (no
problem in clang).

Problem:

In actual live project I have a large struct/class with a lot of data in it ("a
lot" is defined as larger than Wframe-larger-than), I've also got many
locations where that struct is copied (directly or indirectly), and got
Wframe-larger-than enabled. In some cases I'm hitting the error on lines that
_indirectly_ copy-construct the object from an object that's stored on heap
into an object that is also stored on heap (i.e. heap-to-heap operation, if you
will).

After finding minimal case that reproduces the issue
(https://godbolt.org/z/xcnP9E39a), disassembling the code and looking into
potential reasons why GCC might want to use the stack (and thus trigger
Wframe-larger-than), I see that (subjectively for no apparent reason) GCC
involves stack as an intermediate buffer, which I am looking to avoid. clang
doesn't use intermediate buffer in this same scenario. Some surprising
observations (which make the problem go away) might be indicative of a bug in
GCC:

1) If private member "y" of class Parent is made public, Wframe-larger-than
goes away (https://godbolt.org/z/G7x5EWKEf);
2) If type of member "s" of struct Child is changed from std::string to int,
the problem goes away (https://godbolt.org/z/zK6Wjj8nK);
3) If class Parent is adjusted so that there is no padding at the end, for
example by changing type of "z" member of class Parent to short
(https://godbolt.org/z/jsbsc6jcb) or by deleting member "z" altogether
(https://godbolt.org/z/3jqaoKse9), the problem goes away;
4) If inheritance is taken out of the equation, for example by aggregating
class Parent below class Child (instead of inheriting;
https://godbolt.org/z/451K1s7bc), the problem goes away.

Expected behavior: GCC not reporting Wframe-larger-than.

Actual behavior: GCC reports Wframe-larger-than.


// Code (attached as test.cpp):

#include <string>

class Parent
{
private:
    unsigned char y[ 1024 * 64 ];           // Beef up class' size past
Wframe-larger-than

public:
    short x;                                // sizeof == 2 -> increases
alignment requirement to 2
    char z;                                 // sizeof == 1 -> triggers padding
(1) at the end of struct
};

struct Child : public Parent
{
    std::string s;
};

int main( int, char** )
{
    auto* ptr = new Child();
    auto* ptr2 = new Child( *ptr );         // g++ reports frame-larger-than
violation; clang doesn't.
    delete ptr2;
    delete ptr;

    return 0;
}

// Environment (gcc version):

$ g++ --version
g++ (Ubuntu 11.3.0-1ubuntu1~22.04) 11.3.0
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
// but it could be any GCC version.

// Command line (gcc; https://godbolt.org/z/xcnP9E39a):

$ g++ -Werror -Wframe-larger-than=4096 -o test test.cpp
/code/Src/E2EE/dstepanovs.cpp: In copy constructor ‘Child::Child(const
Child&)’:
/code/Src/E2EE/dstepanovs.cpp:14:8: error: the frame size of 65568 bytes is
larger than 4096 bytes [-Werror=frame-larger-than=]
   14 | struct Child : public Parent
      |        ^~~~~
cc1plus: all warnings being treated as errors

// Compiling same code with same flags with clang yields no error (clang
command line; https://godbolt.org/z/PMq5Yr3Tn):
$ clang++ -Werror -Wframe-larger-than=4096 -o test test.cpp

[Bug c++/109495] Stack is used (unexpectedly) for copying on-heap objects (no problem in clang)

[gcc-bug-reports at xhtml dot guru]

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

--- Comment #1 from gcc-bug-reports at xhtml dot guru ---
Created attachment 54849
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54849&action=edit
test.cpp: test code that triggers Wframe-larger-than in GCC

[Bug middle-end/109495] Stack is used (unexpectedly) for copying on-heap objects (no problem in clang)

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization
      Known to fail|                            |13.0, 7.5.0
            Version|unknown                     |13.0
                 CC|                            |hubicka at gcc dot gnu.org,
                   |                            |rguenth at gcc dot gnu.org
             Target|                            |x86_64-*-*
   Last reconfirmed|                            |2023-04-13
             Status|UNCONFIRMED                 |NEW
          Component|c++                         |middle-end
     Ever confirmed|0                           |1

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
Selected stringop expansion strategy: libcall
Selected stringop expansion strategy: libcall
Selected stringop expansion strategy: libcall
Selected stringop expansion strategy: libcall

;; MEM[(struct Child *)_6].D.35468 = MEM[(const struct Child &)_3].D.35468;

and for some reason we expand this to a memcpy to a stack temporary
and then a memcpy from that stack temporary!?

[Bug middle-end/109495] Stack is used (unexpectedly) for copying on-heap objects (no problem in clang)

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Adjusted testcase so that it doesn't need headers:
struct A
{
  A ();
  ~A ();
  A (const A &);
};

struct B
{
private:
  unsigned char y[1024 * 64];
public:
  short x;
  char z;
};

struct C : public B
{
  A s;
};

int
main ()
{
  C *ptr = new C ();
  C *ptr2 = new C (*ptr);
  delete ptr2;
  delete ptr;
}

Commenting out the private: line makes it go away.  Happens already in r105000
when I look at the frame sizes in the assembly (-Wframe-larger-than= wasn't
supported back then).

[Bug middle-end/109495] Stack is used (unexpectedly) for copying on-heap objects (no problem in clang)

[rguenth at gcc dot gnu.org]

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

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
In store_field we run into

  /* If the structure is in a register or if the component
     is a bit field, we cannot use addressing to access it.
     Use bit-field techniques or SUBREG to store in it.  */

because

      /* If the RHS and field are a constant size and the size of the
         RHS isn't the same size as the bitfield, we must use bitfield
         operations.  */
      || (known_size_p (bitsize)
          && poly_int_tree_p (TYPE_SIZE (TREE_TYPE (exp)))
          && maybe_ne (wi::to_poly_offset (TYPE_SIZE (TREE_TYPE (exp))),
                       bitsize)

as bitsize == 524312 and TYPE_SIZE == 524320 (we have a COMPONENT_REF
here and the size of the FIELD_DECL is 524312!)

and since !TREE_ADDRESSABLE we're happily oblieging here.

And then we proceed with

      temp = expand_normal (exp);

which results in a copy on the stack.

I wonder why we should ever, for smaller bitsize, use a copy here, when
bitsize/bitpos is a multiple of BITS_PER_UNIT?  In particular I don't
understand why we need TREE_ADDRESSABLE to let a proper COMPONENT_REF
through?

          /* And except for bitwise copying of TREE_ADDRESSABLE types,
             where the FIELD_DECL has the right bitsize, but TREE_TYPE (exp)
             includes some extra padding.  store_expr / expand_expr will in
             that case call get_inner_reference that will have the bitsize
             we check here and thus the block move will not clobber the
             padding that shouldn't be clobbered.  In the future we could
             replace the TREE_ADDRESSABLE check with a check that
             get_base_address needs to live in memory.  */
          && (!TREE_ADDRESSABLE (TREE_TYPE (exp))
              || TREE_CODE (exp) != COMPONENT_REF
              || !multiple_p (bitsize, BITS_PER_UNIT)
              || !multiple_p (bitpos, BITS_PER_UNIT)
              || !poly_int_tree_p (DECL_SIZE (TREE_OPERAND (exp, 1)),
                                   &decl_bitsize)
              || maybe_ne (decl_bitsize, bitsize))

removing the !TREE_ADDRESSABLE check fixes the testcase.

[Bug middle-end/109495] Stack is used (unexpectedly) for copying on-heap objects (no problem in clang)

[rguenth at gcc dot gnu.org]

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

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
That said, the comment says

             padding that shouldn't be clobbered.  In the future we could
             replace the TREE_ADDRESSABLE check with a check that
             get_base_address needs to live in memory.  */

but maybe testing for BLKmode is enough here.  So

          && ((mode != BLKmode && !TREE_ADDRESSABLE (TREE_TYPE (exp)))

[Bug middle-end/109495] Stack is used (unexpectedly) for copying on-heap objects (no problem in clang)

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The private keyword makes it non-POD and so the tail padding can be reused and
we need to make sure we copy only the bits except for type padding.
I guess we need some archeology why the exact conditions are in there.

[Bug middle-end/109495] Stack is used (unexpectedly) for copying on-heap objects (no problem in clang)

[rguenth at gcc dot gnu.org]

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

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
Created attachment 54857
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54857&action=edit
patch

I have tested the attached successfully.  Can you think of a case where
we'd have a BLKmode target but a same size component ref source that
has not?  We could also check the components mode (but emulating what
get_inner_reference does is tricky - but it might be useful to split
the head of it out).

In any case if the concern is just padding it's odd that we have to massage
the _source_.  And if we have to, for larger sizes we could at least
block-copy the large head and just deal with the tail covering the padding
with a temporary.