a bizarre avr-gcc behaviour

Go To Last Post
21 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Even describe it is lengthy... I was hunting it down half the day.

In a main() running some 2k lines (don't frown, it's sort of generated) with lots of local variables (neatly enclosed in blocks), one place goes like:

  typedef struct __attribute__((__packed__)) {
    uint16_t outVector;
    uint16_t time; 
    uint8_t x;   
  } T_something;

// we don't need x
  #define SIZEOF_T_something_copy (sizeof(T_something) - 1)  

{    
  union {
      T_something a;
      uint8_t b[0];
    } buf;
    uint8_t i;
    for (i = 0; i < SIZEOF_T_something_copy; i++) {
      buf.b[i] = PopByteFromCommBuffer();
    }
[...]
  if (buf.a.time > someTime) {  // only use of buf.a.time
    [...]
  }
}

The problem is in b[0] in the typedef, intended as a placeholder I use routinely. Now we had that discussion on how unions should not be used to convert types, but I said back then that I like to use them so and I routinely do and never had any problem (yes, these are exactly the words for which I slap on hands by a keyboard). (Those who'd suggest b[1]: the problematic behaviour remains the same).

The compiler decides that there is no way how I can write to buf.a.time, as the b[] array does not reach that far. So, it fills a local variable (in stack frame - note the stack frame is around 600 bytes so it cannot reach all of it through Y+d and has to calculate the address) just before the main loop starts, basically by garbage:

 6582 1c18 2B89      		ldd r18,Y+19
 6583 1c1a 3C89      		ldd r19,Y+20
 6585 1c1c C85D      		subi r28,lo8(-552)
 6586 1c1e DD4F      		sbci r29,hi8(-552)
 6587 1c20 3983      		std Y+1,r19
 6588 1c22 2883      		st Y,r18
 6589 1c24 C852      		subi r28,lo8(552)
 6590 1c26 D240      		sbci r29,hi8(552)

uses a different place in stack frame for buf.b (Y+18 and on):

 16672 4624 8E01      		movw r16,r28
 16675 4626 0F5E      		subi r16,lo8(-(17))
 16676 4628 1F4F      		sbci r17,hi8(-(17))
 16677               	.L1090:
 16692 4638 0E94 0000 		call PopByteFromCommBuffer
 16696 463c D801      		movw r26,r16
 16698 463e 8D93      		st X+,r24
 16700 4640 8D01      		movw r16,r26

 16703 4642 CA5C      		subi r28,lo8(-566)
 16704 4644 DD4F      		sbci r29,hi8(-566)
 16705 4646 E881      		ld r30,Y
 16706 4648 F981      		ldd r31,Y+1
 16707 464a C653      		subi r28,lo8(566)
 16708 464c D240      		sbci r29,hi8(566)

 16710 464e AE17      		cp r26,r30
 16711 4650 BF07      		cpc r27,r31

 16713 4652 01F4      		brne .L1090

[at Y+566, there is a pre-calculated end pointer for this loop, another "optimisation feature"],
and then at the place, where buf.a.time is used, it merrily picks it from the stack frame (where it is never changed):

 16774 469e C85D      		subi r28,lo8(-552)
 16775 46a0 DD4F      		sbci r29,hi8(-552)
 16776 46a2 2881      		ld r18,Y
 16777 46a4 3981      		ldd r19,Y+1
 16778 46a6 C852      		subi r28,lo8(552)
 16779 46a8 D240      		sbci r29,hi8(552)
 16781 46aa 8217      		cp r24,r18
 16782 46ac 9307      		cpc r25,r19
 16784 46ae 00F4      		brsh .L1098

Declaring the byte array in typedef as b[sizeof(T_something)] solves the "problem".

 16755 468c 2B89      		ldd r18,Y+19
 16756 468e 3C89      		ldd r19,Y+20
 16758 4690 838D      		ldd r24,Z+27
 16759 4692 948D      		ldd r25,Z+28
 16761 4694 8217      		cp r24,r18
 16762 4696 9307      		cpc r25,r19
 16764 4698 00F4      		brsh .L1097

JW

Last Edited: Wed. Jun 2, 2010 - 07:53 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Jan,

I know that you do not like C, but it is a little naughty deliberately making it unreadable. I know that you are more than 15 years old, so I cannot believe that you could possibly read that font size either!

No. I just skimmed your post. But if you just want to do a copy of part of your structure, it is easy.

  typedef struct __attribute__((__packed__)) {
    uint16_t outVector;
    uint16_t time;
    uint8_t x;   
  } T_something;

...

  memcpy(dest, &T_something_variable, SIZEOF_T_something_copy);

Now I see that you are actually popping from a stack rather than a forward copy. But you can write the equivalent of a memcpy() quite easily. Its formal parameters are void* and inside the function you cast them to uint8_t* to perform your task.

The messy way is to have your function take uint8_t* formal parameters. And you cast on each invocation.

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

david.prentice wrote:
David,
[Re font size]
Do you really believe my C hatred would demonstrate itself so lowly? ;-)

