Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

On Aug 6, 2024, at 1:01 PM, Steven Rostedt [off-list ref] wrote:

On Tue, 6 Aug 2024 16:00:49 -0400
Steven Rostedt [off-list ref] wrote:

quoted

quoted

quoted

quoted

+ if (IS_ENABLED(CONFIG_LTO_CLANG) && !addr)
+ addr = kallsyms_lookup_name_without_suffix(trace_kprobe_symbol(tk));
+    

So you do the lookup twice if this is enabled?

Why not just use "kallsyms_lookup_name_without_suffix()" the entire time,
and it should work just the same as "kallsyms_lookup_name()" if it's not
needed?    

We still want to give priority to full match. For example, we have:

[root@~]# grep c_next /proc/kallsyms
ffffffff81419dc0 t c_next.llvm.7567888411731313343
ffffffff81680600 t c_next
ffffffff81854380 t c_next.llvm.14337844803752139461

If the goal is to explicitly trace c_next.llvm.7567888411731313343, the
user can provide the full name. If we always match _without_suffix, all
of the 3 will match to the first one. 

Does this make sense?  

Yes. Sorry, I missed the "&& !addr)" after the "IS_ENABLED()", which looked
like you did the command twice.

But that said, does this only have to be for llvm? Or should we do this for
even gcc? As I believe gcc can give strange symbols too.

I think most of the issue comes with LTO, as LTO promotes local static
functions to global functions. IIUC, we don't have GCC built, LTO enabled
kernel yet.

In my GCC built, we have suffixes like ".constprop.0", ".part.0", ".isra.0", 
and ".isra.0.cold". We didn't do anything about these before this set. So I 
think we are OK not handling them now. We sure can enable it for GCC built
kernel in the future. 

Thanks,
Song