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: <20230620132025.105563-1-wangrui@loongson.cn> <CAM9d7chjhFNjDMADJTTz5TNgCCuyPzy8VCDvZxw5K5girfWu1A@mail.gmail.com>
 <CAHirt9hDc-22AKrRZEEUW9LUa3hjmnEVrYDNyWCx4kkWU4WzXA@mail.gmail.com>
In-Reply-To: <CAHirt9hDc-22AKrRZEEUW9LUa3hjmnEVrYDNyWCx4kkWU4WzXA@mail.gmail.com>
From:   Namhyung Kim <namhyung@kernel.org>
Date:   Tue, 20 Jun 2023 21:54:04 -0700
Message-ID: <CAM9d7cgkV3CZ_2xAp+v62hpfutAB+RpsnvGa2u09P4C0Fm5ULw@mail.gmail.com>
Subject: Re: [PATCH] perf annotate: Fix instruction association and parsing
 for LoongArch
To:     WANG Rui <wangrui@loongson.cn>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Huacai Chen <chenhuacai@loongson.cn>,
        loongarch@lists.linux.dev, Tiezhu Yang <yangtiezhu@loongson.cn>,
        WANG Xuerui <kernel@xen0n.name>, linux-kernel@vger.kernel.org,
        linux-perf-users@vger.kernel.org, loongson-kernel@lists.loongnix.cn
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Tue, Jun 20, 2023 at 9:37 PM WANG Rui <wangrui@loongson.cn> wrote:
>
> Hello Namhyung,
>
> On Wed, Jun 21, 2023 at 2:42 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > > +static int loongarch_call__parse(struct arch *arch, struct ins_operands *ops, struct map_symbol *ms)
> > > +{
> > > +       char *c, *endptr, *tok, *name;
> > > +       struct map *map = ms->map;
> > > +       struct addr_map_symbol target = {
> > > +               .ms = { .map = map, },
> >
> > Looking here...
> >
> > > +       };
> > > +
> > > +       c = strchr(ops->raw, '#');
> > > +       if (c++ == NULL)
> > > +               return -1;
> > > +
> > > +       ops->target.addr = strtoull(c, &endptr, 16);
> > > +
> > > +       name = strchr(endptr, '<');
> > > +       name++;
> > > +
> > > +       if (arch->objdump.skip_functions_char &&
> > > +           strchr(name, arch->objdump.skip_functions_char))
> > > +               return -1;
> > > +
> > > +       tok = strchr(name, '>');
> > > +       if (tok == NULL)
> > > +               return -1;
> > > +
> > > +       *tok = '\0';
> > > +       ops->target.name = strdup(name);
> > > +       *tok = '>';
> > > +
> > > +       if (ops->target.name == NULL)
> > > +               return -1;
> > > +
> > > +       target.addr = map__objdump_2mem(map, ops->target.addr);
> > > +
> > > +       if (maps__find_ams(ms->maps, &target) == 0 &&
> > > +           map__rip_2objdump(target.ms.map, map__map_ip(target.ms.map, target.addr)) == ops->target.addr)
> >
> > So the target.ms.map is 'map', right?  Then we can reduce the line.
>
> toos/perf/util/maps.c:
>
> > int maps__find_ams(struct maps *maps, struct addr_map_symbol *ams)
> > {
> >     if (ams->addr < map__start(ams->ms.map) || ams->addr >= map__end(ams->ms.map)) {
> >         if (maps == NULL)
> >             return -1;
> >         ams->ms.map = maps__find(maps, ams->addr);
>
> Is it possible that `target.ms.map` might be reassigned within the
> `maps_find_ams` function in this case?

Ah, ok.  Missed that part.  It can change if the target is from a
different library.

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

>
> >         if (ams->ms.map == NULL)
> >             return -1;
> >     }
> >
> >     ams->al_addr = map__map_ip(ams->ms.map, ams->addr);
> >     ams->ms.sym = map__find_symbol(ams->ms.map, ams->al_addr);
> >
> >     return ams->ms.sym ? 0 : -1;
> > }

>