This is the mail archive of the ecos-patches@sourceware.org mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Fixed some bugs in hal_io.h


On Tue, 2006-12-19 at 02:55 +0000, Jonathan Larmour wrote:
...
> > It also adds memory string macros,
> 
> I'm a bit hesitant about these, as they aren't part of the HAL API, which 
> could be detrimental to code reuse. But I'll let them pass as they're 
> entirely novel and no-one can fail to notice if their HAL doesn't have them.

Ok, I see the compatibility issue. I saw the need for those when I
realize that there weren't memory copy functions that take into account
the volatile nature of some buffers, as HAL_MEMWRITE_UINTx and
HAL_MEMREAD_UINTx do.

Could be interesting to plan the additions of that kind of APIs to the
other HALs.

> But those aren't the only changes:
> 
> >  #define HAL_READ_UINT8( _register_, _value_ )   \
> > -CYG_MACRO_START                                 \
> > -{                                               \
> > -    asm volatile ( "xor %%eax,%%eax ;"          \
> > -                   "inb %%dx, %%al"             \
> > +({                                              \
> > +    asm volatile(                               \
> > +    "       xor     %%eax,%%eax     \n"         \
> > +    "       inb     %%dx, %%al      \n"         \
> >                     : "=a" (_value_)             \
> >                     :  "d"(_register_)           \
> >          );                                      \
> > -}                                               \
> > -CYG_MACRO_END
> > +    (_value_);                                  \
> > +})
> 
> You make this an expression which can be an rvalue. This is outside the 
> HAL spec, and would mean that code that might initially seem usable by 
> non-i386 packages, would not be. It does not really add anything. I would 
> be against this change and the others like it.
> 

Ok, I see againg the compatibility issue with other platfoirm HALs.

I made this change, because it is more efficient to write
read/modify/write operations if you can use rvalues ending in registers
than if you are forced to use some automatic or global variable.

It also seemd appropriate because the HAL_MEMxxx_UINTx functions behave
as rvalues as well.

Do you think it could be interesting to add that kind of behaviour for
the next versions of the HAL?

> >  #define HAL_READ_UINT8_STRING( _register_, _buf_, _count_)      \
> >  CYG_MACRO_START                                                 \
> > -    asm volatile ( "insb"                                       \
> > -                   :                                            \
> > -                   : "c" (_count_), "d"(_register_), "D"(_buf_) \
> > +    typedef volatile struct { CYG_BYTE m[_count_]; } *mp;   \
> > +    asm volatile(                                           \
> > +    "       rep insb                \n"                     \
> > +    :  "=m" (*(mp) (_buf_))                                 \
> > +    :   "c" (_count_),                                      \
> > +        "d" (_register_),                                   \
> > +        "D" (_buf_)                                         \
> >          );                                                      \
> >  CYG_MACRO_END
> 
> Here you have added an output constraint for the _buf_ array. Won't this 
> make GCC allocate an extra unused register? I'm not sure what it is 
> intended to achieve either. Similarly in other _STRING functions.
> 

This is the way the gcc manual (C Extensions, Extended Asm) describes
"known" memory areas being modified (either as input, output, or
clobbered). I'm not sure if gcc internally reserves unused any register
if the constrain appears as output, but I suspect that it only actually
uses a register if you reference the operand in the assembly language
(with %<n>).

> A ChangeLog entry would also be appreciated, thanks.
> 

I'll redo the patch leaving out the rvalue things.

Let me know what you think about the other issues.

David.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]