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

[Leo Yan <leo.yan@linaro.org>]

Hi Salvatore, Andrey,

On Fri, Nov 20, 2020 at 04:53:17PM +0100, Salvatore Bonaccorso 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")
> 
> ?
> 
> Leo ist that what you were proposing?

Though this isn't my proposing, I totally agree with this :)

Just some notes: prior to apply the commit 95c6fe970a01,
tools/perf/util/cs-etm-decoder/cs-etm-decoder.c is the only source
file which uses the variable "traceid_list"; after applied the commit
95c6fe970a01, the code for using the variable "traceid_list" has been
moved out from tools/perf/util/cs-etm-decoder/cs-etm-decoder.c and
moved in tools/perf/util/cs-etm.c.

So the commit 168200b6d6ea moves the definition of "traceid_list" from
the header file to the source file tools/perf/util/cs-etm.c and it
defines the variable as "static".

As you mentioned, backporting 95c6fe970a01 and 168200b6d6ea can fix
both for the older (8.3.0) and new GCC (10.0.1).  And I confirmed this
should not cause logic issue.

Thanks for looking at this issue.

Leo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel