[PATCH] kbuild: linker script do not match C names unless LD_DEAD_CODE_DATA_ELIMINATION is configured

[Masahiro Yamada]

2017-08-08 13:26 GMT+09:00 Nicholas Piggin <npiggin at gmail.com>:
> On Tue, 8 Aug 2017 12:42:18 +0900
> Masahiro Yamada <yamada.masahiro at socionext.com> wrote:
>
>> Hi Nicholas,
>>
>> 2017-08-08 12:19 GMT+09:00 Nicholas Piggin <npiggin at gmail.com>:
>> > On Tue, 8 Aug 2017 10:53:56 +0900
>> > Masahiro Yamada <yamada.masahiro at socionext.com> wrote:
>> >
>> >> Hi Nicholas,
>> >>
>> >> 2017-07-26 21:46 GMT+09:00 Nicholas Piggin <npiggin at gmail.com>:
>> >> > The .data and .bss sections were modified in the generic linker script to
>> >> > pull in sections named .data.<C identifier>, which are generated by gcc with
>> >> > -ffunction-sections and -fdata-sections options.
>> >> >
>> >> > The problem with this pattern is it can also match section names that Linux
>> >> > defines explicitly, e.g., .data.unlikely. This can cause Linux sections to
>> >> > get moved into the wrong place.
>> >> >
>> >> > The way to avoid this is to use ".." separators for explicit section names
>> >> > (the dot character is valid in a section name but not a C identifier).
>> >> > However currently there are sections which don't follow this rule, so for
>> >> > now just disable the wild card by default.
>> >> >
>> >> > Example: http://marc.info/?l=linux-arm-kernel&m=150106824024221&w=2
>> >> >
>> >> > Cc: <stable at vger.kernel.org> # 4.9
>> >> > Fixes: b67067f1176df ("kbuild: allow archs to select link dead code/data elimination")
>> >> > Signed-off-by: Nicholas Piggin <npiggin at gmail.com>
>> >> > ---
>> >> >  include/asm-generic/vmlinux.lds.h | 38 ++++++++++++++++++++++++++------------
>> >> >  1 file changed, 26 insertions(+), 12 deletions(-)
>> >> >
>> >> > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
>> >> > index da0be9a8d1de..9623d78f8494 100644
>> >> > --- a/include/asm-generic/vmlinux.lds.h
>> >> > +++ b/include/asm-generic/vmlinux.lds.h
>> >> > @@ -60,6 +60,22 @@
>> >> >  #define ALIGN_FUNCTION()  . = ALIGN(8)
>> >> >
>> >> >  /*
>> >> > + * LD_DEAD_CODE_DATA_ELIMINATION option enables -fdata-sections, which
>> >> > + * generates .data.identifier sections, which need to be pulled in with
>> >> > + * .data. We don't want to pull in .data..other sections, which Linux
>> >> > + * has defined. Same for text and bss.
>> >> > + */
>> >> > +#ifdef CONFIG_LD_DEAD_CODE_DATA_ELIMINATION
>> >> > +#define TEXT_MAIN .text .text.[0-9a-zA-Z_]*
>> >> > +#define DATA_MAIN .data .data.[0-9a-zA-Z_]*
>> >> > +#define BSS_MAIN .bss .bss.[0-9a-zA-Z_]*
>> >> > +#else
>> >> > +#define TEXT_MAIN .text
>> >> > +#define DATA_MAIN .data
>> >> > +#define BSS_MAIN .bss
>> >> > +#endif
>> >>
>> >>
>> >> From the git-log, I think you are planning to rename
>> >>   .data.unlikely   ->   .data..unlikey
>> >> then the problem will be fixed.
>> >>
>> >> ("git grep data.unlikely" gave me only 7 instances.)
>> >>
>> >>
>> >> Why is this patch necessary?
>> >>
>> >>
>> >> You are trying to migrate some architectures to DCDE,
>> >> so this #ifdef does not solve the problem, I think.
>> >
>> > Well basically I want to preserve previous behaviour and avoid
>> > regressions as far as possible. Patches to rename existing
>> > sections I think would not be so suitable for @stable. And
>> > there could be other things we've missed.
>> >
>> > Yes I would like to ensure all our sections use .. separators
>> > which (hopefully). Until then I think this is a minimal fix.
>> >
>>
>>
>> After you rename the sections, will you revert this patch?
>>
>> I think the #ifdef CONFIG_LD_DEAD_CODE_DATA_ELIMINATION
>> should not be permanent.
>
> Once we have the code reasonably well used on a number of archs,
> I think we should remove the ifdefs, yes we definitely should be
> moving toward sharing common code wherever possible.
>


OK, I will pick up this for fixes.
Thanks.


-- 
Best Regards
Masahiro Yamada