Re: [PATCH 3/9] Refactor etmv3 config class and etmv3 packet class to improve encapsulation.

On 3 June 2016 at 02:26, Mike Leach mike.leach@linaro.org wrote:

These no longer use the C-API data struct as a base class, but use the struct as a private member and provide = and () operator overrides for conversion.

This line is longer than 80 characters.

Signed-off-by: Mike Leach mike.leach@linaro.org

decoder/include/common/ocsd_pe_context.h | 10 +- decoder/include/etmv3/etmv3_decoder.h | 1 + decoder/include/etmv3/trc_cmp_cfg_etmv3.h | 45 ++-- decoder/include/etmv3/trc_pkt_elem_etmv3.h | 84 ++++--- decoder/source/c_api/ocsd_c_api.cpp | 4 +- decoder/source/etmv3/trc_cmp_cfg_etmv3.cpp | 17 +- decoder/source/etmv3/trc_pkt_elem_etmv3.cpp | 267 +++++++++++---------- decoder/source/etmv3/trc_pkt_proc_etmv3_impl.cpp | 7 +- .../snapshot_parser_lib/source/ss_to_dcdtree.cpp | 13 +- 9 files changed, 241 insertions(+), 207 deletions(-)

diff --git a/decoder/include/common/ocsd_pe_context.h b/decoder/include/common/ocsd_pe_context.h index 5ec17e2..37764d9 100644 --- a/decoder/include/common/ocsd_pe_context.h +++ b/decoder/include/common/ocsd_pe_context.h @@ -36,7 +36,11 @@

#include "ocsd_if_types.h"

-/*! Handler for the ocsd_pe_context structure +/*! @class OcsdPeContext

• • @brief Handler for the ocsd_pe_context structure.
• • Reads and writes structure values, enforcing interaction rules between values
• • and flags.
  */

class OcsdPeContext { @@ -67,12 +71,12 @@ private: ocsd_pe_context m_context; };

-OcsdPeContext::OcsdPeContext() +inline OcsdPeContext::OcsdPeContext() { resetCtxt(); }

-void OcsdPeContext::resetCtxt() +inline void OcsdPeContext::resetCtxt() { // initialise the context m_context.bits64 = 0; diff --git a/decoder/include/etmv3/etmv3_decoder.h b/decoder/include/etmv3/etmv3_decoder.h index bad57df..48c9fc7 100644 --- a/decoder/include/etmv3/etmv3_decoder.h +++ b/decoder/include/etmv3/etmv3_decoder.h @@ -40,6 +40,7 @@ #include "etmv3/trc_pkt_elem_etmv3.h" #include "etmv3/trc_pkt_proc_etmv3.h" #include "etmv3/trc_pkt_types_etmv3.h" +#include "etmv3/trc_pkt_decode_etmv3.h"

#endif // ARM_ETMV3_DECODER_H_INCLUDED

diff --git a/decoder/include/etmv3/trc_cmp_cfg_etmv3.h b/decoder/include/etmv3/trc_cmp_cfg_etmv3.h index 016ac71..e9df5e8 100644 --- a/decoder/include/etmv3/trc_cmp_cfg_etmv3.h +++ b/decoder/include/etmv3/trc_cmp_cfg_etmv3.h @@ -53,10 +53,11 @@

• Primarily inlined for efficient code.

*/ -class EtmV3Config : public ocsd_etmv3_cfg +class EtmV3Config //: public ocsd_etmv3_cfg { public: EtmV3Config(); /**< Default constructor */

• EtmV3Config(const ocsd_etmv3_cfg *cfg_regs); ~EtmV3Config() {}; /**< Default destructor */

  /* register bit constants. */

@@ -73,9 +74,16 @@ public:

 static const uint32_t IDR_ALTBRANCH = 0x100000;

• //! copy assignment operator for base structure into class.

+// operations to convert to and from C-API structure

• //! copy assignment operator for C-API base structure into class. EtmV3Config & operator=(const ocsd_etmv3_cfg *p_cfg);

• //! cast operator returning struct const reference

• operator const ocsd_etmv3_cfg &() const { return m_cfg; };

• //! cast operator returning struct const pointer

• operator const ocsd_etmv3_cfg *() const { return &m_cfg; };

• //! combination enum to describe trace mode. enum EtmTraceMode { TM_INSTR_ONLY, //!< instruction only trace

@@ -111,6 +119,9 @@ public:

 const uint8_t getTraceID() const; //!< CoreSight Trace ID for this device.

+private:

• ocsd_etmv3_cfg m_cfg;

};

@@ -118,86 +129,86 @@ public:

inline EtmV3Config & EtmV3Config::operator=(const ocsd_etmv3_cfg *p_cfg) {

• *dynamic_cast<ocsd_etmv3_cfg *>(this) = *p_cfg;

• m_cfg = *p_cfg; return *this;

}

inline const bool EtmV3Config::isCycleAcc() const {

• return (bool)((reg_ctrl & CTRL_CYCLEACC) != 0);

• return (bool)((m_cfg.reg_ctrl & CTRL_CYCLEACC) != 0);

}

//! return X revision in 3.X inline const int EtmV3Config::MinorRev() const {

• return ((int)reg_idr & 0xF0) >> 4;

• return ((int)m_cfg.reg_idr & 0xF0) >> 4;

}

inline const bool EtmV3Config::isInstrTrace() const {

• return (bool)((reg_ctrl & CTRL_DATAONLY) == 0);

• return (bool)((m_cfg.reg_ctrl & CTRL_DATAONLY) == 0);

}

inline const bool EtmV3Config::isDataValTrace() const {

• return (bool)((reg_ctrl & CTRL_DATAVAL) != 0);

• return (bool)((m_cfg.reg_ctrl & CTRL_DATAVAL) != 0);

}

inline const bool EtmV3Config::isDataAddrTrace() const {

• return (bool)((reg_ctrl & CTRL_DATAADDR) != 0);

• return (bool)((m_cfg.reg_ctrl & CTRL_DATAADDR) != 0);

}

//! either or both data trace present inline const bool EtmV3Config::isDataTrace() const {

• return (bool)((reg_ctrl & (CTRL_DATAADDR | CTRL_DATAVAL)) != 0);

• return (bool)((m_cfg.reg_ctrl & (CTRL_DATAADDR | CTRL_DATAVAL)) != 0);

}

inline const bool EtmV3Config::isV7MArch() const {

• return (bool)((arch_ver == ARCH_V7) && (core_prof == profile_CortexM));

• return (bool)((m_cfg.arch_ver == ARCH_V7) && (m_cfg.core_prof == profile_CortexM));

}

//! has alternate branch encoding inline const bool EtmV3Config::isAltBranch() const {

• return (bool)(((reg_idr & IDR_ALTBRANCH) != 0) && (MinorRev() >= 4));

• return (bool)(((m_cfg.reg_idr & IDR_ALTBRANCH) != 0) && (MinorRev() >= 4));

}

//! processor implements virtualisation extensions. inline const bool EtmV3Config::hasVirtExt() const {

• return (bool)((reg_ccer & CCER_VIRTEXT) != 0);

• return (bool)((m_cfg.reg_ccer & CCER_VIRTEXT) != 0);

}

//! TS packet is 64 bit. inline const bool EtmV3Config::TSPkt64() const {

• return (bool)((reg_ccer & CCER_TS64BIT) != 0);

• return (bool)((m_cfg.reg_ccer & CCER_TS64BIT) != 0);

}

//! TS implemented. inline const bool EtmV3Config::hasTS() const {

• return (bool)((reg_ccer & CCER_HAS_TS) != 0);

• return (bool)((m_cfg.reg_ccer & CCER_HAS_TS) != 0);

}

//! TS is enabled in the trace inline const bool EtmV3Config::isTSEnabled() const {

• return (bool)((reg_ctrl & CTRL_TS_ENA) != 0);

• return (bool)((m_cfg.reg_ctrl & CTRL_TS_ENA) != 0);

}

//! tracing VMID inline const bool EtmV3Config::isVMIDTrace() const {

• return (bool)((reg_ctrl & CTRL_VMID_ENA) != 0);

• return (bool)((m_cfg.reg_ctrl & CTRL_VMID_ENA) != 0);

}

inline const uint8_t EtmV3Config::getTraceID() const {

• return (uint8_t)(reg_trc_id & 0x7F);

• return (uint8_t)(m_cfg.reg_trc_id & 0x7F);

}

/** @}*/ diff --git a/decoder/include/etmv3/trc_pkt_elem_etmv3.h b/decoder/include/etmv3/trc_pkt_elem_etmv3.h index 5d04719..bade8cc 100644 --- a/decoder/include/etmv3/trc_pkt_elem_etmv3.h +++ b/decoder/include/etmv3/trc_pkt_elem_etmv3.h @@ -49,16 +49,20 @@

