mbox series

[PATCHv4,0/3] pahole/kernel: Workaround dwarf bug for function encoding

Message ID 20201106222512.52454-1-jolsa@kernel.org (mailing list archive)
Headers show
Series pahole/kernel: Workaround dwarf bug for function encoding | expand

Message

Jiri Olsa Nov. 6, 2020, 10:25 p.m. UTC
hi,
because of gcc bug [1] we can no longer rely on DW_AT_declaration
attribute to filter out declarations and end up with just
one copy of the function in the BTF data.

It seems this bug is not easy to fix, but regardless if the
it's coming soon, it's probably good idea not to depend so
much only on dwarf data and make some extra checks.

Thus for function encoding we are now doing following checks:
  - argument names are defined for the function
  - there's symbol and address defined for the function
  - function address belongs to ftrace locations (new in v2)
  - function is generated only once

v4 changes:
  - added acks
  - renames and change functions_valid to be local var [Andrii]
  - fixed error path (return err) of collect_symbols

v3 changes:
  - added Hao's ack for patch 1
  - fixed realloc memory leak [Andrii]
  - fixed addrs_cmp function [Andrii]
  - removed SET_SYMBOL macro [Andrii]
  - fixed the 'valid' function logic
  - added .init.bpf.preserve_type check
  - added iterator functions to new kernel section
    .init.bpf.preserve_type [Yonghong]

v2 changes:
  - add check ensuring functions belong to ftrace's mcount
    locations, this way we ensure to have in BTF only
    functions available for ftrace - patch 2 changelog
    describes all details
  - use collect* function names [Andrii]
  - use conventional size increase in realloc [Andrii]
  - drop elf_sym__is_function check
  - drop patch 3, it's not needed, because we follow ftrace
    locations

thanks,
jirka


[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97060

Comments

Andrii Nakryiko Nov. 6, 2020, 10:56 p.m. UTC | #1
On Fri, Nov 6, 2020 at 2:25 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> hi,
> because of gcc bug [1] we can no longer rely on DW_AT_declaration
> attribute to filter out declarations and end up with just
> one copy of the function in the BTF data.
>
> It seems this bug is not easy to fix, but regardless if the
> it's coming soon, it's probably good idea not to depend so
> much only on dwarf data and make some extra checks.
>
> Thus for function encoding we are now doing following checks:
>   - argument names are defined for the function
>   - there's symbol and address defined for the function
>   - function address belongs to ftrace locations (new in v2)
>   - function is generated only once
>
> v4 changes:
>   - added acks
>   - renames and change functions_valid to be local var [Andrii]
>   - fixed error path (return err) of collect_symbols
>
> v3 changes:
>   - added Hao's ack for patch 1
>   - fixed realloc memory leak [Andrii]
>   - fixed addrs_cmp function [Andrii]
>   - removed SET_SYMBOL macro [Andrii]
>   - fixed the 'valid' function logic
>   - added .init.bpf.preserve_type check
>   - added iterator functions to new kernel section
>     .init.bpf.preserve_type [Yonghong]
>
> v2 changes:
>   - add check ensuring functions belong to ftrace's mcount
>     locations, this way we ensure to have in BTF only
>     functions available for ftrace - patch 2 changelog
>     describes all details
>   - use collect* function names [Andrii]
>   - use conventional size increase in realloc [Andrii]
>   - drop elf_sym__is_function check
>   - drop patch 3, it's not needed, because we follow ftrace
>     locations
>
> thanks,
> jirka
>
>
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97060
>

For the series:

Acked-by: Andrii Nakryiko <andrii@kernel.org>
Arnaldo Carvalho de Melo Nov. 9, 2020, 5:29 p.m. UTC | #2
Em Fri, Nov 06, 2020 at 02:56:45PM -0800, Andrii Nakryiko escreveu:
> On Fri, Nov 6, 2020 at 2:25 PM Jiri Olsa <jolsa@kernel.org> wrote:
 
> For the series:
 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>

Thanks, applied, testing now.

- Arnaldo
Arnaldo Carvalho de Melo Nov. 9, 2020, 7:11 p.m. UTC | #3
Em Mon, Nov 09, 2020 at 02:29:11PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Nov 06, 2020 at 02:56:45PM -0800, Andrii Nakryiko escreveu:
> > On Fri, Nov 6, 2020 at 2:25 PM Jiri Olsa <jolsa@kernel.org> wrote:
>  
> > For the series:
>  
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> Thanks, applied, testing now.

Now we have:

$ pfunct -F btf /sys/kernel/btf/vmlinux  | wc -l
38816
$ pfunct -F btf /sys/kernel/btf/vmlinux  | head
get_e820_md5
relocate_restore_code
resume_play_dead
bsp_pm_callback
msr_initialize_bdw
msr_save_cpuid_features
pm_check_save_msr
amd_bus_cpu_online
update_res
pci_read
$

$ pfunct -F btf /sys/kernel/btf/vmlinux -f msr_save_cpuid_features
int msr_save_cpuid_features(const struct x86_cpu_id  * c);
$
$ pfunct -F btf /sys/kernel/btf/vmlinux -f tcp_v4_rcv
int tcp_v4_rcv(struct sk_buff * skb);
$

[acme@five ~]$ pfunct -F btf /sys/kernel/btf/vmlinux --class=sk_buff | head
pskb_expand_head
skb_put
audit_list_rules_send
netlink_ack
consume_skb
skb_queue_head
skb_queue_tail
netlink_broadcast
__nlmsg_put
kfree_skb
[acme@five ~]$ pfunct -F btf /sys/kernel/btf/vmlinux -f audit_list_rules_send
int audit_list_rules_send(struct sk_buff * request_skb, int seq);
[acme@five ~]$ pfunct -F btf /sys/kernel/btf/vmlinux -f netlink_broadcast
int netlink_broadcast(struct sock * ssk, struct sk_buff * skb, __u32 portid, __u32 group, gfp_t allocation);
[acme@five ~]$

Seems to work :-)

In a future version I'll make it work with btf and
/sys/kernel/btf/vmlinux by default if only function names are provided,
like pahole with types:

[acme@five ~]$ pahole sk_buff_head
struct sk_buff_head {
	struct sk_buff *           next;                 /*     0     8 */
	struct sk_buff *           prev;                 /*     8     8 */
	__u32                      qlen;                 /*    16     4 */
	spinlock_t                 lock;                 /*    20     4 */

	/* size: 24, cachelines: 1, members: 4 */
	/* last cacheline: 24 bytes */
};
[acme@five ~]$ pahole list_head
struct list_head {
	struct list_head *         next;                 /*     0     8 */
	struct list_head *         prev;                 /*     8     8 */

	/* size: 16, cachelines: 1, members: 2 */
	/* last cacheline: 16 bytes */
};
[acme@five ~]$

Pushed out.

- Arnaldo