[RFA 3/5] New port: CR16: gdb port
Pedro Alves
palves@redhat.com
Fri Jan 18 18:25:00 GMT 2013
On 01/17/2013 08:59 AM, Joel Brobecker wrote:
>> + /* Read 6 bytes, max 48 bit opcode. */
>> + target_read_memory (pc, buf, 6);
>> + cr16_words[0] = buf[1] << 8 | buf[0];
>> + cr16_words[1] = buf[3] << 8 | buf[2];
>> + cr16_words[2] = buf[5] << 8 | buf[4];
>> + cr16_allWords = (((ULONGLONG) cr16_words[0] << 32)
>> + + ((ULONGLONG) cr16_words[1] << 16)
>> + + cr16_words[2]);
>> +
>> + /* Find a matching opcode in table.
>> + Nonzero means instruction has a match. */
>> + is_decoded = cr16_match_opcode ();
>> + cr16_make_instruction ();
>> + length = cr16_currInsn.size;
>
> It hurts every time I read this code... Nothing you can do short
> of improving opcode, but this is really awful :-(.
This seems pretty isolated. How about exporting a function that
hides these opcodes details? I don't even pretend to understand
what the code is trying to do, and it'd be an opportunity to
comment it in the function description. :-)
/* Take BUF, do something with it, and write length
to LENGTH. Blah, blah. */
cr16_do_something (buf, *length, ...);
> static const char *const reg_names[] =
> +{
...
> + "r0r1_orig",
This too looks like a ptrace detail escaping all the
way to the user, similar to the gdbserver issues.
Any reason not to split those up? I think it'd be nicer.
...
> +};
Joel Brobecker wrote:
> The rest still looks pretty good to me :)
I agree. :-)
--
Pedro Alves
More information about the Binutils
mailing list