EmbDev.net

Forum: ARM programming with GCC/GNU tools Optimizer broke code


Author: jrmymllr jrmymllr (jrmymllr)
Posted on:

Rate this post
0 useful
not useful
If anyone would like a challenge, the code below doesn't work when I 
have optimization set to -o2.  -o0 works fine.

The function is a somewhat complex word-wrap function.  I rewrote this 
function to add some enhancements, but the old one still works so the 
issue is not with any other code.  The code below straight out fails and 
passes a few characters of garbage to lcd_DspStr().  I'm using ARM GCC 
compiler.

I suspect the issue lies with statements like "string = string + x + 
1;", but I don't know the proper way to do this.
int lcd_DisplayStr(unsigned char* string, int maxlines, int maxwidth, int startaddr, const unsigned short *font, int ellipsis, int invert)
{
  unsigned char tempc[maxwidth + 1], lines;
  unsigned int x;

  lines = 0;

  while(lines < maxlines)
  {
    x = 0;
    if(lines > 0)
    {
      while(*(string) == 0x20)            //eliminate leading spaces on all but first line
      {
        string++;
      }
    }
    while((string[x] != 0) && (x < maxwidth))    //copy characters from input string into display line string
    {
      tempc[x] = string[x];
      x++;
    }
    if((string[x] == 0x20))              //if next character in input string is a space no hyphen required
    {
      string = string + x + 1;

      while(x < maxwidth)                //fill in rest of line with spaces to make string maxwidth in length
      {
        tempc[x] = ' ';
        x++;
      }
      tempc[x] = 0;

      if((ellipsis == 1) && (lines == (maxlines - 1)))  //if ellipsis is enabled and at the last displayed line
      {
        tempc[maxwidth - 1] = '.';          //insert ellipsis
        tempc[maxwidth - 2] = '.';
        tempc[maxwidth - 3] = '.';
        lcd_DspStr(tempc, startaddr, invert, font);
        return startaddr;
      }
      lcd_DspStr(tempc, startaddr, invert, font);
    }
    else if((string[x] == 0))            //if next character in input string is null, it is end of input string
    {
      while(x < maxwidth)                //fill in rest of line with spaces to make string maxwidth in length
      {
        tempc[x] = ' ';
        x++;
      }
      tempc[x] = 0;
      lcd_DspStr(tempc, startaddr, invert, font);
      return startaddr;                //done since end of input string was reached
    }
    else
    {
      if((ellipsis == 1) && (lines == (maxlines - 1)))  //if ellipsis is enabled and at the last displayed line
      {
        tempc[maxwidth - 1] = '.';          //insert ellipsis
        tempc[maxwidth - 2] = '.';
        tempc[maxwidth - 3] = '.';
        tempc[maxwidth] = 0;
        lcd_DspStr(tempc, startaddr, invert, font);
        return startaddr;
      }
      else if((ellipsis == 0) && (lines == (maxlines - 1)))  //if ellipsis is disabled and at the last displayed line
      {
        tempc[maxwidth] = 0;
        lcd_DspStr(tempc, startaddr, invert, font);
        return startaddr;
      }

      else
      {
        while((tempc[x] != 0x20) && (x > 0) && (FRAGMENT >= (maxwidth - x)))  //go backwards through display string FRAGMENT characters
        {                                    //looking for a space for a natural place to word wrap
          x--;
        }
        if(FRAGMENT == (maxwidth - x - 1))                  //hyphen will be used since last word in line string was too long
        {
          tempc[maxwidth - 1] = '-';
          tempc[maxwidth] = 0;
          string = string + (maxwidth - 1);
          lcd_DspStr(tempc, startaddr, invert, font);
        }
        else                                //no hyphen, move last word to next line and wrap on space
        {
          string = string + x;
          while(x < maxwidth)                        //fill in the rest of the line with spaces
          {
            tempc[x] = ' ';
            x++;
          }
          tempc[x] = 0;

          lcd_DspStr(tempc, startaddr, invert, font);
        }
      }
    }
    startaddr = startaddr + (*(font + 1) * LCD_GRAPHIC_LINE);
    lines++;
  }
  return startaddr;
}

Author: Bernhard S. (berns)
Posted on:

Rate this post
0 useful
not useful
Sorry, I've lost the track ... (and it would be too time consuming to 
get back on)

