[Bug c/95052] New: Excess padding of partially initialized strings/char arrays

[Bug c/95052] New: Excess padding of partially initialized strings/char arrays

[krzysztof.a.nowicki+gcc at gmail dot com]

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

            Bug ID: 95052
           Summary: Excess padding of partially initialized strings/char
                    arrays
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: krzysztof.a.nowicki+gcc at gmail dot com
  Target Milestone: ---

Created attachment 48506
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48506&action=edit
generated assembly (GCC 11.0 trunk, -Os -g0)

When compiling the following code with -Os:

  extern void func(char *buf, unsigned size);
  int main(int argc, char *argv[])
  {
    char str[1*1024*1024] =
"fooiuhluhpiuhliuhliyfyukyfklyugkiuhpoipoipoipoipoipoipoipoipoipoipoipoipoimipoipiuhoulouihnliuhl";
    char arr[1*1024*1024] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 5, 6, 7,
8, 9, 0, 1, 6, 2, 3, 4, 5, 6, 7, 8, 9, 0, 3, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1,
2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0};
    func(str, sizeof(str));
    func(arr, sizeof(arr));
 }

GCC generates initializers for both local variables (str and arr) in the
.rodata section and at run-time initializes the explicit part of the variable
with the provided contents, and zero-inits the remainder.

Unfortunately the initializer stored in the .rodata section is padded up to the
target array size:

.LC0:
        .string
"fooiuhluhpiuhliuhliyfyukyfklyugkiuhpoipoipoipoipoipoipoipoipoipoipoipoipoimipoipiuhoulouihnliuhl"
        .zero   1048479
.LC1:
        .string "\001\026\003\004\005?\007\b'"
        .string "\001\002\003\004\005>\033\b1"
        .string "\001\006\002\003\004\005\006\007\b\t"
        .string "\003\001\002\003\004\005\006\007\b\t"
        .string "\001\002\003\004\005\006\007\b\t"
        .string "\001\002\003\004\005\006\007\b\t"
        .string "\002\002\003\004\005>\033\b1"
        .string "\001\006\002\003\004\005\006\007\b\t"
        .string "\003\001\002\003\004\005\006\007\b\t"
        .string "\001\002\003\004\005\006\007\b\t"
        .string "\001\002\003\004\005"
        .zero   1048466

