[Bug ipa/64963] New: IPA Cloning/Splitting does not copy function section attributes resulting in kernel miscompilation

[Bug ipa/64963] New: IPA Cloning/Splitting does not copy function section attributes resulting in kernel miscompilation

[jgreenhalgh at gcc dot gnu.org]

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

            Bug ID: 64963
           Summary: IPA Cloning/Splitting does not copy function section
                    attributes resulting in kernel miscompilation
           Product: gcc
           Version: 5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: ipa
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jgreenhalgh at gcc dot gnu.org
                CC: hubicka at gcc dot gnu.org, jamborm at gcc dot gnu.org
              Host: x86_64-unknown-linux-gnu
            Target: aarch64-none-elf

Created attachment 34688
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=34688&action=edit
Cloning fails to maintain section attributes on foo

When we create a clone for constant propagation, or in ipa-split,
we don't clone the section attribute for that function. For kernel
builds this can result in code ending up in the wrong section, resulting
in all sorts of carnage.

The testcase attached shows the issue for cloning (it is much harder
to engineer one for splitting, but the problem is analogous).

You can reproduce the bug with -O3. For AArch64 this gives:

    .cpu generic+fp+simd
    .file    "ipa-clone-1.c"
    .text
    .align    2
    .p2align 3,,7
    .type    foo.constprop.0, %function
foo.constprop.0:
    mov    w0, 35
    ret
    .size    foo.constprop.0, .-foo.constprop.0
    .align    2
    .p2align 3,,7
    .global    bar
    .type    bar, %function
bar:
    b    foo.constprop.0
    .size    bar, .-bar
    .ident    "GCC: (unknown) 5.0.0 20150205 (experimental)"

Note that foo.constprop.0 is in the text section, while foo was declared with:

static int __attribute__ ((noinline section ("test_section")))
foo (int arg)

I would have expected to see:

    .arch armv8-a+fp+simd
    .file    "ipa-clone-1.c"
    .section    test_section,"ax",%progbits
    .align    2
    .p2align 3,,7
    .type    foo.constprop.0, %function
foo.constprop.0:
    mov    w0, 35
    ret
    .size    foo.constprop.0, .-foo.constprop.0
    .text
    .align    2
    .p2align 3,,7
    .global    bar
    .type    bar, %function
bar:
    b    foo.constprop.0
    .size    bar, .-bar
    .ident    "GCC: (unknown) 5.0.0 20150205 (experimental)"

[Bug ipa/64963] IPA Cloning/Splitting does not copy function section attributes resulting in kernel miscompilation

[jgreenhalgh at gcc dot gnu.org]

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

James Greenhalgh <jgreenhalgh at gcc dot gnu.org> changed:

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

--- Comment #1 from James Greenhalgh <jgreenhalgh at gcc dot gnu.org> ---
Created attachment 34689
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=34689&action=edit
Patch I was playing with, which breaks thunks

This patch I was playing with ensures that we copy the section attribute when
producing a specialized function, and fixes the kernel failures we've been
seeing.

However, this patch breaks a number of IPA tests (g++.dg/ipa/pr61160-*.C and
g++.dg/ipa/pr63814.C ) so I think I'm not handling thunks properly. I had a try
at fixing it, but everything I touched made things worse!

I'm sure I'm missing something obvious, so any explanation or hints you could
give towards the solution would be great!

[Bug ipa/64963] IPA Cloning/Splitting does not copy function section attributes resulting in kernel miscompilation

[jgreenhalgh at gcc dot gnu.org]

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

James Greenhalgh <jgreenhalgh at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|aarch64-none-elf            |aarch64-none-elf,
                   |                            |x86_64-unknown-linux-gnu
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2015-02-10
     Ever confirmed|0                           |1

--- Comment #2 from James Greenhalgh <jgreenhalgh at gcc dot gnu.org> ---
Adding x86_64 to the list of targets, as this is going to be a problem for
everybody.

build-x86/install/bin/gcc test.c -O3 -c

readelf -S test.o
There are 12 section headers, starting at offset 0x128:

Section Headers:
  [Nr] Name              Type             Address           Offset
       Size              EntSize          Flags  Link  Info  Align
  [ 0]                   NULL             0000000000000000  00000000
       0000000000000000  0000000000000000           0     0     0
  [ 1] .text             PROGBITS         0000000000000000  00000040
       0000000000000012  0000000000000000  AX       0     0     16
  [ 2] .data             PROGBITS         0000000000000000  00000052
       0000000000000000  0000000000000000  WA       0     0     1
  [ 3] .bss              NOBITS           0000000000000000  00000052
       0000000000000000  0000000000000000  WA       0     0     1
  [ 4] .text.unlikely    PROGBITS         0000000000000000  00000052
       0000000000000000  0000000000000000  AX       0     0     1
  [ 5] .comment          PROGBITS         0000000000000000  00000052
       000000000000002a  0000000000000001  MS       0     0     1
  [ 6] .note.GNU-stack   PROGBITS         0000000000000000  0000007c
       0000000000000000  0000000000000000           0     0     1
  [ 7] .eh_frame         PROGBITS         0000000000000000  00000080
       0000000000000040  0000000000000000   A       0     0     8
  [ 8] .rela.eh_frame    RELA             0000000000000000  00000550
       0000000000000030  0000000000000018          10     7     8
  [ 9] .shstrtab         STRTAB           0000000000000000  000000c0
       0000000000000063  0000000000000000           0     0     1
  [10] .symtab           SYMTAB           0000000000000000  00000428
       0000000000000108  0000000000000018          11    10     8
  [11] .strtab           STRTAB           0000000000000000  00000530
       000000000000001b  0000000000000000           0     0     1

