[PATCH] Revert "scripts/faddr2line: Check only two symbols when calculating symbol size"

[PATCH] Revert "scripts/faddr2line: Check only two symbols when calculating symbol size"

[Carlos Llamas]

This reverts commit c02904f05ff805d6c0631634d5751ebd338f75ec.

Such commit assumed that only two symbols are relevant for the symbol
size calculation. However, this can lead to an incorrect symbol size
calculation when there are mapping symbols emitted by readelf.

For instance, when feeding 'update_irq_load_avg+0x1c/0x1c4', faddr2line
might need to processes the following readelf lines:

 784284: ffffffc0081cca30   428 FUNC    GLOBAL DEFAULT     2 update_irq_load_avg
  87319: ffffffc0081ccb0c     0 NOTYPE  LOCAL  DEFAULT     2 $x.62522
  87321: ffffffc0081ccbdc     0 NOTYPE  LOCAL  DEFAULT     2 $x.62524
  87323: ffffffc0081ccbe0     0 NOTYPE  LOCAL  DEFAULT     2 $x.62526
  87325: ffffffc0081ccbe4     0 NOTYPE  LOCAL  DEFAULT     2 $x.62528
  87327: ffffffc0081ccbe8     0 NOTYPE  LOCAL  DEFAULT     2 $x.62530
  87329: ffffffc0081ccbec     0 NOTYPE  LOCAL  DEFAULT     2 $x.62532
  87331: ffffffc0081ccbf0     0 NOTYPE  LOCAL  DEFAULT     2 $x.62534
  87332: ffffffc0081ccbf4     0 NOTYPE  LOCAL  DEFAULT     2 $x.62535
 783403: ffffffc0081ccbf4   424 FUNC    GLOBAL DEFAULT     2 sched_pelt_multiplier

The symbol size of 'update_irq_load_avg' should be calculated with the
address of 'sched_pelt_multiplier', after skipping the mapping symbols
seen in between. However, the offending commit cuts the list short and
faddr2line incorrectly assumes 'update_irq_load_avg' is the last symbol
in the section, resulting in:

  $ scripts/faddr2line vmlinux update_irq_load_avg+0x1c/0x1c4
  skipping update_irq_load_avg address at 0xffffffc0081cca4c due to size mismatch (0x1c4 != 0x3ff9a59988)
  no match for update_irq_load_avg+0x1c/0x1c4

After reverting the commit the issue is resolved:

  $ scripts/faddr2line vmlinux update_irq_load_avg+0x1c/0x1c4
  update_irq_load_avg+0x1c/0x1c4:
  cpu_of at kernel/sched/sched.h:1109
  (inlined by) update_irq_load_avg at kernel/sched/pelt.c:481