• This class represents a single ETMv3 trace packet, along with intra packet state.

*/ -class EtmV3TrcPacket : public ocsd_etmv3_pkt, trcPrintableElem +class EtmV3TrcPacket : public trcPrintableElem { public: EtmV3TrcPacket(); ~EtmV3TrcPacket();

+// conversions between C-API struct and C++ object types

• // assign from C-API struct EtmV3TrcPacket &operator =(const ocsd_etmv3_pkt* p_pkt);
• // allow const cast to C-API struct to pass C++ object
• operator const ocsd_etmv3_pkt*() const { return &m_pkt_data; };
• operator const ocsd_etmv3_pkt&() const { return m_pkt_data; };

• // update interface - set packet values

+// update interface - set packet values void Clear(); //!< clear update data in packet ready for new one. void ResetState(); //!< reset intra packet state data -on full decoder reset.

@@ -91,21 +95,21 @@ public: void SetISyncIsLSiP(); void SetISyncNoAddr();

• // packet status interface - get packet info.
• const ocsd_etmv3_pkt_type getType() const { return type; };

+// packet status interface - get packet info.

• const ocsd_etmv3_pkt_type getType() const { return m_pkt_data.type; };

• const int AltISA() const { return context.curr_alt_isa; };
• const ocsd_isa ISA() const { return curr_isa; };

• const int AltISA() const { return m_pkt_data.context.curr_alt_isa; };
• const ocsd_isa ISA() const { return m_pkt_data.curr_isa; }; const bool isBadPacket() const;

• const uint32_t getCycleCount() const { return cycle_count; };
• const uint32_t getCtxtID() const { return context.ctxtID; };
• const uint32_t getVMID() const { return context.VMID; };
• const uint64_t getTS() const { return timestamp; };

• const uint32_t getCycleCount() const { return m_pkt_data.cycle_count; };
• const uint32_t getCtxtID() const { return m_pkt_data.context.ctxtID; };
• const uint32_t getVMID() const { return m_pkt_data.context.VMID; };
• const uint64_t getTS() const { return m_pkt_data.timestamp; };

• const bool isExcepCancel() const { return (exception.bits.present == 1) && (exception.bits.cancel == 1); };

• const bool isExcepCancel() const { return (m_pkt_data.exception.bits.present == 1) && (m_pkt_data.exception.bits.cancel == 1); };

• // printing

+// printing virtual void toString(std::string &str) const; virtual void toStringFmt(const uint32_t fmtFlags, std::string &str) const;

@@ -116,100 +120,102 @@ private: void getISyncStr(std::string &valStr) const; void getISAStr(std::string &isaStr) const; void getExcepStr(std::string &excepStr) const;

• ocsd_etmv3_pkt m_pkt_data;

};

inline void EtmV3TrcPacket::UpdateNS(const int NS) {

• context.curr_NS = NS;
• context.updated = 1;

• m_pkt_data.context.curr_NS = NS;
• m_pkt_data.context.updated = 1;

};

inline void EtmV3TrcPacket::UpdateAltISA(const int AltISA) {

• context.curr_alt_isa = AltISA;
• context.updated = 1;

• m_pkt_data.context.curr_alt_isa = AltISA;
• m_pkt_data.context.updated = 1;

}

inline void EtmV3TrcPacket::UpdateHyp(const int Hyp) {

• context.curr_Hyp = Hyp;
• context.updated = 1;

• m_pkt_data.context.curr_Hyp = Hyp;
• m_pkt_data.context.updated = 1;

}

inline void EtmV3TrcPacket::UpdateISA(const ocsd_isa isa) {

• prev_isa = curr_isa;
• curr_isa = isa;

• m_pkt_data.prev_isa = m_pkt_data.curr_isa;
• m_pkt_data.curr_isa = isa;

}

inline void EtmV3TrcPacket::SetType(const ocsd_etmv3_pkt_type p_type) {

• type = p_type;

• m_pkt_data.type = p_type;

}

inline void EtmV3TrcPacket::SetErrType(const ocsd_etmv3_pkt_type e_type) {

• err_type = type;
• type = e_type;

• m_pkt_data.err_type = m_pkt_data.type;
• m_pkt_data.type = e_type;

}

inline const bool EtmV3TrcPacket::isBadPacket() const {

• return (type >= ETM3_PKT_BAD_SEQUENCE);

• return (m_pkt_data.type >= ETM3_PKT_BAD_SEQUENCE);

}

