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;
MIME-Version: 1.0
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>
 <20220731123702.GA205142@leoy-ThinkPad-X240s>
In-Reply-To: <20220731123702.GA205142@leoy-ThinkPad-X240s>
From:   Ian Rogers <irogers@google.com>
Date:   Sun, 31 Jul 2022 09:51:53 -0700
Message-ID: <CAP-5=fVKkd92FP9D2qccmRHMJHbYNNpVC-XAuh5i7jrrTWHg8g@mail.gmail.com>
Subject: Re: [PATCH v3 1/2] perf symbol: Correct address for bss symbols
To:     Leo Yan <leo.yan@linaro.org>
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
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sun, Jul 31, 2022 at 5:37 AM Leo Yan <leo.yan@linaro.org> wrote:
>
> 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!

Done. Thanks for sanity checking all of this! Please double check and
add the reviewed/acked-by:
https://lore.kernel.org/lkml/20220731164923.691193-1-irogers@google.com/

Thanks,
Ian

> 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
> > > > >