Re: [PATCH 0/1] gas: add new command line option --no-group-check

[Tan Yuan <tanyuan@tinylab.org>]

On Mon, Jul 24, 2023 at 1:15 AM Fangrui Song <i@maskray.me> wrote:
> On Sun, Jul 23, 2023 at 2:10 AM Tan Yuan <tanyuan@tinylab.org> wrote:
> >
> > Hi,
> >
> > Thanks for your reply and your advice inspired me a lot.
> >
> > On Sat, 22 Jul 2023 at 00:32 Song Fangrui <i@maskray.me> wrote:
> > > On Fri, Jul 21, 2023 at 8:14 AM Tan Yuan <tanyuan@tinylab.org> wrote:
> > > >
> > > > Hi, list
> > > >
> > > > This patch introduces a new option that allows suppressing the 
> > > > warning when attaching a group to a section that already belongs to a group.
> > > >
> > > > The kernel extensively uses the ".pushsection" directive, which 
> > > > poses issues with proper reference building and garbage 
> > > > collection. To address this problem, the kernel uses "KEEP" in the 
> > > > linker script to prevent certain sections from being garbage 
> > > > collected. Consequently, some sections that should have been 
> > > > garbage collected are retained. For instance, if the section 
> > > > pusher is being garbage collected, the pushed sections are also 
> > > > supposed to be collected, but the linker script retains them.
> > > >
> > > > To resolve this, we can utilize ".attach_to_group" alongside 
> > > > ".pushsection," which enables the section pusher and the pushed 
> > > > sections to be grouped together, making them either eligible for 
> > > > garbage collection or kept together.
> > > >
> > > > Currently, the kernel encapsulates the use of ".pushsection" 
> > > > within macros, leading to thousands of instances where these 
> > > > macros are expanded. Rather than adding ".attach_to_group" to a 
> > > > thousand functions, we can incorporate it into this macro. 
> > > > However, some functions expand this macro multiple times, 
> > > > resulting in multiple attachments to the group for those 
> > > > functions. Unfortunately, there is no alternative method to ensure 
> > > > a single expansion of ".attach_to_group" As a solution, we propose adding an option to address this issue.
> > > >
> > > > Thanks.
> > >
> > > I am curious whether the "kernel" in your message refers to the Linux kernel.
> > > .attach_to_group is a new directive from Oct 2020 and the Linux 
> > > kernel doesn't use it.
> >
> > The Linux kernel uses ".pushsection" and forcefully "KEEP" them. It 
> > unnecessarily retains some pushed sections, increasing the size of 
> > vmlinux. I came up with an idea that we can group section pushers and 
> > pushed sections and make them garbage collect together, as if there's 
> > no section pusher, there won't be a need for pushed sections anymore.
> >
> > > If you have plans to use .attach_to_group in the upstream kernel, 
> > > I'd like to hear more as LLVM integrated assembler doesn't support 
> > > .attach_to_group and I think it would be difficult to add the support.
> > > Some use cases of .attach_to_group (which doesn't intend to support 
> > > GRP_COMDAT groups) can be replaced with SHF_LINK_ORDER [1] (with 
> > > good support in newer binutils; I am unsure whether it's 2.35/2.36 
> > > that it looks stable):
> > >
> > > [1]: 
> > > https://maskray.me/blog/2021-01-31-metadata-sections-comdat-and-shf-
> > > link-order I shall revise the article, but it basically covers most 
> > > things I can think of.
> >
> > I had the pleasure of reading your article and was pleasantly 
> > surprised to discover the functionality of SHF_LINK_ORDER. After some 
> > experiments, the SHF_LINK_ORDER methods results same with the “.attach_to_group” methods.
> >
> > However, when it comes to .s file, SHF_LINK_ORDER methods cannot 
> > replace with ".attach_to_group" methods. It's seems to be hard to find 
> > a symbol name to refer when assembly macro expand.
> >
> > For example, if we modify the kernel code as follows, adding a label 
> > before the .pushsection to make the pushed section refer to the section pusher.
> >
> >     diff --git a/arch/riscv/include/asm/asm-extable.h b/arch/riscv/include/asm/asm-extable.h
> >     index 14be0673f5b5..1ade4f8fd79e 100644
> >     --- a/arch/riscv/include/asm/asm-extable.h
> >     +++ b/arch/riscv/include/asm/asm-extable.h
> >     @@ -7,10 +7,13 @@
> >     #define EX_TYPE_BPF                    2
> >     #define EX_TYPE_UACCESS_ERR_ZERO       3
> >
> >     +#define ___PASTE(a,b) a##b
> >     +#define __PASTE(a,b) ___PASTE(a,b)
> >     +
> >     #ifdef __ASSEMBLY__
> >
> >     #define __ASM_EXTABLE_RAW(insn, fixup, type, data)     \
> >     -       .pushsection    __ex_table, "a";                \
> >     +       __PASTE(__extable,__LINE__): .pushsection       __ex_table, "ao",__PASTE(__extable,__LINE__);           \
> >             .balign         4;                              \
> >             .long           ((insn) - .);                   \
> >             .long           ((fixup) - .);                  \
> >
> > __ASM_EXTABLE_RAW will expand to linux/arch/riscv/lib/uaccess.s:
> >
> >     .macro _asm_extable, insn, fixup
> >     __extable30: .pushsection __ex_table, "ao",__extable30; .balign 4; .long ((\insn) - .); .long ((\fixup) - .); .short (1); .short (0); .popsection;
> >     .endm
> >
> > The C macro expands to an assembly macro, which, in turn, is expanded 
> > multiple times, leading to error of multiple definition of symbols.  
> > This situation is similar to receiving a warning when attempting to 
> > attach to a group multiple times.
> >
> > And it seems that there is no way to set symbol only in section scope, 
> > and there is no fixed symbol in sections pusher that pushed section can refer to.
> > Is there any possible solution to such a situation?
> >
> > At last, thank you once again for suggesting the SHF_LINK_ORDER method.
> >
> > Tan Yuan


Hi,

We just found an alternative way to solve this problem. As the perfect
functionality of SHF_LINK_ORDER, it can replace .attach_to_group completely in
our working patch to Linux kernel.

Very grateful for your advice.


> Hi Tan, is the following your intended use case?
> 
> cat > a.s <<eof
> .macro _asm_extable, insn, fixup
>   .pushsection __ex_table, "a"
>   .attach_to_group .text
>   .byte 0
>   .popsection
> .endm
> 
> .globl __asm_copy_to_user
> __asm_copy_to_user:
> 100:
>   _asm_extable 100b, 10f
>   addi a1, a1, 1
>   _asm_extable 100b, 10f
>   addi a0, a0, 1
> 10:
> eof
> riscv64-linux-gnu-gcc -c a.s
> 
> % readelf -g a.o
> 
> group section [    1] `.group' [.text] contains 2 sections:
>    [Index]    Name
>    [    5]   __ex_table
>    [    6]   __ex_table
> 
> There are two __ex_table sections in the section group. This is a bit wasteful due to excess section headers, but the linker will combine them.
> 
> If you switch to SHF_LINK_ORDER, you can use the section symbol .text:
> .pushsection __ex_table, "ao", @progbits, .text This is well supported in gas from 2.35 onwards and LLVM integrated assembler.

It is a lot complicated, as -ffunction-sections should be enable so the .text
become something else like .text.function_name. But it won’t be a problem
anymore. We use some macro magic to solve it.

I just check GCC 7.3 with binutils 2.30 works well with our SHF_LINK_ORDER
patch when compiling kernel. By the way, do you know when LLVM integrated
assembler supports SHF_LINK_ORDER?

> > > > Tan Yuan (1): gas: add new command line option --no-group-check
> > > >
> > > >  gas/as.c             | 10 +++++++++- gas/as.h             |  3 +++
> > > >  gas/config/obj-elf.c |  2 +- gas/doc/as.texi      |  4 ++++ 4 files
> > > >  changed, 17 insertions(+), 2 deletions(-)
> > > >
> > > > -- 2.41.0
> > > >
> 
>