Re: [RFT v3 1/4] perf cs-etm: Generate branch sample for missed packets

[Leo Yan]

On Wed, May 30, 2018 at 11:39:00PM +0800, Leo Yan wrote:
> Hi Mike,
> 
> On Wed, May 30, 2018 at 04:04:34PM +0100, Mike Leach wrote:
> 
> [...]
> 
> > >>> +               /* Generate sample for exception packet */
> > >>> +               if (etmq->prev_packet->exc == true)
> > >>> +                       generate_sample = true;
> > >>
> > >>
> > >> Please don't do that.  Exception packets have a type of their own and can
> > >> be
> > >> added to the decoder packet queue the same way INST_RANGE and TRACE_ON
> > >> packets
> > >> are.  Moreover exception packet containt an address that, if I'm reading
> > >> the
> > >> documenation properly, can be used to keep track of instructions that were
> > >> executed between the last address of the previous range packet and the
> > >> address
> > >> executed just before the exception occurred.  Mike and Rob will have to
> > >> confirm
> > >> this as the decoder may be doing all that hard work for us.
> > >>
> > 
> > clarification on the exception packets....
> > 
> > The Opencsd output exception packet gives you the exception number,
> > and optionally the preferred return address. If this address is
> > present does depend a lot on the underlying protocol - will normally
> > be there with ETMv4.
> > Exceptions are marked differently in the underlying protocol - the
> > OCSD packets abstract away these differences.
> > 
> > consider the code:
> > 
> > 0x1000: <some instructions>
> > 0x1100: BR 0x2000
> > ....
> > 0x2000: <some instructions>
> > 0x2020  BZ r4
> > 
> > Without an exception this would result in the packets
> > 
> > OCSD_RANGE(0x1000,0x1104, Last instr type=Br, taken)   // recall that
> > range packets have start addr inclusive, end addr exclusive.
> > OCSD_RANGE(0x2000,0x2024, Last instr type=Br, <taken / not taken -
> > depends on condition>
> > 
> > Now consider an exception occurring before the BR 0x2000
> > 
> > this will result in:-
> > OCSD_RANGE(0x1000, 0x1100, Last instr type=Other)
> > OCSD_EXECEPTION(IRQ, ret-addr 0x1100)
> > OCSD_RANGE(IRQ_START, IRQ_END+4, Last instr type = BR, taken)    //
> > this is more likely to have multiple ranges / branches before any
> > return, but simplified here.
> > OCSD_EXCEPTION_RETURN()  // present if exception returns are
> > explicitly marked in underlying trace - may not always be depending on
> > circumstances.
> > OCSD_RANGE(0x1100,0x1104, Last=BR, taken) // continue on with short
> > range - just the branch
> > OCSD_RANGE(0x2000,0x2024, Last instr type=Br, <taken / not taken -
> > depends on condition>
> > 
> > Now consider the exception occurring after the BR, but before any
> > other instructions are executed.
> > 
> > OCSD_RANGE(0x1000,0x1104, Last instr type=Br, taken)   // recall that
> > range packets have start addr inclusive, end addr exclusive.
> > OCSD_EXECEPTION(IRQ, ret-addr 0x2000)     // here the preferred return
> > address is actually the target of the branch.
> > OCSD_RANGE(IRQ_START, IRQ_END+4, Last instr type = BR, taken)    //
> > this is more likely to have multiple ranges / branches before any
> > return, but simplified here.
> > OCSD_RANGE(0x2000,0x2024, Last instr type=Br, <taken / not taken -
> > depends on condition>
> > 
> > So in general it is possible to arrive in the IRQ_START range with the
> > previous packet having been either a taken branch, a not taken branch,
> > or not a branch.
> > Care must be taken - whether AutoFDO or normal trace disassembly not
> > to assume that having the last range packet as a taken branch means
> > that the next range packet is the target, if there is an intervening
> > exception packet.
> 
> Thanks a lot for detailed explaination.
> 
> IIUC, AutoFDO will not have such issue due every range packet will be
> handled for it.  On the other hand, as you remind, the branch samples
> (and its consumer trace disassembler) is very dependent on the flag
> 'last_instr_taken_branch'.
> 
> According to your explaination, I think we consider the branch is
> taken for below situations:
> 
> - The new coming packet is exception packet (both for exception entry
>   and exit packets);
> - The previous packet is expcetion packet;
> - The previous packet is normal range packet with
>   'last_instr_taken_branch' = true;
> 
> So I'd like to use below function to demonstrate my understanding for
> exception packets handling.  I also will send out one new patch for
> support exception packet for reviewing.
> 
> If you have concern or I miss anything, please let me know.
> 
> 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;
> }

Just clarify, I missed to mention I introduce two extra sample types:
CS_ETM_EXCEPTION and CS_ETM_EXCEPTION_RET, one is for exception
entry packet and another is for exception exit packet.   If this is
hard for understanding, you could hold on for reveiwing new patch.

Thanks,
Leo Yan