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

[Georg-Johann Lay]

URL:
<http://savannah.nongnu.org/bugs/?43828>

Summary: wdt.h: Wrong inline assembler arguments
Project: AVR C Runtime Library
Submitted by: gjlayde
Submitted on: Mo 15 Dez 2014 19:25:34 GMT
Category: Header
Severity: 3 - Normal
Priority: 5 - Normal
Item Group: Header files
Status: None
Percent Complete: 0%
Assigned to: None
Originator Email:
Open/Closed: Open
Discussion Lock: Any
Release: 1.8.0
Fixed Release: None

_______________________________________________________

Details:

wdt.h comes with wrong inline assembler arguments. Let me give an example
(2014-12-15 SVN trunk r2641):



#define wdt_enable(timeout) \
do { \
uint8_t temp = 0; \
__asm__ __volatile__ ( \
"in __tmp_reg__, %[rampd]" "\n\t" \
"out %[rampd], __zero_reg__" "\n\t" \
"out %[ccp_reg], %[ioreg_cen_mask]" "\n\t" \
"sts %[wdt_reg], %[wdt_enable_timeout]" "\n\t" \
"1:lds %[tmp], %[wdt_status_reg]" "\n\t" \
"sbrc %[tmp], %[wdt_syncbusy_bit]" "\n\t" \
"rjmp 1b" "\n\t" \
"out %[rampd], __tmp_reg__" "\n\t" \
: "=r" (temp) \
: [rampd] "M" (_SFR_MEM_ADDR(RAMPD)), \
[ccp_reg] "I" (_SFR_MEM_ADDR(CCP)), \
[ioreg_cen_mask] "r" ((uint8_t)CCP_IOREG_gc), \
[wdt_reg] "M" (_SFR_MEM_ADDR(WDT_CTRL)), \
[wdt_enable_timeout] "r" ((uint8_t)(WDT_CEN_bm | WDT_ENABLE_bm |
timeout)), \
[wdt_status_reg] "M" (_SFR_MEM_ADDR(WDT_STATUS)), \
[wdt_syncbusy_bit] "I" (WDT_SYNCBUSY_bm), \
[tmp] "r" (temp) \
: "r0" \
); \
} while(0)


Operand %[tmp] (%8) is changed but not indicated as so by means of constraint
modifiers.

Notice that the compiler may allocate different registers to %0 and %8.
Hence, the fix is to refer to %0 instead of to %[tmp] resp. moving %[tmp] to
the first operand. Also notice that %8 is unused and can be dropped. temp is
loaded with 0 but respective content of %[tmp] is discarded by LDS.

The other asm in wdt.h (or maybe even more headers or source files) might
suffer from the same or similar flaws.





_______________________________________________________

Reply to this item at:

<http://savannah.nongnu.org/bugs/?43828>

_______________________________________________
Nachricht gesendet von/durch Savannah
http://savannah.nongnu.org/

[Pitchumani]

Update of bug #43828 (project avr-libc):

Status: None => In Progress
Assigned to: None => pitchumani

_______________________________________________________

Follow-up Comment #1:

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 fc15885..3130e67 100644
--- a/avr-libc/include/avr/wdt.h
+++ b/avr-libc/include/avr/wdt.h
@@ -148,25 +148,24 @@
*/
#define wdt_enable(timeout) \
do { \
-uint8_t temp = 0; \
+uint8_t temp; \
__asm__ __volatile__ ( \
"in __tmp_reg__, %[rampd]" "\n\t" \
"out %[rampd], __zero_reg__" "\n\t" \
"out %[ccp_reg], %[ioreg_cen_mask]" "\n\t" \
"sts %[wdt_reg], %[wdt_enable_timeout]" "\n\t" \
"1:lds %[tmp], %[wdt_status_reg]" "\n\t" \
- "sbrc %[tmp], %[wdt_syncbusy_bit]" "\n\t" \
+ "sbrc %[tmp], %[wdt_syncbusy_bit]" "\n\t" \
"rjmp 1b" "\n\t" \
"out %[rampd], __tmp_reg__" "\n\t" \
- : "=r" (temp) \
+ : [tmp] "=r" (temp) \
: [rampd] "M" (_SFR_MEM_ADDR(RAMPD)), \
[ccp_reg] "I" (_SFR_MEM_ADDR(CCP)), \
[ioreg_cen_mask] "r" ((uint8_t)CCP_IOREG_gc), \
[wdt_reg] "M" (_SFR_MEM_ADDR(WDT_CTRL)), \
[wdt_enable_timeout] "r" ((uint8_t)(WDT_CEN_bm | WDT_ENABLE_bm |
timeout)), \
[wdt_status_reg] "M" (_SFR_MEM_ADDR(WDT_STATUS)), \
- [wdt_syncbusy_bit] "I" (WDT_SYNCBUSY_bm), \
- [tmp] "r" (temp) \
+ [wdt_syncbusy_bit] "I" (WDT_SYNCBUSY_bm) \
: "r0" \
); \
} while(0)

