Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Date:   Wed, 13 Sep 2023 10:06:12 +0200
From:   Petr Mladek <pmladek@suse.com>
To:     Alessandro Carminati <alessandro.carminati@gmail.com>
Cc:     Alexander Lobakin <aleksander.lobakin@intel.com>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Daniel Bristot de Oliveira <bristot@kernel.org>,
        Josh Poimboeuf <jpoimboe@kernel.org>,
        Masahiro Yamada <masahiroy@kernel.org>,
        Luis Chamberlain <mcgrof@kernel.org>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Nicolas Schier <nicolas@fjasle.eu>,
        Nick Alcock <nick.alcock@oracle.com>,
        Kris Van Hees <kris.van.hees@oracle.com>,
        Eugene Loh <eugene.loh@oracle.com>,
        Francis Laniel <flaniel@linux.microsoft.com>,
        Viktor Malik <vmalik@redhat.com>, linux-kbuild@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org,
        live-patching@vger.kernel.org
Subject: Re: [PATCH v3] scripts/link-vmlinux.sh: Add alias to duplicate
 symbols for kallsyms
Message-ID: <ZQFtdJEKJ9taYpA0@alley>
References: <20230828080423.3539686-1-alessandro.carminati@gmail.com>
 <d385548e-9788-2814-05c9-bb0f275b233f@intel.com>
 <CAPp5cGSHdU6z2J=bUYkXMOakFzSWzjeCznTsV=DrTSvWmaqStA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAPp5cGSHdU6z2J=bUYkXMOakFzSWzjeCznTsV=DrTSvWmaqStA@mail.gmail.com>
Precedence: bulk

On Tue 2023-09-12 16:18:00, Alessandro Carminati wrote:
> <aleksander.lobakin@intel.com> ha scritto:
> > From: Alessandro Carminati (Red Hat) <alessandro.carminati@gmail.com>
> > > sample from new v3
> > >
> > >  ~ # cat /proc/kallsyms | grep gic_mask_irq
> > >  ffffd0b03c04dae4 t gic_mask_irq
> > >  ffffd0b03c04dae4 t gic_mask_irq@_drivers_irqchip_irq-gic_c_167
> > >  ffffd0b03c050960 t gic_mask_irq
> > >  ffffd0b03c050960 t gic_mask_irq@_drivers_irqchip_irq-gic-v3_c_404
> >
> > BTW, why normalize them? Why not just
> >
> > gic_mask_irq@drivers/irqchip/...
> >
> > Aaaaand why line number? Line numbers break reproducible builds and also
> > would make it harder to refer to a particular symbol by its path and
> > name since we also have to pass its line number which may change once
> > you add a debug print there, for example.
> > OTOH there can't be 2 symbols with the same name within one file, so
> > just path + name would be enough. Or not?

I am afraid that there can be more symbols with the same name in a
single source file. For example, static variables defined inside
functions:

$> cat >test-duplicate-symbols.c <<EOT
#include <stdio.h>

void a(void)
{
	static int duplicate_var = 100;

	printf("%s: %d\n", __func__, duplicate_var);
}

void b(void)
{
	static int duplicate_var = 200;

	printf("%s: %d\n", __func__, duplicate_var);
}

int main(int argc, char *argv)
{
	a();
	b();
}
EOT

$> gcc -o test-duplicate-symbols test-duplicate-symbols.c

$> ./test-duplicate-symbols 
a: 100
b: 200

$> objdump -t test-duplicate-symbols | grep duplicate
test-duplicate-symbols:     file format elf64-x86-64
0000000000000000 l    df *ABS*  0000000000000000              test-duplicate-symbols.c
0000000000402018 l     O .data  0000000000000004              duplicate_var.2190
000000000040201c l     O .data  0000000000000004              duplicate_var.2195


> Regarding the use of full file paths and line numbers for symbol decoration,
> it indeed provides the highest level of uniqueness for each symbol.
> However, I understand your point that this level of detail might be more than
> necessary.
> 
> This approach was implemented in response to a specific request expressed in
> the live-patching list, and I wanted to ensure we met those requirements.
> I am open to revisiting this aspect, and I am willing to accommodate changes
> based on feedback.

Yeah, livepatching needs to be able to find any symbol which might
need to be accessed from the livepatch.

The line number is perfectly fine for livepatching because there
is 1:1:1 relationship between the kernel sources, binary, and
livepatch.

And it might be even useful for the tracing. It helps to find
and investigate the traced code easily.

Hmm, I understand that it complicates the live for the trace
tooling. I wonder if we could allow searching the symbols
with a pattern, e.g. the bash style
"duplicated_symbol_name-source_file_c*"


> If you believe that simplifying the format to just path + name would be more
> practical, or if you think that eliminating line numbers is a better choice
> to avoid potential issues while debugging builds, I'm open to considering
> these adjustments.
> Additionally, I've interpreted and implemented Francis's suggestion as making
> the separator a configurable option, but maybe a proper format string here,
> would be more appropriate.

Please, do not make the format configurable. I think that it will
cause more harm than good. It would make the life more complicated
for developing tracing tools.

The tools would need to support all the formats as well. Or they
would support only some and will not be able to trace kernels
with the others. Both is bad.

Anyway, thanks a lot for working on this.

Best Regards,
Petr