[1/2] Add unit test to aarch64 prologue analyzer
Commit Message
On Wed, Nov 30, 2016 at 06:53:08AM -0500, Antoine Tremblay wrote:
> >> Also I wonder if we need to specify the default constructor explicitly ?
> >> Is there a rationale for it?
> >>
> >> It's never used too, unless you apply my previous comment.
> >
> > The instruction_reader_test default constructor is never used. How
> > about using "= delete"?
> >
> > instruction_reader_test () = delete;
> > instruction_reader_test (std::initializer_list<uint32_t> init)
> > : insns{init} {}
>
> Yes that would be more appropriate if we're going to specify that.
>
> I just wrote a patch with a C++ class and did not include explicit
> default constructors do you think we should make it a code convention to
> explicitly specify their existence or non-existence (=default, =delete) ?
If you don't want default constructor to be used, "=delete" is useful,
IMO, which tells compiler not to generate the default constructor. The
intention is quite clear that I don't want you to use the default
constructor.
Using "=default" is not that clear. I personally prefer to write code
in an explicit way, so I prefer putting "=default" at the end.
>
> I could not find mention of that in GCC's C++ conventions...
IMO, using "=default" is a personal programming habit, so it is
reasonable not to mention it in C++ code conventions.
Comments
Yao Qi writes:
> On Wed, Nov 30, 2016 at 06:53:08AM -0500, Antoine Tremblay wrote:
>> >> Also I wonder if we need to specify the default constructor explicitly ?
>> >> Is there a rationale for it?
>> >>
>> >> It's never used too, unless you apply my previous comment.
>> >
>> > The instruction_reader_test default constructor is never used. How
>> > about using "= delete"?
>> >
>> > instruction_reader_test () = delete;
>> > instruction_reader_test (std::initializer_list<uint32_t> init)
>> > : insns{init} {}
>>
>> Yes that would be more appropriate if we're going to specify that.
>>
>> I just wrote a patch with a C++ class and did not include explicit
>> default constructors do you think we should make it a code convention to
>> explicitly specify their existence or non-existence (=default, =delete) ?
>
> If you don't want default constructor to be used, "=delete" is useful,
> IMO, which tells compiler not to generate the default constructor. The
> intention is quite clear that I don't want you to use the default
> constructor.
>
OK.
> Using "=default" is not that clear. I personally prefer to write code
> in an explicit way, so I prefer putting "=default" at the end.
>
>>
>> I could not find mention of that in GCC's C++ conventions...
>
> IMO, using "=default" is a personal programming habit, so it is
> reasonable not to mention it in C++ code conventions.
OK thanks for the clarification.
The patch LGTM.
On 11/30/2016 04:34 PM, Yao Qi wrote:
> On Wed, Nov 30, 2016 at 06:53:08AM -0500, Antoine Tremblay wrote:
>>>> Also I wonder if we need to specify the default constructor explicitly ?
>>>> Is there a rationale for it?
>>>>
>>>> It's never used too, unless you apply my previous comment.
>>>
>>> The instruction_reader_test default constructor is never used. How
>>> about using "= delete"?
>>>
>>> instruction_reader_test () = delete;
>>> instruction_reader_test (std::initializer_list<uint32_t> init)
>>> : insns{init} {}
>>
>> Yes that would be more appropriate if we're going to specify that.
>>
>> I just wrote a patch with a C++ class and did not include explicit
>> default constructors do you think we should make it a code convention to
>> explicitly specify their existence or non-existence (=default, =delete) ?
>
> If you don't want default constructor to be used, "=delete" is useful,
> IMO, which tells compiler not to generate the default constructor. The
> intention is quite clear that I don't want you to use the default
> constructor.
Note that if the class has a user-defined constructor, then the
default ctor is not generated at all. So you don't need to delete
the default one then. If I see a "=delete" in such case (as above),
I'll scratch my head wondering why the redundancy is there in the
first place, and maybe if so inclined that day remove the unnecessary
code as an obvious cleanup patch. :-)
(deleted functions participate in overload resolution, so there's
a difference from the method not being defined, but I don't believe
that's what you were going for here.)
Thanks,
Pedro Alves
@@ -44,6 +44,7 @@
#include "infcall.h"
#include "ax.h"
#include "ax-gdb.h"
+#include "selftest.h"
#include "aarch64-tdep.h"
@@ -195,6 +196,29 @@ show_aarch64_debug (struct ui_file *file, int from_tty,
fprintf_filtered (file, _("AArch64 debugging is %s.\n"), value);
}
+/* Abstract instruction reader. */
+
+class abstract_instruction_reader
+{
+public:
+ /* Read in one instruction. */
+ virtual ULONGEST read (CORE_ADDR memaddr, int len,
+ enum bfd_endian byte_order) = 0;
+};
+
+/* Instruction reader from real target. */
+
+class instruction_reader : public abstract_instruction_reader
+{
+ public:
+ instruction_reader () = default;
+
+ ULONGEST read (CORE_ADDR memaddr, int len, enum bfd_endian byte_order)
+ {
+ return read_memory_unsigned_integer (memaddr, len, byte_order);
+ }
+};
+
/* Analyze a prologue, looking for a recognizable stack frame
and frame pointer. Scan until we encounter a store that could
clobber the stack frame unexpectedly, or an unknown instruction. */
@@ -202,7 +226,8 @@ show_aarch64_debug (struct ui_file *file, int from_tty,
static CORE_ADDR
aarch64_analyze_prologue (struct gdbarch *gdbarch,
CORE_ADDR start, CORE_ADDR limit,
- struct aarch64_prologue_cache *cache)
+ struct aarch64_prologue_cache *cache,
+ abstract_instruction_reader& reader)
{
enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
int i;
@@ -221,7 +246,7 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
uint32_t insn;
aarch64_inst inst;
- insn = read_memory_unsigned_integer (start, 4, byte_order_for_code);
+ insn = reader.read (start, 4, byte_order_for_code);
if (aarch64_decode_insn (insn, &inst, 1) != 0)
break;
@@ -436,6 +461,89 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
return start;
}
+static CORE_ADDR
+aarch64_analyze_prologue (struct gdbarch *gdbarch,
+ CORE_ADDR start, CORE_ADDR limit,
+ struct aarch64_prologue_cache *cache)
+{
+ instruction_reader reader { };
+
+ return aarch64_analyze_prologue (gdbarch, start, limit, cache,
+ reader);
+}
+
+#if GDB_SELF_TEST
+
+namespace selftests {
+
+ /* Instruction reader from manually cooked instruction sequences. */
+ class instruction_reader_test : public abstract_instruction_reader
+ {
+ public:
+ instruction_reader_test () = delete;
+ instruction_reader_test (std::initializer_list<uint32_t> init)
+ : insns{init} {}
+
+ ULONGEST read (CORE_ADDR memaddr, int len, enum bfd_endian byte_order)
+ {
+ SELF_CHECK (len == 4);
+ SELF_CHECK (memaddr % 4 == 0);
+ SELF_CHECK (memaddr / 4 < insns.size());
+
+ return insns[memaddr / 4];
+ }
+
+ private:
+ std::vector<uint32_t> insns;
+ };
+
+static void
+aarch64_analyze_prologue_test (void)
+{
+ struct gdbarch_info info;
+
+ gdbarch_info_init (&info);
+ info.bfd_arch_info = bfd_scan_arch ("aarch64");
+
+ struct gdbarch *gdbarch = gdbarch_find_by_info (info);
+ SELF_CHECK (gdbarch != NULL);
+
+ struct aarch64_prologue_cache cache;
+ cache.saved_regs = trad_frame_alloc_saved_regs (gdbarch);
+
+ instruction_reader_test test {
+ 0xa9af7bfd, /* stp x29, x30, [sp,#-272]! */
+ 0x910003fd, /* mov x29, sp */
+ 0x97ffffe6, /* bl 0x400580 */
+ };
+
+ CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128,
+ &cache, test);
+ SELF_CHECK (end == 4 * 2);
+
+ SELF_CHECK (cache.framereg == AARCH64_FP_REGNUM);
+ SELF_CHECK (cache.framesize == 272);
+
+ for (int i = 0; i < AARCH64_X_REGISTER_COUNT; i++)
+ {
+ if (i == AARCH64_FP_REGNUM)
+ SELF_CHECK (cache.saved_regs[i].addr == -272);
+ else if (i == AARCH64_LR_REGNUM)
+ SELF_CHECK (cache.saved_regs[i].addr == -264);
+ else
+ SELF_CHECK (cache.saved_regs[i].addr == -1);
+ }
+
+ for (int i = 0; i < AARCH64_D_REGISTER_COUNT; i++)
+ {
+ int regnum = gdbarch_num_regs (gdbarch);
+
+ SELF_CHECK (cache.saved_regs[i + regnum + AARCH64_D0_REGNUM].addr == -1);
+ }
+}
+}
+#endif /* GDB_SELF_TEST */
+
/* Implement the "skip_prologue" gdbarch method. */
static CORE_ADDR
@@ -2864,6 +2972,10 @@ When on, AArch64 specific debugging is enabled."),
NULL,
show_aarch64_debug,
&setdebuglist, &showdebuglist);
+
+#if GDB_SELF_TEST
+ register_self_test (selftests::aarch64_analyze_prologue_test);
+#endif
}
/* AArch64 process record-replay related structures, defines etc. */
@@ -43,16 +43,10 @@ trad_frame_cache_zalloc (struct frame_info *this_frame)
return this_trad_cache;
}
-/* A traditional frame is unwound by analysing the function prologue
- and using the information gathered to track registers. For
- non-optimized frames, the technique is reliable (just need to check
- for all potential instruction sequences). */
-
struct trad_frame_saved_reg *
-trad_frame_alloc_saved_regs (struct frame_info *this_frame)
+trad_frame_alloc_saved_regs (struct gdbarch *gdbarch)
{
int regnum;
- struct gdbarch *gdbarch = get_frame_arch (this_frame);
int numregs = gdbarch_num_regs (gdbarch) + gdbarch_num_pseudo_regs (gdbarch);
struct trad_frame_saved_reg *this_saved_regs
= FRAME_OBSTACK_CALLOC (numregs, struct trad_frame_saved_reg);
@@ -65,6 +59,19 @@ trad_frame_alloc_saved_regs (struct frame_info *this_frame)
return this_saved_regs;
}
+/* A traditional frame is unwound by analysing the function prologue
+ and using the information gathered to track registers. For
+ non-optimized frames, the technique is reliable (just need to check
+ for all potential instruction sequences). */
+
+struct trad_frame_saved_reg *
+trad_frame_alloc_saved_regs (struct frame_info *this_frame)
+{
+ struct gdbarch *gdbarch = get_frame_arch (this_frame);
+
+ return trad_frame_alloc_saved_regs (gdbarch);
+}
+
enum { TF_REG_VALUE = -1, TF_REG_UNKNOWN = -2 };
int
@@ -104,6 +104,7 @@ int trad_frame_realreg_p (struct trad_frame_saved_reg this_saved_regs[],
/* Return a freshly allocated (and initialized) trad_frame array. */
struct trad_frame_saved_reg *trad_frame_alloc_saved_regs (struct frame_info *);
+struct trad_frame_saved_reg *trad_frame_alloc_saved_regs (struct gdbarch *);
/* Given the trad_frame info, return the location of the specified
register. */