Compiler Bug Report: Read of registers in peripheral address space
|
View:
New views
9 Messages
—
Rating Filter:
Alert me
|
 |
Compiler Bug Report: Read of registers in peripheral address space
by Rohit Pagariya
::
Rate this Message:
I don't if this bug has been already reported, I couldn't find a bug
report. So here goes: The latest version of mspgcc3 (I built it on October 11) has a bug which affects the ADC12 operation. The specification says : The address space from 0100 to 01FFh is reserved for 16-bit peripheral modules. These modules should be accessed with word instructions. If byte instructions are used, only even addresses are permissible, and the high byte of the result is always 0. Thus byte reads at odd addresses are not permissible and the result is unpredictable. For the following ADC related function in TinyOS 2.1, typedef struct __nesc_unnamed4254 { volatile unsigned adc12sc : 1, enc : 1, adc12tovie : 1, adc12ovie : 1, adc12on : 1, refon : 1, r2_5v : 1, msc : 1, sht0 : 4, sht1 : 4; } __attribute((packed)) adc12ctl0_t; volatile unsigned int ADC12CTL0 __asm ("0x01A0"); static inline adc12ctl0_t getCtl0(void ) { return * (adc12ctl0_t *)&ADC12CTL0; } The assembly produced at -O0 for msp430F1611 is <getCtl0>: push r5 push r4 clr r14 mov.b &0x01a0,r15 and.b #-1, r15 ;r3 As==11 and #-256, r14 ;#0xff00 bis r15, r14 mov.b &0x01a1,r15 /* A byte read at an odd address in the peripheral address space is not permitted. */* and.b #-1, r15 ;r3 As==11 swpb r15 and.b #-1, r14 ;r3 As==11 bis r15, r14 mov r14, r15 pop r4 pop r5 ret This bug is present at all levels of optimization. Has anyone else run into this bug as well? Rohit ------------------------------------------------------------------------------ Come build with us! The BlackBerry(R) Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9 - 12, 2009. Register now! http://p.sf.net/sfu/devconference _______________________________________________ Mspgcc-users mailing list Mspgcc-users@... https://lists.sourceforge.net/lists/listinfo/mspgcc-users |
|
|
|
|
|
Re: Compiler Bug Report: Read of registers in peripheral address space
by John Regehr
::
Rate this Message:
Hi,
I'd agree that in the code Rohit sent, it is perhaps not reasonable for teh compiler to know that byte accesses are disallowed. The casting-away-volatile issue is a red herring I think: notice that the struct elements are themselves volatile. What I'd like to call your attention to is that face that the msp430-gcc currently used by TinyOS does the right thing with the test code, producing this output at -Os: getCtl0: mov &0x01A0, r15 ret Pehaps this is a symptom of a more serious regression? John Regehr ------------------------------------------------------------------------------ Come build with us! The BlackBerry(R) Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9 - 12, 2009. Register now! http://p.sf.net/sfu/devconference _______________________________________________ Mspgcc-users mailing list Mspgcc-users@... https://lists.sourceforge.net/lists/listinfo/mspgcc-users |
|
|
Re: Compiler Bug Report: Read of registers in peripheral address space
by John Regehr
::
Rate this Message:
At -Os the older mspgcc also turns this:
> adc12ctl0_t getCtl0(void ) > { > int i = ADC12CTL0; > return * (adc12ctl0_t *)&i; > } into: getCtl0: mov &0x01A0, r15 ret Rather than this: > With optimisation -Os you'll get the smaller, but still unnecessary big > > 212 0000 2183 sub #2, r1 ; 2, fpn 0 > 218 0002 9142 A001 mov &0x01A0, @r1 > 221 0008 6E41 mov.b @r1, r14 > 222 000a 5F41 0100 mov.b 1(r1), r15 > 223 000e 8F10 swpb r15 > 227 0010 0FDE bis r14, r15 > 230 0012 2153 add #2, r1 > 231 0014 3041 ret John Regehr ------------------------------------------------------------------------------ Come build with us! The BlackBerry(R) Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9 - 12, 2009. Register now! http://p.sf.net/sfu/devconference _______________________________________________ Mspgcc-users mailing list Mspgcc-users@... https://lists.sourceforge.net/lists/listinfo/mspgcc-users |
|
|
Re: Compiler Bug Report: Read of registers in peripheral address space
by Bugzilla from handzisk@tkn.tu-berlin.de
::
Rate this Message:
On Wed, Oct 14, 2009 at 13:57, JMGross <mspgcc@...> wrote:
> I'd say it is not a compiler bug but a programmer bug. > > return (adc12ctl0_t) ADC12CTL0; > > should do better. And should be much faster. But fails due to the incompatible type of data (non-scalar struct and scalar word value). This is also the reason why the compiler does not use a word instruction for > constructing the result: the target is not a scalar type. And it has been packed, so it cannot be assumed word-aligned at all. The compiler does not know that in this special case it could just transfer a source word to > a target word-size structure. It just assembles the target, and it does so (unoptimized) field by field, using the source only byte-wise. even after optimisation, when the optimisation algorithm discovers that the bit-wise > assembly is unnecessary, the byte-wise source access still remains, as the optimisation does not know that it could be put together again. It does not even know for sure whether the target is word-aligned. Only the > linker can tell. As John has said, something has changed recently to trigger this. With a mspgcc build from 03/2008 the assembler looks fine: mov &0x01A0, r15 But I agree on the alignment.The adc12ctl0_t in the mspgcc adc12.h header file should be explicitly word aligned like this: typedef struct { volatile unsigned adc12sc:1, enc:1, adc12tovie:1, adc12ovie:1, adc12on:1, refon:1, r2_5v:1, msc:1, sht0:4, sht1:4; volatile unsigned int : 0; } __attribute__ ((packed)) adc12ctl0_t; Independent from this, from the point of view of TinyOS, the access of the ADC registers should be wrapped in the standard DEFINE_UNION_CAST macro just like the timer and the usart drivers #define DEFINE_UNION_CAST(func_name,to_type,from_type) \ to_type func_name(from_type x) @safe() { union {from_type f; to_type t;} c = {f:x}; return c.t; } i.e. async command adc12ctl0_t HplAdc12.getCtl0(){ return *((adc12ctl0_t*) &ADC12CTL0); } should be converted to: DEFINE_UNION_CAST(adc12ctl2int,uint16_t,adc12ctl0_t) async command adc12ctl0_t HplAdc12.getCtl0(){ return adc12ctl2int(ADC12CTL0); } Vlado ------------------------------------------------------------------------------ Come build with us! The BlackBerry(R) Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9 - 12, 2009. Register now! http://p.sf.net/sfu/devconference _______________________________________________ Mspgcc-users mailing list Mspgcc-users@... https://lists.sourceforge.net/lists/listinfo/mspgcc-users |
|
|
|
|
|
Re: Compiler Bug Report: Read of registers in peripheral address space
by John Regehr
::
Rate this Message:
Thanks JM!
John > > ----- Ursprüngliche Nachricht ----- > Von: John Regehr > Gesendet am: 15.Oktober.2009 05:06:27 > > >> I'd agree that in the code Rohit sent, it is perhaps not reasonable for >> teh compiler to know that byte accesses are disallowed. > >> The casting-away-volatile issue is a red herring I think: notice that the >> struct elements are themselves volatile. > > The problem in this case is not that the struct is or is not volatile. The volatile flag applies to the location, not the value itself. So it just changes the way the compiler accesses source and destination location. The > volatile flag in the struct just tell the compiler that there may no read-modify-write being used to set the single bits in the struct. Actually it is superfluous in this case, as it is only of any importance when defining the > address of the hardware register and not for any variables of the same type. > > What I meant is that the compiler should not do any byte-level access if the source is a volatile word. Since the source is defined as volatile, the compiler cannot assume that two byte transfers will give the same > result as one word transfer. So the compiler should in any case make a word transfer, even if into a temporary register. > But by casting away the volatile attribute from the source, it is allowed to do byte level access to the source address, leading into trouble. And it does. Why it wants to, in this special case,is another thing (see your > other posting). > > Anyway, you could do something like > int x=getCtl0().adc12sc > where the low byte of the source wouldn't be necessary at all. > Since the unoptimized code should still do a word access to the source before extravting the MSB, even after optimisation there should be still a word access despite the fact the the low byte is not necessary at all. > > > > I tried some other thing and got the following result: > > volatile adc12ctl0_t ADC12CTL0 __asm ("0x01A0"); > > static inline adc12ctl0_t getCtl0(void) { return ADC12CTL0; } > > volatile adc12ctl0_t adc = {}; > > void test (void){ > adc=getCtl0_x(); > } > > test: > 59:ez3.c **** adc=getCtl0_x(); > 225 0002 3F40 A001 mov #0x01A0, r15 > 226 0006 F14F 0000 mov.b @r15+, @r1 > 227 000a F14F 0100 mov.b @r15+, 1(r1) > 231 000e 0F41 mov r1, r15 > 232 0010 F24F 0000 mov.b @r15+, &adc > 233 0014 F24F 0000 mov.b @r15+, &adc+1 > 234 0006 3041 ret > > which is different but still incorrect. > > removing the packed attribute from the struct gives - surprise - > > test: > 226 0000 9242 A001 mov &0x01A0, &adc > 229 0006 3041 ret > > > >> What I'd like to call your attention to is that face that the msp430-gcc >> currently used by TinyOS does the right thing with the test code, >> producing this output at -Os: > >> getCtl0: >> mov &0x01A0, r15 > > ret > >> Pehaps this is a symptom of a more serious regression? > > Maybe. I'd say that the handling of packed structs has been 'improved', leading to unwanted side-effects. > > I'm nor sure about the 'does the right thing'. It is of course the (in this case) desired result, but is it the right thing? > The struct is defined as being packed. So the compiler cannot (and may not) rely on anything inside the struct as being word-aligned. > In case of a register as destination, of course it is. But since the code is defined inline, it is of no interest what happens in the function body. Therefore I have pasted what the compiler generates inline. > And there is no r15 register for returning a function result. > > Anyway, when looking at the function body, I see the same crappy result as I got for the original code (with -Os): > 539 getCtl0: > 549 0176 5E42 A001 mov.b &0x01A0, r14 > 550 017a 5F42 A101 mov.b &0x01A0+1, r15 > 551 017e 8F10 swpb r15 > 554 0180 0FDE bis r14, r15 > 555 0182 3041 ret > > And it immediately disappears when I remove the packed attribute from the struct. > > Remember that the processor silently ignores the LSB of any word-access address. so the code above would be the right way to access (read) any word value that is probably not word-aligned. And unfortunately the > wrong way for the 16bit I/O area. > > My guess is that there has been a fix for alignment problems, leading to problems with 16 bit I/O space (which probably nobody has thought of before) > Anyway, you must consider that the address of the source is not known to the compiler as it is an ASM assignment and therefore an 'unknown thing', even if it resolves to an absolute address in the assembler stage. > As long as it was an INT type, the compiler could have assumed word alignment. But type has forcibly changed. So since the type of the source now is a packed and therefore probably not aligned struct, the compiler > may not assume that the given source location (which is still just an arbitrary identifier string) will point to an aligned location. Hence the byte-level access. In case of an int, the compiler will trust the programmer that > ints are word-aligned and will do a word access. > > Try the following: change the address to 0x01A1. You'll see that with _your_ compiler version the compiler will generate a word access to 0x01A1 (trusting you that ints are aligned), which leads to unpredictable > results. > > With older compiler versions I got into trouble when putting packed structs with bytes and words with odd size into a byte stream, accessing the structs later with a pointer into the byte stream. Then the ints inside > the structs were not word aligned and the compiler generated faulty word-access code. It seems that in newer versions this has been fixed with the now showing side-effects of fragile code being broken. > I solved the problem by defining every non-byte-sized element of my structure as char[], extracting the values using macros. Maybe this is no longer necessary. But since the macro approach also solved possible > problems with big- and little-endian machines, doing an implicit translation on one side, I'm quite happy with it. > > >> From your other posting: > >> At -Os the older mspgcc also turns this: > >>> adc12ctl0_t getCtl0(void ) >>> { >>> int i = ADC12CTL0; >>> return * (adc12ctl0_t *)&i; >>> } >> >> into: >> >> getCtl0: >> mov &0x01A0, r15 >> ret >> >> Rather than this: > >>> 212 0000 2183 sub #2, r1 ; 2, fpn 0 >>> 218 0002 9142 A001 mov &0x01A0, @r1 >>> 221 0008 6E41 mov.b @r1, r14 >>> 222 000a 5F41 0100 mov.b 1(r1), r15 >>> 223 000e 8F10 swpb r15 >>> 227 0010 0FDE bis r14, r15 >>> 230 0012 2153 add #2, r1 >>> 231 0014 3041 ret > > > Yes, the code looks ugly and unnecessarily bloated, but this is also because the compiler in the first step tries to avoid possible alignment problems and then in the optimization phase isn't smart enough to 'see' that > it could be optimized in this case. > Remember that between line 218 and 221 the storage place has changed its type from an assumed word-aligned int into a packed struct that is not necessarily word-aligned (and could be placed at an uneven stack > address as well), so it is read byte by byte and assembled to the word-sized return value. > > If I remove the packed attribute from the struct, I get the same result as you (or rather, as it is inlined code): > 53:ez3.c **** void test (void){ > 224 0000 9242 A001 mov &0x01A0, &adc > 224 0000 > 54:ez3.c **** adc=getCtl0(); > 55:ez3.c **** } > 227 0006 3041 ret > > > So on the bottom line three advices remain: > > Don't add a packed qualifier where it isn't necessary, as it forces the compiler to use byte-access to avoid alignment problems (similar for volatile, as it prevents much optimization) > and > Don't do typecasting that removes and adds attributes or qualifiers from and to pointers, unless you know of the side-effects. > and > Don't take compiler warnings lightly. Discover where they come from and what problems they might indicate. And then change the code to remove them. > > In most cases this only leads to non-optimal code (even with optimization). > But in this special case it has lead to trouble. And I really see no reason why there is a volatile and packed in the struct. > > JMGross > > > ------------------------------------------------------------------------------ > Come build with us! The BlackBerry(R) Developer Conference in SF, CA > is the only developer event you need to attend this year. Jumpstart your > developing skills, take BlackBerry mobile applications to market and stay > ahead of the curve. Join us from November 9 - 12, 2009. Register now! > http://p.sf.net/sfu/devconference > _______________________________________________ > Mspgcc-users mailing list > Mspgcc-users@... > https://lists.sourceforge.net/lists/listinfo/mspgcc-users > Come build with us! The BlackBerry(R) Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9 - 12, 2009. Register now! http://p.sf.net/sfu/devconference _______________________________________________ Mspgcc-users mailing list Mspgcc-users@... https://lists.sourceforge.net/lists/listinfo/mspgcc-users |
|
|
Re: Compiler Bug Report: Read of registers in peripheral address space
by John Regehr
::
Rate this Message:
> #define DEFINE_UNION_CAST(func_name,to_type,from_type) \
> to_type func_name(from_type x) @safe() { union {from_type f; to_type > t;} c = {f:x}; return c.t; } > > DEFINE_UNION_CAST(adc12ctl2int,uint16_t,adc12ctl0_t) > > async command adc12ctl0_t HplAdc12.getCtl0(){ > return adc12ctl2int(ADC12CTL0); > } This gives the desired asm with the latest version of mspgcc. John ------------------------------------------------------------------------------ Come build with us! The BlackBerry(R) Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9 - 12, 2009. Register now! http://p.sf.net/sfu/devconference _______________________________________________ Mspgcc-users mailing list Mspgcc-users@... https://lists.sourceforge.net/lists/listinfo/mspgcc-users |
|
|
Re: Compiler Bug Report: Read of registers in peripheral address space
by Rohit Pagariya
::
Rate this Message:
John Regehr wrote:
>> #define DEFINE_UNION_CAST(func_name,to_type,from_type) \ >> to_type func_name(from_type x) @safe() { union {from_type f; to_type >> t;} c = {f:x}; return c.t; } >> >> DEFINE_UNION_CAST(adc12ctl2int,uint16_t,adc12ctl0_t) >> >> async command adc12ctl0_t HplAdc12.getCtl0(){ >> return adc12ctl2int(ADC12CTL0); >> } >> > > This gives the desired asm with the latest version of mspgcc. > > John > Rohit ------------------------------------------------------------------------------ Come build with us! The BlackBerry(R) Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9 - 12, 2009. Register now! http://p.sf.net/sfu/devconference _______________________________________________ Mspgcc-users mailing list Mspgcc-users@... https://lists.sourceforge.net/lists/listinfo/mspgcc-users |
| Free embeddable forum powered by Nabble | Forum Help |