Cc: Brian Johannesmeyer <bjohannesmeyer@gmail.com>
Cc: John Stultz <jstultz@google.com>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 scripts/faddr2line | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/faddr2line b/scripts/faddr2line
index fe0cc45f03be..1fa6beef9f97 100755
--- a/scripts/faddr2line
+++ b/scripts/faddr2line
@@ -252,7 +252,7 @@ __faddr2line() {
 				found=2
 				break
 			fi
-		done < <(echo "${ELF_SYMS}" | sed 's/\[.*\]//' | ${AWK} -v sec=$sym_sec '$7 == sec' | sort --key=2 | ${GREP} -A1 --no-group-separator " ${sym_name}$")
+		done < <(echo "${ELF_SYMS}" | sed 's/\[.*\]//' | ${AWK} -v sec=$sym_sec '$7 == sec' | sort --key=2)
 
 		if [[ $found = 0 ]]; then
 			warn "can't find symbol: sym_name: $sym_name sym_sec: $sym_sec sym_addr: $sym_addr sym_elf_size: $sym_elf_size"
-- 
2.46.0.76.ge559c4bf1a-goog

Re: [PATCH] Revert "scripts/faddr2line: Check only two symbols when calculating symbol size"

[Will Deacon]

On Mon, Aug 12, 2024 at 11:01:20PM +0000, Carlos Llamas wrote:
> This reverts commit c02904f05ff805d6c0631634d5751ebd338f75ec.
> 
> Such commit assumed that only two symbols are relevant for the symbol
> size calculation. However, this can lead to an incorrect symbol size
> calculation when there are mapping symbols emitted by readelf.
> 
> For instance, when feeding 'update_irq_load_avg+0x1c/0x1c4', faddr2line
> might need to processes the following readelf lines:
> 
>  784284: ffffffc0081cca30   428 FUNC    GLOBAL DEFAULT     2 update_irq_load_avg
>   87319: ffffffc0081ccb0c     0 NOTYPE  LOCAL  DEFAULT     2 $x.62522
>   87321: ffffffc0081ccbdc     0 NOTYPE  LOCAL  DEFAULT     2 $x.62524
>   87323: ffffffc0081ccbe0     0 NOTYPE  LOCAL  DEFAULT     2 $x.62526
>   87325: ffffffc0081ccbe4     0 NOTYPE  LOCAL  DEFAULT     2 $x.62528
>   87327: ffffffc0081ccbe8     0 NOTYPE  LOCAL  DEFAULT     2 $x.62530
>   87329: ffffffc0081ccbec     0 NOTYPE  LOCAL  DEFAULT     2 $x.62532
>   87331: ffffffc0081ccbf0     0 NOTYPE  LOCAL  DEFAULT     2 $x.62534
>   87332: ffffffc0081ccbf4     0 NOTYPE  LOCAL  DEFAULT     2 $x.62535
>  783403: ffffffc0081ccbf4   424 FUNC    GLOBAL DEFAULT     2 sched_pelt_multiplier
> 
> The symbol size of 'update_irq_load_avg' should be calculated with the
> address of 'sched_pelt_multiplier', after skipping the mapping symbols
> seen in between. However, the offending commit cuts the list short and
> faddr2line incorrectly assumes 'update_irq_load_avg' is the last symbol
> in the section, resulting in:
> 
>   $ scripts/faddr2line vmlinux update_irq_load_avg+0x1c/0x1c4
>   skipping update_irq_load_avg address at 0xffffffc0081cca4c due to size mismatch (0x1c4 != 0x3ff9a59988)
>   no match for update_irq_load_avg+0x1c/0x1c4
> 
> After reverting the commit the issue is resolved:
> 
>   $ scripts/faddr2line vmlinux update_irq_load_avg+0x1c/0x1c4
>   update_irq_load_avg+0x1c/0x1c4:
>   cpu_of at kernel/sched/sched.h:1109
>   (inlined by) update_irq_load_avg at kernel/sched/pelt.c:481
> 
> Cc: Brian Johannesmeyer <bjohannesmeyer@gmail.com>
> Cc: John Stultz <jstultz@google.com>
> Cc: Will Deacon <will@kernel.org>
> Signed-off-by: Carlos Llamas <cmllamas@google.com>
> ---
>  scripts/faddr2line | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Will Deacon <will@kernel.org>

I'm assuming that Josh will pick this up.

Cheers,

Will

Re: [PATCH] Revert "scripts/faddr2line: Check only two symbols when calculating symbol size"

[Brian Johannesmeyer]

On Fri, Aug 16, 2024 at 5:50 AM Will Deacon <will@kernel.org> wrote:
>
> On Mon, Aug 12, 2024 at 11:01:20PM +0000, Carlos Llamas wrote:
> > This reverts commit c02904f05ff805d6c0631634d5751ebd338f75ec.
> >
> > Such commit assumed that only two symbols are relevant for the symbol
> > size calculation. However, this can lead to an incorrect symbol size
> > calculation when there are mapping symbols emitted by readelf.
> >
> > For instance, when feeding 'update_irq_load_avg+0x1c/0x1c4', faddr2line
> > might need to processes the following readelf lines:
> >
> >  784284: ffffffc0081cca30   428 FUNC    GLOBAL DEFAULT     2 update_irq_load_avg
> >   87319: ffffffc0081ccb0c     0 NOTYPE  LOCAL  DEFAULT     2 $x.62522
> >   87321: ffffffc0081ccbdc     0 NOTYPE  LOCAL  DEFAULT     2 $x.62524
> >   87323: ffffffc0081ccbe0     0 NOTYPE  LOCAL  DEFAULT     2 $x.62526
> >   87325: ffffffc0081ccbe4     0 NOTYPE  LOCAL  DEFAULT     2 $x.62528
> >   87327: ffffffc0081ccbe8     0 NOTYPE  LOCAL  DEFAULT     2 $x.62530
> >   87329: ffffffc0081ccbec     0 NOTYPE  LOCAL  DEFAULT     2 $x.62532
> >   87331: ffffffc0081ccbf0     0 NOTYPE  LOCAL  DEFAULT     2 $x.62534
> >   87332: ffffffc0081ccbf4     0 NOTYPE  LOCAL  DEFAULT     2 $x.62535
> >  783403: ffffffc0081ccbf4   424 FUNC    GLOBAL DEFAULT     2 sched_pelt_multiplier
> >
> > The symbol size of 'update_irq_load_avg' should be calculated with the
> > address of 'sched_pelt_multiplier', after skipping the mapping symbols
> > seen in between. However, the offending commit cuts the list short and
> > faddr2line incorrectly assumes 'update_irq_load_avg' is the last symbol
> > in the section, resulting in:
> >
> >   $ scripts/faddr2line vmlinux update_irq_load_avg+0x1c/0x1c4
> >   skipping update_irq_load_avg address at 0xffffffc0081cca4c due to size mismatch (0x1c4 != 0x3ff9a59988)
> >   no match for update_irq_load_avg+0x1c/0x1c4
> >
> > After reverting the commit the issue is resolved:
> >
> >   $ scripts/faddr2line vmlinux update_irq_load_avg+0x1c/0x1c4
> >   update_irq_load_avg+0x1c/0x1c4:
> >   cpu_of at kernel/sched/sched.h:1109
> >   (inlined by) update_irq_load_avg at kernel/sched/pelt.c:481
> >
> > Cc: Brian Johannesmeyer <bjohannesmeyer@gmail.com>
> > Cc: John Stultz <jstultz@google.com>
> > Cc: Will Deacon <will@kernel.org>
> > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> > ---
> >  scripts/faddr2line | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Acked-by: Will Deacon <will@kernel.org>
>
> I'm assuming that Josh will pick this up.
>
> Cheers,
>
> Will

Acked-by: Brian Johannesmeyer <bjohannesmeyer@gmail.com>

Good catch. Thanks for the fix.

Just curious: What compiler did you use to build the kernel, where you
hit this error with 'update_irq_load_avg'? I was unable to reproduce
it with LLVM 14. Thanks!

-Brian


-Brian

Re: [PATCH] Revert "scripts/faddr2line: Check only two symbols when calculating symbol size"

[Carlos Llamas]

On Tue, Aug 20, 2024 at 09:38:46AM -0700, Brian Johannesmeyer wrote:
> 
> Acked-by: Brian Johannesmeyer <bjohannesmeyer@gmail.com>
> 
> Good catch. Thanks for the fix.
> 
> Just curious: What compiler did you use to build the kernel, where you
> hit this error with 'update_irq_load_avg'? I was unable to reproduce
> it with LLVM 14. Thanks!
> 

You should be able to reproduce this issue as long as you hit a NOTYPE
symbol as the second entry. Using aarch64 and CFI_CLANG will make this
scenario more likely to happen. I believe I used the following:

  $ make LLVM=1 ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- defconfig
  $ ./scripts/config -e CFI_CLANG
  ...

Cheers,
Carlos Llamas

Re: [PATCH] Revert "scripts/faddr2line: Check only two symbols when calculating symbol size"

[Carlos Llamas]

On Fri, Aug 16, 2024 at 01:50:46PM +0100, Will Deacon wrote:
> On Mon, Aug 12, 2024 at 11:01:20PM +0000, Carlos Llamas wrote:
> > This reverts commit c02904f05ff805d6c0631634d5751ebd338f75ec.
> > 
> > Such commit assumed that only two symbols are relevant for the symbol
> > size calculation. However, this can lead to an incorrect symbol size
> > calculation when there are mapping symbols emitted by readelf.
> > 
> > For instance, when feeding 'update_irq_load_avg+0x1c/0x1c4', faddr2line
> > might need to processes the following readelf lines:
> > 
> >  784284: ffffffc0081cca30   428 FUNC    GLOBAL DEFAULT     2 update_irq_load_avg
> >   87319: ffffffc0081ccb0c     0 NOTYPE  LOCAL  DEFAULT     2 $x.62522
> >   87321: ffffffc0081ccbdc     0 NOTYPE  LOCAL  DEFAULT     2 $x.62524
> >   87323: ffffffc0081ccbe0     0 NOTYPE  LOCAL  DEFAULT     2 $x.62526
> >   87325: ffffffc0081ccbe4     0 NOTYPE  LOCAL  DEFAULT     2 $x.62528
> >   87327: ffffffc0081ccbe8     0 NOTYPE  LOCAL  DEFAULT     2 $x.62530
> >   87329: ffffffc0081ccbec     0 NOTYPE  LOCAL  DEFAULT     2 $x.62532
> >   87331: ffffffc0081ccbf0     0 NOTYPE  LOCAL  DEFAULT     2 $x.62534
> >   87332: ffffffc0081ccbf4     0 NOTYPE  LOCAL  DEFAULT     2 $x.62535
> >  783403: ffffffc0081ccbf4   424 FUNC    GLOBAL DEFAULT     2 sched_pelt_multiplier
> > 
> > The symbol size of 'update_irq_load_avg' should be calculated with the
> > address of 'sched_pelt_multiplier', after skipping the mapping symbols
> > seen in between. However, the offending commit cuts the list short and
> > faddr2line incorrectly assumes 'update_irq_load_avg' is the last symbol
> > in the section, resulting in:
> > 
> >   $ scripts/faddr2line vmlinux update_irq_load_avg+0x1c/0x1c4
> >   skipping update_irq_load_avg address at 0xffffffc0081cca4c due to size mismatch (0x1c4 != 0x3ff9a59988)
> >   no match for update_irq_load_avg+0x1c/0x1c4
> > 
> > After reverting the commit the issue is resolved:
> > 
> >   $ scripts/faddr2line vmlinux update_irq_load_avg+0x1c/0x1c4
> >   update_irq_load_avg+0x1c/0x1c4:
> >   cpu_of at kernel/sched/sched.h:1109
> >   (inlined by) update_irq_load_avg at kernel/sched/pelt.c:481
> > 
> > Cc: Brian Johannesmeyer <bjohannesmeyer@gmail.com>
> > Cc: John Stultz <jstultz@google.com>
> > Cc: Will Deacon <will@kernel.org>
> > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> > ---
> >  scripts/faddr2line | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Acked-by: Will Deacon <will@kernel.org>
> 
> I'm assuming that Josh will pick this up.

I don't think this was picked up yet, at least I don't see it in tip. Or
maybe I looked in the wrong place?

Cheers,
Carlos Llamas

Re: [PATCH] Revert "scripts/faddr2line: Check only two symbols when calculating symbol size"

[Josh Poimboeuf]

On Thu, Oct 17, 2024 at 12:12:16AM +0000, Carlos Llamas wrote:
> On Fri, Aug 16, 2024 at 01:50:46PM +0100, Will Deacon wrote:
> > Acked-by: Will Deacon <will@kernel.org>
> > 
> > I'm assuming that Josh will pick this up.
> 
> I don't think this was picked up yet, at least I don't see it in tip. Or
> maybe I looked in the wrong place?

Queued, thanks!

-- 
Josh