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.
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?
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.
> 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-rule-of-programming-its-always-your-fault.html> 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:
1
intx=0,wordstart=0,wordend=0,inword=0;
2
intactline=0,actlinepos=0,i;
3
4
//multiple lines possible
5
intactlinemaxwidth=maxwidth;
6
7
unsignedcharline[maxwidth+1];
8
9
line[maxwidth]=0x00;
10
11
while((text[x]!=0)&&(line<maxlines)){
12
if(text[x]==0x20){
13
if(inword)
14
{
15
wordend=x;
16
inword=0;
17
}
18
}else{
19
if(inword==0){
20
wordstart=x;
21
}
22
inword++;
23
}
24
25
if(wordend>wordstart){
26
if((actline==0)&&(actlinepos==0)){
27
wordstart=0;
28
inword=wordend;
29
}
30
31
if((actlinepos+inword)>actlinemaxwidth){
32
if(hyphen){
33
actlinemaxwidth--;
34
line[actlinemaxwidth]='-';
35
36
while(actlinepos<actlinemaxwidth){
37
//copy the word
38
line[actlinepos]=text[wordstart];
39
actlinepos++;
40
wordstart++;
41
}
42
}else{
43
while(actlinepos<actlinemaxwidth){
44
line[actlinepos]=' ';
45
actlinepos++;
46
}
47
}
48
lcd_DspStr(...);
49
50
actline++;
51
if((actline==maxlines-1)&&(ellipsis))
52
{
53
actlinemaxwidth=maxwidth-3;
54
line[maxwidth-1]='.';
55
line[maxwidth-2]='.';
56
line[maxwidth-3]='.';
57
}
58
}
59
60
if(actline<maxlines)
61
{
62
//copy the word (or the rest) to our buffer
63
for(i=wordstart;i<wordend;i++){
64
line[actlinepos]=text[i];
65
actlinepos++;
66
}
67
wordstart=wordend;
68
}
69
}
70
71
x++;
72
}
73
74
//TODO: handle last line here.
75
lcd_DspStr(...);
With two additional (possible self implemented) string methods "fillstr"
and "strncpy" things get even simpler.
> 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.
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 ;)
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.
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.
> 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.
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.
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.
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.
> 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.
>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
> 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% ;-)
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.
> 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."
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.
Watch this topic
Disable multi-page view