[PATCH] covoar: Add aarch64 target
Alex White
alex.white at oarcorp.com
Wed Mar 24 18:27:02 UTC 2021
On Wed, Mar 24, 2021 at 12:25 PM Gedare Bloom <gedare at rtems.org> wrote:
>
> On Wed, Mar 24, 2021 at 10:46 AM Alex White <alex.white at oarcorp.com> wrote:
> > 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
See final comment below.
>
> > + */
> > +
> > +#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.
See final comment below.
>
> > + }
> > +
> > + 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?
No reason, no. I should change it to 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.
See final comment below.
>
> > +
> > + 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?
See final comment below.
>
> > + "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.
As you have pointed out, there are a lot of areas where this code can
be improved. The reason I didn't make any improvements to this code
when I wrote it was that it is almost all a copy-and-paste of the code
from the other Target_* classes. Most of them appear to have the same
issues that you pointed out (excluding my use of strcmp).
It made sense to me to keep the code as close as possible to the other
Target_* classes so that a patch could be made in the future to improve
all of it at once rather than having a (potentially confusing) style
change for only Target_aarch64.
Additionally, my goal with this patch was only to add support for the
AArch64 architecture. I did this in the safest way I know how, by
imitating known-working code.
That being said, I agree with most of your suggestions, and I would be
happy to make the fixes for this patch if that is the strategy we want
to follow.
Thanks,
Alex
>
> > + 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 at rtems.org
> > http://lists.rtems.org/mailman/listinfo/devel
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
More information about the devel
mailing list