Hello Alexander, Thank you for your mail.
Il giorno lun 11 set 2023 alle ore 16:26 Alexander Lobakin <aleksander.loba...@intel.com> ha scritto: > > From: Alessandro Carminati (Red Hat) <alessandro.carmin...@gmail.com> > Date: Mon, 28 Aug 2023 08:04:23 +0000 > > > From: Alessandro Carminati <alessandro.carmin...@gmail.com> > > > > It is not uncommon for drivers or modules related to similar peripherals > > to have symbols with the exact same name. > > [...] > > > Changes from v2: > > - Alias tags are created by querying DWARF information from the vmlinux. > > - The filename + line number is normalized and appended to the original > > name. > > - The tag begins with '@' to indicate the symbol source. > > - Not a change, but worth mentioning, since the alias is added to the > > existing > > list, the old duplicated name is preserved, and the livepatch way of > > dealing > > with duplicates is maintained. > > - Acknowledging the existence of scenarios where inlined functions declared > > in > > header files may result in multiple copies due to compiler behavior, > > though > > it is not actionable as it does not pose an operational issue. > > - Highlighting a single exception where the same name refers to different > > functions: the case of "compat_binfmt_elf.c," which directly includes > > "binfmt_elf.c" producing identical function copies in two separate > > modules. > > Oh, I thought you managed to handle this in v3 since you didn't reply in > the previous thread... I want to thank you for this observation because it gives me the chance to discuss this topic. It is evident that the corner case in question is inherently challenging to address using the addr2line approach. Attempting to conceal this limitation would be counterproductive. compat_binfmt_elf.c includes directly binfmt_elf.c, addr2line can't help but report all functions and data declared in that file, coming from that file. compat_binfmt_elf.c is just a bunch of macro definitions that rename a few symbols and define some items used in macro-defined compilation in binfmt_elf.c. Looking at the functions, only two of the functions defined by compat_binfmt_elf.c are binary different from their counterpart in binfmt_elf.c. These differences, while present, are indeed minimal, but this fact not relevant to this discussion. My position is that, rather than producing a more complicated pipeline to handle this corner case, it is better to fix it. Before reading your message, I was about to send the v4, but now I'd prefer to hear the others' opinions on this matter before taking any future action. > > > > > 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? 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. 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. > > (sorry if some of this was already discussed previously) > > [...] > > Thanks, > Olek Thank you, Alessandro
