On 31 May 2018 at 19:57, Leo Yan leo.yan@linaro.org wrote:
On Thu, May 31, 2018 at 11:23:13AM -0600, Mathieu Poirier wrote:
On Thu, May 31, 2018 at 01:22:07PM +0800, Leo Yan wrote:
Every exception packet actually appears as two duplicated elements in the decoder layer, the first element has 'elem_type' = OCSD_GEN_TRC_ELEM_INSTR_RANGE and the 'elem_type' in the second element is OCSD_GEN_TRC_ELEM_EXCEPTION or OCSD_GEN_TRC_ELEM_EXCEPTION_RET, which present for exception entry and exit respectively. The decoder set packet fields 'packet->exc' and 'packet->exc_ret' to indicate the exception packets according to the second element; but exception packets don't have dedicated sample type and shares the same sample type CS_ETM_RANGE with normal instruction packets.
As result, the exception packets are taken as normal instruction packets and they will be processed for branch sample only when 'packet->last_instr_taken_branch' is true, otherwise they will be omitted.
To process exception packets properly, this patch introduce two new sample type: CS_ETM_EXCEPTION and CS_ETM_EXCEPTION_RET; for these two kind packets, they will be handled by cs_etm__sample(). Except we need to consider branch taken packets for normal CS_ETM_RANGE packet with 'last_instr_taken_branch' is true, we also make below three kind packets as branch taken packets:
CS_ETM_EXCEPTION and CS_ETM_EXCEPTION_RET packets; Instruction range packet before exception packets; Instruction range packet after exception packets.
This patch is to generate branch samples and update last branch stack for all packets as mentioned above. After exception packets have their own sample type, the packet fields 'packet->exc' and 'packet->exc_ret' aren't needed anymore, so remove them.
Signed-off-by: Leo Yan leo.yan@linaro.org
tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 11 ++++----- tools/perf/util/cs-etm-decoder/cs-etm-decoder.h | 10 ++++---- tools/perf/util/cs-etm.c | 31 +++++++++++++++++++++---- 3 files changed, 36 insertions(+), 16 deletions(-)
diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c index 4d5fc37..e009e01 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c @@ -264,8 +264,6 @@ static void cs_etm_decoder__clear_buffer(struct cs_etm_decoder *decoder) decoder->packet_buffer[i].start_addr = 0xdeadbeefdeadbeefUL; decoder->packet_buffer[i].end_addr = 0xdeadbeefdeadbeefUL; decoder->packet_buffer[i].last_instr_taken_branch = false;
decoder->packet_buffer[i].exc = false;decoder->packet_buffer[i].exc_ret = false; decoder->packet_buffer[i].cpu = INT_MIN;} @@ -292,8 +290,6 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder, decoder->packet_count++;
decoder->packet_buffer[et].sample_type = sample_type;
- decoder->packet_buffer[et].exc = false;
- decoder->packet_buffer[et].exc_ret = false; decoder->packet_buffer[et].cpu = *((int *)inode->priv); decoder->packet_buffer[et].start_addr = 0xdeadbeefdeadbeefUL; decoder->packet_buffer[et].end_addr = 0xdeadbeefdeadbeefUL;
@@ -353,6 +349,7 @@ static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer( { ocsd_datapath_resp_t resp = OCSD_RESP_CONT; struct cs_etm_decoder *decoder = (struct cs_etm_decoder *) context;
struct cs_etm_packet *packet;
switch (elem->elem_type) { case OCSD_GEN_TRC_ELEM_UNKNOWN:
@@ -370,10 +367,12 @@ static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer( trace_chan_id); break; case OCSD_GEN_TRC_ELEM_EXCEPTION:
decoder->packet_buffer[decoder->tail].exc = true;
packet = &decoder->packet_buffer[decoder->tail];packet->sample_type = CS_ETM_EXCEPTION; break;
decoder->packet_buffer[decoder->tail].exc_ret = true;
packet = &decoder->packet_buffer[decoder->tail];packet->sample_type = CS_ETM_EXCEPTION_RET;Am I missing something the above won't work? ->tail needs to be incremented before being used. A such what this is doing is clobbering the type of the previous packet.
Here the logic is a bit tricky. The elements OCSD_GEN_TRC_ELEM_EXCEPTION/OCSD_GEN_TRC_ELEM_EXCEPTION_RET don't include any useful address range info, so I think this is the main reason now we take the exception elements as an 'attribution' and attach it to the previous instruction range element.
You are correct only for the exc/exc_ret part. The sample_type gets clobbered leading to very difficult (and fragile) code to manage in the front end.
You need to insert a new packet in the decoder packet queue using function cs_etm_decoder__buffer_packet() and then set the sample type.
I will write one new patch by following this method.
Yes please.
break; case OCSD_GEN_TRC_ELEM_PE_CONTEXT: case OCSD_GEN_TRC_ELEM_EO_TRACE:diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h index 612b575..cf31a9c 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h @@ -23,9 +23,11 @@ struct cs_etm_buffer { };
enum cs_etm_sample_type {
- CS_ETM_EMPTY = 0,
- CS_ETM_RANGE = 1 << 0,
- CS_ETM_TRACE_ON = 1 << 1,
- CS_ETM_EMPTY = 0,
- CS_ETM_RANGE = 1 << 0,
- CS_ETM_TRACE_ON = 1 << 1,
- CS_ETM_EXCEPTION = 1 << 2,
- CS_ETM_EXCEPTION_RET = 1 << 3,
};
Thank you for that.
struct cs_etm_packet { @@ -33,8 +35,6 @@ struct cs_etm_packet { u64 start_addr; u64 end_addr; u8 last_instr_taken_branch;
- u8 exc;
- u8 exc_ret; int cpu;
};
Ok
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index 8ec2f2c..6453a1b 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -862,6 +862,27 @@ static int cs_etm__synth_events(struct cs_etm_auxtrace *etm, return 0; }
+static bool cs_etm__is_taken_branch(struct cs_etm_packet *prev_packet,
struct cs_etm_packet *packet)+{
- /* The branch is taken for normal range packet with taken branch flag */
- if (prev_packet->sample_type == CS_ETM_RANGE &&
prev_packet->last_instr_taken_branch)return true;- /* The branch is taken if previous packet is exception packet */
- if (prev_packet->sample_type == CS_ETM_EXCEPTION ||
prev_packet->sample_type == CS_ETM_EXCEPTION_RET)return true;- /* The branch is taken for an intervening exception packet */
- if (packet->sample_type == CS_ETM_EXCEPTION ||
packet->sample_type == CS_ETM_EXCEPTION_RET)return true;- return false;
+}
static int cs_etm__sample(struct cs_etm_queue *etmq) { struct cs_etm_auxtrace *etm = etmq->etm; @@ -878,8 +899,7 @@ static int cs_etm__sample(struct cs_etm_queue *etmq) */ if (etm->synth_opts.last_branch && etmq->prev_packet &&
etmq->prev_packet->sample_type == CS_ETM_RANGE &&etmq->prev_packet->last_instr_taken_branch)
cs_etm__is_taken_branch(etmq->prev_packet, etmq->packet)) cs_etm__update_last_branch_rb(etmq);if (etm->sample_instructions &&
@@ -917,9 +937,8 @@ static int cs_etm__sample(struct cs_etm_queue *etmq) if (etmq->prev_packet->sample_type == CS_ETM_TRACE_ON) generate_sample = true;
/* Generate sample for normal branch packet */if (etmq->prev_packet->sample_type == CS_ETM_RANGE &&etmq->prev_packet->last_instr_taken_branch)
/* Generate sample for exception and normal branch packet */if (cs_etm__is_taken_branch(etmq->prev_packet, etmq->packet)) generate_sample = true; if (generate_sample) {@@ -1051,6 +1070,8 @@ static int cs_etm__run_decoder(struct cs_etm_queue *etmq)
switch (etmq->packet->sample_type) { case CS_ETM_RANGE:
case CS_ETM_EXCEPTION:case CS_ETM_EXCEPTION_RET:I think this is too much coupling and will invariably lead to errors in the code. Function cs_etm__sample() works well for range packets. I suggest spinning off another function (or two) to handle exception packets. That way we have a neat state machine and handling functions are kept simple.
Sure, will do this and let's see if it's doable.
/* * If the packet contains an instruction * range, generate instruction sequence-- 2.7.4