search

anakryiko / retsnoop

Investigate kernel error call stacks
BSD 2-Clause "Simplified" License
186 stars 32 forks source link

Add functions defined in compile unit(s) to the list of globs #5

Closed ThinkerYzu1 closed 2 years ago

ThinkerYzu1 commented 2 years ago

By prefixing ':' before the name of a compile unit, all functions defined in the compile unit will be added to the specified list of globs. For example, giving '-a :path/to/XXX.c', all functions defined in XXX.c will be added to the allow list.

anakryiko commented 2 years ago

Oh, two more things. It would be good to emit list of matched CUs and functions within each in either verbose or very verbose mode, so that one can use this to see what will be matched. Can you please add some logging like that, if it's not too cumbersome (from retsnoop side, not sidecar).

And please also update README with an example of using this :file/path/glob* syntax, it's super cool feature, but not many will realize that it is supported. Also please mention that it requires the presence of DWARF and vmlinux image.

ThinkerYzu1 commented 2 years ago

Looks great overall, few styling nits, but also query_symbols API probably is better reworked to not impose unnecessary restrictions.

Also we've discussed being able to query something like -a :kernel/bpf/syscall.c. I've tried that and it didn't work. I had to do -a :*kernel/bpf/syscall.c. Can we still support the former as well?

How about to prepend a '*' to all globs if it is not begin with '/'?

anakryiko commented 2 years ago

How about to prepend a '*' to all globs if it is not begin with '/'?

this is hacky and error-prone. If I specify -a andrii* it shouldn't match all files just because they were built in /home/users/andrii, right?

anakryiko commented 2 years ago

If there is no such information in DWARF (but I thought you said there is?), we can re-implement https://github.com/anakryiko/retsnoop/blob/master/src/retsnoop.c#L741 on the Rust side. Seems to be working totally fine in practice.

ThinkerYzu1 commented 2 years ago

How about to prepend a '*' to all globs if it is not begin with '/'?

this is hacky and error-prone. If I specify -a andrii* it shouldn't match all files just because they were built in /home/users/andrii, right?

<1777d83> DW_AT_comp_dir : (indirect string, offset: 0x3b): /build/linux-gyl3ry/linux-5.13.0/debian/build/build-generic <1790bda> DW_AT_name : (indirect string, offset: 0xabffc): /build/linux-gyl3ry/linux-5.13.0/kernel/power/snapshot.c Above is an example of attributes of a compile unit. Maybe we can try the common parent of DW_AT_comp_dir and DW_AT_name. We can do several things for users. 1. Suggest a common parent from names and compile dirs, 2. allow users to override the suggested common parent, and 3. allow users to query all compile unit names or names matching a pattern.
ThinkerYzu1 commented 2 years ago

If there is no such information in DWARF (but I thought you said there is?), we can re-implement https://github.com/anakryiko/retsnoop/blob/master/src/retsnoop.c#L741 on the Rust side. Seems to be working totally fine in practice.

anakryiko commented 2 years ago

How about to prepend a '*' to all globs if it is not begin with '/'?

this is hacky and error-prone. If I specify -a andrii* it shouldn't match all files just because they were built in /home/users/andrii, right?

<1777d83> DW_AT_comp_dir : (indirect string, offset: 0x3b): /build/linux-gyl3ry/linux-5.13.0/debian/build/build-generic <1790bda> DW_AT_name : (indirect string, offset: 0xabffc): /build/linux-gyl3ry/linux-5.13.0/kernel/power/snapshot.c Above is an example of attributes of a compile unit. Maybe we can try the common parent of DW_AT_comp_dir and DW_AT_name. We can do several things for users. 1. Suggest a common parent from names and compile dirs, 2. allow users to override the suggested common parent, and 3. allow users to query all compile unit names or names matching a pattern.

Ok, it makes sense that there is not "canonical" source dir path. Can you please just implement https://github.com/anakryiko/retsnoop/blob/master/src/retsnoop.c#L741 for the Rust side then?

anakryiko commented 2 years ago

Thanks a lot, this is a great feature! I've applied it, but I've noticed that glob matching doesn't always work as expected. Any idea why the following examples don't work: -a ':fs/btrfs/*/*.c', -a ':fs/btrfs/*.c', -a ':fs/btrfs/*', but something like -a 'fs/btrfs/inode.c' does?

ThinkerYzu1 commented 2 years ago

Thanks a lot, this is a great feature! I've applied it, but I've noticed that glob matching doesn't always work as expected. Any idea why the following examples don't work: -a ':fs/btrfs/*/*.c', -a ':fs/btrfs/*.c', -a ':fs/btrfs/*', but something like -a 'fs/btrfs/inode.c' does?

I am not sure. Without prefixing ':', it will do symbol lookup, not trying to find compile units. It should not work. Could you share me your kernel binary? I can try it locally.

anakryiko commented 2 years ago

Oh, the last example that does work had to be with a colon: -a ':fs/btrfs/inode.c'. I'll share kernel details offline.