- The control or program flow is not really clear (i.e. several "ends" 
within code)
- There's a lot of code duplication
- There are some things I do not know what they are, i.e. startaddr, ...
- You shouldn't name a variable string

Man that would make my head ache ;). I believe the thing is much 
simpler, so why make it so complicated?

Anyway - have you tried to check at which location the error occurs, so 
which call (out of 7) to lcd_DspStr() got garbage?

Author: jrmymllr jrmymllr (jrmymllr)
Posted on:

Rate this post
0 useful
not useful
Bernhard S. wrote:
> Sorry, I've lost the track ... (and it would be too time consuming to
> get back on)
>
> - The control or program flow is not really clear (i.e. several "ends"
> within code)
> - There's a lot of code duplication
> - There are some things I do not know what they are, i.e. startaddr, ...
> - You shouldn't name a variable string
>
> Man that would make my head ache ;). I believe the thing is much
> simpler, so why make it so complicated?
>
> Anyway - have you tried to check at which location the error occurs, so
> which call (out of 7) to lcd_DspStr() got garbage?

Hi, thanks for replying.  I didn't expect anyone to try and follow that 
code, as it is very complex and took me some time to write.  The code 
works perfectly when tested on Win32 GCC, and on ARM GCC with no 
optimization.  I thought there might be a problem with how I'm using a 
pointer, which may not require knowing exactly how the code works.

As a side note, if anyone has a simpler way to do this I'd be happy to 
use it:

-split up a long string by writing multiple lines to LCD
-if last word on an individual line fits more than FRAGMENT characters 
on that line, hyphenate when word wrapping.  Otherwise, word wrap on the 
previous space
-if ellipsis is set, and the entire input string won't fit on the 
allowed number of lines and characters per line, add "..." to end.


Trust me, it's tough for even me to follow this code, and I wrote it. 
There's too many branches which I don't see any easy way to eliminate.

Author: Bernhard S. (berns)
Posted on:

Rate this post
0 useful
not useful
> I thought there might be a problem with how I'm using a
> pointer, which may not require knowing exactly how the code works.

I believe it's necessary to understand how the code works to check what 
there might get wrong while optimizing.
Optimization as such is not allowed to break code so either there is a 
fault within the optimizer or in your code (without beeing visible 
without optimization).

http://www.codinghorror.com/blog/2008/03/the-first...

> As a side note, if anyone has a simpler way to do this I'd be happy to
> use it:

There are several outside - I found some code with google.

> Trust me, it's tough for even me to follow this code, and I wrote it.

Thats a bad sign, isn't it?

> There's too many branches which I don't see any easy way to eliminate.

Line break/word wrap/... is a very common problem. Here one less 
complex, untested code I've just written into this Textfield:
int x=0,wordstart=0,wordend=0,inword=0;
int actline=0, actlinepos=0, i;

//multiple lines possible
int actlinemaxwidth = maxwidth;

unsigned char line[maxwidth+1];

line[maxwidth] = 0x00;

while ((text[x] != 0) && (line<maxlines)) {
  if (text[x]==0x20) {
    if (inword)
    {
      wordend=x;
      inword=0;
    }
  } else {
    if (inword == 0) {
      wordstart=x;
    }
    inword++;
  }

  if (wordend>wordstart) {
    if ((actline==0) && (actlinepos==0)) {
      wordstart = 0;
      inword = wordend;
    }

    if ((actlinepos + inword)>actlinemaxwidth) {
      if (hyphen) {
        actlinemaxwidth--;
        line[actlinemaxwidth]='-';

        while (actlinepos<actlinemaxwidth) {
          //copy the word
          line[actlinepos]=text[wordstart];
          actlinepos++;
          wordstart++;
        }
      } else {
        while (actlinepos<actlinemaxwidth) {
          line[actlinepos] = ' ';
          actlinepos++;
        }
      }
      lcd_DspStr(...);

      actline++;
      if ((actline == maxlines-1) && (ellipsis))
      {
        actlinemaxwidth=maxwidth-3;
        line[maxwidth-1] = '.';
        line[maxwidth-2] = '.';
        line[maxwidth-3] = '.';
      }
    } 

    if (actline<maxlines)
    {
      //copy the word (or the rest) to our buffer
      for (i=wordstart;i<wordend;i++) {
        line[actlinepos] = text[i];
        actlinepos++;
      }
      wordstart=wordend;
    }
  }

  x++;
}

//TODO: handle last line here.
lcd_DspStr(...);

With two additional (possible self implemented) string methods "fillstr" 
and "strncpy" things get even simpler.

Author: Klaus (Guest)
Posted on:

Rate this post
0 useful
not useful
> either there is a fault within the optimizer

this is the last option, you should think at. Because in 99,999% cases, 
it is an error in the user code.

Author: Bernhard S. (berns)
Posted on:

Rate this post
0 useful
not useful
Klaus wrote:
>> either there is a fault within the optimizer
>
> this is the last option, you should think at. Because in 99,999% cases,
> it is an error in the user code.

That's the reason why I linked the blog-post: "The First Rule of 
Programming: It's Always Your Fault" to my posting above ;)

Author: jrmymllr jrmymllr (jrmymllr)
Posted on:

Rate this post
0 useful
not useful
It occurred to me there might be a bug in the optimizer, but I didn't 
think the chance was too great.  I've ran into "strange" problems before 
but eventually figured them out.  Regardless, I downloaded the newest 
version of CS G++ Lite just to try it out, but it does the same thing.

I was a bit misleading about saying I didn't know how my own code works. 
I thought I wrote it fairly carefully and tested it thoroughly on win32 
in Eclipse, which is much easier than debugging on a micro especially 
since it's only a text processing function.  What I was thinking is that 
having to rewrite it again would be a pain.


However something strange is going on.  I'm not saying it's the 
compiler's fault.  I think I'm overlooking something obvious.  The 
problem seems to be related to the "tempc" array.  When compiling with 
-o2, I believe "tempc"  gets optimized out.  In addition, even with the 
simple code below, the exact same problem is occurring.  The first 
lcd_DspStr displays the input string "instr" correctly.  The second 
lcd_DspStr displays a character or two of garbage, and using the 
debugger is of no use since what is occurring makes no sense.  Once it's 
out of the while loop, "tempc" no longer contains the correct text, but 
rather gets clobbered with something else.
int lcd_DisplayStr(unsigned char* instr, int maxlines, int maxwidth, int startaddr, const unsigned short *font, int ellipsis, int invert)
{
  unsigned char tempc[maxwidth + 1];

  unsigned char x;

  lcd_DspStr(instr, 0, invert, font);            //display on LCD line #0
  x=0;

  while((instr[x] != 0) && (x < maxwidth))    //copy characters from input string into display line string
  {
    tempc[x] = instr[x];
    x++;
  }


  tempc[x]= 0;
  lcd_DspStr(tempc, LCD_LINE_NUM(40), invert, font);    //display on LCD line #40
}

Author: Fanicula (Guest)
Posted on:

Rate this post
0 useful
not useful
Just a wild guess: are you sure that, e.g., instr is actually a valid 
pointer when calling lcd_DisplayStr()?

Author: jrmymllr jrmymllr (jrmymllr)
Posted on:

Rate this post
0 useful
not useful
Fanicula wrote:
> Just a wild guess: are you sure that, e.g., instr is actually a valid
> pointer when calling lcd_DisplayStr()?

instr must be valid because the first LCD output call,

lcd_DspStr(instr, 0, invert, font);

prints what is passed in instr perfectly.

It's the second call of lcd_DspStr() that display junk.

Author: Fanicula (Guest)
Posted on:

Rate this post
0 useful
not useful
> instr must be valid because the first LCD output call,

Not necessarily if the memory the previously valid pointer pointed to 
hadn't been modified.

> It's the second call of lcd_DspStr() that display junk.

That's exactly the reason why I made the guess: if instr is invalid, the 
compiler might reuse instr's memory for tempc.

But I already said it would just be a wild guess.

Author: Bernhard S. (berns)
Posted on:

Rate this post
0 useful
not useful
What would happen if you'd call
lcd_DspStr(tempc, 0, invert, font);
instead of the current one? Would there be garbage?

Anyway if debugging your code, you can't debug the "C" instructions, as 
-O2 allows the compiler a lot of things that could make it impossible to 
remap the op-codes back to the original C-Code. So you'd need to debug 
the assembler code.

Author: Bernhard S. (berns)
Posted on:

Rate this post
0 useful
not useful
One additional thing you could try to check what optimizations are in 
effect:

You could call gcc with "-Q --help=optimizers" to see what optimizations 
are in effect and then try to deactivate one-by-one to get a clue on 
what happens.

Author: jrmymllr jrmymllr (jrmymllr)
Posted on:

Rate this post
0 useful
not useful
Fanicula wrote:
>> instr must be valid because the first LCD output call,
>
> Not necessarily if the memory the previously valid pointer pointed to
> hadn't been modified.
>
>> It's the second call of lcd_DspStr() that display junk.
>
> That's exactly the reason why I made the guess: if instr is invalid, the
> compiler might reuse instr's memory for tempc.
>
> But I already said it would just be a wild guess.

instr is staying intact.  It's tempc that is getting clobbered.  I'm 
stumped.

Author: jrmymllr jrmymllr (jrmymllr)
Posted on:

Rate this post
0 useful
not useful
Bernhard S. wrote:
> What would happen if you'd call
>
> lcd_DspStr(tempc, 0, invert, font);
> 
> instead of the current one? Would there be garbage?
>
> Anyway if debugging your code, you can't debug the "C" instructions, as
> -O2 allows the compiler a lot of things that could make it impossible to
> remap the op-codes back to the original C-Code. So you'd need to debug
> the assembler code.

lcd_DspStr is definitely working.  It's called directly in some places 
when word wrap isn't required, and it always works.  When I debug the 
function in question, I see garbage being passed to lcd_DspStr's first 
parameter.

The crazy thing is that the problematic function has similarities to the 
old, functioning one.  I can substitute the old function in it's place, 
and things work the way they used to, although the new function does its 
job better when it works.  I even declare the problematic array the same 
way.

Maybe working on it another day will bring something to light.  I've 
looked at the assembler but I'm not familiar with ARM asm.  I'll check 
out the other suggestions you give also.

Author: Klaus Wachtler (mfgkw)
Posted on:

Rate this post
0 useful
not useful
This is a fine example of unmaintainable code.
Complex code is not worthwhile, and as soon as you don't understand
it rewrite it!

Author: Klaus Wachtler (mfgkw)
Posted on:

Rate this post
0 useful
not useful
BTW I tried the code with arm-gcc version 3.4.5 and found it working in 
a simple case:
#include <stdlib.h>
#include <stddef.h>
#include <stdio.h>
#include <string.h>

#define FRAGMENT         (10)
#define LCD_GRAPHIC_LINE (20)

void lcd_DspStr( const unsigned char *str, size_t startadr, int invert, const unsigned short *font )
{
  printf( "lcd_DspStr: <%3d> <%s>\n", startadr, str );
}

int lcd_DisplayStr(const unsigned char* string, int maxlines, int maxwidth, int startaddr, const unsigned short *font, int ellipsis, int invert)
{
   .... function as given ...
}


int main( int nargs, char **args )
{
  const unsigned char    str[] = "complex code is wonderfull for hackers and other dark people, so try to avoid it if possible";
  unsigned short int font[2] = { 0, 10 };
  lcd_DisplayStr( str, 4, 20, 100, font, 1, 0 );
  return 0;
}

gives me (with -O2 as well as -O0):
root@CentiPad-v11:~# ./unmaintainable_arm 
lcd_DspStr: <100> <complex code is     >
lcd_DspStr: <300> <wonderfull for      >
lcd_DspStr: <500> <hackers and other   >
lcd_DspStr: <700> <dark people, so t...>

Author: Oliver (Guest)
Posted on:

Rate this post
0 useful
not useful
>Maybe working on it another day will bring something to light

Probably yes.

Although your code is absolutly unreadable and weird, it only uses basic 
operations. The chance, that gcc will screw up on this, ist below 0%. 
The chance, that one of your pointers or array indexes is going wild 
(maybe even in another part of your program), ist higher than 100%.

Oliver

Author: Klaus (Guest)
Posted on:

Rate this post
0 useful
not useful
> The chance, that gcc will screw up on this, ist below 0%.
> The chance, that one of your pointers or array indexes is going wild
> ist higher than 100%.

At least the chances add up to 100% ;-)

Author: jrmymllr jrmymllr (jrmymllr)
Posted on:

Rate this post
0 useful
not useful
Oliver wrote:
>>Maybe working on it another day will bring something to light
>
> Probably yes.
>
> Although your code is absolutly unreadable and weird, it only uses basic
> operations. The chance, that gcc will screw up on this, ist below 0%.
> The chance, that one of your pointers or array indexes is going wild
> (maybe even in another part of your program), ist higher than 100%.
>
> Oliver

That could all be true, but I can't come up with a better coding 
strategy for what I'm trying to do.  Also code looks more confusing to 
the person who didn't write it.

But ignoring the original function for now and looking at the simple one 
where it's simply copying a string, I'd like to know why that one gets 
screwed up first.  That's the root of the problem.

Author: Bernhard S. (berns)
Posted on:

Rate this post
0 useful
not useful
> lcd_DspStr is definitely working.  It's called directly in some places
> when word wrap isn't required, and it always works.  When I debug the
> function in question, I see garbage being passed to lcd_DspStr's first
> parameter.

I can't reproduce the issue (and it seems I'm not the only one) - see 
the post of Klaus Wachtler above.

The second "short" code fragment is IMO ok. The first code part is 
"unreadable".

> That could all be true, but I can't come up with a better coding
> strategy for what I'm trying to do.

?!

> Also code looks more confusing to the person who didn't write it.

Yes, cause the person who has not written it, can't know what you were 
thinking while writing it. Neither will you in a half-year.

> But ignoring the original function for now and looking at the simple one
> where it's simply copying a string, I'd like to know why that one gets
> screwed up first.  That's the root of the problem.

I believe that the issue you have here is not limited to the one 
function. If the rest of your code is designed as your snipet in the 
first post, I think we simply can't find any issue.

-> btw. there was a nice ad beside the text field: "It's hard to fix 
what you can't see."

Author: jrmymllr jrmymllr (jrmymllr)
Posted on:

Rate this post
0 useful
not useful
Bernhard S. wrote:
>
> I can't reproduce the issue (and it seems I'm not the only one) - see
> the post of Klaus Wachtler above.
>
> The second "short" code fragment is IMO ok. The first code part is
> "unreadable".
>

I can't reproduce it either on GCC in Windows, even using the same 
optimization level.  When I compile for ARM and run it on my Cortex-M3 
microcontroller, the issue crops up.  There is more than one unrelated 
periodic interrupt running which could be causing some problem.  However 
keep in mind this is an already functioning project that is fairly 
complex with TCP/IP, software MP3 decode, and support for multiple 
network protocols.  It's been in use for about 8 months and has worked 
as originally designed quite reliably.  I'm simply trying to improve the 
function in question, but the new version malfunctions.  Same function 
prototype, same code everywhere else.  I can put the old function back 
in and it works as it always did.  I had optimization issues before, but 
they were always related to not using "volatile" as I recall.  Of the 
thousands of lines of code, I never encountered such an odd problem 
related to optimization.

Obviously this doesn't give much useful information to anyone.  I'm just 
explaining the situation a little better.

>> That could all be true, but I can't come up with a better coding
>> strategy for what I'm trying to do.
>
> ?!
>
>> Also code looks more confusing to the person who didn't write it.
>
> Yes, cause the person who has not written it, can't know what you were
> thinking while writing it. Neither will you in a half-year.
>

That's what code comments are for, and frankly, the concern of not 
understanding what I wrote in a half-year from now is not a big concern 
for me.  I've went back to other sections of my code a year or more 
after last touching it to add features and didn't seem to have a 
problem.  This is for a hobbyist project, and while I prefer to develop 
well-written code, I'm not a software engineer by trade.


>> But ignoring the original function for now and looking at the simple one
>> where it's simply copying a string, I'd like to know why that one gets
>> screwed up first.  That's the root of the problem.
>
> I believe that the issue you have here is not limited to the one
> function. If the rest of your code is designed as your snipet in the
> first post, I think we simply can't find any issue.
>
> -> btw. there was a nice ad beside the text field: "It's hard to fix
> what you can't see."


When I get some time (soon) I will get to the bottom of this and report 
back for anyone who is curious.

Reply

Entering an e-mail address is optional. If you want to receive reply notifications by e-mail, please log in.

Rules — please read before posting

  • Post long source code as attachment, not in the text
  • Posting advertisements is forbidden.

Formatting options

  • [c]C code[/c]
  • [avrasm]AVR assembler code[/avrasm]
  • [code]code in other languages, ASCII drawings[/code]
  • [math]formula (LaTeX syntax)[/math]




Bild automatisch verkleinern, falls nötig
Note: the original post is older than 6 months. Please don't ask any new questions in this thread, but start a new one.