Is it better now?

David wrote:
Now I see that you are actually popping from a stack rather than a forward copy. But you can write the equivalent of a memcpy() quite easily. Its formal parameters are void* and inside the function you cast them to uint8_t* to perform your task.

Here, it's enough to
uint8_t *p = &T_something_variable;
for (blah; blah; blah) {
  *p = FunctionReturningByte();
  p++;
}

(please note how my C-hatred made me to avoid *p++).

However, I grew to like the "union" way. As the Derek Jones' C book says, both are equivalently bad ( https://www.avrfreaks.net/index.p... ) ... ;-)

JW

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
The problem is in b[0] in the typedef
Yes, it is, uint8_t b[0] is just silly. Why not:

  union {
      T_something a;
      uint8_t b[sizeof(T_something)];
    } buf;

Regards,
Steve A.

The Board helps those that help themselves.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Steve,

yes - see the end of my original post.
It's OK in this particular case, but as a general methodology it is cumbersome. Consider

union {
  type1 t1;
  type2 t2;
  type3 t3;
  [...]
  uint8_t b[what?]
}

JW

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Wouldn't [what?] be the size of the largest of the constituent items?

I've never gone to that level of indirection that you are, wek. I figger I just "know" the size of the constituent items, and size it for that.

Lee

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Do you get the same behavior if left empty?

uint8_t b[];
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I have used an empty array for a variable size structure in the past.

uint8_t b[0];

But most modern compilers do not like it. There is probably an official C99 opinion, but there are neater ways of achieving the same thing.

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Is something like this what is desired?

typedef ... Good;
union AFLCIO {
    Good glinda;
    unit8_t www[sizeof(Good)];
} ;

Iluvatar is the better part of Valar.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The "legal" status is:

  • b[0] is illegal as per C99:
    C99 6.7.5.2 #1 explicitly wrote:
    If the expression [inside []] is a constant expression , it shall have a value greater than zero.
    However, this is an extension in GCC, so it does not warn (it probably would if I would use --std-c99 or similar).
  • b[] would be a nice way to tell "an array as big as the union is"; this is known to work under certain gcc variant/invocation, but it does not work on my particular setup (throws an error). However, b[] as member of union is illegal as per C99, which in 6.7.2.1 #2 allows incomplete arrays only as the last member of struct (which has to have at least one more complete member) and not union.
  • accessing struct members through union/array of uint8_t[sizeof(struct)], although commonly used, is "legally" broken in the same way as accessing them through pointer cast to uint8_t*. Both suffer from the implementation-defined behaviour (alignment&padding, endianness, other perversions). The only 100% kosher way is the tedious method explicitly filling the struct members, e.g.
      typedef struct __attribute__((__packed__)) {
        uint16_t outVector;
        uint16_t time;
        uint8_t x;   
      } T_something;
    
      T_something a;
      
      a.outVector = PopByteFromCommBuffer();
      a.outVector |= PopByteFromCommBuffer() << 8;
      [etc]
    

JW

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

David wrote:

But most modern compilers do not like it. There is probably an official C99 opinion, but there are neater ways of achieving the same thing.

I don't think the original question was about looking for a better way as much as it was why the output behavior using the current method.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Jan,

Of course the legal way of assigning to a structure member is to use its correct type etc. And you copy each member with a separate assignment. This copes with inter platform copies.

But you can do what you like internal to your program. The most common of which is a straightforward assignment of a whole struct with another struct of the same type.