$ build-x86/install/bin/gcc test.c -O0 -c
$ readelf -S test.o

There are 13 section headers, starting at offset 0x158:

Section Headers:
  [Nr] Name              Type             Address           Offset
       Size              EntSize          Flags  Link  Info  Align
  [ 0]                   NULL             0000000000000000  00000000
       0000000000000000  0000000000000000           0     0     0
  [ 1] .text             PROGBITS         0000000000000000  00000040
       0000000000000017  0000000000000000  AX       0     0     1
  [ 2] .rela.text        RELA             0000000000000000  000005b0
       0000000000000018  0000000000000018          11     1     8
  [ 3] .data             PROGBITS         0000000000000000  00000057
       0000000000000000  0000000000000000  WA       0     0     1
  [ 4] .bss              NOBITS           0000000000000000  00000057
       0000000000000000  0000000000000000  WA       0     0     1
  [ 5] test_section      PROGBITS         0000000000000000  00000057
       0000000000000013  0000000000000000  AX       0     0     1
  [ 6] .comment          PROGBITS         0000000000000000  0000006a
       000000000000002a  0000000000000001  MS       0     0     1
  [ 7] .note.GNU-stack   PROGBITS         0000000000000000  00000094
       0000000000000000  0000000000000000           0     0     1
  [ 8] .eh_frame         PROGBITS         0000000000000000  00000098
       0000000000000058  0000000000000000   A       0     0     8
  [ 9] .rela.eh_frame    RELA             0000000000000000  000005c8
       0000000000000030  0000000000000018          11     8     8
  [10] .shstrtab         STRTAB           0000000000000000  000000f0
       0000000000000066  0000000000000000           0     0     1
  [11] .symtab           SYMTAB           0000000000000000  00000498
       0000000000000108  0000000000000018          12    10     8
  [12] .strtab           STRTAB           0000000000000000  000005a0
       000000000000000f  0000000000000000           0     0     1

[Bug ipa/64963] IPA Cloning/Splitting does not copy function section attributes resulting in kernel miscompilation

[jakub at gcc dot gnu.org]

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

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> ---
I'm not so sure that putting the clones into the same section si the right
thing.  The clone is by definition a local compiler generated function, it
might not be desirable in the user defined section, which could have all kinds
of weird handling on it.  If the kernel can't grok some functions being cloned,
I'd say it should just use noclone attribute on those.

[Bug ipa/64963] IPA Cloning/Splitting does not copy function section attributes resulting in kernel miscompilation

[jgreenhalgh at gcc dot gnu.org]

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

--- Comment #4 from James Greenhalgh <jgreenhalgh at gcc dot gnu.org> ---
The warning message given by the kernel build is:

WARNING: net/ipv4/built-in.o(.text.unlikely+0x10c): Section mismatch in
reference from the function ip_auto_config.part.6() to the function
.init.text:ic_is_init_dev()
The function ip_auto_config.part.6() references
the function __init ic_is_init_dev().
This is often because ip_auto_config.part.6 lacks a __init 
annotation or the annotation of ic_is_init_dev is wrong.

So we've ended up keeping around code for longer than was intended by cloning
it out of the init section.

If the implication is that code destined for user-defined sections might be
unsafe to clone, then surely we should turn off cloning for functions annotated
with __attribute__ ((section ("foo"))) ?

[Bug ipa/64963] IPA Cloning/Splitting does not copy function section attributes resulting in kernel miscompilation

[rguenth at gcc dot gnu.org]

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

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
I think cloning into the same section make as much sense as not doing it.  That
is, I see valid uses for both behaviors.

We've been cloning not to the same section since quite a few releases so
changing
that would be surprising.

But apart from disabling cloning with noclone we don't have a way to tell the
compiler to put clones into the same section as the parents.

I can also think of heuristics like if there is a caller of the clone from
the same section as the clone origin then putting the clone into the very
same section makes sense.

We could invent a new flag like -fsticky-section=XXXX and/or a similar
attribute
or flag on the section attribute (__attribute__((section("foo",sticky)))).

[Bug ipa/64963] IPA Cloning/Splitting does not copy function section attributes resulting in kernel miscompilation