inline void EtmV3TrcPacket::SetDataOOOTag(const uint8_t tag) {

• data.ooo_tag = tag;

• m_pkt_data.data.ooo_tag = tag;

}

inline void EtmV3TrcPacket::SetDataValue(const uint32_t value) {

• data.value = value;
• data.update_dval = 1;

• m_pkt_data.data.value = value;
• m_pkt_data.data.update_dval = 1;

}

inline void EtmV3TrcPacket::UpdateContextID(const uint32_t contextID) {

• context.updated_c = 1;
• context.ctxtID = contextID;

• m_pkt_data.context.updated_c = 1;
• m_pkt_data.context.ctxtID = contextID;

}

inline void EtmV3TrcPacket::UpdateVMID(const uint8_t VMID) {

• context.updated_v = 1;
• context.VMID = VMID;

• m_pkt_data.context.updated_v = 1;
• m_pkt_data.context.VMID = VMID;

}

inline void EtmV3TrcPacket::UpdateDataEndian(const uint8_t BE_Val) {

• data.be = BE_Val;
• data.update_be = 1;

• m_pkt_data.data.be = BE_Val;
• m_pkt_data.data.update_be = 1;

}

inline void EtmV3TrcPacket::SetCycleCount(const uint32_t cycleCount) {

• cycle_count = cycleCount;

• m_pkt_data.cycle_count = cycleCount;

}

inline void EtmV3TrcPacket::SetISyncReason(const ocsd_iSync_reason reason) {

• isync_info.reason = reason;

• m_pkt_data.isync_info.reason = reason;

}

inline void EtmV3TrcPacket::SetISyncHasCC() {

• isync_info.has_cycle_count = 1;

• m_pkt_data.isync_info.has_cycle_count = 1;

}

inline void EtmV3TrcPacket::SetISyncIsLSiP() {

• isync_info.has_LSipAddress = 1;

• m_pkt_data.isync_info.has_LSipAddress = 1;

}

inline void EtmV3TrcPacket::SetISyncNoAddr() {

• isync_info.no_address = 1;

• m_pkt_data.isync_info.no_address = 1;

}

/** @}*/ diff --git a/decoder/source/c_api/ocsd_c_api.cpp b/decoder/source/c_api/ocsd_c_api.cpp index 28f6145..1dae2d3 100644 --- a/decoder/source/c_api/ocsd_c_api.cpp +++ b/decoder/source/c_api/ocsd_c_api.cpp @@ -725,7 +725,7 @@ ocsd_datapath_resp_t EtmV3CBObj::PacketDataIn( const ocsd_datapath_op_t op, const ocsd_trc_index_t index_sop, const EtmV3TrcPacket *p_packet_in) {

• return m_c_api_cb_fn(m_p_cb_context, op,index_sop,p_packet_in);

• return m_c_api_cb_fn(m_p_cb_context, op,index_sop,*p_packet_in);

}

/****************** Etmv3 packet processor monitor callback function ***********/ @@ -741,7 +741,7 @@ void EtmV3PktMonCBObj::RawPacketDataMon( const ocsd_datapath_op_t op, const uint32_t size, const uint8_t *p_data) {

• return m_c_api_cb_fn(m_p_cb_context, op, index_sop, p_packet_in, size, p_data);

• return m_c_api_cb_fn(m_p_cb_context, op, index_sop, *p_packet_in, size, p_data);

}

diff --git a/decoder/source/etmv3/trc_cmp_cfg_etmv3.cpp b/decoder/source/etmv3/trc_cmp_cfg_etmv3.cpp index 1e90cc1..f374052 100644 --- a/decoder/source/etmv3/trc_cmp_cfg_etmv3.cpp +++ b/decoder/source/etmv3/trc_cmp_cfg_etmv3.cpp @@ -38,11 +38,16 @@ EtmV3Config::EtmV3Config() { // defaults set ETMv3.4, V7A, instruction only.

• arch_ver = ARCH_V7;
• core_prof = profile_CortexA;
• reg_ccer = 0;
• reg_idr = 0x4100F240;
• reg_ctrl = 0;

• m_cfg.arch_ver = ARCH_V7;
• m_cfg.core_prof = profile_CortexA;
• m_cfg.reg_ccer = 0;
• m_cfg.reg_idr = 0x4100F240;

It wouldn't be a bad idea to write a comment explaining the hard coded value.

• m_cfg.reg_ctrl = 0;

+}

