https://bugs.kde.org/show_bug.cgi?id=423195
--- Comment #16 from Carl Love <c...@us.ibm.com> ---
I have uploaded the latest version of the three Valgrind ISA 3.1 support
patches.
A small error was found in the patch for "Add check for isa 3.1 support". The
original line in coregrind/m_initimg/initimg-linux.c, setup_client_stack() was
+ /* ISA 3.1 */
+ auxv_3_1 = (auxv->u.a_val & 0x01000000ULL) == 0x01000000ULL;
+ hw_caps_3_1 = (vex_archinfo->hwcaps & VEX_HWCAPS_PPC64_ISA3_1)
+ == VEX_HWCAPS_PPC64_ISA3_1;
the hex values were changed to:
+ /* ISA 3.1 */
+ auxv_3_1 = (auxv->u.a_val & 0x00040000ULL) == 0x00040000ULL;
+ hw_caps_3_1 = (vex_archinfo->hwcaps & VEX_HWCAPS_PPC64_ISA3_1)
+ == VEX_HWCAPS_PPC64_ISA3_1;
The following are the fixes for the second patch, "Instruction prefix support"
to address Julian's comments
> 0002-ISA-3.1-Instruction-Prefix-Support.patch
>
> Adds the support for prefix instructions
static Addr64 nextInsnAddr( void )
{
- return guest_CIA_curr_instr + 4;
+ return guest_CIA_curr_instr + WORD_INST_SIZE;
}
Is this correct? What if this insn is an 8-byte insn?
Yes it is correct. Added comment for clarity:
static Addr64 nextInsnAddr( void )
{
+ /* Note in the case of a prefix instruction, delta has already been
+ incremented by WORD_INST_SIZE to move past the prefix part of the
+ instruction. So only need to increment by WORD_INST_SIZE to get to
+ the start of the next instruction. */
return guest_CIA_curr_instr + WORD_INST_SIZE;
}
The prefix check macro check
+#define ENABLE_PREFIX_CHECK 1
+
+#if ENABLE_PREFIX_CHECK
+#define PREFIX_CHECK { vassert( !prefix_instruction( prefixInstr ) ); }
+#else
+#define PREFIX_CHECK { }
+#endif
I'm not sure whether you're intending to use PREFIX_CHECK just for debugging
the implementation
(meaning, it will fail if there are implementation bugs, but can't be made to
fail regardless of
the input) or whether it will fail if there is some kind of invalid input. The
first use is OK,
but for the second use we need to generate SIGILL instead of asserting. Can
you say which it is?
It is just a development check to make sure the instruction parsing is correct.
There are no instruction inputs that would cause this to fail. I added the
following comment for clarification:
+#define ISA_3_1_PREFIX_CHECK if (prefixInstr) {if (!allow_isa_3_1) goto
decode_noIsa3_1;}
+
+/* ENABLE_PREFIX_CHECK is for development purposes. Turn off for production
+ releases */
#define ENABLE_PREFIX_CHECK 1
I went ahead and turned it off for the commit. I will turn it on and off again
as we do each set of patches.
The issue with dis_nop_prefix:
+static Bool dis_nop_prefix ( UInt prefixInstr, UInt theInstr )
+{
...
+ return PREFIX_NOP_INVALID;
This seems not-correct from a types point of view. dis_nop_prefix is declared
to return a Bool,
but PREFIX_NOP_INVALID is -1. I imagine this only compiles because of how lame
C's type system
is. Can you fix it so that Bool values are only True or False?
@@ -28598,6 +29000,35 @@ DisResult disInstr_PPC_WRK (
+ int ret;
+ ret = dis_nop_prefix( prefixInstr, theInstr);
+ if (ret == True)
+ goto decode_success;
+ else if (ret == PREFIX_NOP_INVALID)
+ goto decode_failure;
Related problem here; pls fix (`ret` being sometimes True and sometimes
PREFIX_NOP_INVALID given
that they have different types)
The function needs to return type Int. We need to know True, False or invalid.
Changed the code to:
static Int dis_nop_prefix ( UInt prefixInstr, UInt theInstr )
{
...
}
--
You are receiving this mail because:
You are watching all bug changes.
