Re: [PATCH v8 06/22] perf arm-spe: Refactor printing string to buffer
Hi David,
On Wed, Nov 11, 2020 at 11:03:47PM +, David Laight wrote:
> From: Dave Martin
> > Sent: 11 November 2020 17:58
> >
> > On Wed, Nov 11, 2020 at 05:39:22PM +, Arnaldo Carvalho de Melo wrote:
> > > Em Wed, Nov 11, 2020 at 03:45:23PM +, Andr� Przywara escreveu:
> > > > On 11/11/2020 15:35, Arnaldo Carvalho de Melo wrote:
> > > >
> > > > Hi Arnaldo,
> > > >
> > > > thanks for taking a look!
> > > >
> > > > > Em Wed, Nov 11, 2020 at 03:11:33PM +0800, Leo Yan escreveu:
> > > > >> When outputs strings to the decoding buffer with function snprintf(),
> > > > >> SPE decoder needs to detects if any error returns from snprintf()
> > > > >> and if
> > > > >> so needs to directly bail out. If snprintf() returns success, it
> > > > >> needs
> > > > >> to update buffer pointer and reduce the buffer length so can
> > > > >> continue to
> > > > >> output the next string into the consequent memory space.
> > > > >>
> > > > >> This complex logics are spreading in the function arm_spe_pkt_desc()
> > > > >> so
> > > > >> there has many duplicate codes for handling error detecting,
> > > > >> increment
> > > > >> buffer pointer and decrement buffer size.
> > > > >>
> > > > >> To avoid the duplicate code, this patch introduces a new helper
> > > > >> function
> > > > >> arm_spe_pkt_snprintf() which is used to wrap up the complex logics,
> > > > >> and
> > > > >> it's used by the caller arm_spe_pkt_desc().
> > > > >>
> > > > >> This patch also moves the variable 'blen' as the function's local
> > > > >> variable, this allows to remove the unnecessary braces and improve
> > > > >> the
> > > > >> readability.
> > > > >>
> > > > >> Suggested-by: Dave Martin
> > > > >> Signed-off-by: Leo Yan
> > > > >> Reviewed-by: Andre Przywara
> > > > >> ---
> > > > >> .../arm-spe-decoder/arm-spe-pkt-decoder.c | 260
> > > > >> +-
> > > > >> 1 file changed, 126 insertions(+), 134 deletions(-)
> > > > >>
> > > > >> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > > > >> b/tools/perf/util/arm-spe-
> > decoder/arm-spe-pkt-decoder.c
> > > > >> index 04fd7fd7c15f..1970686f7020 100644
> > > > >> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > > > >> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > > > >> @@ -9,6 +9,7 @@
> > > > >> #include
> > > > >> #include
> > > > >> #include
> > > > >> +#include
> > > > >>
> > > > >> #include "arm-spe-pkt-decoder.h"
> > > > >>
> > > > >> @@ -258,192 +259,183 @@ int arm_spe_get_packet(const unsigned char
> > > > >> *buf, size_t len,
> > > > >> return ret;
> > > > >> }
> > > > >>
> > > > >> +static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t
> > > > >> *blen,
> > > > >> +const char *fmt, ...)
> > > > >> +{
> > > > >> +va_list ap;
> > > > >> +int ret;
> > > > >> +
> > > > >> +/* Bail out if any error occurred */
> > > > >> +if (err && *err)
> > > > >> +return *err;
> > > > >> +
> > > > >> +va_start(ap, fmt);
> > > > >> +ret = vsnprintf(*buf_p, *blen, fmt, ap);
> > > > >> +va_end(ap);
> > > > >> +
> > > > >> +if (ret < 0) {
> > > > >> +if (err && !*err)
> > > > >> +*err = ret;
> > > > >> +
> > > > >> +/*
> > > > >> + * A return value of (*blen - 1) or more means that the
> > > > >> + * output was truncated and the buffer is overrun.
> > > > >> + */
> > > > >> +} else if (ret >= ((int)*blen - 1)) {
> > > > >> +(*buf_p)[*blen - 1] = '\0';
> > > > >> +
> > > > >> +/*
> > > > >> + * Set *err to 'ret' to avoid overflow if tries to
> > > > >> + * fill this buffer sequentially.
> > > > >> + */
> > > > >> +if (err && !*err)
> > > > >> +*err = ret;
> > > > >> +} else {
> > > > >> +*buf_p += ret;
> > > > >> +*blen -= ret;
> > > > >> +}
> > > > >> +
> > > > >> +return ret;
> > > > >> +}
> > > > >> +
>
> I'm not entirely sure that snprintf() can actually return a negative value.
>
> Every implementation (except the microsoft one) also always writes a '\0'
> even when the buffer is too short.
>
> A simple wrapper that lets you append output and detect overflow is:
> ret = vsnprintf(buf, len, ...);
> if (ret < 0)
> /* just in case */
> return 0;
> return ret > len ? len : ret;
>
> So on overflow the sum of the lengths is equal to the buffer size
> (ie includes the terminating '\0'.
We had some discussion for the return value in the old patch set, since
we want to keep the same scenmatics for the return value with
vsnprintf(), so the function arm_spe_pkt_snprintf() directly delivers
the return value from vsnprintf().
And if look at the patch 07/22, you could find the 'ret' value is not
really used at the end; the parameter 'err' is used as an accumulative
error value and it will be returned to up
Re: [PATCH v8 06/22] perf arm-spe: Refactor printing string to buffer
On Wed, Nov 11, 2020 at 03:01:27PM -0300, Arnaldo Carvalho de Melo wrote: > Em Wed, Nov 11, 2020 at 05:58:27PM +, Dave Martin escreveu: > > > > On Wed, Nov 11, 2020 at 05:39:22PM +, Arnaldo Carvalho de Melo wrote: > > > Em Wed, Nov 11, 2020 at 03:45:23PM +, Andr� Przywara escreveu: > > > > On 11/11/2020 15:35, Arnaldo Carvalho de Melo wrote: > > > > > Isn't this 'ret +=' ? Otherwise if any of these arm_spe_pkt_snprintf() > > > > > calls are made the previous 'ret' value is simply discarded. Can you > > > > > clarify this? > > > > > ret is the same as err. If err is negative (from previous calls), we > > > > return that straight away, so it does nothing but propagating the error. > > > > Usually the return of a snprintf is used to account for buffer space, ok > > > I'll have to read it, which I shouldn't as snprintf has a well defined > > > meaning... > > > > Ok, now that I look at it, I realize it is not a snprintf() routine, but > > > something with different semantics, that will look at a pointer to an > > > integer and then do nothing if it comes with some error, etc, confusing > > > :-/ > > > Would you be happier if the function were renamed? > > > Originally we were aiming for snprintf() semantics, but this still > > spawns a lot of boilerplate code and encourages mistakes in the local > > caller here -- hence the current sticky error approach. > > > So maybe the name should now be less "snprintf"-like. > > Please, its important to stick to semantics for such well known type of > routines, helps reviewing, etc. My bad, will change the function name to arm_spe_pkt_out_string(). > I'll keep the series up to that point and will run my build tests, then > push it publicly to acme/perf/core and you can go from there, ok? Will follow up and rebase patches for next version. > I've changed the BIT() to BIT_ULL() as Andre suggested and I'm testing > it again. I worry that consumed your (Arnaldo/Andre/Dave) much time, but very appreciate you guy's helping. Thanks, Leo
RE: [PATCH v8 06/22] perf arm-spe: Refactor printing string to buffer
From: Dave Martin
> Sent: 11 November 2020 17:58
>
> On Wed, Nov 11, 2020 at 05:39:22PM +, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Nov 11, 2020 at 03:45:23PM +, Andr� Przywara escreveu:
> > > On 11/11/2020 15:35, Arnaldo Carvalho de Melo wrote:
> > >
> > > Hi Arnaldo,
> > >
> > > thanks for taking a look!
> > >
> > > > Em Wed, Nov 11, 2020 at 03:11:33PM +0800, Leo Yan escreveu:
> > > >> When outputs strings to the decoding buffer with function snprintf(),
> > > >> SPE decoder needs to detects if any error returns from snprintf() and
> > > >> if
> > > >> so needs to directly bail out. If snprintf() returns success, it needs
> > > >> to update buffer pointer and reduce the buffer length so can continue
> > > >> to
> > > >> output the next string into the consequent memory space.
> > > >>
> > > >> This complex logics are spreading in the function arm_spe_pkt_desc() so
> > > >> there has many duplicate codes for handling error detecting, increment
> > > >> buffer pointer and decrement buffer size.
> > > >>
> > > >> To avoid the duplicate code, this patch introduces a new helper
> > > >> function
> > > >> arm_spe_pkt_snprintf() which is used to wrap up the complex logics, and
> > > >> it's used by the caller arm_spe_pkt_desc().
> > > >>
> > > >> This patch also moves the variable 'blen' as the function's local
> > > >> variable, this allows to remove the unnecessary braces and improve the
> > > >> readability.
> > > >>
> > > >> Suggested-by: Dave Martin
> > > >> Signed-off-by: Leo Yan
> > > >> Reviewed-by: Andre Przywara
> > > >> ---
> > > >> .../arm-spe-decoder/arm-spe-pkt-decoder.c | 260 +-
> > > >> 1 file changed, 126 insertions(+), 134 deletions(-)
> > > >>
> > > >> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > > >> b/tools/perf/util/arm-spe-
> decoder/arm-spe-pkt-decoder.c
> > > >> index 04fd7fd7c15f..1970686f7020 100644
> > > >> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > > >> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > > >> @@ -9,6 +9,7 @@
> > > >> #include
> > > >> #include
> > > >> #include
> > > >> +#include
> > > >>
> > > >> #include "arm-spe-pkt-decoder.h"
> > > >>
> > > >> @@ -258,192 +259,183 @@ int arm_spe_get_packet(const unsigned char
> > > >> *buf, size_t len,
> > > >>return ret;
> > > >> }
> > > >>
> > > >> +static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
> > > >> + const char *fmt, ...)
> > > >> +{
> > > >> + va_list ap;
> > > >> + int ret;
> > > >> +
> > > >> + /* Bail out if any error occurred */
> > > >> + if (err && *err)
> > > >> + return *err;
> > > >> +
> > > >> + va_start(ap, fmt);
> > > >> + ret = vsnprintf(*buf_p, *blen, fmt, ap);
> > > >> + va_end(ap);
> > > >> +
> > > >> + if (ret < 0) {
> > > >> + if (err && !*err)
> > > >> + *err = ret;
> > > >> +
> > > >> + /*
> > > >> + * A return value of (*blen - 1) or more means that the
> > > >> + * output was truncated and the buffer is overrun.
> > > >> + */
> > > >> + } else if (ret >= ((int)*blen - 1)) {
> > > >> + (*buf_p)[*blen - 1] = '\0';
> > > >> +
> > > >> + /*
> > > >> + * Set *err to 'ret' to avoid overflow if tries to
> > > >> + * fill this buffer sequentially.
> > > >> + */
> > > >> + if (err && !*err)
> > > >> + *err = ret;
> > > >> + } else {
> > > >> + *buf_p += ret;
> > > >> + *blen -= ret;
> > > >> + }
> > > >> +
> > > >> + return ret;
> > > >> +}
> > > >> +
I'm not entirely sure that snprintf() can actually return a negative value.
Every implementation (except the microsoft one) also always writes a '\0'
even when the buffer is too short.
A simple wrapper that lets you append output and detect overflow is:
ret = vsnprintf(buf, len, ...);
if (ret < 0)
/* just in case */
return 0;
return ret > len ? len : ret;
So on overflow the sum of the lengths is equal to the buffer size
(ie includes the terminating '\0'.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
Re: [PATCH v8 06/22] perf arm-spe: Refactor printing string to buffer
Em Wed, Nov 11, 2020 at 03:02:48PM -0300, Arnaldo Carvalho de Melo escreveu: > > I'll keep the series up to that point and will run my build tests, then > > push it publicly to acme/perf/core and you can go from there, ok? > > I've changed the BIT() to BIT_ULL() as Andre suggested and I'm testing > > it again. > To make it clear, this is what I have locally: > 0a04244cabc5560c (HEAD -> perf/core) perf arm-spe: Fix packet length handling > b65577baf4829092 perf arm-spe: Refactor arm_spe_get_events() > b2ded2e2e2764e50 perf arm-spe: Refactor payload size calculation > 903b659436b70692 perf arm-spe: Fix a typo in comment > c185f1cde46653cd perf arm-spe: Include bitops.h for BIT() macro > 40714c58630aaaf1 perf mem: Support ARM SPE events > c825f7885178f994 perf c2c: Support AUX trace > 13e5df1e3f1ba1a9 perf mem: Support AUX trace > 014a771c7867fda5 perf auxtrace: Add itrace option '-M' for memory events > 436cce00710a3f23 perf mem: Only initialize memory event for recording > 8b8173b45a7a9709 perf c2c: Support memory event PERF_MEM_EVENTS__LOAD_STORE > 4ba2452cd88f39da perf mem: Support new memory event > PERF_MEM_EVENTS__LOAD_STORE > eaf6aaeec5fa301c perf mem: Introduce weak function perf_mem_events__ptr() > f9f16dfbe76e63ba perf mem: Search event name with more flexible path > 644bf4b0f7acde64 (tag: perf-tools-tests-v5.11-2020-11-04, acme/perf/core) > perf jevents: Add test for arch std events So with the above it works with at least these: [perfbuilder@five ~]$ dm android-ndk:r15c-arm ubuntu:18.04-x-arm 122.37 android-ndk:r15c-arm : Ok arm-linux-androideabi-gcc (GCC) 4.9.x 20150123 (prerelease) 228.52 ubuntu:18.04-x-arm: Ok arm-linux-gnueabihf-gcc (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04) 7.5.0 [perfbuilder@five ~]$ previously it was failing in all 32-bit build test containers: [perfbuilder@five linux-perf-tools-build]$ grep FAIL dm.log/summary android-ndk:r12b-arm: FAIL android-ndk:r15c-arm: FAIL fedora:24-x-ARC-uClibc: FAIL fedora:30-x-ARC-uClibc: FAIL ubuntu:16.04-x-arm: FAIL ubuntu:16.04-x-powerpc: FAIL ubuntu:18.04-x-arm: FAIL ubuntu:18.04-x-m68k: FAIL ubuntu:18.04-x-powerpc: FAIL ubuntu:18.04-x-sh4: FAIL ubuntu:19.10-x-hppa: FAIL [perfbuilder@five linux-perf-tools-build]$ I'll redo the full set of tests and push perf/core publicly. - Arnaldo
Re: [PATCH v8 06/22] perf arm-spe: Refactor printing string to buffer
Em Wed, Nov 11, 2020 at 03:01:27PM -0300, Arnaldo Carvalho de Melo escreveu: > Em Wed, Nov 11, 2020 at 05:58:27PM +, Dave Martin escreveu: > > > > On Wed, Nov 11, 2020 at 05:39:22PM +, Arnaldo Carvalho de Melo wrote: > > > Em Wed, Nov 11, 2020 at 03:45:23PM +, Andr� Przywara escreveu: > > > > On 11/11/2020 15:35, Arnaldo Carvalho de Melo wrote: > > > > > Isn't this 'ret +=' ? Otherwise if any of these arm_spe_pkt_snprintf() > > > > > calls are made the previous 'ret' value is simply discarded. Can you > > > > > clarify this? > > > > > ret is the same as err. If err is negative (from previous calls), we > > > > return that straight away, so it does nothing but propagating the error. > > > > Usually the return of a snprintf is used to account for buffer space, ok > > > I'll have to read it, which I shouldn't as snprintf has a well defined > > > meaning... > > > > Ok, now that I look at it, I realize it is not a snprintf() routine, but > > > something with different semantics, that will look at a pointer to an > > > integer and then do nothing if it comes with some error, etc, confusing > > > :-/ > > > Would you be happier if the function were renamed? > > > Originally we were aiming for snprintf() semantics, but this still > > spawns a lot of boilerplate code and encourages mistakes in the local > > caller here -- hence the current sticky error approach. > > > So maybe the name should now be less "snprintf"-like. > > Please, its important to stick to semantics for such well known type of > routines, helps reviewing, etc. > > I'll keep the series up to that point and will run my build tests, then > push it publicly to acme/perf/core and you can go from there, ok? > > I've changed the BIT() to BIT_ULL() as Andre suggested and I'm testing > it again. To make it clear, this is what I have locally: 0a04244cabc5560c (HEAD -> perf/core) perf arm-spe: Fix packet length handling b65577baf4829092 perf arm-spe: Refactor arm_spe_get_events() b2ded2e2e2764e50 perf arm-spe: Refactor payload size calculation 903b659436b70692 perf arm-spe: Fix a typo in comment c185f1cde46653cd perf arm-spe: Include bitops.h for BIT() macro 40714c58630aaaf1 perf mem: Support ARM SPE events c825f7885178f994 perf c2c: Support AUX trace 13e5df1e3f1ba1a9 perf mem: Support AUX trace 014a771c7867fda5 perf auxtrace: Add itrace option '-M' for memory events 436cce00710a3f23 perf mem: Only initialize memory event for recording 8b8173b45a7a9709 perf c2c: Support memory event PERF_MEM_EVENTS__LOAD_STORE 4ba2452cd88f39da perf mem: Support new memory event PERF_MEM_EVENTS__LOAD_STORE eaf6aaeec5fa301c perf mem: Introduce weak function perf_mem_events__ptr() f9f16dfbe76e63ba perf mem: Search event name with more flexible path 644bf4b0f7acde64 (tag: perf-tools-tests-v5.11-2020-11-04, acme/perf/core) perf jevents: Add test for arch std events The perf-tools-tests-v5.11-2020-11-04, is in git.kernel.org, as it was tested, etc, test results are in that signed tag, as usual for some months. - Arnaldo
Re: [PATCH v8 06/22] perf arm-spe: Refactor printing string to buffer
Em Wed, Nov 11, 2020 at 05:58:27PM +, Dave Martin escreveu: > > On Wed, Nov 11, 2020 at 05:39:22PM +, Arnaldo Carvalho de Melo wrote: > > Em Wed, Nov 11, 2020 at 03:45:23PM +, Andr� Przywara escreveu: > > > On 11/11/2020 15:35, Arnaldo Carvalho de Melo wrote: > > > > Isn't this 'ret +=' ? Otherwise if any of these arm_spe_pkt_snprintf() > > > > calls are made the previous 'ret' value is simply discarded. Can you > > > > clarify this? > > > ret is the same as err. If err is negative (from previous calls), we > > > return that straight away, so it does nothing but propagating the error. > > Usually the return of a snprintf is used to account for buffer space, ok > > I'll have to read it, which I shouldn't as snprintf has a well defined > > meaning... > > Ok, now that I look at it, I realize it is not a snprintf() routine, but > > something with different semantics, that will look at a pointer to an > > integer and then do nothing if it comes with some error, etc, confusing > > :-/ > Would you be happier if the function were renamed? > Originally we were aiming for snprintf() semantics, but this still > spawns a lot of boilerplate code and encourages mistakes in the local > caller here -- hence the current sticky error approach. > So maybe the name should now be less "snprintf"-like. Please, its important to stick to semantics for such well known type of routines, helps reviewing, etc. I'll keep the series up to that point and will run my build tests, then push it publicly to acme/perf/core and you can go from there, ok? I've changed the BIT() to BIT_ULL() as Andre suggested and I'm testing it again. - Arnaldo
Re: [PATCH v8 06/22] perf arm-spe: Refactor printing string to buffer
On Wed, Nov 11, 2020 at 05:39:22PM +, Arnaldo Carvalho de Melo wrote:
> Em Wed, Nov 11, 2020 at 03:45:23PM +, Andr� Przywara escreveu:
> > On 11/11/2020 15:35, Arnaldo Carvalho de Melo wrote:
> >
> > Hi Arnaldo,
> >
> > thanks for taking a look!
> >
> > > Em Wed, Nov 11, 2020 at 03:11:33PM +0800, Leo Yan escreveu:
> > >> When outputs strings to the decoding buffer with function snprintf(),
> > >> SPE decoder needs to detects if any error returns from snprintf() and if
> > >> so needs to directly bail out. If snprintf() returns success, it needs
> > >> to update buffer pointer and reduce the buffer length so can continue to
> > >> output the next string into the consequent memory space.
> > >>
> > >> This complex logics are spreading in the function arm_spe_pkt_desc() so
> > >> there has many duplicate codes for handling error detecting, increment
> > >> buffer pointer and decrement buffer size.
> > >>
> > >> To avoid the duplicate code, this patch introduces a new helper function
> > >> arm_spe_pkt_snprintf() which is used to wrap up the complex logics, and
> > >> it's used by the caller arm_spe_pkt_desc().
> > >>
> > >> This patch also moves the variable 'blen' as the function's local
> > >> variable, this allows to remove the unnecessary braces and improve the
> > >> readability.
> > >>
> > >> Suggested-by: Dave Martin
> > >> Signed-off-by: Leo Yan
> > >> Reviewed-by: Andre Przywara
> > >> ---
> > >> .../arm-spe-decoder/arm-spe-pkt-decoder.c | 260 +-
> > >> 1 file changed, 126 insertions(+), 134 deletions(-)
> > >>
> > >> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > >> b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > >> index 04fd7fd7c15f..1970686f7020 100644
> > >> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > >> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > >> @@ -9,6 +9,7 @@
> > >> #include
> > >> #include
> > >> #include
> > >> +#include
> > >>
> > >> #include "arm-spe-pkt-decoder.h"
> > >>
> > >> @@ -258,192 +259,183 @@ int arm_spe_get_packet(const unsigned char *buf,
> > >> size_t len,
> > >> return ret;
> > >> }
> > >>
> > >> +static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
> > >> +const char *fmt, ...)
> > >> +{
> > >> +va_list ap;
> > >> +int ret;
> > >> +
> > >> +/* Bail out if any error occurred */
> > >> +if (err && *err)
> > >> +return *err;
> > >> +
> > >> +va_start(ap, fmt);
> > >> +ret = vsnprintf(*buf_p, *blen, fmt, ap);
> > >> +va_end(ap);
> > >> +
> > >> +if (ret < 0) {
> > >> +if (err && !*err)
> > >> +*err = ret;
> > >> +
> > >> +/*
> > >> + * A return value of (*blen - 1) or more means that the
> > >> + * output was truncated and the buffer is overrun.
> > >> + */
> > >> +} else if (ret >= ((int)*blen - 1)) {
> > >> +(*buf_p)[*blen - 1] = '\0';
> > >> +
> > >> +/*
> > >> + * Set *err to 'ret' to avoid overflow if tries to
> > >> + * fill this buffer sequentially.
> > >> + */
> > >> +if (err && !*err)
> > >> +*err = ret;
> > >> +} else {
> > >> +*buf_p += ret;
> > >> +*blen -= ret;
> > >> +}
> > >> +
> > >> +return ret;
> > >> +}
> > >> +
> > >> int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> > >> size_t buf_len)
> > >> {
> > >> int ret, ns, el, idx = packet->index;
> > >> unsigned long long payload = packet->payload;
> > >> const char *name = arm_spe_pkt_name(packet->type);
> > >> +size_t blen = buf_len;
> > >> +int err = 0;
> > >>
> > >> switch (packet->type) {
> > >> case ARM_SPE_BAD:
> > >> case ARM_SPE_PAD:
> > >> case ARM_SPE_END:
> > >> -return snprintf(buf, buf_len, "%s", name);
> > >> -case ARM_SPE_EVENTS: {
> > >> -size_t blen = buf_len;
> > >> -
> > >> -ret = 0;
> > >> -ret = snprintf(buf, buf_len, "EV");
> > >> -buf += ret;
> > >> -blen -= ret;
> > >> -if (payload & 0x1) {
> > >> -ret = snprintf(buf, buf_len, " EXCEPTION-GEN");
> > >> -buf += ret;
> > >> -blen -= ret;
> > >> -}
> > >> -if (payload & 0x2) {
> > >> -ret = snprintf(buf, buf_len, " RETIRED");
> > >> -buf += ret;
> > >> -blen -= ret;
> > >> -}
> > >> -if (payload & 0x4) {
> > >> -ret = snprintf(buf, buf_len, "
Re: [PATCH v8 06/22] perf arm-spe: Refactor printing string to buffer
Em Wed, Nov 11, 2020 at 03:58:27PM +, Dave Martin escreveu: > On Wed, Nov 11, 2020 at 03:53:20PM +, Dave Martin wrote: > > On Wed, Nov 11, 2020 at 07:11:33AM +, Leo Yan wrote: > > > When outputs strings to the decoding buffer with function snprintf(), > > > SPE decoder needs to detects if any error returns from snprintf() and if > > > so needs to directly bail out. If snprintf() returns success, it needs > > > to update buffer pointer and reduce the buffer length so can continue to > > > output the next string into the consequent memory space. > > > > > > This complex logics are spreading in the function arm_spe_pkt_desc() so > > > there has many duplicate codes for handling error detecting, increment > > > buffer pointer and decrement buffer size. > > > > > > To avoid the duplicate code, this patch introduces a new helper function > > > arm_spe_pkt_snprintf() which is used to wrap up the complex logics, and > > > it's used by the caller arm_spe_pkt_desc(). > > > > > > This patch also moves the variable 'blen' as the function's local > > > variable, this allows to remove the unnecessary braces and improve the > > > readability. > > > > > > Suggested-by: Dave Martin > > > Signed-off-by: Leo Yan > > > Reviewed-by: Andre Przywara > > > > Mostly looks fine to me now, thought there are a few potentionalu > > issues -- comments below. > > Hmm, looks like patch 7 anticipated some of my comments here. > > Rather than fixing up patch 6, maybe it would be better to squash these > patches together after all... sorry! I'll take a look and probably do that, as it is what Andre suggests. - Arnaldo
Re: [PATCH v8 06/22] perf arm-spe: Refactor printing string to buffer
Em Wed, Nov 11, 2020 at 03:45:23PM +, André Przywara escreveu:
> On 11/11/2020 15:35, Arnaldo Carvalho de Melo wrote:
>
> Hi Arnaldo,
>
> thanks for taking a look!
>
> > Em Wed, Nov 11, 2020 at 03:11:33PM +0800, Leo Yan escreveu:
> >> When outputs strings to the decoding buffer with function snprintf(),
> >> SPE decoder needs to detects if any error returns from snprintf() and if
> >> so needs to directly bail out. If snprintf() returns success, it needs
> >> to update buffer pointer and reduce the buffer length so can continue to
> >> output the next string into the consequent memory space.
> >>
> >> This complex logics are spreading in the function arm_spe_pkt_desc() so
> >> there has many duplicate codes for handling error detecting, increment
> >> buffer pointer and decrement buffer size.
> >>
> >> To avoid the duplicate code, this patch introduces a new helper function
> >> arm_spe_pkt_snprintf() which is used to wrap up the complex logics, and
> >> it's used by the caller arm_spe_pkt_desc().
> >>
> >> This patch also moves the variable 'blen' as the function's local
> >> variable, this allows to remove the unnecessary braces and improve the
> >> readability.
> >>
> >> Suggested-by: Dave Martin
> >> Signed-off-by: Leo Yan
> >> Reviewed-by: Andre Przywara
> >> ---
> >> .../arm-spe-decoder/arm-spe-pkt-decoder.c | 260 +-
> >> 1 file changed, 126 insertions(+), 134 deletions(-)
> >>
> >> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> >> b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> >> index 04fd7fd7c15f..1970686f7020 100644
> >> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> >> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> >> @@ -9,6 +9,7 @@
> >> #include
> >> #include
> >> #include
> >> +#include
> >>
> >> #include "arm-spe-pkt-decoder.h"
> >>
> >> @@ -258,192 +259,183 @@ int arm_spe_get_packet(const unsigned char *buf,
> >> size_t len,
> >>return ret;
> >> }
> >>
> >> +static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
> >> + const char *fmt, ...)
> >> +{
> >> + va_list ap;
> >> + int ret;
> >> +
> >> + /* Bail out if any error occurred */
> >> + if (err && *err)
> >> + return *err;
> >> +
> >> + va_start(ap, fmt);
> >> + ret = vsnprintf(*buf_p, *blen, fmt, ap);
> >> + va_end(ap);
> >> +
> >> + if (ret < 0) {
> >> + if (err && !*err)
> >> + *err = ret;
> >> +
> >> + /*
> >> + * A return value of (*blen - 1) or more means that the
> >> + * output was truncated and the buffer is overrun.
> >> + */
> >> + } else if (ret >= ((int)*blen - 1)) {
> >> + (*buf_p)[*blen - 1] = '\0';
> >> +
> >> + /*
> >> + * Set *err to 'ret' to avoid overflow if tries to
> >> + * fill this buffer sequentially.
> >> + */
> >> + if (err && !*err)
> >> + *err = ret;
> >> + } else {
> >> + *buf_p += ret;
> >> + *blen -= ret;
> >> + }
> >> +
> >> + return ret;
> >> +}
> >> +
> >> int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> >> size_t buf_len)
> >> {
> >>int ret, ns, el, idx = packet->index;
> >>unsigned long long payload = packet->payload;
> >>const char *name = arm_spe_pkt_name(packet->type);
> >> + size_t blen = buf_len;
> >> + int err = 0;
> >>
> >>switch (packet->type) {
> >>case ARM_SPE_BAD:
> >>case ARM_SPE_PAD:
> >>case ARM_SPE_END:
> >> - return snprintf(buf, buf_len, "%s", name);
> >> - case ARM_SPE_EVENTS: {
> >> - size_t blen = buf_len;
> >> -
> >> - ret = 0;
> >> - ret = snprintf(buf, buf_len, "EV");
> >> - buf += ret;
> >> - blen -= ret;
> >> - if (payload & 0x1) {
> >> - ret = snprintf(buf, buf_len, " EXCEPTION-GEN");
> >> - buf += ret;
> >> - blen -= ret;
> >> - }
> >> - if (payload & 0x2) {
> >> - ret = snprintf(buf, buf_len, " RETIRED");
> >> - buf += ret;
> >> - blen -= ret;
> >> - }
> >> - if (payload & 0x4) {
> >> - ret = snprintf(buf, buf_len, " L1D-ACCESS");
> >> - buf += ret;
> >> - blen -= ret;
> >> - }
> >> - if (payload & 0x8) {
> >> - ret = snprintf(buf, buf_len, " L1D-REFILL");
> >> - buf += ret;
> >> - blen -= ret;
> >> - }
> >> - if (payload & 0x10) {
> >> - ret = snprintf(buf, buf_len, " TLB-ACCESS");
> >> - buf += ret;
> >> - blen -= ret;
> >> - }
> >> - if (payload & 0x20) {
> >> - ret = snprintf(buf, buf_len, " TLB-REFILL");
> >> - buf += ret;
> >> - blen -= ret;
> >> -
Re: [PATCH v8 06/22] perf arm-spe: Refactor printing string to buffer
On Wed, Nov 11, 2020 at 03:53:20PM +, Dave Martin wrote: > On Wed, Nov 11, 2020 at 07:11:33AM +, Leo Yan wrote: > > When outputs strings to the decoding buffer with function snprintf(), > > SPE decoder needs to detects if any error returns from snprintf() and if > > so needs to directly bail out. If snprintf() returns success, it needs > > to update buffer pointer and reduce the buffer length so can continue to > > output the next string into the consequent memory space. > > > > This complex logics are spreading in the function arm_spe_pkt_desc() so > > there has many duplicate codes for handling error detecting, increment > > buffer pointer and decrement buffer size. > > > > To avoid the duplicate code, this patch introduces a new helper function > > arm_spe_pkt_snprintf() which is used to wrap up the complex logics, and > > it's used by the caller arm_spe_pkt_desc(). > > > > This patch also moves the variable 'blen' as the function's local > > variable, this allows to remove the unnecessary braces and improve the > > readability. > > > > Suggested-by: Dave Martin > > Signed-off-by: Leo Yan > > Reviewed-by: Andre Przywara > > Mostly looks fine to me now, thought there are a few potentionalu > issues -- comments below. Hmm, looks like patch 7 anticipated some of my comments here. Rather than fixing up patch 6, maybe it would be better to squash these patches together after all... sorry! [...] Cheers ---Dave
Re: [PATCH v8 06/22] perf arm-spe: Refactor printing string to buffer
On Wed, Nov 11, 2020 at 07:11:33AM +, Leo Yan wrote:
> When outputs strings to the decoding buffer with function snprintf(),
> SPE decoder needs to detects if any error returns from snprintf() and if
> so needs to directly bail out. If snprintf() returns success, it needs
> to update buffer pointer and reduce the buffer length so can continue to
> output the next string into the consequent memory space.
>
> This complex logics are spreading in the function arm_spe_pkt_desc() so
> there has many duplicate codes for handling error detecting, increment
> buffer pointer and decrement buffer size.
>
> To avoid the duplicate code, this patch introduces a new helper function
> arm_spe_pkt_snprintf() which is used to wrap up the complex logics, and
> it's used by the caller arm_spe_pkt_desc().
>
> This patch also moves the variable 'blen' as the function's local
> variable, this allows to remove the unnecessary braces and improve the
> readability.
>
> Suggested-by: Dave Martin
> Signed-off-by: Leo Yan
> Reviewed-by: Andre Przywara
Mostly looks fine to me now, thought there are a few potentionalu
issues -- comments below.
Cheers
---Dave
> ---
> .../arm-spe-decoder/arm-spe-pkt-decoder.c | 260 +-
> 1 file changed, 126 insertions(+), 134 deletions(-)
>
> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> index 04fd7fd7c15f..1970686f7020 100644
> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> @@ -9,6 +9,7 @@
> #include
> #include
> #include
> +#include
>
> #include "arm-spe-pkt-decoder.h"
>
> @@ -258,192 +259,183 @@ int arm_spe_get_packet(const unsigned char *buf,
> size_t len,
> return ret;
> }
>
> +static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
> + const char *fmt, ...)
> +{
> + va_list ap;
> + int ret;
> +
> + /* Bail out if any error occurred */
> + if (err && *err)
> + return *err;
> +
> + va_start(ap, fmt);
> + ret = vsnprintf(*buf_p, *blen, fmt, ap);
> + va_end(ap);
> +
> + if (ret < 0) {
> + if (err && !*err)
> + *err = ret;
> +
> + /*
> + * A return value of (*blen - 1) or more means that the
> + * output was truncated and the buffer is overrun.
> + */
(*blen - 1) chars, + 1 for '\0', is exactly *blen bytes. So ret ==
*blen - 1 is not an error.
> + } else if (ret >= ((int)*blen - 1)) {
So I suggest
if (ret >= *blen)
here.
Nit: If gcc moans about signedness in the comparison, I think it's
preferable to say
if ((size_t)ret >= *blen)
rather than casting *blen to an int. We already know that ret >= 0, and
UINT_MAX always fits in a size_t. On this code path it probably doesn't
matter in practice through, since *blen will be much less than INT_MAX.
vsnprintf() probably doesn't cope gracefully with super-large buffers
anyway, and the ISO C standards don't describe this situation
adequately.
If gcc doesn't warn, just drop the cast.
> + (*buf_p)[*blen - 1] = '\0';
> +
> + /*
> + * Set *err to 'ret' to avoid overflow if tries to
> + * fill this buffer sequentially.
> + */
> + if (err && !*err)
> + *err = ret;
> + } else {
> + *buf_p += ret;
> + *blen -= ret;
> + }
> +
> + return ret;
> +}
> +
> int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>size_t buf_len)
> {
> int ret, ns, el, idx = packet->index;
> unsigned long long payload = packet->payload;
> const char *name = arm_spe_pkt_name(packet->type);
> + size_t blen = buf_len;
> + int err = 0;
>
> switch (packet->type) {
> case ARM_SPE_BAD:
> case ARM_SPE_PAD:
> case ARM_SPE_END:
> - return snprintf(buf, buf_len, "%s", name);
> - case ARM_SPE_EVENTS: {
> - size_t blen = buf_len;
> -
> - ret = 0;
> - ret = snprintf(buf, buf_len, "EV");
> - buf += ret;
> - blen -= ret;
> - if (payload & 0x1) {
> - ret = snprintf(buf, buf_len, " EXCEPTION-GEN");
> - buf += ret;
> - blen -= ret;
> - }
> - if (payload & 0x2) {
> - ret = snprintf(buf, buf_len, " RETIRED");
> - buf += ret;
> - blen -= ret;
> - }
> - if (payload & 0x4) {
> - ret = snprintf(buf, buf_len, " L1D-ACCESS");
> - buf += ret;
> - blen -= ret;
> - }
> - if (payload & 0x8) {
> - ret = snprintf(buf, buf_len, " L1D-REFILL");
> - buf += ret;
> -
Re: [PATCH v8 06/22] perf arm-spe: Refactor printing string to buffer
On 11/11/2020 15:35, Arnaldo Carvalho de Melo wrote:
Hi Arnaldo,
thanks for taking a look!
> Em Wed, Nov 11, 2020 at 03:11:33PM +0800, Leo Yan escreveu:
>> When outputs strings to the decoding buffer with function snprintf(),
>> SPE decoder needs to detects if any error returns from snprintf() and if
>> so needs to directly bail out. If snprintf() returns success, it needs
>> to update buffer pointer and reduce the buffer length so can continue to
>> output the next string into the consequent memory space.
>>
>> This complex logics are spreading in the function arm_spe_pkt_desc() so
>> there has many duplicate codes for handling error detecting, increment
>> buffer pointer and decrement buffer size.
>>
>> To avoid the duplicate code, this patch introduces a new helper function
>> arm_spe_pkt_snprintf() which is used to wrap up the complex logics, and
>> it's used by the caller arm_spe_pkt_desc().
>>
>> This patch also moves the variable 'blen' as the function's local
>> variable, this allows to remove the unnecessary braces and improve the
>> readability.
>>
>> Suggested-by: Dave Martin
>> Signed-off-by: Leo Yan
>> Reviewed-by: Andre Przywara
>> ---
>> .../arm-spe-decoder/arm-spe-pkt-decoder.c | 260 +-
>> 1 file changed, 126 insertions(+), 134 deletions(-)
>>
>> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
>> b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
>> index 04fd7fd7c15f..1970686f7020 100644
>> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
>> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
>> @@ -9,6 +9,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include "arm-spe-pkt-decoder.h"
>>
>> @@ -258,192 +259,183 @@ int arm_spe_get_packet(const unsigned char *buf,
>> size_t len,
>> return ret;
>> }
>>
>> +static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
>> +const char *fmt, ...)
>> +{
>> +va_list ap;
>> +int ret;
>> +
>> +/* Bail out if any error occurred */
>> +if (err && *err)
>> +return *err;
>> +
>> +va_start(ap, fmt);
>> +ret = vsnprintf(*buf_p, *blen, fmt, ap);
>> +va_end(ap);
>> +
>> +if (ret < 0) {
>> +if (err && !*err)
>> +*err = ret;
>> +
>> +/*
>> + * A return value of (*blen - 1) or more means that the
>> + * output was truncated and the buffer is overrun.
>> + */
>> +} else if (ret >= ((int)*blen - 1)) {
>> +(*buf_p)[*blen - 1] = '\0';
>> +
>> +/*
>> + * Set *err to 'ret' to avoid overflow if tries to
>> + * fill this buffer sequentially.
>> + */
>> +if (err && !*err)
>> +*err = ret;
>> +} else {
>> +*buf_p += ret;
>> +*blen -= ret;
>> +}
>> +
>> +return ret;
>> +}
>> +
>> int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>> size_t buf_len)
>> {
>> int ret, ns, el, idx = packet->index;
>> unsigned long long payload = packet->payload;
>> const char *name = arm_spe_pkt_name(packet->type);
>> +size_t blen = buf_len;
>> +int err = 0;
>>
>> switch (packet->type) {
>> case ARM_SPE_BAD:
>> case ARM_SPE_PAD:
>> case ARM_SPE_END:
>> -return snprintf(buf, buf_len, "%s", name);
>> -case ARM_SPE_EVENTS: {
>> -size_t blen = buf_len;
>> -
>> -ret = 0;
>> -ret = snprintf(buf, buf_len, "EV");
>> -buf += ret;
>> -blen -= ret;
>> -if (payload & 0x1) {
>> -ret = snprintf(buf, buf_len, " EXCEPTION-GEN");
>> -buf += ret;
>> -blen -= ret;
>> -}
>> -if (payload & 0x2) {
>> -ret = snprintf(buf, buf_len, " RETIRED");
>> -buf += ret;
>> -blen -= ret;
>> -}
>> -if (payload & 0x4) {
>> -ret = snprintf(buf, buf_len, " L1D-ACCESS");
>> -buf += ret;
>> -blen -= ret;
>> -}
>> -if (payload & 0x8) {
>> -ret = snprintf(buf, buf_len, " L1D-REFILL");
>> -buf += ret;
>> -blen -= ret;
>> -}
>> -if (payload & 0x10) {
>> -ret = snprintf(buf, buf_len, " TLB-ACCESS");
>> -buf += ret;
>> -blen -= ret;
>> -}
>> -if (payload & 0x20) {
>> -ret = snprintf(buf, buf_len, " TLB-REFILL");
>> -buf += ret;
>> -blen -= ret;
>> -}
>> -if (payload & 0x40) {
>> -ret = snprintf(buf, buf_len, " NOT-TAKEN");
>> -buf += ret;
>> -blen -= ret;
>> -
Re: [PATCH v8 06/22] perf arm-spe: Refactor printing string to buffer
Em Wed, Nov 11, 2020 at 03:11:33PM +0800, Leo Yan escreveu:
> When outputs strings to the decoding buffer with function snprintf(),
> SPE decoder needs to detects if any error returns from snprintf() and if
> so needs to directly bail out. If snprintf() returns success, it needs
> to update buffer pointer and reduce the buffer length so can continue to
> output the next string into the consequent memory space.
>
> This complex logics are spreading in the function arm_spe_pkt_desc() so
> there has many duplicate codes for handling error detecting, increment
> buffer pointer and decrement buffer size.
>
> To avoid the duplicate code, this patch introduces a new helper function
> arm_spe_pkt_snprintf() which is used to wrap up the complex logics, and
> it's used by the caller arm_spe_pkt_desc().
>
> This patch also moves the variable 'blen' as the function's local
> variable, this allows to remove the unnecessary braces and improve the
> readability.
>
> Suggested-by: Dave Martin
> Signed-off-by: Leo Yan
> Reviewed-by: Andre Przywara
> ---
> .../arm-spe-decoder/arm-spe-pkt-decoder.c | 260 +-
> 1 file changed, 126 insertions(+), 134 deletions(-)
>
> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> index 04fd7fd7c15f..1970686f7020 100644
> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> @@ -9,6 +9,7 @@
> #include
> #include
> #include
> +#include
>
> #include "arm-spe-pkt-decoder.h"
>
> @@ -258,192 +259,183 @@ int arm_spe_get_packet(const unsigned char *buf,
> size_t len,
> return ret;
> }
>
> +static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
> + const char *fmt, ...)
> +{
> + va_list ap;
> + int ret;
> +
> + /* Bail out if any error occurred */
> + if (err && *err)
> + return *err;
> +
> + va_start(ap, fmt);
> + ret = vsnprintf(*buf_p, *blen, fmt, ap);
> + va_end(ap);
> +
> + if (ret < 0) {
> + if (err && !*err)
> + *err = ret;
> +
> + /*
> + * A return value of (*blen - 1) or more means that the
> + * output was truncated and the buffer is overrun.
> + */
> + } else if (ret >= ((int)*blen - 1)) {
> + (*buf_p)[*blen - 1] = '\0';
> +
> + /*
> + * Set *err to 'ret' to avoid overflow if tries to
> + * fill this buffer sequentially.
> + */
> + if (err && !*err)
> + *err = ret;
> + } else {
> + *buf_p += ret;
> + *blen -= ret;
> + }
> +
> + return ret;
> +}
> +
> int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>size_t buf_len)
> {
> int ret, ns, el, idx = packet->index;
> unsigned long long payload = packet->payload;
> const char *name = arm_spe_pkt_name(packet->type);
> + size_t blen = buf_len;
> + int err = 0;
>
> switch (packet->type) {
> case ARM_SPE_BAD:
> case ARM_SPE_PAD:
> case ARM_SPE_END:
> - return snprintf(buf, buf_len, "%s", name);
> - case ARM_SPE_EVENTS: {
> - size_t blen = buf_len;
> -
> - ret = 0;
> - ret = snprintf(buf, buf_len, "EV");
> - buf += ret;
> - blen -= ret;
> - if (payload & 0x1) {
> - ret = snprintf(buf, buf_len, " EXCEPTION-GEN");
> - buf += ret;
> - blen -= ret;
> - }
> - if (payload & 0x2) {
> - ret = snprintf(buf, buf_len, " RETIRED");
> - buf += ret;
> - blen -= ret;
> - }
> - if (payload & 0x4) {
> - ret = snprintf(buf, buf_len, " L1D-ACCESS");
> - buf += ret;
> - blen -= ret;
> - }
> - if (payload & 0x8) {
> - ret = snprintf(buf, buf_len, " L1D-REFILL");
> - buf += ret;
> - blen -= ret;
> - }
> - if (payload & 0x10) {
> - ret = snprintf(buf, buf_len, " TLB-ACCESS");
> - buf += ret;
> - blen -= ret;
> - }
> - if (payload & 0x20) {
> - ret = snprintf(buf, buf_len, " TLB-REFILL");
> - buf += ret;
> - blen -= ret;
> - }
> - if (payload & 0x40) {
> - ret = snprintf(buf, buf_len, " NOT-TAKEN");
> - buf += ret;
> - blen -= ret;
> - }
> - if (payload & 0x80) {
> - ret = snprintf(buf, buf_len, " MISPRED");
> - buf +=
[PATCH v8 06/22] perf arm-spe: Refactor printing string to buffer
When outputs strings to the decoding buffer with function snprintf(),
SPE decoder needs to detects if any error returns from snprintf() and if
so needs to directly bail out. If snprintf() returns success, it needs
to update buffer pointer and reduce the buffer length so can continue to
output the next string into the consequent memory space.
This complex logics are spreading in the function arm_spe_pkt_desc() so
there has many duplicate codes for handling error detecting, increment
buffer pointer and decrement buffer size.
To avoid the duplicate code, this patch introduces a new helper function
arm_spe_pkt_snprintf() which is used to wrap up the complex logics, and
it's used by the caller arm_spe_pkt_desc().
This patch also moves the variable 'blen' as the function's local
variable, this allows to remove the unnecessary braces and improve the
readability.
Suggested-by: Dave Martin
Signed-off-by: Leo Yan
Reviewed-by: Andre Przywara
---
.../arm-spe-decoder/arm-spe-pkt-decoder.c | 260 +-
1 file changed, 126 insertions(+), 134 deletions(-)
diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index 04fd7fd7c15f..1970686f7020 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
@@ -9,6 +9,7 @@
#include
#include
#include
+#include
#include "arm-spe-pkt-decoder.h"
@@ -258,192 +259,183 @@ int arm_spe_get_packet(const unsigned char *buf, size_t
len,
return ret;
}
+static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
+ const char *fmt, ...)
+{
+ va_list ap;
+ int ret;
+
+ /* Bail out if any error occurred */
+ if (err && *err)
+ return *err;
+
+ va_start(ap, fmt);
+ ret = vsnprintf(*buf_p, *blen, fmt, ap);
+ va_end(ap);
+
+ if (ret < 0) {
+ if (err && !*err)
+ *err = ret;
+
+ /*
+* A return value of (*blen - 1) or more means that the
+* output was truncated and the buffer is overrun.
+*/
+ } else if (ret >= ((int)*blen - 1)) {
+ (*buf_p)[*blen - 1] = '\0';
+
+ /*
+* Set *err to 'ret' to avoid overflow if tries to
+* fill this buffer sequentially.
+*/
+ if (err && !*err)
+ *err = ret;
+ } else {
+ *buf_p += ret;
+ *blen -= ret;
+ }
+
+ return ret;
+}
+
int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
size_t buf_len)
{
int ret, ns, el, idx = packet->index;
unsigned long long payload = packet->payload;
const char *name = arm_spe_pkt_name(packet->type);
+ size_t blen = buf_len;
+ int err = 0;
switch (packet->type) {
case ARM_SPE_BAD:
case ARM_SPE_PAD:
case ARM_SPE_END:
- return snprintf(buf, buf_len, "%s", name);
- case ARM_SPE_EVENTS: {
- size_t blen = buf_len;
-
- ret = 0;
- ret = snprintf(buf, buf_len, "EV");
- buf += ret;
- blen -= ret;
- if (payload & 0x1) {
- ret = snprintf(buf, buf_len, " EXCEPTION-GEN");
- buf += ret;
- blen -= ret;
- }
- if (payload & 0x2) {
- ret = snprintf(buf, buf_len, " RETIRED");
- buf += ret;
- blen -= ret;
- }
- if (payload & 0x4) {
- ret = snprintf(buf, buf_len, " L1D-ACCESS");
- buf += ret;
- blen -= ret;
- }
- if (payload & 0x8) {
- ret = snprintf(buf, buf_len, " L1D-REFILL");
- buf += ret;
- blen -= ret;
- }
- if (payload & 0x10) {
- ret = snprintf(buf, buf_len, " TLB-ACCESS");
- buf += ret;
- blen -= ret;
- }
- if (payload & 0x20) {
- ret = snprintf(buf, buf_len, " TLB-REFILL");
- buf += ret;
- blen -= ret;
- }
- if (payload & 0x40) {
- ret = snprintf(buf, buf_len, " NOT-TAKEN");
- buf += ret;
- blen -= ret;
- }
- if (payload & 0x80) {
- ret = snprintf(buf, buf_len, " MISPRED");
- buf += ret;
- blen -= ret;
- }
+ return arm_spe_pkt_snprintf(, , , "%s", name);
+ case ARM_SPE_EVENTS:
+ ret
