diff mbox series

[v4,3/6] Change the symbols order when --ffuntion-sections is enabled

Message ID 20241014213342.1480681-4-xur@google.com (mailing list archive)
State New
Headers show
Series Add AutoFDO and Propeller support for Clang build | expand

Commit Message

Rong Xu Oct. 14, 2024, 9:33 p.m. UTC 
When the -ffunction-sections compiler option is enabled, each function
is placed in a separate section named .text.function_name rather than
putting all functions in a single .text section.

However, using -function-sections can cause problems with the
linker script. The comments included in include/asm-generic/vmlinux.lds.h
note these issues.:
  “TEXT_MAIN here will match .text.fixup and .text.unlikely if dead
   code elimination is enabled, so these sections should be converted
   to use ".." first.”

It is unclear whether there is a straightforward method for converting
a suffix to "..". This patch modifies the order of subsections within the
text output section when the -ffunction-sections flag is enabled.
Specifically, it repositions sections with certain fixed patterns (for
example .text.unlikely) before TEXT_MAIN, ensuring that they are grouped
and matched together.

Note that the limitation arises because the linker script employs glob
patterns instead of regular expressions for string matching. While there
is a method to maintain the current order using complex patterns, this
significantly complicates the pattern and increases the likelihood of
errors.

Co-developed-by: Han Shen <shenhan@google.com>
Signed-off-by: Han Shen <shenhan@google.com>
Signed-off-by: Rong Xu <xur@google.com>
Suggested-by: Sriraman Tallam <tmsriram@google.com>
Suggested-by: Krzysztof Pszeniczny <kpszeniczny@google.com>
---
 include/asm-generic/vmlinux.lds.h | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

Comments

Masahiro Yamada Oct. 21, 2024, 2:15 a.m. UTC | #1 
On Tue, Oct 15, 2024 at 6:33 AM Rong Xu <xur@google.com> wrote:
>
> When the -ffunction-sections compiler option is enabled, each function
> is placed in a separate section named .text.function_name rather than
> putting all functions in a single .text section.
>
> However, using -function-sections can cause problems with the
> linker script. The comments included in include/asm-generic/vmlinux.lds.h
> note these issues.:
>   “TEXT_MAIN here will match .text.fixup and .text.unlikely if dead
>    code elimination is enabled, so these sections should be converted
>    to use ".." first.”
>
> It is unclear whether there is a straightforward method for converting
> a suffix to "..".



Why not for ".text.fixup"?

$ git grep --name-only '\.text\.fixup' | xargs sed -i
's/\.text\.fixup/.text..fixup/g'



I do not know how to rename other sections that are generated by compilers.




> This patch modifies the order of subsections within the
> text output section when the -ffunction-sections flag is enabled.
> Specifically, it repositions sections with certain fixed patterns (for
> example .text.unlikely) before TEXT_MAIN, ensuring that they are grouped
> and matched together.
>
> Note that the limitation arises because the linker script employs glob
> patterns instead of regular expressions for string matching. While there
> is a method to maintain the current order using complex patterns, this
> significantly complicates the pattern and increases the likelihood of
> errors.
>
> Co-developed-by: Han Shen <shenhan@google.com>
> Signed-off-by: Han Shen <shenhan@google.com>
> Signed-off-by: Rong Xu <xur@google.com>
> Suggested-by: Sriraman Tallam <tmsriram@google.com>
> Suggested-by: Krzysztof Pszeniczny <kpszeniczny@google.com>
> ---
>  include/asm-generic/vmlinux.lds.h | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index eeadbaeccf88..5df589c60401 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -554,9 +554,21 @@
>   * during second ld run in second ld pass when generating System.map
>   *
>   * TEXT_MAIN here will match .text.fixup and .text.unlikely if dead
> - * code elimination is enabled, so these sections should be converted
> - * to use ".." first.
> + * code elimination or function-section is enabled. Match these symbols
> + * first when in these builds.
>   */
> +#if defined(CONFIG_LD_DEAD_CODE_DATA_ELIMINATION) || defined(CONFIG_LTO_CLANG)
> +#define TEXT_TEXT                                                      \


Why did you do this conditionally?

You are making this even more unmaintainable.





