Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 31 Jul 2022 20:37:02 +0800
From:   Leo Yan <leo.yan@linaro.org>
To:     Ian Rogers <irogers@google.com>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Fangrui Song <maskray@google.com>,
        Chang Rui <changruinj@gmail.com>,
        linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 1/2] perf symbol: Correct address for bss symbols
Message-ID: <20220731123702.GA205142@leoy-ThinkPad-X240s>
References: <20220724060013.171050-1-leo.yan@linaro.org>
 <20220724060013.171050-2-leo.yan@linaro.org>
 <CAP-5=fVY=_WUpke0sRMpvbJFu9JuYwqiwAmKdCS+=u7vEG-2uA@mail.gmail.com>
 <20220730093819.GA574473@leoy-ThinkPad-X240s>
 <CAP-5=fWA8R9Ee5L9xgPtva9dvQ12uKqC1o4zgzPs0B9XWSMxhQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAP-5=fWA8R9Ee5L9xgPtva9dvQ12uKqC1o4zgzPs0B9XWSMxhQ@mail.gmail.com>
Precedence: bulk

On Sat, Jul 30, 2022 at 08:21:10AM -0700, Ian Rogers wrote:

[...]

> > On the other hand, I can accept to simply change pr_warning() to
> > pr_debug4() to avoid warning flood, the log still can help us to find
> > potential symbol parsing issue, so far they are not false-positive
> > reporting.
> 
> Thanks, I suspect the ELF that the Java agent has created isn't good.
> The Java agent is part of perf as and so is the ELF file generation
> code:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/genelf.c?h=perf/core#n367

I think it's no need to change the function jit_write_elf(), please see
below comment.

> I took a quick look but most of the logic is in libelf - it seems less
> obvious the bug would be there rather than perf. Could you take a
> look? Ideally there'd be a quick fix that keeps all the benefits of
> your change and the jvmti code working.

A bit more finding for java JIT symbols.  Let's see below two samples:

     6.72%  java             /home/leoy/.debug/jit/java-jit-20220731.XXKRpgmR/jitted-214330-29.so    0x5b54             B [.] Interpreter
     0.90%  java             /home/leoy/.debug/jit/java-jit-20220731.XXKRpgmR/jitted-214330-3475.so  0x4fc              B [.] jdk.internal.math.FloatingDecimal$BinaryToASCIIBuffer.dtoa(int, long, int, boolean)

I can see the DSO "jitted-214330-29.so" only contains a java JIT symbol
"Interpreter", and I also observed the same behavior for other JIT
symbols, e.g. jitted-214330-3475.so only contains the symbol
"jdk.internal.math.FloatingDecimal$BinaryToASCIIBuffer.dtoa(int, long,
int, boolean)".

We always see these JIT symbols always have the consistent start file
address, but this would not introduce conflit since every JIT symbol has
its own DSO rather than share the same DSO.

I think now I understand your meaning, and your below change is fine for
me, I tested it and confirmed it can show up java JIT symbols properly.
Just a comment, it would be better to add a comment for why we need to
add symbols when failed to find program header, like:

  if (elf_read_program_header(syms_ss->elf,
                            (u64)sym.st_value, &phdr)) {
        pr_debug4("%s: failed to find program header for "
                  "symbol: %s\n", __func__, elf_name);
        pr_debug4("%s: adjusting symbol: st_value: %#" PRIx64 " "
                  "sh_addr: %#" PRIx64 " sh_offset: %#" PRIx64 "\n",
                  __func__, (u64)sym.st_value, (u64)shdr.sh_addr,
                  (u64)shdr.sh_offset);
        /*
         * Fail to find program header, let's rollback to use shdr.sh_addr
         * and shdr.sh_offset to calibrate symbol's file address, though
         * this is not necessary for normal C ELF file, we still need to
         * handle java JIT symbols in this case.
         */
        sym.st_value -= shdr.sh_addr - shdr.sh_offset;
  } else {
        ...
  }

Could you send a formal patch for this?  Thanks!

Leo

> > Thanks,
> > Leo
> >
> > > ```
> > > --- a/tools/perf/util/symbol-elf.c
> > > +++ b/tools/perf/util/symbol-elf.c
> > > @@ -1305,16 +1305,21 @@ dso__load_sym_internal(struct dso *dso, struct
> > > map *map, struct symsrc *syms
> > > _ss,
> > >
> > >                        if (elf_read_program_header(syms_ss->elf,
> > >                                                    (u64)sym.st_value, &phdr)) {
> > > -                               pr_warning("%s: failed to find program
> > > header for "
> > > +                               pr_debug4("%s: failed to find program
> > > header for "
> > >                                           "symbol: %s st_value: %#" PRIx64 "\n",
> > >                                           __func__, elf_name,
> > > (u64)sym.st_value);
> > > -                               continue;
> > > +                               pr_debug4("%s: adjusting symbol:
> > > st_value: %#" PRIx64 " "
> > > +                                       "sh_addr: %#" PRIx64 "
> > > sh_offset: %#" PRIx64 "\n",
> > > +                                       __func__, (u64)sym.st_value,
> > > (u64)shdr.sh_addr,
> > > +                                       (u64)shdr.sh_offset);
> > > +                               sym.st_value -= shdr.sh_addr - shdr.sh_offset;
> > > +                       } else {
> > > +                               pr_debug4("%s: adjusting symbol:
> > > st_value: %#" PRIx64 " "
> > > +                                       "p_vaddr: %#" PRIx64 "
> > > p_offset: %#" PRIx64 "\n",
> > > +                                       __func__, (u64)sym.st_value,
> > > (u64)phdr.p_vaddr,
> > > +                                       (u64)phdr.p_offset);
> > > +                               sym.st_value -= phdr.p_vaddr - phdr.p_offset;
> > >                        }
> > > -                       pr_debug4("%s: adjusting symbol: st_value: %#"
> > > PRIx64 " "
> > > -                                 "p_vaddr: %#" PRIx64 " p_offset: %#"
> > > PRIx64 "\n",
> > > -                                 __func__, (u64)sym.st_value,
> > > (u64)phdr.p_vaddr,
> > > -                                 (u64)phdr.p_offset);
> > > -                       sym.st_value -= phdr.p_vaddr - phdr.p_offset;
> > >                }
> > >
> > >                demangled = demangle_sym(dso, kmodule, elf_name);
> > > ```
> > >
> > > Thanks,
> > > Ian
> > >
> > > >
> > > >                 demangled = demangle_sym(dso, kmodule, elf_name);
> > > > --
> > > > 2.25.1
> > > >