+EtmV3Config::EtmV3Config(const ocsd_etmv3_cfg *cfg_regs) +{

• m_cfg = *cfg_regs;

}

EtmV3Config::EtmTraceMode const EtmV3Config::GetTraceMode() const @@ -54,7 +59,7 @@ EtmV3Config::EtmTraceMode const EtmV3Config::GetTraceMode() const const int EtmV3Config::CtxtIDBytes() const { int ctxtIdsizes[] = { 0, 1, 2, 4 };

• return ctxtIdsizes[(reg_ctrl >> 14) & 0x3];

• return ctxtIdsizes[(m_cfg.reg_ctrl >> 14) & 0x3];

}

/* End of File trc_cmp_cfg_etmv3.cpp */ diff --git a/decoder/source/etmv3/trc_pkt_elem_etmv3.cpp b/decoder/source/etmv3/trc_pkt_elem_etmv3.cpp index 332ff63..16c3a2e 100644 --- a/decoder/source/etmv3/trc_pkt_elem_etmv3.cpp +++ b/decoder/source/etmv3/trc_pkt_elem_etmv3.cpp @@ -39,7 +39,7 @@

EtmV3TrcPacket::EtmV3TrcPacket() {

• addr.size = VA_32BIT; // etm v3 only handles 32 bit addresses.

• m_pkt_data.addr.size = VA_32BIT; // etm v3 only handles 32 bit addresses.

}

EtmV3TrcPacket::~EtmV3TrcPacket() @@ -51,40 +51,40 @@ EtmV3TrcPacket::~EtmV3TrcPacket() // clear this packet info void EtmV3TrcPacket::Clear() {

• addr.pkt_bits = 0;
• prev_isa = curr_isa; // mark ISA as not changed
• exception.bits.present = 0;
• atom.num = 0;
• cycle_count = 0;
• context.updated = 0;
• context.updated_c = 0;
• context.updated_v = 0;
• data.ooo_tag = 0;
• data.value = 0;
• data.update_addr = 0;
• data.update_be = 0;
• data.update_dval = 0;
• ts_update_bits = 0;
• isync_info.has_cycle_count = 0;
• isync_info.has_LSipAddress = 0;
• isync_info.no_address = 0;

• m_pkt_data.addr.pkt_bits = 0;
• m_pkt_data.prev_isa = m_pkt_data.curr_isa; // mark ISA as not changed
• m_pkt_data.exception.bits.present = 0;
• m_pkt_data.atom.num = 0;
• m_pkt_data.cycle_count = 0;
• m_pkt_data.context.updated = 0;
• m_pkt_data.context.updated_c = 0;
• m_pkt_data.context.updated_v = 0;
• m_pkt_data.data.ooo_tag = 0;
• m_pkt_data.data.value = 0;
• m_pkt_data.data.update_addr = 0;
• m_pkt_data.data.update_be = 0;
• m_pkt_data.data.update_dval = 0;
• m_pkt_data.ts_update_bits = 0;
• m_pkt_data.isync_info.has_cycle_count = 0;
• m_pkt_data.isync_info.has_LSipAddress = 0;
• m_pkt_data.isync_info.no_address = 0;

There isn't a better way to do this? Something similar to C's memset() or bzero()?

}

// reset all state including intra packet void EtmV3TrcPacket::ResetState() {

• addr.val = 0;
• addr.valid_bits = 0;
• curr_isa = prev_isa = ocsd_isa_unknown;
• context.curr_alt_isa = 0;
• context.curr_NS = 0;
• context.curr_Hyp = 0;
• context.VMID = 0;
• context.ctxtID = 0;
• timestamp = 0;
• data.addr.valid_bits = 0;
• data.addr.val = 0;
• data.be = 0;

• m_pkt_data.addr.val = 0;
• m_pkt_data.addr.valid_bits = 0;
• m_pkt_data.curr_isa = m_pkt_data.prev_isa = ocsd_isa_unknown;
• m_pkt_data.context.curr_alt_isa = 0;
• m_pkt_data.context.curr_NS = 0;
• m_pkt_data.context.curr_Hyp = 0;
• m_pkt_data.context.VMID = 0;
• m_pkt_data.context.ctxtID = 0;
• m_pkt_data.timestamp = 0;
• m_pkt_data.data.addr.valid_bits = 0;
• m_pkt_data.data.addr.val = 0;
• m_pkt_data.data.be = 0;

Same comment as above.

 Clear();

}