This causes the resulting binary to become unnecessarily large, even though the
zero padding is completely redundant (the run-time initializer code does not
copy these bytes to the target variable, but zero-initializes them.

I suspect that this is caused by GCC not being able to distinguish between:
 - initialization of a global (or static local) variable,
 - initialization of a local variable

In the former case the contents of the variable live in the read/write data
section and are initialized by the compiler. In such case padding is necessary
as any further changes to the variable will be done in-place.

In the latter case the contents of the variable live on the stack and are
initialized from a read-only copy in the read-only data section. In such case
only the non-zero explicitly initialized part needs to be stored - any padding
can be skipped as it will not be used.

This mis-optimization occurs depending on compiler flags, architecture and size
of the array as well as the initialized part, as GCC may choose (and usually
does) to initialize the variable by using store assembly instructions with
immediate values, as this method is usually faster at the cost of increased
code size.

[Bug c/95052] Excess padding of partially initialized strings/char arrays

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2020-05-11
           Keywords|                            |missed-optimization

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
I'm not sure what you describe as padding is padding.  Instead it's valid to
access all elements of the array you declare and thus it must be initialized.

What could be done is elide zero-padding parts to a memset() call.

[Bug c/95052] Excess padding of partially initialized strings/char arrays

[krzysztof.a.nowicki+gcc at gmail dot com]

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

--- Comment #2 from Krzysztof Nowicki <krzysztof.a.nowicki+gcc at gmail dot com> ---
(In reply to Richard Biener from comment #1)
> I'm not sure what you describe as padding is padding.  Instead it's valid to
> access all elements of the array you declare and thus it must be initialized.

Not in this case, as this is a constant initializer, which is anonymous and is
never accessible from the code directly.

> What could be done is elide zero-padding parts to a memset() call.

Exactly, this is what I mean. Currently this actually happens in the generated
code (see attachment), but the part of GCC which allocates the variable in the
.rodata section doesn't know that and allocates memory for the full contents
including the zero padding.

[Bug c/95052] Excess padding of partially initialized strings/char arrays

[krzysztof.a.nowicki+gcc at gmail dot com]

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

--- Comment #3 from Krzysztof Nowicki <krzysztof.a.nowicki+gcc at gmail dot com> ---
Note, that this missed optimization is actually a regression (at least in our
case on MIPS64). Commit 23aa9f7c4637ad51587e536e245ae6adb5391bbc (released in
GCC 8.x) added the possibility to optimize initialization of a char array into
a string initialization. This means that this code:

  #include <unistd.h>
  int main(int argc, char *argv[])
  { 
    char buf[1*1024*1024] = { 0 };
    return read(0, buf, sizeof buf);
  }

generates the following binary sizes for MIPS64:

 * 13632 bytes (GCC-6.4)
 * 1061656 bytes (GCC-9.1)

[Bug target/95052] Excess padding of partially initialized strings/char arrays

[msebor at gcc dot gnu.org]

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

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |msebor at gcc dot gnu.org
          Component|c                           |target

--- Comment #4 from Martin Sebor <msebor at gcc dot gnu.org> ---
The optimization in g:23aa9f7c4637ad51587e536e245ae6adb5391bbc should only
convert into a STRING_CST the initial portion of the braced initializer list up
to just past the last non-nul character.  The remaining nuls should be skipped.
 In other words, this

    char buf[1*1024*1024] = { 0 };

should result in the same IL as this equivalent:

    char buf[1*1024*1024] = "";

I don't expect the commit above to have changed anything for the latter form,
and I would expect each back end to choose the same optimal code to emit in
both cases.  So I don't think the commit above is a regression; it just exposed
an inefficiency that was already present.

[Bug target/95052] Excess padding of partially initialized strings/char arrays

[krzysztof.a.nowicki+gcc at gmail dot com]

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

--- Comment #5 from Krzysztof Nowicki <krzysztof.a.nowicki+gcc at gmail dot com> ---
(In reply to Martin Sebor from comment #4)
> I don't expect the commit above to have changed anything for the latter
> form, and I would expect each back end to choose the same optimal code to
> emit in both cases.  So I don't think the commit above is a regression; it
> just exposed an inefficiency that was already present.

Yes, from the implementation point of view I agree - the missed optimization
was there all the time and this commit exposed it in one more use case, where
it wasn't seen before. However from the point of view of an application
developer on an embedded system, who has developed it with some memory
constraints in mind, a sudden increase of memory usage, possibly causing an OOM
since the memory budget on this system is very tight (legacy platform), just by
upgrading the compiler is a regression.

[Bug middle-end/95052] Excess padding of partially initialized strings/char arrays

[jakub at gcc dot gnu.org]

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 48511
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48511&action=edit
gcc11-pr95052.patch

Untested fix.

[Bug middle-end/95052] Excess padding of partially initialized strings/char arrays

[krzysztof.a.nowicki+gcc at gmail dot com]

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

--- Comment #7 from Krzysztof Nowicki <krzysztof.a.nowicki+gcc at gmail dot com> ---
Thanks for the very quick response. I've applied the patch on top of GCC 9.1
and it indeed fixes the problem we've seen on MIPS64.

[Bug middle-end/95052] [9/10/11 Regression] Excess padding of partially initialized strings/char arrays

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |9.4
            Summary|Excess padding of partially |[9/10/11 Regression] Excess
                   |initialized strings/char    |padding of partially
                   |arrays                      |initialized strings/char
                   |                            |arrays

[Bug middle-end/95052] [9/10/11 Regression] Excess padding of partially initialized strings/char arrays

[cvs-commit at gcc dot gnu.org]

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

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

https://gcc.gnu.org/g:43a4fc095e30188392cc42299c4081297e321104

commit r11-711-g43a4fc095e30188392cc42299c4081297e321104
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri May 29 10:42:50 2020 +0200

    expander: Optimize store_expr from STRING_CST [PR95052]

    In the following testcase, store_expr of e.g. 97 bytes long string literal
    into 1MB long array is implemented by copying the 97 bytes from .rodata
    section, followed by clearing the remaining bytes.  But, as the STRING_CST
    has type char[1024*1024], we actually allocate whole 1MB in .rodata section
    for it, even when we only use the first 97 bytes from that.

    The following patch tweaks it so that if we are going to initialize only
the
    small part from it, we don't emit all the zeros that we never use after it.

    2020-05-29  Jakub Jelinek  <jakub@redhat.com>

            PR middle-end/95052
            * expr.c (store_expr): If expr_size is constant and significantly
            larger than TREE_STRING_LENGTH, set temp to just the
            TREE_STRING_LENGTH portion of the STRING_CST.

            * gcc.target/i386/pr95052.c: New test.

[Bug middle-end/95052] [9/10/11 Regression] Excess padding of partially initialized strings/char arrays

[mkuvyrkov at gcc dot gnu.org]

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

Maxim Kuvyrkov <mkuvyrkov at gcc dot gnu.org> changed:

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

--- Comment #9 from Maxim Kuvyrkov <mkuvyrkov at gcc dot gnu.org> ---
Hi Jakub,

Above patch has causes gcc to crash when building linux kernel for
arm-linux-gnueabihf:
# 00:02:09 drivers/char/tpm/eventlog/tpm1.c:148:7: internal compiler error: in
store_expr, at expr.c:5845
# 00:02:09 make[3]: *** [drivers/char/tpm/eventlog/tpm1.o] Error 1
# 00:02:14 make[2]: *** [drivers/char/tpm] Error 2
# 00:02:14 make[1]: *** [drivers/char] Error 2
# 00:02:58 drivers/ata/libata-eh.c:2293:8: internal compiler error: in
store_expr, at expr.c:5845
# 00:02:58 make[2]: *** [drivers/ata/libata-eh.o] Error 1
# 00:03:22 make[1]: *** [drivers/ata] Error 2
# 00:05:35 make: *** [drivers] Error 2

I've started a reproduction build to make a testcase.

[Bug middle-end/95052] [9/10/11 Regression] Excess padding of partially initialized strings/char arrays

[mkuvyrkov at gcc dot gnu.org]

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

--- Comment #10 from Maxim Kuvyrkov <mkuvyrkov at gcc dot gnu.org> ---
To reproduce:

1. Configure GCC for arm-linux-gnueabihf (e.g., for x86_64->armhf cross):
--with-gnu-as --with-gnu-ld --disable-libmudflap --enable-lto --enable-shared
--without-included-gettext --enable-nls --with-system-zlib
--disable-sjlj-exceptions --enable-gnu-unique-object --enable-linker-build-id
--disable-libstdcxx-pch --enable-c99 --enable-clocale=gnu
--enable-libstdcxx-debug --enable-long-long --with-cloog=no --with-ppl=no
--with-isl=no --disable-multilib --with-float=hard --with-fpu=vfpv3-d16
--with-mode=thumb --with-tune=cortex-a9 --with-arch=armv7-a
--enable-threads=posix --enable-multiarch --enable-libstdcxx-time=yes
--enable-gnu-indirect-function --disable-libssp --disable-libquadmath
--disable-threads --without-headers --with-newlib --disable-libmudflap
--disable-bootstrap --disable-decimal-float --disable-libgomp
--disable-libatomic --disable-libsanitizer --disable-plugins --disable-libitm
--disable-shared --with-glibc-version=2.18 --disable-libstdcxx --disable-libvtv
--enable-languages=c,c++ --build=x86_64-unknown-linux-gnu
--host=x86_64-unknown-linux-gnu --target=arm-linux-gnueabihf

2. Try compiling attached pre-processed file:
.../cc1 -fpreprocessed libata-eh.i -quiet -dumpdir drivers/ata/ -dumpbase
libata-eh.c -dumpbase-ext .c -mlittle-endian -mabi=aapcs-linux -mfpu=vfp -marm
-mfloat-abi=soft -mtune=cortex-a9 -mtls-dialect=gnu -march=armv7-a -O2 -Wall
-Wundef -Werror=strict-prototypes -Wno-trigraphs
-Werror=implicit-function-declaration -Werror=implicit-int -Wno-format-security
-Wno-frame-address -Wformat-truncation=0 -Wformat-overflow=0
-Wno-address-of-packed-member -Wframe-larger-than=1024
-Wno-unused-but-set-variable -Wimplicit-fallthrough=3 -Wunused-const-variable=0
-Wdeclaration-after-statement -Wvla -Wno-pointer-sign -Wno-stringop-truncation
-Wno-zero-length-bounds -Wno-array-bounds -Wstringop-overflow=0 -Wno-restrict
-Wno-maybe-uninitialized -Werror=date-time -Werror=incompatible-pointer-types
-Werror=designated-init -Wno-packed-not-aligned -std=gnu90 -version
-fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE -fno-dwarf2-cfi-asm
-fno-ipa-sra -funwind-tables -fno-delete-null-pointer-checks
-fno-allow-store-data-races -fstack-protector-strong -fomit-frame-pointer
-fno-var-tracking-assignments -fno-strict-overflow -fno-merge-all-constants
-fmerge-constants -fstack-check=no -fconserve-stack -fmacro-prefix-map=./=  -o
libata-eh.s

3. It should crash with:
...
during RTL pass: expand
drivers/ata/libata-eh.c: In function \u2018ata_eh_link_report\u2019:
drivers/ata/libata-eh.c:2293:8: internal compiler error: in store_expr, at
expr.c:5845
0x8e8a56 store_expr(tree_node*, rtx_def*, int, bool, bool)

[Bug middle-end/95052] [9/10/11 Regression] Excess padding of partially initialized strings/char arrays

[mkuvyrkov at gcc dot gnu.org]

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

--- Comment #11 from Maxim Kuvyrkov <mkuvyrkov at gcc dot gnu.org> ---
Created attachment 48634
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48634&action=edit
Crash testcase (from Linux kernel)

[Bug middle-end/95052] [9/10/11 Regression] Excess padding of partially initialized strings/char arrays

[jakub at gcc dot gnu.org]

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

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Reduced testcase with -O2 -fconserve-stack:

void bar (char *);

void
foo (void)
{
  char buf[70] = "";
  bar (buf);
}

[Bug middle-end/95052] [9/10/11 Regression] Excess padding of partially initialized strings/char arrays

[cvs-commit at gcc dot gnu.org]

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

--- Comment #13 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:dc8c02ca1cd18f8c22d70cf17b47125fc25ab243

commit r11-748-gdc8c02ca1cd18f8c22d70cf17b47125fc25ab243
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Sun May 31 10:45:21 2020 +0200

    expr: Fix fallout from optimize store_expr from STRING_CST [PR95052]

    > Can't hurt, and debugging the assert tripping is likely a hell of a lot
easier
    > than debugging the resultant incorrect code.   So if it passes, then I'd
say go
    > for it.

    Testing passed, so I've committed it with those asserts (and thankfully
I've
    added them!) but it apparently broke Linux kernel build on arm.

    The problem is that if the STRING_CST is very short, while the full object
    has BLKmode, the short string could very well have
    QImode/HImode/SImode/DImode and in that case it wouldn't take the path that
    copies the string and then clears the remaining space, but different paths
    in which it will ICE because of those asserts and without those it would
    just emit wrong-code.

    The following patch fixes it by enforcing BLKmode for the string MEM, even
    if it is short, so that we copy it and memset the rest.

    2020-05-31  Jakub Jelinek  <jakub@redhat.com>

            PR middle-end/95052
            * expr.c (store_expr): For shortedned_string_cst, ensure temp has
            BLKmode.

            * gcc.dg/pr95052.c: New test.

[Bug middle-end/95052] [9/10/11 Regression] Excess padding of partially initialized strings/char arrays

[ptsneves at gmail dot com]

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

Paulo Neves <ptsneves at gmail dot com> changed:

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

--- Comment #14 from Paulo Neves <ptsneves at gmail dot com> ---
Thanks a lot with the response. We will try to build a whole distribution with
a back port of this patch to gcc 9.1 and report if we were successful.

[Bug middle-end/95052] [9/10 Regression] Excess padding of partially initialized strings/char arrays

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2
            Summary|[9/10/11 Regression] Excess |[9/10 Regression] Excess
                   |padding of partially        |padding of partially
                   |initialized strings/char    |initialized strings/char
                   |arrays                      |arrays
      Known to work|                            |11.0

[Bug middle-end/95052] [9/10 Regression] Excess padding of partially initialized strings/char arrays

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|9.4                         |9.5

--- Comment #15 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 9.4 is being released, retargeting bugs to GCC 9.5.

[Bug middle-end/95052] [10 Regression] Excess padding of partially initialized strings/char arrays

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|9.5                         |10.4

--- Comment #16 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 9 branch is being closed

[Bug middle-end/95052] [10 Regression] Excess padding of partially initialized strings/char arrays

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.4                        |10.5

--- Comment #17 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 10.4 is being released, retargeting bugs to GCC 10.5.

[Bug middle-end/95052] [10 Regression] Excess padding of partially initialized strings/char arrays

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
   Target Milestone|10.5                        |11.0
         Resolution|---                         |FIXED
      Known to fail|                            |10.5.0

--- Comment #18 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed for GCC 11.