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

[Sivanupandi, Pitchumani]

> -----Original Message-----
> From: address@hidden
> [mailto:address@hidden On
> Behalf Of Sivanupandi, Pitchumani
> Sent: 21 May 2015 16:26
> To: Georg-Johann Lay
> Cc: address@hidden
> Subject: Re: [avr-libc-dev] [bug #43828] wdt.h: Wrong inline assembler
> arguments
> 
> > -----Original Message-----
> > From: Georg-Johann Lay [mailto:address@hidden
> > Sent: Thursday, April 30, 2015 7:04 PM
> > To: Sivanupandi, Pitchumani
> > Cc: address@hidden
> > Subject: Re: [bug #43828] wdt.h: Wrong inline assembler arguments
> >
> > Am 04/30/2015 um 12:09 PM schrieb Sivanupandi, Pitchumani:
> > >> -----Original Message-----
> > >> From: Georg-Johann Lay [mailto:address@hidden
> > >> Sent: Wednesday, April 29, 2015 3:11 PM
> > >> To: Sivanupandi, Pitchumani
> > >> Cc: address@hidden
> > >> Subject: Re: [bug #43828] wdt.h: Wrong inline assembler arguments
> > >>
> > >> Am 04/28/2015 um 12:53 PM schrieb Sivanupandi, Pitchumani:
> > >>> Hi Johann,
> > >>>
> > >>> ...
> > >>>> Below is the patch to fix this issue.
> > >>>>
> > >>>> diff --git a/avr-libc/include/avr/wdt.h
> > >>>> b/avr-libc/include/avr/wdt.h index [...]
> > >>>
> > >>> Is this change Ok?
> > >>
> > >> That patch is malformed, presumably because of missing / spurious
> > >> spaces or new lines.
> > >
> > > Attached the patch.
> >
> > Several notes:
> >
> > Some asm use ORI with operand of constraint r, e.g. temp or temp_reg
> > in wdt_disable().  Correct constraint is d (or a subset thereof).
> >
> > Some flag values are using constraint "I" which is 0 <= * <= 63.
> > Dunno whether there are devices that have flags located in bit 6 or 7.
> > Proposed constraint is "n", "I" doesn't have any benefit here.
> >
> > Some memory (non-I/O) addresses use "M" as constraint which is 0 <= *
> > <= 0xff.
> >   Dunno whether there are devices that have SFRs with memory addresses
> > outside that range.  LDS and STS are fine with "i" (known at link
> > time) or even "n"
> > (known at compile time).  "M" has no advantage here.
> >
> > 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.

Regards,
Pitchumani