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

[Georg-Johann Lay]

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)),


There are magic values like 0xd8, 0x8, 0x7.  Can't these be represented by 
macros from io.h?

Named asm operands are easier to grasp, but they use / expose identifiers which 
are in the namespace of the application, not in the namespace of the 
implementation.  For example, the code will raise strange errors if

#include <avr/wdh.t>

#define tmp
#define temp
#define SIGNATURE 0x1234

void nofun (void)
{
    wdt_enable (0);
}


Johann