_______________________________________________________

Reply to this item at:

<http://savannah.nongnu.org/bugs/?43828>

_______________________________________________
Message sent via/by Savannah
http://savannah.nongnu.org/

[Sivanupandi, Pitchumani]

Hi Johann,

-----Original Message-----
Sent: Wednesday, December 24, 2014 12:10 PM
Subject: [bug #43828] wdt.h: Wrong inline assembler arguments

...

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
fc15885..3130e67 100644
--- a/avr-libc/include/avr/wdt.h
+++ b/avr-libc/include/avr/wdt.h
@@ -148,25 +148,24 @@
*/
#define wdt_enable(timeout) \
do { \
-uint8_t temp = 0; \
+uint8_t temp; \
__asm__ __volatile__ ( \
"in __tmp_reg__, %[rampd]" "\n\t" \
"out %[rampd], __zero_reg__" "\n\t" \
"out %[ccp_reg], %[ioreg_cen_mask]" "\n\t" \
"sts %[wdt_reg], %[wdt_enable_timeout]" "\n\t" \
"1:lds %[tmp], %[wdt_status_reg]" "\n\t" \
- "sbrc %[tmp], %[wdt_syncbusy_bit]" "\n\t" \
+ "sbrc %[tmp], %[wdt_syncbusy_bit]" "\n\t" \
"rjmp 1b" "\n\t" \
"out %[rampd], __tmp_reg__" "\n\t" \
- : "=r" (temp) \
+ : [tmp] "=r" (temp) \
: [rampd] "M" (_SFR_MEM_ADDR(RAMPD)), \
[ccp_reg] "I" (_SFR_MEM_ADDR(CCP)), \
[ioreg_cen_mask] "r" ((uint8_t)CCP_IOREG_gc), \
[wdt_reg] "M" (_SFR_MEM_ADDR(WDT_CTRL)), \
[wdt_enable_timeout] "r" ((uint8_t)(WDT_CEN_bm | WDT_ENABLE_bm |
timeout)), \
[wdt_status_reg] "M" (_SFR_MEM_ADDR(WDT_STATUS)), \
- [wdt_syncbusy_bit] "I" (WDT_SYNCBUSY_bm), \
- [tmp] "r" (temp) \
+ [wdt_syncbusy_bit] "I" (WDT_SYNCBUSY_bm) \
: "r0" \
); \
} while(0)

Is this change Ok?

Regards,
Pitchumani

[Georg-Johann Lay]

Post by Sivanupandi, Pitchumani
Hi Johann,
...

Post by Pitchumani
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.

Johann

[Sivanupandi, Pitchumani]

-----Original Message-----
Sent: Wednesday, April 29, 2015 3:11 PM
To: Sivanupandi, Pitchumani
Subject: Re: [bug #43828] wdt.h: Wrong inline assembler arguments

Post by Sivanupandi, Pitchumani
Hi Johann,
...

Post by Pitchumani
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.

Regards,
Pitchumani

[Georg-Johann Lay]

Post by Sivanupandi, Pitchumani

-----Original Message-----
Sent: Wednesday, April 29, 2015 3:11 PM
To: Sivanupandi, Pitchumani
Subject: Re: [bug #43828] wdt.h: Wrong inline assembler arguments

Post by Sivanupandi, Pitchumani
Hi Johann,
...

Post by Pitchumani
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

[Sivanupandi, Pitchumani]

-----Original Message-----
Sent: Thursday, April 30, 2015 7:04 PM
To: Sivanupandi, Pitchumani
Subject: Re: [bug #43828] wdt.h: Wrong inline assembler arguments

Post by Sivanupandi, Pitchumani

-----Original Message-----
Sent: Wednesday, April 29, 2015 3:11 PM
To: Sivanupandi, Pitchumani
Subject: Re: [bug #43828] wdt.h: Wrong inline assembler arguments

Post by Sivanupandi, Pitchumani
Hi Johann,
...

Post by Pitchumani
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.

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
"sts %0, %2"
: /* no outputs */
: "M" (_SFR_MEM_ADDR(_WD_CONTROL_REG)),
"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.

@@ -202,8 +201,8 @@
[SIGNATURE] "r" ((uint8_t)0xD8), \
[WDTREG] "I" (_SFR_IO_ADDR(_WD_CONTROL_REG)), \
[WDVALUE] "r" ((uint8_t)((value & 0x08 ? _WD_PS3_MASK : 0x00) \
- | _BV(WDE) | value)) \
- : "r16" \
+ | _BV(WDE) | (value & 0x07) )) \
+ : "r16", "memory" \
)

#define wdt_disable() \

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

Signature (0xD8) is not defined in device headers. 0x8 and 0x7 are bit value and mask.

I'll commit your patch if no other comments from anyone.

Thanks.

Regards,
Pitchumani

[Sivanupandi, Pitchumani]

-----Original Message-----
Behalf Of Sivanupandi, Pitchumani
Sent: 21 May 2015 16:26
To: Georg-Johann Lay
Subject: Re: [avr-libc-dev] [bug #43828] wdt.h: Wrong inline assembler
arguments

-----Original Message-----
Sent: Thursday, April 30, 2015 7:04 PM
To: Sivanupandi, Pitchumani
Subject: Re: [bug #43828] wdt.h: Wrong inline assembler arguments

Post by Sivanupandi, Pitchumani

-----Original Message-----
Sent: Wednesday, April 29, 2015 3:11 PM
To: Sivanupandi, Pitchumani
Subject: Re: [bug #43828] wdt.h: Wrong inline assembler arguments

Post by Sivanupandi, Pitchumani
Hi Johann,
...

Post by Pitchumani
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.

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
"sts %0, %2"
: /* no outputs */
: "M" (_SFR_MEM_ADDR(_WD_CONTROL_REG)),
"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

[Georg-Johann Lay]

Post by Sivanupandi, Pitchumani

Post by Sivanupandi, Pitchumani

Post by Georg-Johann Lay
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
"sts %0, %2"
: /* no outputs */
: "M" (_SFR_MEM_ADDR(_WD_CONTROL_REG)),
"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.

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.

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...

Johann

[Bob Paddock]

Post by Georg-Johann Lay
My observation is that users are less comfortable with unexpected code motion

Yes.

Post by Georg-Johann Lay
than with rare and minor optimization loss.

Generated code should always be that of 'least surprise'.

Post by Georg-Johann Lay
But that's a matter of taste, of course...

No, it is a mater of liability and traceability.

[Markus Hitter]

Post by Bob Paddock

Post by Georg-Johann Lay
But that's a matter of taste, of course...

No, it is a mater of liability and traceability.

In my projects I couldn't care less wether the generated assembly is traceable.

Anyways, that's why there are so many optimisation parameters. Some prefer sloppy coding (e.g. forgetting a "volatile"), others prefer optimisation to the last clock cycle. Generated code should always show correct behaviour, of course.


Markus

[Sivanupandi, Pitchumani]

-----Original Message-----
Sent: 18 June 2015 23:46
To: Sivanupandi, Pitchumani
Subject: Re: [avr-libc-dev] [bug #43828] wdt.h: Wrong inline assembler
arguments

Post by Sivanupandi, Pitchumani

Post by Sivanupandi, Pitchumani

Post by Georg-Johann Lay
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
"sts %0, %2"
: /* no outputs */
: "M" (_SFR_MEM_ADDR(_WD_CONTROL_REG)),
"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.

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.

Post by Sivanupandi, Pitchumani
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.

Post by Sivanupandi, Pitchumani
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).

Post by Sivanupandi, Pitchumani
*(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

[Georg-Johann Lay]

Additional Item Attachment, bug #43828 (project avr-libc):

File name: wdt-h.diff Size:8 KB


_______________________________________________________

Reply to this item at:

<http://savannah.nongnu.org/bugs/?43828>

_______________________________________________
Nachricht gesendet von/durch Savannah
http://savannah.nongnu.org/

[Pitchumani]

Update of bug #43828 (project avr-libc):

Status: In Progress => Fixed
Open/Closed: Open => Closed

_______________________________________________________

Follow-up Comment #2:

Committed.
http://svn.savannah.nongnu.org/viewvc?view=rev&root=avr-libc&revision=2476

Discussions:
http://lists.nongnu.org/archive/html/avr-libc-dev/2015-04/msg00025.html
http://lists.nongnu.org/archive/html/avr-libc-dev/2015-06/msg00002.html

_______________________________________________________

Reply to this item at:

<http://savannah.nongnu.org/bugs/?43828>

_______________________________________________
Message sent via/by Savannah
http://savannah.nongnu.org/