On Wed, Mar 24, 2021 at 10:46 AM Alex White <alex.wh...@oarcorp.com> wrote:
>
> ---
> tester/covoar/TargetFactory.cc | 2 +
> tester/covoar/Target_aarch64.cc | 100 ++++++++++++++++++++++++++++++++
> tester/covoar/Target_aarch64.h | 77 ++++++++++++++++++++++++
> tester/covoar/wscript | 1 +
> 4 files changed, 180 insertions(+)
> create mode 100644 tester/covoar/Target_aarch64.cc
> create mode 100644 tester/covoar/Target_aarch64.h
>
> diff --git a/tester/covoar/TargetFactory.cc b/tester/covoar/TargetFactory.cc
> index 12de94d..57ba686 100644
> --- a/tester/covoar/TargetFactory.cc
> +++ b/tester/covoar/TargetFactory.cc
> @@ -17,6 +17,7 @@
>
> #include "TargetFactory.h"
>
> +#include "Target_aarch64.h"
> #include "Target_arm.h"
> #include "Target_i386.h"
> #include "Target_m68k.h"
> @@ -51,6 +52,7 @@ namespace Target {
> //! All must be derived from TargetBase.
> //!
> static FactoryEntry_t FactoryTable[] = {
> + { "aarch64", Target_aarch64_Constructor },
> { "arm", Target_arm_Constructor },
> { "i386", Target_i386_Constructor },
> { "lm32", Target_lm32_Constructor },
> diff --git a/tester/covoar/Target_aarch64.cc b/tester/covoar/Target_aarch64.cc
> new file mode 100644
> index 0000000..64472d6
> --- /dev/null
> +++ b/tester/covoar/Target_aarch64.cc
> @@ -0,0 +1,100 @@
> +/*! @file Target_aarch64.cc
> + * @brief Target_aarch64 Implementation
> + *
> + * This file contains the implementation of the base class for
> + * functions supporting target unique functionallity.
only one l in functionality
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +#include <rld.h>
> +
> +#include "Target_aarch64.h"
> +
> +namespace Target {
> +
> + Target_aarch64::Target_aarch64( std::string targetName ):
> + TargetBase( targetName )
> + {
> + conditionalBranchInstructions.push_back("cbnz");
> + conditionalBranchInstructions.push_back("cbz");
> + conditionalBranchInstructions.push_back("tbnz");
> + conditionalBranchInstructions.push_back("tbz");
> + conditionalBranchInstructions.push_back("b.eq");
> + conditionalBranchInstructions.push_back("b.ne");
> + conditionalBranchInstructions.push_back("b.cs");
> + conditionalBranchInstructions.push_back("b.hs");
> + conditionalBranchInstructions.push_back("b.cc");
> + conditionalBranchInstructions.push_back("b.lo");
> + conditionalBranchInstructions.push_back("b.mi");
> + conditionalBranchInstructions.push_back("b.pl");
> + conditionalBranchInstructions.push_back("b.vs");
> + conditionalBranchInstructions.push_back("b.vc");
> + conditionalBranchInstructions.push_back("b.hi");
> + conditionalBranchInstructions.push_back("b.ls");
> + conditionalBranchInstructions.push_back("b.ge");
> + conditionalBranchInstructions.push_back("b.lt");
> + conditionalBranchInstructions.push_back("b.gt");
> + conditionalBranchInstructions.push_back("b.le");
> +
> + conditionalBranchInstructions.sort();
this is kind of lazy :) you could sort them as you push them.
> + }
> +
> + Target_aarch64::~Target_aarch64()
> + {
> + }
> +
> + bool Target_aarch64::isNopLine(
> + const char* const line,
> + int& size
> + )
> + {
> + if (!strcmp( &line[strlen(line)-3], "nop")) {
any reason this is strcmp but the others are strncmp?
> + size = 4;
> + return true;
> + }
I wonder if you guys want to take a stab at using C++ strings or not?
just a thought.
This stuff how it is being done is very fickle with these constant
numbers, but I guess the interface shouldn't change too much since it
derives from the ISA. It just isn't clear at all how this stuff works
by casual observation.
> +
> + if (!strncmp( &line[strlen(line)-6], "udf", 3)) {
> + size = 4;
> + return true;
> + }
> +
> + // On ARM, there are literal tables at the end of methods.
> + // We need to avoid them.
> + if (!strncmp( &line[strlen(line)-10], ".byte", 5)) {
> + size = 1;
> + return true;
> + }
> + if (!strncmp( &line[strlen(line)-13], ".short", 6)) {
> + size = 2;
> + return true;
> + }
> + if (!strncmp( &line[strlen(line)-16], ".word", 5)) {
> + size = 4;
> + return true;
> + }
> +
> + return false;
> + }
> +
> + bool Target_aarch64::isBranch(
> + const char* instruction
> + )
> + {
> + throw rld::error(
> + "DETERMINE BRANCH INSTRUCTIONS FOR THIS ARCHITECTURE! -- fix me",
Seems like this could be a better message. Is this the "standard"
error message being used in covoar for this condition?
> + "Target_aarch64::isBranch"
> + );
> + }
> +
> + TargetBase *Target_aarch64_Constructor(
> + std::string targetName
> + )
> + {
> + return new Target_aarch64( targetName );
> + }
> +
> +}
> diff --git a/tester/covoar/Target_aarch64.h b/tester/covoar/Target_aarch64.h
> new file mode 100644
> index 0000000..8c15daa
> --- /dev/null
> +++ b/tester/covoar/Target_aarch64.h
> @@ -0,0 +1,77 @@
> +/*! @file Target_aarch64.h
> + * @brief Target_aarch64 Specification
> + *
> + * This file contains the specification of the Target_aarch64 class.
> + */
> +
> +#ifndef __TARGET_AARCH64_H__
> +#define __TARGET_AARCH64_H__
> +
> +#include <list>
> +#include <string>
> +#include "TargetBase.h"
> +
> +namespace Target {
> +
> + /*! @class Target_aarch64
> + *
> + * This class is the Target class for the aarch64 processor.
> + *
> + */
> + class Target_aarch64: public TargetBase {
> +
> + public:
> +
> + /*!
> + * This method constructs an Target_aarch64 instance.
> + */
> + Target_aarch64( std::string targetName );
> +
> + /*!
> + * This method destructs an Target_aarch64 instance.
> + */
> + virtual ~Target_aarch64();
> +
> + /*!
> + * This method determines whether the specified line from a
> + * objdump file is a nop instruction.
> + *
> + * @param[in] line contains the object dump line to check
> + * @param[out] size is set to the size in bytes of the nop
> + *
> + * @return Returns TRUE if the instruction is a nop, FALSE otherwise.
> + */
> + bool isNopLine(
> + const char* const line,
> + int& size
> + );
> +
> + /*!
> + * This method determines if the specified line from an
> + * objdump file is a branch instruction.
> + */
> + bool isBranch(
> + const char* const instruction
> + );
> +
i'm surprised these aren't part of the TargetBase interface. since
they are not, why they should be public?
I still struggle reviewing this codebase, in part because it is C+C++
(TM) and in part because I'm not so proficient in C++ to make concrete
recommendations how to write this better. I think, if the goal is
eventually to make this more C++ like code, then new code coming in
should aim to move the needle in that direction rather than continue
to propagate the old ways of doing.
> + private:
> +
> + };
> +
> + //!
> + //! @brief Constructor Helper
> + //!
> + //! This is a constructor helper for this class which can be used in
> support
> + //! of factories.
> + //!
> + //! @param [in] Targetname is the name of the Target being constructed.
> + //!
> + //! @return This method constructs a new instance of the Target and returns
> + //! that to the caller.
> + //!
> + TargetBase *Target_aarch64_Constructor(
> + std::string targetName
> + );
> +
> +}
> +#endif
> diff --git a/tester/covoar/wscript b/tester/covoar/wscript
> index 165a1b8..82599b0 100644
> --- a/tester/covoar/wscript
> +++ b/tester/covoar/wscript
> @@ -106,6 +106,7 @@ def build(bld):
> 'ReportsText.cc',
> 'ReportsHtml.cc',
> 'SymbolTable.cc',
> + 'Target_aarch64.cc',
> 'Target_arm.cc',
> 'TargetBase.cc',
> 'TargetFactory.cc',
> --
> 2.27.0
>
> _______________________________________________
> devel mailing list
> devel@rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
