On Mon, 2011-03-21 at 14:56 -0400, Nicolas Pitre wrote:
On Mon, 21 Mar 2011, Tixy wrote:
On Mon, 2011-03-21 at 13:18 -0400, Nicolas Pitre wrote:
On Mon, 21 Mar 2011, Tixy wrote:
[...]
I initially thought of fixing do_undefinstr to use the full 32 bits of the instruction, which seems the right thing to do. But backed off because it would introduce compatibility problems with existing handlers like ptrace and possibly other out-of-tree code.
We don't care about out-of-tree code. There is a reason for merging code into mainline after all.
From a quick glance only one hook so far expects a 32-bit Thumb2 instruction i.e. for ptrace. So IMHO this is worth doing.
OK, I'll add the changes for this.
One of my biggest headaches with this is deciding the best way of storing a 32-bit thumb instruction in an integer. The human readable way [ 1st-half : 2nd-half ], or the way that a 32-bit value would be stored in memory, which varies between little/big-endian, or even always as little-endian.
I'd suggest:
insn = ((u16 *)pc)[0]; if (insn_is_wide(insn)) { insn <<= 16; insn |= ((u16 *)pc)[1]; }
effectively using a big endian representation, but which is similar to ARM mode instructions, and which would make sense regardless of the instruction mode/width when printed.
This is what I've done in the kprobe code that handles thumb, except that I've also shifted 16-bit thumb instructions into the top half. This was so we always know where the first half-word is and can test it for 16/32-bit size with "if(first_half<0xe800)".
However, I've just realised that I didn't need to do this, with your example "if(inst<0xe800)" still works; so my other patches can be simplified :-)
We could even just test the upper 16-bits for non-zero to indicate 32-bit. E.g. to store a thumb instruction:
u16* pc = (u16 *)addr; u16 half = inst>>16; if (half) *pc++ = half; *pc++ = half&0xffff;
which should compile to just three or four CPU instructions.