Created attachment 47496 [details] test_bitmap.i A bug is reported on the linux kernel mailing list: https://lore.kernel.org/lkml/20191213211901.GL32742@smile.fi.intel.com/T/#t In linux kernel there must be no references from .text* sections to data or functions in .init* sections. In the attached source there's no such reference: __init test_replace calls regular bitmap_replace passing it a pointer to __initconst exp2_to_exp3_mask variable. But during optimization process such reference appears. When I look into test_bitmap.i.085t.fixup_cfg3 I see the following: __attribute__((no_instrument_function, unused, gnu_inline)) bitmap_replace.constprop (long unsigned int * dst, const long unsigned int * old, const long unsigned int * new) { ... __bitmap_replace (dst_11(D), old_3(D), new_6(D), &exp2_to_exp3_mask, 64); ... } __attribute__((cold, section (".init.text"))) test_replace () { ... bitmap_replace.constprop (&bmap, &exp2[0], &exp2[1]); ... } It looks like the function bitmap_replace.constprop should belong to the same section as the function test_replace. Compiler command line: cc1 -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE -std=gnu89 -ffreestanding -mlongcalls -mtext-section-literals -mforce-no-pic -mno-serialize-volatile -fno-delete-null-pointer-checks -O2 -fstack-protector-strong -fno-omit-frame-pointer -fno-optimize-sibling-calls -fno-var-tracking-assignments -g -femit-struct-debug-baseonly -fno-var-tracking -fno-inline-functions-called-once -fno-strict-overflow -fno-merge-all-constants -fmerge-constants -fno-stack-check -fconserve-stack test_bitmap.i
The issue does not reproduce with the current gcc trunk after SVN revision 277054 (generalized IPA predicate on parameter), but it doesn't look like this change fixes the root cause, it just results in inlining of bitmap_replace() into the test_replace().
I don't think this is a GCC bug. The reasoning is bitmap_replace is not marked as being in the section .init_* at all. GCC decides to clone it for constant-prop. Really any function which is marked as __gnu_inline__ should not be marked as static or should be marked as always_inline instead.
(In reply to jcmvbkbc from comment #1) > The issue does not reproduce with the current gcc trunk after SVN revision > 277054 (generalized IPA predicate on parameter), but it doesn't look like > this change fixes the root cause, it just results in inlining of > bitmap_replace() into the test_replace(). I DON'T think it is a GCC issue. The code is broken. If bitmap_replace was marked as always_inline, that would have worked or if it was marked as extern, it would have worked. The inlining is not happening since test_replace is marked as cold so not inlining is "expected".
(In reply to Andrew Pinski from comment #2) > The reasoning is bitmap_replace is not marked as being in the section > .init_* at all. Yes, and thus it should not make direct references to data which is in .init* sections. > GCC decides to clone it for constant-prop. Really any > function which is marked as __gnu_inline__ should not be marked as static or > should be marked as always_inline instead. I've removed __gnu_inline__ attribute, so that function definition looks like this: static inline void bitmap_replace(unsigned long *dst, const unsigned long *old, const unsigned long *new, const unsigned long *mask, unsigned int nbits) { if ((__builtin_constant_p(nbits) && (nbits) <= 32 && (nbits) > 0)) *dst = (*old & ~(*mask)) | (*new & *mask); else __bitmap_replace(dst, old, new, mask, nbits); } This does not resolve the issue.
(In reply to Andrew Pinski from comment #3) > I DON'T think it is a GCC issue. The code is broken. Even if I remove all attributes from this function I see this issue: static void bitmap_replace(unsigned long *dst, const unsigned long *old, const unsigned long *new, const unsigned long *mask, unsigned int nbits) { if ((__builtin_constant_p(nbits) && (nbits) <= 32 && (nbits) > 0)) *dst = (*old & ~(*mask)) | (*new & *mask); else __bitmap_replace(dst, old, new, mask, nbits); } Why is it broken?
(In reply to jcmvbkbc from comment #5) > (In reply to Andrew Pinski from comment #3) > > I DON'T think it is a GCC issue. The code is broken. > > Even if I remove all attributes from this function I see this issue: > > static void bitmap_replace(unsigned long *dst, > const unsigned long *old, > const unsigned long *new, > const unsigned long *mask, > unsigned int nbits) > { > if ((__builtin_constant_p(nbits) && (nbits) <= 32 && (nbits) > 0)) > *dst = (*old & ~(*mask)) | (*new & *mask); > else > __bitmap_replace(dst, old, new, mask, nbits); > } > > Why is it broken? What do you expect? That it is either inlined or a call to __bitmap_replace is emitted (with presumably is in .init* sections?)?
(In reply to Richard Biener from comment #6) > (In reply to jcmvbkbc from comment #5) > > (In reply to Andrew Pinski from comment #3) > > > I DON'T think it is a GCC issue. The code is broken. > > > > Even if I remove all attributes from this function I see this issue: > > > > static void bitmap_replace(unsigned long *dst, > > const unsigned long *old, > > const unsigned long *new, > > const unsigned long *mask, > > unsigned int nbits) > > { > > if ((__builtin_constant_p(nbits) && (nbits) <= 32 && (nbits) > 0)) > > *dst = (*old & ~(*mask)) | (*new & *mask); > > else > > __bitmap_replace(dst, old, new, mask, nbits); > > } > > > > Why is it broken? > > What do you expect? That it is either inlined or a call to __bitmap_replace > is emitted Yes, either any of these, or if a constprop function is generated it is either in the .init* section or it doesn't take a reference to a variable in .init*. > (with presumably is in .init* sections?)? It is not in .init*, but that's ok: the reference from .init to .text is present in the original source. It's the reference from .text* to .init* which is not present in the source but is generated by gcc that is the issue.
(In reply to jcmvbkbc from comment #7) > or it doesn't take a reference to a variable in .init*. That is, it doesn't have a direct reference to a variable in .init* in its body.
(In reply to jcmvbkbc from comment #7) > It is not in .init*, but that's ok: the reference from .init to .text is > present in the original source. It's the reference from .text* to .init* > which is not present in the source but is generated by gcc that is the issue. >It's the reference from .text* to .init* > which is not present in the source but is generated by gcc that is the issue. HUH? You mean exp2_to_exp3_mask is in the .init section? Well GCC can never know that cannot be referenced directly from a .text* section. The Linux kernel still has many issues like this. Maybe add the noclone attribute for all functions in the linux kernel which are called from the .init section. BUT THIS still requires sources changes of the linux kernel. THIS CANNOT be fixed in GCC as GCC does not know and should never know about the special semantics of .init section. Though maybe there could be an attribute which is applied to all .init functions which causes noclone of functions called to happen. A version of noclone which applies to the callees rather than the function itself.
(In reply to Andrew Pinski from comment #9) > (In reply to jcmvbkbc from comment #7) > >It's the reference from .text* to .init* > > which is not present in the source but is generated by gcc that is the issue. > HUH? > > You mean exp2_to_exp3_mask is in the .init section? Yes, exactly. > Well GCC can never know that cannot be referenced directly from a .text* > section. Then it probably should not have added that reference, right? > THIS CANNOT be fixed in GCC as GCC does not know and should never know about > the special semantics of .init section. The issue is not about special semantics. Semantic correctness is taken care of by the kernel developers. It's about gcc making references that are not present in the source code and breaking these semantics.