[andi-gcc at firstfloor dot org]

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

Andi Kleen <andi-gcc at firstfloor dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |andi-gcc at firstfloor dot org

--- Comment #6 from Andi Kleen <andi-gcc at firstfloor dot org> ---
In theory the kernel could mark __init functions with noclone.

But I think sticky behavior would be better. That's the behavior that the
kernel expects. There isn't any code as far as I know that would expect only a
single function per section.

[Bug ipa/64963] [5 Regression] IPA Cloning/Splitting does not copy function section attributes resulting in kernel miscompilation

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |5.0
            Summary|IPA Cloning/Splitting does  |[5 Regression] IPA
                   |not copy function section   |Cloning/Splitting does not
                   |attributes resulting in     |copy function section
                   |kernel miscompilation       |attributes resulting in
                   |                            |kernel miscompilation

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Ah, though it seems we've actually been using the user section for the clones
in the past, it is just Honza's r211363/r211365 changes (or so, r211350 works,
r211400 doesn't), that broke this.  So in that case it is a regression and
supposedly cloning should ensure the section name is copied (which has been
done automatically in the past because it was part of the FUNCTION_DECL that
was copied and modified, but after moving that out into cgraph that doesn't
happen anymore).

[Bug ipa/64963] [5 Regression] IPA Cloning/Splitting does not copy function section attributes resulting in kernel miscompilation

[hubicka at gcc dot gnu.org]

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

--- Comment #8 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Yeah, we was copying section as long as it was part of decl more or less by
accident than by design. I suppose keeping section (or not clonning at all)
makes sense.  What about inlining across functions with user sections?

[Bug ipa/64963] [5 Regression] IPA Cloning/Splitting does not copy function section attributes resulting in kernel miscompilation

[jakub at gcc dot gnu.org]

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

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I think we should try to keep older gcc behavior, unless there is a strong
reason to change that.

[Bug ipa/64963] [5 Regression] IPA Cloning/Splitting does not copy function section attributes resulting in kernel miscompilation

[andi-gcc at firstfloor dot org]

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

--- Comment #10 from Andi Kleen <andi-gcc at firstfloor dot org> ---
Yes it has to be fixed. For example with the kernel __kprobes attribute it
could cause a real bug (__kprobes marks function that cannot be safely
instrumented)

We shouldn't inline over different section names either, this could also cause
problems for the same reason.

[Bug ipa/64963] [5 Regression] IPA Cloning/Splitting does not copy function section attributes resulting in kernel miscompilation

[hubicka at ucw dot cz]

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

--- Comment #11 from Jan Hubicka <hubicka at ucw dot cz> ---
> Yes it has to be fixed. For example with the kernel __kprobes attribute it
> could cause a real bug (__kprobes marks function that cannot be safely
> instrumented)
> 
> We shouldn't inline over different section names either, this could also cause
> problems for the same reason.

Yeah, I have feeling that if clonning hurts we basically want to avoid code
motion in between
user named sections, including ipa-split that will take part of body and pull
it out, localization
and other optimizations we perform...  I will take a look.

Honza

[Bug ipa/64963] [5 Regression] IPA Cloning/Splitting does not copy function section attributes resulting in kernel miscompilation

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1

[Bug ipa/64963] [5 Regression] IPA Cloning/Splitting does not copy function section attributes resulting in kernel miscompilation

[jakub at gcc dot gnu.org]

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

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 34779
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=34779&action=edit
gcc5-pr64963.patch

Updated untested patch (well, tested with dg.exp=ipa/* ipa.exp so far), whcih
doesn't introduce these regressions and I think roughly matches what we used to
do with DECL_SECTION_NAME in the past.
Another thing is DECL_COMDAT_GROUP, seems we used to copy that and only clear
in some cases (non-local ones).

[Bug ipa/64963] [5 Regression] IPA Cloning/Splitting does not copy function section attributes resulting in kernel miscompilation

[jakub at gcc dot gnu.org]

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

--- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Author: jakub
Date: Mon Feb 16 19:22:57 2015
New Revision: 220742

URL: https://gcc.gnu.org/viewcvs?rev=220742&root=gcc&view=rev
Log:
    PR ipa/64963
    * cgraphclones.c (cgraph_node::create_virtual_clone): Copy
    section if not linkonce.  Fix up formatting.
    (cgraph_node::create_version_clone_with_body): Copy section.
    * trans-mem.c (ipa_tm_create_version): Likewise.

    * gcc.dg/ipa/ipa-clone-1.c: New test.

Added:
    trunk/gcc/testsuite/gcc.dg/ipa/ipa-clone-1.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/cgraphclones.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/trans-mem.c

[Bug ipa/64963] [5 Regression] IPA Cloning/Splitting does not copy function section attributes resulting in kernel miscompilation

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #14 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Should be fixed now.  Please reopen with additional testcases if that is not
the case.