[ell-i-developers] About type correctness

  • From: Pekka Nikander <pekka.nikander@xxxxxx>
  • To: Ivan Raul <supra.material@xxxxxxxxx>
  • Date: Sat, 15 Feb 2014 20:14:59 +0200

> First, a question about 'quality' or 'correctness' about the code.
> 
> The implementation of ChibiOS uses this typedef to refer to registers:
> 
> typedef void *regarm_t;

I think this is error prone.  It may cause problems if the code is ever used 
with some other processor, or even with a 64-bit ARM.  I'd prefer to use 
uint32_t for that kind of a need.  But that is largely a matter of taste, if 
the only thing one ever does is either storing or retrieving data, not 
computing.  In such a context C is notoriously ugly as it does not provide a 
completely opaque (but sized) data type for that.
 
> And then simply generates structs with them:
> 
> struct extctx {
>   regarm_t      r0;
> ...
> };
> 
> It turns out to be quite handy, in particular when it is necessary to point 
> to functions, i.e.:
> 
> struct extctx *ctxp;
> ctxp->pc = (void *)_port_switch_from_isr;

I don't see how void pointer is any handier here than uint32_t.  In both cases 
you need a typecast.

> Do you consider it 'correct', or do you know any other way that is more 
> adequate, or maybe more correct, as registers are uint32_t. I know from you 
> typecasting is not the best alternative unless is ultimately required.

As I wrote above, uint32_t is slightly better.  It is more customary, i.e. used 
more often in existing source code.

If you want to try to do better, I guess you could try using void (*)(), but 
even that without typecasts is likely to generate warnings from the C++ 
compiler.  If you want to do it "completely right", you should use an unnamed 
union of all the kinds of function pointers that you want to explicitly assign 
to the PC.  Something like the following:

typedef void (*interrupt_handler)(void) __attribute__((naked));

union {
  interrupt_handler _pc_interrupt_handler;
  regarm_t _pc_reg;
} pc;

That would make it explicit the various ways you may use in the code the 
memory-stored PC-register image, and would not require any typecasts.

> IIRC for the emulator you overload uint32_t to generate the output, or 
> something similar.

Not yet, but that's the plan.  Each uint32_t object would know its address, and 
also if it is a part of a peripheral, or what kind of memory it is.

> Also I'm putting close attention at the alignment to 8 bytes that the stack 
> should have when entering interrupts, to prevent any problem at later stages.

Please a reference to the documentation?  I don't remember such a requirement, 
but most probably my memory fails here.  I thought 4 byte alignment would be 
enough.

--Pekka


Other related posts: