Imagecraft ICCAVR & ANSI Standards compliance

Do you have a question? Post it now! No Registration Necessary

Translate This Thread From English to

Threaded View
Hi everyone.

I have found what I consider to be a deficiency, if not a bug, in the
ImageCraft ICCAVR C-compiler, ver 6.29. I would like to find out if what
I consider to be a bug makes this compiler non-ANSI C complaint or not
and what others here feel about my discovery.

I have e-mailed the author (please see my text at the end) but he does
not feel this issue makes the compiler non-ANSI complaint and did not
express an interest in changing the behavior.

The issue is that the compiler seems to always encode an immediate as an
int data type (int on the AVR is 16-bits) and does not check the lvalue
datatype (to see if it is something other than an int) during variable
declarations or even usage in some cases.

As an example the following declaration:

long my_long_variable = (1<<17);

results in my_long_variable holding decimal 0 since the immediate value
of 1 is encoded as a 16-bit quantity, *then* shifted.

Note: That a long in this architecture[AVR] is 32-bits (signed) and
would have enough bits to hold decimal 131072.

As another example,

long second_long_variable = (65532+250);

results in second_long_variable holding decimal 246 instead of decimal
65782 at run-time.

The compiler does not evaluate the expression (65532+250) before an
assignment, but instead encodes 65532 as a 16-bit quantity, then adds
250, to a 16-bit quantity which results in a wrap-around to 246. Without
adding a cast operator in the declaration, the long variable is
initialized to an unexpected value (of 246).

If the compiler did look at the lvalue during the compile time and
realized it needed to store 65532 as a long, then the subsequent
addition operation would result in the variable holding the correct
value, even though it would involve an unnecessary add.

The only way I found to get the compiler to generate what I consider
"expected" behavior is to forcefully use the casting operator, as
follows:

long my_long_variable = ((long)1<<17);

or

long second_long_variable = ( (long)(65532+250) );

I was bitten by the encoding behavior not because I was declaring my
variables using the shift operator, but instead I had a code fragment as
follows:

/* Data typedefs */
typedef unsigned char uint8;
typedef unsigned int uint16;
typedef unsigned long uint32;

/* Macro to 'select' a bit */
#define BitPosition(bit) (1 << (bit))

void Send_IndividualBits_over_SPI(uint8 data_length, uint32 data_out)
{

    uint8 index;

    /* Send bits MSB first */
    /* When index underflows to 255, all bits have been sent */
    for(index=(data_length-1); index !25%5; index--)
    {        
        /* Grab each bit in data_word, MSB first and send it out */
        if( data_out & ( BitPosition(index) ))
        {
            /* Code for sending when bit is high */
        }
        else
        {
            /* Code for sending when bit is low */

    }
}

Due to the aforementioned encoding behavior, data_out would be ANDed
with (1<<index), except the immediate, 1 , would be represented by a 16-
bits, thus the shifted value would be 0 for data_lengths greater than
16-bits. The end result is that all the bits past the 16th would be sent
out as 0, regardless of their value.

My workaround was to declare a new BitPosition32 macro that included the
(uint32) casting operator in front of the '1' and then to have two
Send_SPI routines, one for 16-bit data and one for 32-bit data, each
using their own BitPosition macro.

I have verified the compiler operates in the manner I have mentioned by
not only studying the generated assembly, but also by testing with
working hardware (AT90S8515) and doing a final cross-check with the
Atmel AVR Studio 4.0 build 240(simulator).

I would like to state for the record that I am evaluating the compiler
and I that I am currently not a paying customer. I have e-mailed the
compiler author and while he was very prompt in responding (especially
given that I have not purchased it, thus I am technically not eligible
for support) he did not seem convinced this was a major issue or
something that should be fixed.

My code, including the examples of variable declarations( with (1<<17)
for instance), produces the "expected" results on all GCC/IAR platforms
I was able to test with (including x86, ARM & AVR). I tried on a few
other x86 only compilers too, including MSFT VisualC++ & Borland 5.02,
all of which behaved as "expected".

I feel that even if this behavior does not make ICCAVR non-ANSI
complaint, the manner in which it treats immediates is very confusing
and certainly nonintuitive/different than other "industry" compilers. I
have spent several days trackinn down this issue and had I been a paying
customer, I would be very displeased.

Finally, I would like to make it clear that I am not attempting to "air
dirty laundry", I simply would like to hear the opinions of other
qualified, working professionals in this field.

Thank you (and apologies for the length of the message!)
--------------------------------------------------------
Vincent.


Re: Imagecraft ICCAVR & ANSI Standards compliance

Quoted text here. Click to load it

That's what I'd expect. If you want to shift a 32 bit
value, you can write

1L << 17

Quoted text here. Click to load it

I don't got IAR, but GCC 3.3.1 for the AVR produces
the same result as ICC:

   sts my_long_variable,__zero_reg__
   sts (my_long_variable)+1,__zero_reg__
   sts (my_long_variable)+2,__zero_reg__
   sts (my_long_variable)+3,__zero_reg__

Please note, that x86 and ARM use 32-bit integers.

In my view ImageCraft is producing the correct result.
The type of the lvalue should not initiate any kind of
automatic conversion on the rvalue.

Regards,
Harald

--
For spam do not replace spamtrap with my name


Re: Imagecraft ICCAVR & ANSI Standards compliance

Quoted text here. Click to load it

That is correct behavior.  If you want a "long" 1, you spell it
1L:

  my_long_variable = 1L<<17;

Quoted text here. Click to load it

Opinions have nothing to do with it.  The standard states that
"1" is an "int". If you don't believe me, go ask in
comp.lang.c, and somebody will quote the standard at you.

What makes you think the behavior you describe is incorrect
(other than your wish that it be so)?

--
Grant Edwards                   grante             Yow!  VICARIOUSLY
                                  at               experience some reason
We've slightly trimmed the long signature. Click to see the full one.
Re: Imagecraft ICCAVR & ANSI Standards compliance
Quoted text here. Click to load it
... snip ...
Quoted text here. Click to load it

Your code is faulty.  The digit 1 describes an int (which I assume
is a 16 bit quantity on your system), and the shift of 17 causes
overflow, and thus undefined behavior.  You should start with a
long, by using "1L" as the initial value.

Quoted text here. Click to load it

Again, the same problem.  The proper promotion of 65532 on a 16
bit system is to unsigned int, and in this case the action of
adding 250 is properly described by the standard.  Converting the
result to long is giving the correct result.

The compiler appears to comply with the standards as it is.

--
Chuck F ( snipped-for-privacy@yahoo.com) ( snipped-for-privacy@worldnet.att.net)
   Available for consulting/temporary embedded and systems.
We've slightly trimmed the long signature. Click to see the full one.
Re: Imagecraft ICCAVR & ANSI Standards compliance
Please cite the ANSI specification that has been violated by this
behavior. All integer constants are by standard assumed to be int.
Shifting by more bits than the size of the target is undefined and if
I recall correctly is stated as such in the ANSI standard.

Doug

Quoted text here. Click to load it



Re: Imagecraft ICCAVR & ANSI Standards compliance
comp.arch.embedded:

Quoted text here. Click to load it

Others have pointed out that it is your understanding of the C
language that is at fault, not the compiler.

    [snip]

Quoted text here. Click to load it

This would not work either.  The addition would still produce the
unsigned int value 246.  The cast to long when assigning to long has
absolutely no effect, the conversion is automatic with the assignment.

To get the result you want, you would minimally need to do something
like this:

   (long)65532 + 250
   65532 + (long)250
   65532L + 250
   65532 + 250L

Quoted text here. Click to load it

Really, you have been bitten by your misunderstanding of C.

Quoted text here. Click to load it

I certainly hope so.  The compiler would be seriously non-conforming
it if did not act the way you observed.

Quoted text here. Click to load it

Are you evaluating the compiler for personal use, or perhaps for use
by your company?  If the latter, you are frankly do not have enough
knowledge of C to be qualified.

Quoted text here. Click to load it

Almost all gcc ports other than ARV are for 32-bit targets, as are all
current and recent Microsoft and Borland compilers.  On a 32-bit
architecture, int is 32 bits so your literals happen to be 32-bit even
though they are int.

Quoted text here. Click to load it

You sound a little like the newbies who post to
alt.comp.lang.learn.c-c++ and comp.lang.c in puzzlement when one of
their first assignments does not seem to work as expected.  This one
is a classic, the centigrade to fahrenheit conversion program:

#include <stdio.h>
#include <stdlib.h>

int main(void)
{
    float cent, fahr;
    char input[100];

    printf("Enter Degrees Centigrade: ");
    fflush(stdout):
    fgets(input, sizeof input, stdin);
    cent = strtol(input, NULL, 0);
    fahr = cent * (5/9);
    printf("Fahrenheit Degrees = %f\n");
    return 0;
}

No matter what you enter at the prompt, the output is 0.000000.  Why?
Because the division 5/9 is a division of two integer values and
produces an integer result, namely 0.  In this case, it makes no
difference whether int has 16 or 32 bits.

Not only is it not the compiler's job to notice that you are
eventually using a floating point value, and so convert 5 and 9 to
float for the division, it would violate the C standard if it did so.

There is an entry in the FAQ for comp.lang.c that exactly covers this
situation:

http://www.eskimo.com/~scs/C-faq/q3.14.html

Quoted text here. Click to load it

If you want become truly knowledgeable about C, start with the FAQ for
comp.lang.c, length in my signature.  For the true professional, the
ANSI/ISO/IEC International Standard for C can be purchased and
downloaded in PDF format for $18.00 from http://www.ansi.org .  Just
search for ISO number 9899.

--
Jack Klein
Home: http://JK-Technology.Com
We've slightly trimmed the long signature. Click to see the full one.
Re: Imagecraft ICCAVR & ANSI Standards compliance

Quoted text here. Click to load it

In addition to the excellent information above, the link below
leads to still more info that you might find helpful.
--
Morris Dovey
West Des Moines, Iowa USA
We've slightly trimmed the long signature. Click to see the full one.
Re: Imagecraft ICCAVR & ANSI Standards compliance


Even once you have fixed your misunderstanding of how literals are treated
in C, you might want to think a little about your code - it is a very
inefficient way to solve your problem.  When doing embedded programming, you
should always be thinking about what the processor has to do, and if that is
the best way to do it.

Quoted text here. Click to load it




Re: Imagecraft ICCAVR & ANSI Standards compliance

[...]

Quoted text here. Click to load it

So 'C' is nonintuitive.

Quoted text here. Click to load it

instead of _wasting_ (sic!) days, spend some money and buy a suitable
static code checker.

Even if you _know_ some day what a C compiler has to do, you will
still make errors by accident. Many of them can be detected by a
static code checker. I suggest PC-Lint, but see the thread "Lint for
microcontrollers" to learn that it's not only my opinion.

Lint early, lint often. Lint is your friend. Yes, I wasted also a lot
of time before I bought it.

Nevertheless you should have some idea of 'C' to be able to use lint
correctly.

Oliver
--
Oliver Betz, Muenchen

Re: Imagecraft ICCAVR & ANSI Standards compliance
Hello,

First, my apologies for not following up the initial thread, my
newsgroup server seems to have expired it, hence a new posting.

Secondly, I would like to thank everyone for pointing out MY mistakes
and informing me of what the ANSI C standard states with respect to
immediates (and thus showing that there was not a bug in ImageCraft
ICCAVR).

I also realize in my post I should have simply asked what the ANSI
standard states with respect to encoding immediates instead of implying
that ICCAVR was treating them in a non-standards compliant manner.

There was also mention by David Brown that my code was inefficient;
while I certainly do not expect other people to solve my problem, if
anyone would like to offer suggestions on what might make the code
smaller/faster, I would appreciate it. I suppose instead of shifting the
data, I could use a is_bit_set type of macro which could make use of the
AVR's bld/bst instructions or branch to a section of code if bit isn't
set, etc.

Finally, I would like to apologize to Richard Man for implying that his
product or his company were at fault(for anything). Even though there
were no errors or mistakes on his part, Richard was still kind enough to
provide a response, I can't say many companies would be willing to go
out of their way as he did.

-------
Vincent


Re: Imagecraft ICCAVR & ANSI Standards compliance

Quoted text here. Click to load it

This post is such a rariety in usenet that it should be printed out and
framed...

Quoted text here. Click to load it

The key inefficiency was that you had a loop that was roughly:

uint32 data; uint8 i;
for (i = len-1; i != 255; i--) {
    if (data & (1UL << i)) ...
}

This means you are doing a great deal of unnecessary shifting.  If your data
is the full 32 bits, your will in total do 31 + 30 + .. + 0 = 496 shifts,
each taking a number of instructions since you are working with 32-bit
values.  It is also poor style to rely on the underflow of (unsigned char)
0 - 1 being 255.  A much more efficient simplifcation would be:

uint32 data; uint32 mask; uint i;
mask = 1UL << (len - 1);
i = len;
while (i--) {
    if (data & mask) ...
    mask >>= 1;
}

32 bits leads to 32 shifts.  You could do even better by dealing with a
single byte at a time, since that's what an AVR handles best, but the main
point was to reduce the O(n^2) algorithm to O(n).



Quoted text here. Click to load it



Re: Imagecraft ICCAVR & ANSI Standards compliance
[...]
Quoted text here. Click to load it



I don't think I can accept this way of counting shifts as a valid one,
unless AVR had only a shift-by-one operator instead of one that takes
the shift width as an operand.  I'm not familiar with the AVR, but for
an architecture that's often described here as C-friendly by design,
that'd be a very silly state of affairs.  I can see no acceptable
reason a compiler should generate code of O(N) runtime to implement a
shift-by-N.

I'd classify a compiler for any 8-bit architecture that uses all of 30
shift instructions to implement (1UL << 30) as seriously
brain-damaged.  It should never take more than 6, and really only one
shift instruction:

    load lowermost byte
    shift it left by 6 bits
    write it to uppermost byte
    erase the other three bytes

Even an 8051 would need no more than 3 shift-like instructions for
that (one nibble-swap, two shifts).  Shifts will generally have to be
O(D) on datatypes D times the machine's word size, right.  But not
O(N) for shifting by N bits.

Quoted text here. Click to load it

Yep.  This will be the major problem.


--
Hans-Bernhard Broeker ( snipped-for-privacy@physik.rwth-aachen.de)
Even if all the snow were burnt, ashes would remain.

Re: Imagecraft ICCAVR & ANSI Standards compliance

Quoted text here. Click to load it
data
shifts,

I think you are gravely misunderstanding what is involved in 8-bit cpu
design, what are sensible decisions in library code for such a micro, and
what is expected of a good programmer for such a micro.  I also think it is
more than a little impolite to start referring to the ICCAVR author as
"brain damaged" - even if you were correct in what you were saying.

First off, the AVR does not have a barrel shifter - even an 8-bit barrel
shifter is expensive in real estate and power consumption for a cpu in which
small size and power is critical, and the rarity of large shifts in real
code means it is not worth having.  It is a different matter for 32-bit
cpus, where a barrel shifter can be made sensibly with a tree structure, and
can save a lot of time for large shifts.  The alternative to a barrel
shifter would be microcoded shifters, which are a big no-no on a risc chip.

I don't see how "C-friendly" would imply a barrel shifter in any way - large
shifts are not that common in most C code.  And anyway, despite claims to
the contrary, the AVR has a great deal of C-unfriendly architecture
decisions, even though it is far more "C-friendly" than traditional
accumulator-style 8-bit cpus.

And even if the chip had an 8-bit barrel shifter, it would be of limited use
for 32-bit shifts - you would have to do so much masking and "cut and paste"
of the bytes that it would hardly be worth it.  It's like doing a 32-bit
division on a cpu with a hardware 16-bit divider - it has to be done
long-hand, without making use of the hardware divider.

So you are left with a using a library routine for the 32-bit shift (or
indeed any other shifts).  There are several ways to implement such a
routine.  The simplest would just loop through the shift-count, doing a
single shift in each loop.  A more complex routine would sort out groups of
eight shifts, and handle these by byte movements, and then do individual
shifts.  A really advanced one would also use nibble swapping for shifts of
4, and consider left shifts of 7 as a right shift of 1 followed by byte
movement, and so on.  But is there any point in such optomisations?  They
would produce much bigger library code, for only a small gain, and almost
never be used.  The huge majority of shifts in real code are of 1 or 2 -
large, variable shifts are very rare (the exception is for constants such as
(1 << 12), which are calculated by the compiler and not the target cpu).
The way to deal with such issues are the same as for any time-consuming
task - either accept that the cpu is not ideal for that operation so it will
take a long time, or re-think your algorithm and what you are asking the cpu
to do.

Writing good code for a cpu like the AVR means knowing the architecture
well, and it means taking its strengths and weaknesses into consideration.
Anyone who uses 32-bit arithmetic on the chip, and doesn't understand the
consequences, does not know their target well enough.  There are plenty of
occasions where 32-bit arithmetic makes sense, and there are plenty of
occasions where the time taken does not matter.  But you have to *know*.


Quoted text here. Click to load it

Ignoring the lack of a "shift by 6 instruction", what you have written here
is code dedicated to the function (x << 30).  That's not what is needed
here - the shift count is variable.  It's about as useful as telling us that
(1UL << 30) can be done with four byte-wide direct load instructions.

You could write a dedicated (1UL << x) function that was more efficient than
using the generic shift routine (for example, you could use a lookup table).
But why bother?  As is frequently the case, optomising the algorithm makes
far more difference than optomising the code.


Quoted text here. Click to load it



Site Timeline