diff mbox series

[v2,1/5] perf cs-etm: Refactor instruction size handling

Message ID 20190923160759.14866-2-leo.yan@linaro.org
State Superseded
Headers show
Series perf cs-etm: Support thread stack and callchain | expand

Commit Message

Leo Yan Sept. 23, 2019, 4:07 p.m. UTC
In cs-etm.c there have several functions need to know instruction size
based on address, e.g. cs_etm__instr_addr() and cs_etm__copy_insn()
these two functions both calculate the instruction size separately.
Furthermore, if we consider to add new features later which also might
require to calculate instruction size.

For this reason, this patch refactors the code to introduce a new
function cs_etm__instr_size(), it will be a central place to calculate
the instruction size based on ISA type and instruction address.

For a neat implementation, cs_etm__instr_addr() will always execute the
loop without checking ISA type, this allows cs_etm__instr_size() and
cs_etm__instr_addr() have no any duplicate code with each other and both
functions can be changed independently later without breaking anything.
As a side effect, cs_etm__instr_addr() will do a few more iterations for
A32/A64 instructions, this would be fine if consider perf tool runs in
the user space.

Signed-off-by: Leo Yan <leo.yan@linaro.org>

---
 tools/perf/util/cs-etm.c | 48 +++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 20 deletions(-)

-- 
2.17.1

Comments

Suzuki K Poulose Sept. 23, 2019, 4:51 p.m. UTC | #1
Hi Leo,

On 23/09/2019 17:07, Leo Yan wrote:
> In cs-etm.c there have several functions need to know instruction size

> based on address, e.g. cs_etm__instr_addr() and cs_etm__copy_insn()

> these two functions both calculate the instruction size separately.

> Furthermore, if we consider to add new features later which also might

> require to calculate instruction size.

> 

> For this reason, this patch refactors the code to introduce a new

> function cs_etm__instr_size(), it will be a central place to calculate

> the instruction size based on ISA type and instruction address.

> 

> For a neat implementation, cs_etm__instr_addr() will always execute the

> loop without checking ISA type, this allows cs_etm__instr_size() and

> cs_etm__instr_addr() have no any duplicate code with each other and both

> functions can be changed independently later without breaking anything.

> As a side effect, cs_etm__instr_addr() will do a few more iterations for

> A32/A64 instructions, this would be fine if consider perf tool runs in

> the user space.

> 

> Signed-off-by: Leo Yan <leo.yan@linaro.org>


Your changes look fine to me. However, please see my comment below.

> ---

>   tools/perf/util/cs-etm.c | 48 +++++++++++++++++++++++-----------------

>   1 file changed, 28 insertions(+), 20 deletions(-)

> 

> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c

> index f87b9c1c9f9a..1de3f9361193 100644

> --- a/tools/perf/util/cs-etm.c

> +++ b/tools/perf/util/cs-etm.c

> @@ -917,6 +917,26 @@ static inline int cs_etm__t32_instr_size(struct cs_etm_queue *etmq,

>   	return ((instrBytes[1] & 0xF8) >= 0xE8) ? 4 : 2;

>   }

>   

> +static inline int cs_etm__instr_size(struct cs_etm_queue *etmq,

> +				     u8 trace_chan_id,

> +				     enum cs_etm_isa isa,

> +				     u64 addr)

> +{

> +	int insn_len;

> +

> +	/*

> +	 * T32 instruction size might be 32-bit or 16-bit, decide by calling

> +	 * cs_etm__t32_instr_size().

> +	 */

> +	if (isa == CS_ETM_ISA_T32)

> +		insn_len = cs_etm__t32_instr_size(etmq, trace_chan_id, addr);

> +	/* Otherwise, A64 and A32 instruction size are always 32-bit. */

> +	else

> +		insn_len = 4;

> +

> +	return insn_len;

> +}

> +

>   static inline u64 cs_etm__first_executed_instr(struct cs_etm_packet *packet)