> +               ALIGN_FUNCTION();                                       \
> +               *(.text.asan.* .text.tsan.*)                            \
> +               *(.text.unknown .text.unknown.*)                        \
> +               *(.text.unlikely .text.unlikely.*)                      \
> +               . = ALIGN(PAGE_SIZE);                                   \
> +               *(.text.hot .text.hot.*)                                \
> +               *(TEXT_MAIN .text.fixup)                                \
> +               NOINSTR_TEXT                                            \
> +               *(.ref.text)
> +#else
>  #define TEXT_TEXT                                                      \
>                 ALIGN_FUNCTION();                                       \
>                 *(.text.hot .text.hot.*)                                \
> @@ -566,6 +578,7 @@
>                 NOINSTR_TEXT                                            \
>                 *(.ref.text)                                            \
>                 *(.text.asan.* .text.tsan.*)
> +#endif
>
>
>  /* sched.text is aling to function alignment to secure we have same
> --
> 2.47.0.rc1.288.g06298d1525-goog
>
>


--
Best Regards
Masahiro Yamada
Rong Xu Oct. 21, 2024, 11:43 p.m. UTC | #2 
On Sun, Oct 20, 2024 at 7:15 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Tue, Oct 15, 2024 at 6:33 AM Rong Xu <xur@google.com> wrote:
> >
> > When the -ffunction-sections compiler option is enabled, each function
> > is placed in a separate section named .text.function_name rather than
> > putting all functions in a single .text section.
> >
> > However, using -function-sections can cause problems with the
> > linker script. The comments included in include/asm-generic/vmlinux.lds.h
> > note these issues.:
> >   “TEXT_MAIN here will match .text.fixup and .text.unlikely if dead
> >    code elimination is enabled, so these sections should be converted
> >    to use ".." first.”
> >
> > It is unclear whether there is a straightforward method for converting
> > a suffix to "..".
>
>
>
> Why not for ".text.fixup"?
>
> $ git grep --name-only '\.text\.fixup' | xargs sed -i
> 's/\.text\.fixup/.text..fixup/g'
>

I did not move .text.fixup because it currently groups together with TEXT_MAIN.

Yes. For all the kernel annotated sections, we can replace them with a
".." string.
For compiler generate strings, like .unlikely, .hot, and .split, we
need a compiler change
for that (maybe under an option). The process will be long.
Or we can use an extra script, like objcopy to change them?

>
>
> I do not know how to rename other sections that are generated by compilers.
>
>
>
>
> > This patch modifies the order of subsections within the
> > text output section when the -ffunction-sections flag is enabled.
> > Specifically, it repositions sections with certain fixed patterns (for
> > example .text.unlikely) before TEXT_MAIN, ensuring that they are grouped
> > and matched together.
> >
> > Note that the limitation arises because the linker script employs glob
> > patterns instead of regular expressions for string matching. While there
> > is a method to maintain the current order using complex patterns, this
> > significantly complicates the pattern and increases the likelihood of
> > errors.
> >
> > Co-developed-by: Han Shen <shenhan@google.com>
> > Signed-off-by: Han Shen <shenhan@google.com>
> > Signed-off-by: Rong Xu <xur@google.com>
> > Suggested-by: Sriraman Tallam <tmsriram@google.com>
> > Suggested-by: Krzysztof Pszeniczny <kpszeniczny@google.com>
> > ---
> >  include/asm-generic/vmlinux.lds.h | 17 +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> > index eeadbaeccf88..5df589c60401 100644
> > --- a/include/asm-generic/vmlinux.lds.h
> > +++ b/include/asm-generic/vmlinux.lds.h
> > @@ -554,9 +554,21 @@
> >   * during second ld run in second ld pass when generating System.map
> >   *
> >   * TEXT_MAIN here will match .text.fixup and .text.unlikely if dead
> > - * code elimination is enabled, so these sections should be converted
> > - * to use ".." first.
> > + * code elimination or function-section is enabled. Match these symbols
> > + * first when in these builds.
> >   */
> > +#if defined(CONFIG_LD_DEAD_CODE_DATA_ELIMINATION) || defined(CONFIG_LTO_CLANG)
> > +#define TEXT_TEXT                                                      \
>
>
> Why did you do this conditionally?
>
> You are making this even more unmaintainable.

Again, we don't want to change the default build.

If you think the change can apply to the default build, we would be
happy to remove the condition.

>
>
>
>
>
> > +               ALIGN_FUNCTION();                                       \
> > +               *(.text.asan.* .text.tsan.*)                            \
> > +               *(.text.unknown .text.unknown.*)                        \
> > +               *(.text.unlikely .text.unlikely.*)                      \
> > +               . = ALIGN(PAGE_SIZE);                                   \
> > +               *(.text.hot .text.hot.*)                                \
> > +               *(TEXT_MAIN .text.fixup)                                \
> > +               NOINSTR_TEXT                                            \
> > +               *(.ref.text)
> > +#else
> >  #define TEXT_TEXT                                                      \
> >                 ALIGN_FUNCTION();                                       \
> >                 *(.text.hot .text.hot.*)                                \
> > @@ -566,6 +578,7 @@
> >                 NOINSTR_TEXT                                            \
> >                 *(.ref.text)                                            \
> >                 *(.text.asan.* .text.tsan.*)
> > +#endif
> >
> >
> >  /* sched.text is aling to function alignment to secure we have same
> > --
> > 2.47.0.rc1.288.g06298d1525-goog
> >
> >
>
>
> --
> Best Regards
> Masahiro Yamada
Masahiro Yamada Oct. 23, 2024, 6:48 a.m. UTC | #3 
On Tue, Oct 22, 2024 at 8:43 AM Rong Xu <xur@google.com> wrote:
>
> On Sun, Oct 20, 2024 at 7:15 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > On Tue, Oct 15, 2024 at 6:33 AM Rong Xu <xur@google.com> wrote:
> > >
> > > When the -ffunction-sections compiler option is enabled, each function
> > > is placed in a separate section named .text.function_name rather than
> > > putting all functions in a single .text section.
> > >
> > > However, using -function-sections can cause problems with the
> > > linker script. The comments included in include/asm-generic/vmlinux.lds.h
> > > note these issues.:
> > >   “TEXT_MAIN here will match .text.fixup and .text.unlikely if dead
> > >    code elimination is enabled, so these sections should be converted
> > >    to use ".." first.”
> > >
> > > It is unclear whether there is a straightforward method for converting
> > > a suffix to "..".
> >
> >
> >
> > Why not for ".text.fixup"?
> >
> > $ git grep --name-only '\.text\.fixup' | xargs sed -i
> > 's/\.text\.fixup/.text..fixup/g'
> >
>
> I did not move .text.fixup because it currently groups together with TEXT_MAIN.

OK. Then, .text.fixup is not a problem.



> >
> > Why did you do this conditionally?
> >
> > You are making this even more unmaintainable.
>
> Again, we don't want to change the default build.
>
> If you think the change can apply to the default build, we would be
> happy to remove the condition.


I believe this should be done unconditionally.

If you are concerned about changing the default,
I am concerned about changing it under any condition.

We should avoid maintaining two section layouts.




--
Best Regards
Masahiro Yamada
diff mbox series

Patch

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index eeadbaeccf88..5df589c60401 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -554,9 +554,21 @@ 
  * during second ld run in second ld pass when generating System.map
  *
  * TEXT_MAIN here will match .text.fixup and .text.unlikely if dead
- * code elimination is enabled, so these sections should be converted
- * to use ".." first.
+ * code elimination or function-section is enabled. Match these symbols
+ * first when in these builds.
  */
+#if defined(CONFIG_LD_DEAD_CODE_DATA_ELIMINATION) || defined(CONFIG_LTO_CLANG)
+#define TEXT_TEXT							\
+		ALIGN_FUNCTION();					\
+		*(.text.asan.* .text.tsan.*)				\
+		*(.text.unknown .text.unknown.*)			\
+		*(.text.unlikely .text.unlikely.*)			\
+		. = ALIGN(PAGE_SIZE);					\
+		*(.text.hot .text.hot.*)				\
+		*(TEXT_MAIN .text.fixup)				\
+		NOINSTR_TEXT						\
+		*(.ref.text)
+#else
 #define TEXT_TEXT							\
 		ALIGN_FUNCTION();					\
 		*(.text.hot .text.hot.*)				\
@@ -566,6 +578,7 @@ 
 		NOINSTR_TEXT						\
 		*(.ref.text)						\
 		*(.text.asan.* .text.tsan.*)
+#endif
 
 
 /* sched.text is aling to function alignment to secure we have same