On Wed, Mar 24, 2021 at 12:27 PM Alex White <alex.wh...@oarcorp.com> wrote:
>
> On Wed, Mar 24, 2021 at 12:25 PM Gedare Bloom <ged...@rtems.org> wrote:
> >
> > On Wed, Mar 24, 2021 at 10:46 AM Alex White <alex.wh...@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.
>
I think the patch can go in based on consistency with other, similar
"port" files, but it would be good if we had a plan for the direction
of this tool and making sure it is to the quality of production level
we want associated with RTEMS. That's all.
> 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@rtems.org
> > > http://lists.rtems.org/mailman/listinfo/devel
> > _______________________________________________
> > 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
