Re: [avr-libc-dev] [bug #43828] wdt.h: Wrong inline assembler arguments

[Sivanupandi, Pitchumani]

> -----Original Message-----
> From: Georg-Johann Lay [mailto:address@hidden
> Sent: 18 June 2015 23:46
> To: Sivanupandi, Pitchumani
> Cc: address@hidden
> Subject: Re: [avr-libc-dev] [bug #43828] wdt.h: Wrong inline assembler
> arguments
> 
> Am 06/17/2015 um 12:02 PM schrieb Sivanupandi, Pitchumani:
> >>> All the asms are changing memory but don't mention that.  Usually it
> >>> is not wanted that (non-volatile) memory accesses are dragged across
> >>> the inline asm.
> >>> Easy fix is to clobber memory, more exact is to explicitly describe
> >>> the effect on memory.  For example, one sequence reads:
> >>>
> >>>     "sts %0, %2"
> >>>     : /* no outputs */
> >>>     : "M" (_SFR_MEM_ADDR(_WD_CONTROL_REG)),
> >>>
> >>> Explicit memory modelling would read something like:
> >>>
> >>>     "sts %1, %3"
> >>>     : "+m" (_WD_CONTROL_REG)
> >>>     : "n" (_SFR_MEM_ADDR(_WD_CONTROL_REG)),
> >>
> >> I am ok with your changes.
> >>
> >> In addition, I would like to add below change (wdt_enable for
> >> __AVR_TINY__) to avoid overwriting unintended bits.
> >
> > Below are my findings in further evaluation of these changes:
> >
> > Clobbering memory lead to un-optimal code when there is any memory
> > reference before and after wdt_enable/disable call.
> >
> > Explicit memory modelling may safeguard the memory access across the
> inline asm.
> > But, even without that explicit clobber (in wdt enable/disable case,
> > it will be _WD_CONTROL_REG), compiler safely reloads the content
> > (instead of re-use the register that is loaded already). This is because the
> _WD_CONTROL_REG will be expanded as the volatile access.
> >
> > For the following case, compiler may not safeguard the memory access
> > as the WDT address is referred directly (without the macro, also not
> qualified as volatile).
> >
> > *(char*)0x60 = c1;
> > wdt_enable(3);
> > c2 = *(char*)0x60;
> >
> > IMHO, this usage is not common in embedded applications. So,
> > clobbering memory in wdt_enable/disable macros may not be required as
> it leads to un-optimal code.
> 
> In general it's not wanted that respective asm is moved around, and one way
> of reducing the chance of motion is a memory barrier.  Except in the case
> where performance is absolutely crucial a generic memory clobber is fine,
> IMO.

Ok.
 
> My observation is that users are less comfortable with unexpected code
> motion than with rare and minor optimization loss.
> 
> But that's a matter of taste, of course...

Joerg, 
what do you think about this memory clobber in wdt_enable/disable?

Regards,
Pitchumani