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