Re: [PATCH] Revert "perf cs-etm: Move definition of 'traceid_list' global variable from header file"

[Andrey Zhizhikin <andrey.z@gmail.com>]

Hello Salvatore,

On Fri, Nov 20, 2020 at 7:30 PM Salvatore Bonaccorso <carnil@debian.org> wrote:
>
> Hi Andrey,
>
> On Fri, Nov 20, 2020 at 05:31:59PM +0100, Andrey Zhizhikin wrote:
> > Hello Salvatore,
> >
> > On Fri, Nov 20, 2020 at 4:53 PM Salvatore Bonaccorso <carnil@debian.org> wrote:
> > >
> > > Hi Andrey,
> > >
> > > On Fri, Nov 20, 2020 at 03:29:39PM +0100, Andrey Zhizhikin wrote:
> > > > Hello Salvatore,
> > > >
> > > > On Fri, Nov 20, 2020 at 2:34 PM Salvatore Bonaccorso <carnil@debian.org> wrote:
> > > > >
> > > > > Hi Andrey,
> > > > >
> > > > > On Fri, Nov 20, 2020 at 10:54:22AM +0100, Andrey Zhizhikin wrote:
> > > > > > On Fri, Nov 20, 2020 at 8:39 AM Salvatore Bonaccorso <carnil@debian.org> wrote:
> > > > > > >
> > > > > > > This reverts commit 168200b6d6ea0cb5765943ec5da5b8149701f36a upstream.
> > > > > > > (but only from 4.19.y)
> > > > > >
> > > > > > This revert would fail the build of 4.19.y with gcc10, I believe the
> > > > > > original commit was introduced to address exactly this case. If this
> > > > > > is intended behavior that 4.19.y is not compiled with newer gcc
> > > > > > versions - then this revert is OK.
> > > > >
> > > > > TTBOMK, this would not regress the build for newer gcc (specifically
> > > > > gcc10) as 4.19.158 is failing perf tool builds there as well (without
> > > > > the above commit reverted). Just as an example v4.19.y does not have
> > > > > cff20b3151cc ("perf tests bp_account: Make global variable static")
> > > > > which is there in v5.6-rc6 to fix build failures with 10.0.1.
> > > > >
> > > > > But it did regress builds with older gcc's as for instance used in
> > > > > Debian buster (gcc 8.3.0) since 4.19.152.
> > > > >
> > > > > Do I possibly miss something? If there is a solution to make it build
> > > > > with newer GCCs and *not* regress previously working GCC versions then
> > > > > this is surely the best outcome though.
> > > >
> > > > I guess (and from what I understand in Leo's reply), porting of
> > > > 95c6fe970a01 ("perf cs-etm: Change tuple from traceID-CPU# to
> > > > traceID-metadata") should solve the issue for both older and newer gcc
> > > > versions.
> > > >
> > > > The breakage is now in
> > > > [tools/perf/util/cs-etm-decoder/cs-etm-decoder.c] file (which uses
> > > > traceid_list inside). This is solved with the above commit, which
> > > > concealed traceid_list internally inside [tools/perf/util/cs-etm.c]
> > > > file and exposed to [tools/perf/util/cs-etm-decoder/cs-etm-decoder.c]
> > > > via cs_etm__get_cpu() call.
> > > >
> > > > Can you try out to port that commit to see if that would solve your
> > > > regression?
> > >
> > > So something like the following will compile as well with the older
> > > gcc version.
> > >
> > > I realize: I mainline the order of the commits was:
> > >
> > > 95c6fe970a01 ("perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata")
> > > 168200b6d6ea ("perf cs-etm: Move definition of 'traceid_list' global variable from header f
> > > ile")
> > >
> > > But to v4.19.y only 168200b6d6ea was backported, and while that was
> > > done I now realize the comment was also changed including the change
> > > fom 95c6fe970a01.
> > >
> > > Thus the proposed backported patch would drop the change in
> > > tools/perf/util/cs-etm.c to the comment as this was already done.
> > > Thecnically currently the comment would be wrong, because it reads:
> > >
> > > /* RB tree for quick conversion between traceID and metadata pointers */
> > >
> > > but backport of 95c6fe970a01 is not included.
> > >
> > > Would the right thing to do thus be:
> > >
> > > - Revert b801d568c7d8 "perf cs-etm: Move definition of 'traceid_list' global variable from header file"
> > > - Backport 95c6fe970a01 ("perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata")
> > > - Backport 168200b6d6ea ("perf cs-etm: Move definition of 'traceid_list' global variable from header file")
> >
> > Yes, I believe this would be the correct course of action here; this
> > should cover the regression you've encountered and should ensure that
> > perf builds on both the "old" and "new" gcc versions.
>
> Although perf tools in v4.19.y won't compile with recent GCCs.
>
> Greg did already queued up the first part of it, so the revert. I
> think we can pick the later two commits again up after the v4.19.159
> release?

Sounds reasonable to me.

>
> Regards,
> Salvatore



-- 
Regards,
Andrey.