x86-64: fix displaced stepping for VEX, XOP, and EVEX encoded insns
Commit Message
Improper decoding has lead to wrong use of onebyte_has_modrm[] in all
those cases, leading to misbehavior in the debuggee (wrong data accessed
or faults raised) when RIP-relative operands were used. Fix VEX handling
and add XOP and EVEX recognition.
I apologize for this not being accompanied by a testsuite extension, but
it was hard enough already to find a couple of hours to investigate and
fix the actual issue here.
(For XOP there may be another problem with the immediates two of the
forms use. I didn't look into that aspect yet.)
gdb/
2019-02-07 Jan Beulich <jbeulich@suse.com>
* amd64-tdep.c (xop_prefix_p, evex_prefix_p): New.
(fixup_riprel): Use them.
(amd64_get_insn_details): Likewise. Handle VEX/XOP/EVEX cases
independent of legacy encoding ones.
Comments
On 02/07/2019 02:09 PM, Jan Beulich wrote:
> Improper decoding has lead to wrong use of onebyte_has_modrm[] in all
> those cases, leading to misbehavior in the debuggee (wrong data accessed
> or faults raised) when RIP-relative operands were used. Fix VEX handling
> and add XOP and EVEX recognition.
>
Thanks!
> I apologize for this not being accompanied by a testsuite extension, but
> it was hard enough already to find a couple of hours to investigate and
> fix the actual issue here.
I'd be really good to add a testcase though.
<https://sourceware.org/ml/gdb-patches/2017-11/msg00748.html>
added gdb.arch/amd64-disp-step-avx.S. Can we add something there?
Can you provide examples of instructions that we were getting wrong?
Note that re. EVEX, back in that referenced patch I said:
~~~
I think we may need a similar treatment for EVEX-encoded instructions,
but I didn't address that simply because I couldn't find any
EVEX-encoded RIP-relative instruction in the gas testsuite.
~~~
since you seem to know about some EVEX-encoded instruction that is
mishandled today, it'd be really good to add a test for it too.
Thanks,
Pedro Alves
>
> (For XOP there may be another problem with the immediates two of the
> forms use. I didn't look into that aspect yet.)
>
> gdb/
> 2019-02-07 Jan Beulich <jbeulich@suse.com>
>
> * amd64-tdep.c (xop_prefix_p, evex_prefix_p): New.
> (fixup_riprel): Use them.
> (amd64_get_insn_details): Likewise. Handle VEX/XOP/EVEX cases
> independent of legacy encoding ones.
>
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -1147,6 +1147,22 @@ vex3_prefix_p (gdb_byte pfx)
> return pfx == 0xc4;
> }
>
> +/* True if PFX is the start of an XOP prefix. */
> +
> +static bool
> +xop_prefix_p (const gdb_byte *pfx)
> +{
> + return pfx[0] == 0x8f && (pfx[1] & 0x38);
> +}
> +
> +/* True if PFX is the start of an EVEX prefix. */
> +
> +static bool
> +evex_prefix_p (const gdb_byte *pfx)
> +{
> + return pfx[0] == 0x62 && !(pfx[1] & 0x0c) && (pfx[2] & 4);
> +}
> +
> /* Skip the legacy instruction prefixes in INSN.
> We assume INSN is properly sentineled so we don't have to worry
> about falling off the end of the buffer. */
> @@ -1260,11 +1276,11 @@ static void
> amd64_get_insn_details (gdb_byte *insn, struct amd64_insn *details)
> {
> gdb_byte *start = insn;
> - int need_modrm;
> + int need_modrm = 0;
>
> details->raw_insn = insn;
>
> - details->opcode_len = -1;
> + details->opcode_len = 1;
> details->enc_prefix_offset = -1;
> details->opcode_offset = -1;
> details->modrm_offset = -1;
> @@ -1283,16 +1299,35 @@ amd64_get_insn_details (gdb_byte *insn,
> /* Don't record the offset in this case because this prefix has
> no REX.B equivalent. */
> insn += 2;
> + need_modrm = -1;
> }
> else if (vex3_prefix_p (*insn))
> {
> details->enc_prefix_offset = insn - start;
> + need_modrm = (insn[1] & 0x1f) == 1 ? -1 : 1;
> + insn += 3;
> + }
> + else if (xop_prefix_p (insn))
> + {
> + details->enc_prefix_offset = insn - start;
> insn += 3;
> + need_modrm = 1;
> + }
> + else if (evex_prefix_p (insn))
> + {
> + details->enc_prefix_offset = insn - start;
> + need_modrm = (insn[1] & 3) == 1 ? -1 : 1;
> + insn += 4;
> }
>
> details->opcode_offset = insn - start;
>
> - if (*insn == TWO_BYTE_OPCODE_ESCAPE)
> + if (need_modrm < 0)
> + {
> + /* VEX/EVEX-encoded would-be-two-byte opcode. */
> + need_modrm = twobyte_has_modrm[*insn];
> + }
> + else if (!need_modrm && *insn == TWO_BYTE_OPCODE_ESCAPE)
> {
> /* Two or three-byte opcode. */
> ++insn;
> @@ -1315,11 +1350,10 @@ amd64_get_insn_details (gdb_byte *insn,
> break;
> }
> }
> - else
> + else if (!need_modrm)
> {
> /* One-byte opcode. */
> need_modrm = onebyte_has_modrm[*insn];
> - details->opcode_len = 1;
> }
>
> if (need_modrm)
> @@ -1363,7 +1397,8 @@ fixup_riprel (struct gdbarch *gdbarch, a
> arch_tmp_regno = amd64_get_unused_input_int_reg (insn_details);
> tmp_regno = amd64_arch_reg_to_regnum (arch_tmp_regno);
>
> - /* Position of the not-B bit in the 3-byte VEX prefix (in byte 1). */
> + /* Position of the not-B bit in the 3-byte VEX prefix, the EVEX prefix,
> + and the XOP prefix (always in byte 1). */
> static constexpr gdb_byte VEX3_NOT_B = 0x20;
>
> /* REX.B should be unset (VEX.!B set) as we were using rip-relative
> @@ -1374,7 +1409,8 @@ fixup_riprel (struct gdbarch *gdbarch, a
> gdb_byte *pfx = &dsc->insn_buf[insn_details->enc_prefix_offset];
> if (rex_prefix_p (pfx[0]))
> pfx[0] &= ~REX_B;
> - else if (vex3_prefix_p (pfx[0]))
> + else if (vex3_prefix_p (pfx[0]) || xop_prefix_p (pfx)
> + || evex_prefix_p (pfx))
> pfx[1] |= VEX3_NOT_B;
> else
> gdb_assert_not_reached ("unhandled prefix");
>
>
>
>>> On 07.02.19 at 17:09, <palves@redhat.com> wrote:
> On 02/07/2019 02:09 PM, Jan Beulich wrote:
>> Improper decoding has lead to wrong use of onebyte_has_modrm[] in all
>> those cases, leading to misbehavior in the debuggee (wrong data accessed
>> or faults raised) when RIP-relative operands were used. Fix VEX handling
>> and add XOP and EVEX recognition.
>>
>
> Thanks!
>
>> I apologize for this not being accompanied by a testsuite extension, but
>> it was hard enough already to find a couple of hours to investigate and
>> fix the actual issue here.
>
> I'd be really good to add a testcase though.
> <https://sourceware.org/ml/gdb-patches/2017-11/msg00748.html>
> added gdb.arch/amd64-disp-step-avx.S. Can we add something there?
> Can you provide examples of instructions that we were getting wrong?
Not a complete set, but
{evex} vmovaps xmm1, [rip+in]
vmovdqa xmm1, [rip+in]
vmovdqa32 xmm1, [rip+in]
vmovdqa64 xmm1, [rip+in]
all cause #GP(0) (due to the violated alignment requirement) and
vmovdqu xmm1, [rip+in]
vmovdqu32 xmm1, [rip+in]
vmovdqu8 xmm1, [rip+in]
vbroadcasti128 ymm1, [rip+in]
vbroadcasti32x4 zmm1, [rip+in]
vbroadcasti64x2 zmm1, [rip+in]
vbroadcastf32x4 zmm1, [rip+in]
vpmovzxbw ymm1, [rip+in]
are ones that I had observed to load wrong data. Now that it is
clear what is going on, on criteria (but not necessarily the only
one) is whether a VEX-encoded insn's main opcode byte has the
respective onebyte_has_modrm[] table entry clear (because
that table got wrongly accessed). This for example explains the
(initially) confusing observation that VMOVAPS was fine but
VMOVDQA was not.
> Note that re. EVEX, back in that referenced patch I said:
> ~~~
> I think we may need a similar treatment for EVEX-encoded instructions,
> but I didn't address that simply because I couldn't find any
> EVEX-encoded RIP-relative instruction in the gas testsuite.
> ~~~
> since you seem to know about some EVEX-encoded instruction that is
> mishandled today, it'd be really good to add a test for it too.
I think all of them were mishandled, because the EVEX prefix
wasn't skipped before reading the supposed ModR/M byte.
Same for XOP.
In general I have to say that I consider this model rather fragile,
as any new insns (not covered by general rules) allowing for
memory operands will first require a gdb change before they
can be successfully stepped over with a breakpoint set on them.
Was it considered to either disable non-stop mode (and hence
displaced stepping) for single threaded debuggees by default,
or otherwise to use displaced stepping only for positively known
insns?
Jan
On 02/08/2019 09:49 AM, Jan Beulich wrote:
> In general I have to say that I consider this model rather fragile,
> as any new insns (not covered by general rules) allowing for
> memory operands will first require a gdb change before they
> can be successfully stepped over with a breakpoint set on them.
Just like it will require changes in the compiler to emit
them, and changes to the assembler to assemble them, and changes
to the disassembler to ... and on and on. The debugger is
just another place that requires addressing, but unfortunately
debuggers tend to be an afterthought.
> Was it considered to either disable non-stop mode (and hence
> displaced stepping) for single threaded debuggees by default,
Threaded programs aren't rare. Better just fix things.
> or otherwise to use displaced stepping only for positively known
> insns?
Initially GDB wasn't able to fallback to the old in-line
step-over-breakpoint method when displaced stepping was active,
but nowadays it can.
That will happen if the gdbarch_displaced_step_copy_insn hook
returns NULL, to indicate that the instruction can't be
handled out-of-line. s390_displaced_step_copy_insn does that,
for example:
~~~~~~~~~
/* If the instruction is too far from the jump pad, punt. This
will usually happen with instructions in shared libraries.
We could probably support these by rewriting them to be
absolute or fully emulating them. */
if (offset < INT32_MIN || offset > INT32_MAX)
{
/* Let the core fall back to stepping over the breakpoint
in-line. */
(...)
return NULL;
~~~~~~~~~
Note, however, that most instructions don't need any special
treatment; only those that use PC/RIP-relative addressing. Other
instructions are just copied verbatim to the scratch pad and
it Just Works. There are too many instructions to try to
come up with a comprehensive whitelist, of course. We want to use
displaced stepping for as many different instructions as possible,
to avoid having to pause all threads all the time.
So we'd need instead a blacklist of instructions that we can't handle.
Of course, adding support for them is just better.
I don't think new RIP-relative instructions are added to the
architecture so often that it's an intractable problem.
Thanks,
Pedro Alves
>>> On 07.02.19 at 15:09, wrote:
> Improper decoding has lead to wrong use of onebyte_has_modrm[] in all
> those cases, leading to misbehavior in the debuggee (wrong data accessed
> or faults raised) when RIP-relative operands were used. Fix VEX handling
> and add XOP and EVEX recognition.
>
> I apologize for this not being accompanied by a testsuite extension, but
> it was hard enough already to find a couple of hours to investigate and
> fix the actual issue here.
>
> (For XOP there may be another problem with the immediates two of the
> forms use. I didn't look into that aspect yet.)
>
> gdb/
> 2019-02-07 Jan Beulich <jbeulich@suse.com>
>
> * amd64-tdep.c (xop_prefix_p, evex_prefix_p): New.
> (fixup_riprel): Use them.
> (amd64_get_insn_details): Likewise. Handle VEX/XOP/EVEX cases
> independent of legacy encoding ones.
>
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -1147,6 +1147,22 @@ vex3_prefix_p (gdb_byte pfx)
> return pfx == 0xc4;
> }
>
> +/* True if PFX is the start of an XOP prefix. */
> +
> +static bool
> +xop_prefix_p (const gdb_byte *pfx)
> +{
> + return pfx[0] == 0x8f && (pfx[1] & 0x38);
> +}
> +
> +/* True if PFX is the start of an EVEX prefix. */
> +
> +static bool
> +evex_prefix_p (const gdb_byte *pfx)
> +{
> + return pfx[0] == 0x62 && !(pfx[1] & 0x0c) && (pfx[2] & 4);
> +}
> +
> /* Skip the legacy instruction prefixes in INSN.
> We assume INSN is properly sentineled so we don't have to worry
> about falling off the end of the buffer. */
> @@ -1260,11 +1276,11 @@ static void
> amd64_get_insn_details (gdb_byte *insn, struct amd64_insn *details)
> {
> gdb_byte *start = insn;
> - int need_modrm;
> + int need_modrm = 0;
>
> details->raw_insn = insn;
>
> - details->opcode_len = -1;
> + details->opcode_len = 1;
> details->enc_prefix_offset = -1;
> details->opcode_offset = -1;
> details->modrm_offset = -1;
> @@ -1283,16 +1299,35 @@ amd64_get_insn_details (gdb_byte *insn,
> /* Don't record the offset in this case because this prefix has
> no REX.B equivalent. */
> insn += 2;
> + need_modrm = -1;
> }
> else if (vex3_prefix_p (*insn))
> {
> details->enc_prefix_offset = insn - start;
> + need_modrm = (insn[1] & 0x1f) == 1 ? -1 : 1;
> + insn += 3;
> + }
> + else if (xop_prefix_p (insn))
> + {
> + details->enc_prefix_offset = insn - start;
> insn += 3;
> + need_modrm = 1;
> + }
> + else if (evex_prefix_p (insn))
> + {
> + details->enc_prefix_offset = insn - start;
> + need_modrm = (insn[1] & 3) == 1 ? -1 : 1;
> + insn += 4;
> }
>
> details->opcode_offset = insn - start;
>
> - if (*insn == TWO_BYTE_OPCODE_ESCAPE)
> + if (need_modrm < 0)
> + {
> + /* VEX/EVEX-encoded would-be-two-byte opcode. */
> + need_modrm = twobyte_has_modrm[*insn];
> + }
> + else if (!need_modrm && *insn == TWO_BYTE_OPCODE_ESCAPE)
> {
> /* Two or three-byte opcode. */
> ++insn;
> @@ -1315,11 +1350,10 @@ amd64_get_insn_details (gdb_byte *insn,
> break;
> }
> }
> - else
> + else if (!need_modrm)
> {
> /* One-byte opcode. */
> need_modrm = onebyte_has_modrm[*insn];
> - details->opcode_len = 1;
> }
>
> if (need_modrm)
> @@ -1363,7 +1397,8 @@ fixup_riprel (struct gdbarch *gdbarch, a
> arch_tmp_regno = amd64_get_unused_input_int_reg (insn_details);
> tmp_regno = amd64_arch_reg_to_regnum (arch_tmp_regno);
>
> - /* Position of the not-B bit in the 3-byte VEX prefix (in byte 1). */
> + /* Position of the not-B bit in the 3-byte VEX prefix, the EVEX prefix,
> + and the XOP prefix (always in byte 1). */
> static constexpr gdb_byte VEX3_NOT_B = 0x20;
>
> /* REX.B should be unset (VEX.!B set) as we were using rip-relative
> @@ -1374,7 +1409,8 @@ fixup_riprel (struct gdbarch *gdbarch, a
> gdb_byte *pfx = &dsc->insn_buf[insn_details->enc_prefix_offset];
> if (rex_prefix_p (pfx[0]))
> pfx[0] &= ~REX_B;
> - else if (vex3_prefix_p (pfx[0]))
> + else if (vex3_prefix_p (pfx[0]) || xop_prefix_p (pfx)
> + || evex_prefix_p (pfx))
> pfx[1] |= VEX3_NOT_B;
> else
> gdb_assert_not_reached ("unhandled prefix");
>
>
>
>
I'd like for this to be merged with a testcase. If you can't
provide one, then maybe someone else can write one based
on your instructions. I have it on my list, though not at
the top though. So if someone beats me to it, it'd be great..
Thanks,
Pedro Alves
@@ -1147,6 +1147,22 @@ vex3_prefix_p (gdb_byte pfx)
return pfx == 0xc4;
}
+/* True if PFX is the start of an XOP prefix. */
+
+static bool
+xop_prefix_p (const gdb_byte *pfx)
+{
+ return pfx[0] == 0x8f && (pfx[1] & 0x38);
+}
+
+/* True if PFX is the start of an EVEX prefix. */
+
+static bool
+evex_prefix_p (const gdb_byte *pfx)
+{
+ return pfx[0] == 0x62 && !(pfx[1] & 0x0c) && (pfx[2] & 4);
+}
+
/* Skip the legacy instruction prefixes in INSN.
We assume INSN is properly sentineled so we don't have to worry
about falling off the end of the buffer. */
@@ -1260,11 +1276,11 @@ static void
amd64_get_insn_details (gdb_byte *insn, struct amd64_insn *details)
{
gdb_byte *start = insn;
- int need_modrm;
+ int need_modrm = 0;
details->raw_insn = insn;
- details->opcode_len = -1;
+ details->opcode_len = 1;
details->enc_prefix_offset = -1;
details->opcode_offset = -1;
details->modrm_offset = -1;
@@ -1283,16 +1299,35 @@ amd64_get_insn_details (gdb_byte *insn,
/* Don't record the offset in this case because this prefix has
no REX.B equivalent. */
insn += 2;
+ need_modrm = -1;
}
else if (vex3_prefix_p (*insn))
{
details->enc_prefix_offset = insn - start;
+ need_modrm = (insn[1] & 0x1f) == 1 ? -1 : 1;
+ insn += 3;
+ }
+ else if (xop_prefix_p (insn))
+ {
+ details->enc_prefix_offset = insn - start;
insn += 3;
+ need_modrm = 1;
+ }
+ else if (evex_prefix_p (insn))
+ {
+ details->enc_prefix_offset = insn - start;
+ need_modrm = (insn[1] & 3) == 1 ? -1 : 1;
+ insn += 4;
}
details->opcode_offset = insn - start;
- if (*insn == TWO_BYTE_OPCODE_ESCAPE)
+ if (need_modrm < 0)
+ {
+ /* VEX/EVEX-encoded would-be-two-byte opcode. */
+ need_modrm = twobyte_has_modrm[*insn];
+ }
+ else if (!need_modrm && *insn == TWO_BYTE_OPCODE_ESCAPE)
{
/* Two or three-byte opcode. */
++insn;
@@ -1315,11 +1350,10 @@ amd64_get_insn_details (gdb_byte *insn,
break;
}
}
- else
+ else if (!need_modrm)
{
/* One-byte opcode. */
need_modrm = onebyte_has_modrm[*insn];
- details->opcode_len = 1;
}
if (need_modrm)
@@ -1363,7 +1397,8 @@ fixup_riprel (struct gdbarch *gdbarch, a
arch_tmp_regno = amd64_get_unused_input_int_reg (insn_details);
tmp_regno = amd64_arch_reg_to_regnum (arch_tmp_regno);
- /* Position of the not-B bit in the 3-byte VEX prefix (in byte 1). */
+ /* Position of the not-B bit in the 3-byte VEX prefix, the EVEX prefix,
+ and the XOP prefix (always in byte 1). */
static constexpr gdb_byte VEX3_NOT_B = 0x20;
/* REX.B should be unset (VEX.!B set) as we were using rip-relative
@@ -1374,7 +1409,8 @@ fixup_riprel (struct gdbarch *gdbarch, a
gdb_byte *pfx = &dsc->insn_buf[insn_details->enc_prefix_offset];
if (rex_prefix_p (pfx[0]))
pfx[0] &= ~REX_B;
- else if (vex3_prefix_p (pfx[0]))
+ else if (vex3_prefix_p (pfx[0]) || xop_prefix_p (pfx)
+ || evex_prefix_p (pfx))
pfx[1] |= VEX3_NOT_B;
else
gdb_assert_not_reached ("unhandled prefix");