>   {

>   	/* Returns 0 for the CS_ETM_DISCONTINUITY packet */

> @@ -941,19 +961,15 @@ static inline u64 cs_etm__instr_addr(struct cs_etm_queue *etmq,

>   				     const struct cs_etm_packet *packet,

>   				     u64 offset)

>   {

> -	if (packet->isa == CS_ETM_ISA_T32) {

> -		u64 addr = packet->start_addr;

> +	u64 addr = packet->start_addr;

>   

> -		while (offset > 0) {

> -			addr += cs_etm__t32_instr_size(etmq,

> -						       trace_chan_id, addr);

> -			offset--;

> -		}

> -		return addr;

> +	while (offset > 0) {


Given that offset is u64, the check above is not appropriate. You could either
change it to :
	while (offset) // if you are sure (s64)offset always is a postive
integer and we always reduce it by 1.

Otherwise you may switch the offset to a signed type. I understand that this
is not introduced by your changes. But you may fix that up in a separate patch.


Kind regards
Suzuki
Leo Yan Sept. 23, 2019, 5:13 p.m. UTC | #2
Hi Suzuki,

On Mon, Sep 23, 2019 at 05:51:04PM +0100, Suzuki K Poulose wrote:
> Hi Leo,

> 

> On 23/09/2019 17:07, Leo Yan wrote:

> > In cs-etm.c there have several functions need to know instruction size

> > based on address, e.g. cs_etm__instr_addr() and cs_etm__copy_insn()

> > these two functions both calculate the instruction size separately.

> > Furthermore, if we consider to add new features later which also might

> > require to calculate instruction size.

> > 

> > For this reason, this patch refactors the code to introduce a new

> > function cs_etm__instr_size(), it will be a central place to calculate

> > the instruction size based on ISA type and instruction address.

> > 

> > For a neat implementation, cs_etm__instr_addr() will always execute the

> > loop without checking ISA type, this allows cs_etm__instr_size() and

> > cs_etm__instr_addr() have no any duplicate code with each other and both

> > functions can be changed independently later without breaking anything.

> > As a side effect, cs_etm__instr_addr() will do a few more iterations for

> > A32/A64 instructions, this would be fine if consider perf tool runs in

> > the user space.

> > 

> > Signed-off-by: Leo Yan <leo.yan@linaro.org>

> 

> Your changes look fine to me. However, please see my comment below.

> 

> > ---

> >   tools/perf/util/cs-etm.c | 48 +++++++++++++++++++++++-----------------

> >   1 file changed, 28 insertions(+), 20 deletions(-)

> > 

> > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c

> > index f87b9c1c9f9a..1de3f9361193 100644

> > --- a/tools/perf/util/cs-etm.c

> > +++ b/tools/perf/util/cs-etm.c

> > @@ -917,6 +917,26 @@ static inline int cs_etm__t32_instr_size(struct cs_etm_queue *etmq,

> >   	return ((instrBytes[1] & 0xF8) >= 0xE8) ? 4 : 2;

> >   }

> > +static inline int cs_etm__instr_size(struct cs_etm_queue *etmq,

> > +				     u8 trace_chan_id,

> > +				     enum cs_etm_isa isa,

> > +				     u64 addr)

> > +{

> > +	int insn_len;

> > +

> > +	/*

> > +	 * T32 instruction size might be 32-bit or 16-bit, decide by calling

> > +	 * cs_etm__t32_instr_size().

> > +	 */

> > +	if (isa == CS_ETM_ISA_T32)

> > +		insn_len = cs_etm__t32_instr_size(etmq, trace_chan_id, addr);

> > +	/* Otherwise, A64 and A32 instruction size are always 32-bit. */

> > +	else

> > +		insn_len = 4;

> > +

> > +	return insn_len;

> > +}

> > +

> >   static inline u64 cs_etm__first_executed_instr(struct cs_etm_packet *packet)

> >   {

> >   	/* Returns 0 for the CS_ETM_DISCONTINUITY packet */

> > @@ -941,19 +961,15 @@ static inline u64 cs_etm__instr_addr(struct cs_etm_queue *etmq,

> >   				     const struct cs_etm_packet *packet,

> >   				     u64 offset)

> >   {

> > -	if (packet->isa == CS_ETM_ISA_T32) {

> > -		u64 addr = packet->start_addr;

> > +	u64 addr = packet->start_addr;

> > -		while (offset > 0) {

> > -			addr += cs_etm__t32_instr_size(etmq,

> > -						       trace_chan_id, addr);

> > -			offset--;

> > -		}

> > -		return addr;

> > +	while (offset > 0) {

> 

> Given that offset is u64, the check above is not appropriate. You could either

> change it to :

> 	while (offset) // if you are sure (s64)offset always is a postive

> integer and we always reduce it by 1.

> 

> Otherwise you may switch the offset to a signed type. I understand that this

> is not introduced by your changes. But you may fix that up in a separate patch.


Thanks a lot for the review.  Seems to me the reliable fix is to change
to a signed type.  Will add this fix in next spin.

Thanks,
Leo Yan
diff mbox series

Patch

diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index f87b9c1c9f9a..1de3f9361193 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -917,6 +917,26 @@  static inline int cs_etm__t32_instr_size(struct cs_etm_queue *etmq,
 	return ((instrBytes[1] & 0xF8) >= 0xE8) ? 4 : 2;
 }
 
+static inline int cs_etm__instr_size(struct cs_etm_queue *etmq,
+				     u8 trace_chan_id,
+				     enum cs_etm_isa isa,
+				     u64 addr)
+{
+	int insn_len;
+
+	/*
+	 * T32 instruction size might be 32-bit or 16-bit, decide by calling
+	 * cs_etm__t32_instr_size().
+	 */
+	if (isa == CS_ETM_ISA_T32)
+		insn_len = cs_etm__t32_instr_size(etmq, trace_chan_id, addr);
+	/* Otherwise, A64 and A32 instruction size are always 32-bit. */
+	else
+		insn_len = 4;
+
+	return insn_len;
+}
+
 static inline u64 cs_etm__first_executed_instr(struct cs_etm_packet *packet)
 {
 	/* Returns 0 for the CS_ETM_DISCONTINUITY packet */
@@ -941,19 +961,15 @@  static inline u64 cs_etm__instr_addr(struct cs_etm_queue *etmq,
 				     const struct cs_etm_packet *packet,
 				     u64 offset)
 {
-	if (packet->isa == CS_ETM_ISA_T32) {
-		u64 addr = packet->start_addr;
+	u64 addr = packet->start_addr;
 
-		while (offset > 0) {
-			addr += cs_etm__t32_instr_size(etmq,
-						       trace_chan_id, addr);
-			offset--;
-		}
-		return addr;
+	while (offset > 0) {
+		addr += cs_etm__instr_size(etmq, trace_chan_id,
+					   packet->isa, addr);
+		offset--;
 	}
 
-	/* Assume a 4 byte instruction size (A32/A64) */
-	return packet->start_addr + offset * 4;
+	return addr;
 }
 
 static void cs_etm__update_last_branch_rb(struct cs_etm_queue *etmq,
@@ -1093,16 +1109,8 @@  static void cs_etm__copy_insn(struct cs_etm_queue *etmq,
 		return;
 	}
 
-	/*
-	 * T32 instruction size might be 32-bit or 16-bit, decide by calling
-	 * cs_etm__t32_instr_size().
-	 */
-	if (packet->isa == CS_ETM_ISA_T32)
-		sample->insn_len = cs_etm__t32_instr_size(etmq, trace_chan_id,
-							  sample->ip);
-	/* Otherwise, A64 and A32 instruction size are always 32-bit. */
-	else
-		sample->insn_len = 4;
+	sample->insn_len = cs_etm__instr_size(etmq, trace_chan_id,
+					      packet->isa, sample->ip);
 
 	cs_etm__mem_access(etmq, trace_chan_id, sample->ip,
 			   sample->insn_len, (void *)sample->insn);