This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [RFA 3/5] New port: CR16: gdb port
- From: Pedro Alves <palves at redhat dot com>
- To: Joel Brobecker <brobecker at adacore dot com>
- Cc: Kaushik Phatak <Kaushik dot Phatak at kpitcummins dot com>, "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>, "binutils at sourceware dot org" <binutils at sourceware dot org>, Pedro Alves <palves at redhat dot com>, Yao Qi <yao at codesourcery dot com>, nick clifton <nickc at redhat dot com>, Tom Tromey <tromey at redhat dot com>
- Date: Fri, 18 Jan 2013 18:25:03 +0000
- Subject: Re: [RFA 3/5] New port: CR16: gdb port
- References: <507279C7.8080401@codesourcery.com> <C6CA53A2A46BA7469348BDBD663AB65845B39A2F@KCHJEXMB02.kpit.com> <20121022224107.GB3713@adacore.com> <C6CA53A2A46BA7469348BDBD663AB65845B3E44A@KCHJEXMB02.kpit.com> <20121023135502.GA3555@adacore.com> <C6CA53A2A46BA7469348BDBD663AB65845B3EB9E@KCHJEXMB02.kpit.com> <20121115174313.GC3790@adacore.com> <C6CA53A2A46BA7469348BDBD663AB65845B614E5@KCHJEXMB02.kpit.com> <20121122175010.GG9964@adacore.com> <C6CA53A2A46BA7469348BDBD663AB65848567829@KCHJEXMB02.kpit.com> <20130117085919.GA3564@adacore.com>
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