Personally, I would write a function that has void* formal parameters. You write initially as an external function with volatile locals so you can debug it. Then change it to 'inline'.

I would guess that avr-gcc would generate efficient code. And most importantly your invocations are neat and tidy.

Judging by the names in your examples, I am guessing that you are in fact serialising to / from an external source. So I would definitely use the kosher method.

OTOH, if you generated the serialised o/p and you know it is coming back in exactly the same serialised format. By all means treat it as an internal struct. i.e. memcpy().

Is your CommBuffer a LIFO stack or a regular FIFO buffer? Your 'Pop' naming implies a stack.

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

More funny stuff.

I've restored the b[0] in the union declaration, and replaced

buf.b[i] = PopByteFromCommBuffer();

by

(*((buf.b) + (i))) = PopByteFromCommBuffer();

which is equivalent as per 6.5.2.1 #2 (that's why the excessive bracketing).

The compiler is apparently frightened of the pointer more than of an indexed array, so it starts to compile in the expected way... ;-)

JW

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Terminology time.

david.prentice wrote:
Is your CommBuffer a LIFO stack or a regular FIFO buffer? Your 'Pop' naming implies a stack.
FIFO (ring) (and yes, it's RS485-related stuff). How would you name the function which pulls one item out of it?

The same for the reverse - we call it PushToBuffer().

Thanks,

Jan

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Jan wrote:
The compiler is apparently frightened of the pointer more than of an indexed array, so it starts to compile in the expected way...

That's interesting. Almost as though the compiler assumes a possible maximum index when using array notation.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

This reminds me of a recent post by Dean (here) with respect to his LUFA package. He gets a series of descriptors that have a common header followed by a variable-length body. Jan's application sounds very much like this scenario.

Jan, you might peruse Dean's code to see how he manages this. I also recall he had similar problems to yours, although his problem was an incorrectly incremented pointer with apparently valid syntax.

You come up with the coolest problems, Jan! :twisted:

stu

Engineering seems to boil down to: Cheap. Fast. Good. Choose two. Sometimes choose only one.

Newbie? Be sure to read the thread Newbie? Start here!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

How would you name the function which pulls one item out of it?

Fetch.

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

If you have a FIFO, you can use memcpy().
If it is implemented as a ring buffer, you use memcpy() in two steps if it has wrapped.
Of course if you cannot access the physical position, you have to access one byte at a time.

Fetch seems a suitable name for retrieving from a FIFO.

I am amazed that you seem to be getting into such contortions. It seems to me that the code can be written quite clearly. OTOH, you may want to complicate the procedure just to emphasize your dislike of C.

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I think something like this is
pretty much guaranteed to work:

typedef struct __attribute__((__packed__)) {
    uint16_t outVector;
    uint16_t time; 
    uint8_t x;   
  } T_something;

{
T_something a;
emum { bound=offsetof(T_something, x) };
for(uint8_t i=0; i< bound; i++) {
    ((unsigned char*)&a)[i] = PopByteFromCommBuffer();
}
[...]
if(buf.a.time> someTime) {  // only use of buf.a.time
    [...]
}
}

The typecasting looks dubious,
but I think that char types have a special dispensation.
Otherwise functions like memcpy would lose a lot of their utility.

Iluvatar is the better part of Valar.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Untested, but it took me a whole 60 seconds to write. And it does not look very complicated to me.

void *popcpy(void *dest, int size)
{
     uint8_t *p = (uint8_t *)dest;
     while (size--) *p++ = PopByteFromBuffer();
     return dest;
}

Whether you want to make it into some static inline or something with lots of underscores and unspellable attributes is up to you.

As I said, untested, but I guess avr-gcc would make a pretty good job of compiling it with or without extra gobbledygook.

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

There are certainly many ways to achieve the same thing, I am not going to discuss it here. Besides, you've seen just a [simplified to the point] snippet from a wider context. That's not the point; I am not asking for help with my work, I am just sharing my findings. Nevertheless, I thank all for the suggestions, made me to rethink things.

The point is, that the compiler (optimiser) sometimes chooses to do a surprising thing. Sometimes it is a genuine bug, sometimes a corner case, sometimes - as here - is the result of the programmer working out of "this worked for me" and not strictly adhering to the standard.

Morale is, that the "this always worked for me" is no argument.

JW