From: Daniel M. <sd...@bl...> - 2013-02-18 11:43:49
Attachments:
simpletest.c
|
Dear sdcc community, I am encountering a problem that is somehow connected to the cast between float and int, and it is unclear to me whether I am using sdcc wrong or whether there might be a bug somewhere. Please find below two pieces of code, the first one works as expected, the value of debug is 6116. The second does not work, I expect the value to be 6116, but I see a different number. It is not clear to me whether this is always the same number and what influences its value. I compile for a PIC16F1938, using sdcc from svn (rev8435), configured with --enable-new-pics (since pic16f1938 uses the "enhanced core") against gputils r932. $ sdcc -v SDCC : mcs51/gbz80/z80/z180/r2k/r3ka/ds390/pic16/pic14/TININative/ds400/hc08/s08 3.2.1 #8435 (Feb 17 2013) (Linux) I compile using $ sdcc -I/usr/local/share/sdcc/non-free/include/ -L/usr/local/share/^Ccc/non-free/lib/pic14/ -L/usr/local/share/^Ccc/lib/pic14 -mpic14 -p16f1938 problem.c When comparing the asm outputs I see that the first example does not call ___fs2uint, why does it not need to do this to cast the value of x? What else can I do to narrow the problem down? Is there a debugger in which I can run these programs on my computer and inspect the values after each step? Currently I use a board with connected LED displays to inspect the value of debug (program attached). Thank you for any help. //Daniel $ cat problem_works.c #define __16f1938 #include <float.h> #include "pic14/pic16f1938.h" static __code int __at(_CONFIG1) __CONFIG1 = _WDTE_OFF; unsigned int debug = 1234; void main(void) { float x = 6116.0; debug = x; //debug is 6116. } $ cat problem.c #define __16f1938 #include <float.h> #include "pic14/pic16f1938.h" /* Setup chip configuration */ static __code int __at(_CONFIG1) __CONFIG1 = _WDTE_OFF; unsigned int debug = 1234; float julianday = 6116.0; void main(void) { float x = 6116.0; debug = julianday; // expect 6116, obtain a vastly different (arbitrary?) number } |
From: Erik P. <epe...@iv...> - 2013-02-20 06:05:41
|
On Mon, 18 Feb 2013, Daniel Michalik wrote: > When comparing the asm outputs I see that the first example does not call > ___fs2uint, why does it not need to do this to cast the value of x? > void main(void) { > float x = 6116.0; > debug = x; > //debug is 6116. > } > unsigned int debug = 1234; > float julianday = 6116.0; > > void main(void) { > float x = 6116.0; > > debug = julianday; > // expect 6116, obtain a vastly different (arbitrary?) number > } I can't answer most of your questions because I do use pic and so I am not very familiar with that part of SDCC. However, I can answer why SDCC does not need to call __fs2uint in the first example. SDCC's optimizations are all at the function level. It can see that x is assigned the float value 6116.0 and then debug is assigned a value from x. It sees that there are no intervening instructions that could modify x's value, therefor debug is assigned the value 6116.0 cast to unsigned int. This cast is being applied to a known constant, so the compiler performs this cast during compilation to avoid the overhead of calling __fs2uint during run-time. This does not happen in the second example because julianday is assigned the value 6116.0 outside of the function. When optimizing the function, the compiler only considers the code within the function and so does not assume any variable still has its globally initialized value. Since the compiler is not certain of the value of julianday, it cannot compute the cast at compile-time and so inserts a call to __fs2uint to perform the cast at run-time. Give this version a try: void main(void) { volatile float x = 6116.0; debug = x; //debug is 6116. } The "volatile" keyword should disable most optimizion related to the variable x and force the cast to be performed at run-time. If this leaves the correct value in debug, the problem is with initialization of global variables. However, if this also fails, then there is likely a problem with the function implementing the run-time cast. Erik |
From: Maarten B. <sou...@ds...> - 2013-02-20 10:32:19
|
>> unsigned int debug = 1234; >> float julianday = 6116.0; >> >> void main(void) { >> float x = 6116.0; >> >> debug = julianday; >> // expect 6116, obtain a vastly different (arbitrary?) number >> } [...] > If this leaves the correct value in debug, the problem is with > initialization of global variables. I seem to remember that the SDCC PIC14 port does not have any working initialization of global variables. Maarten |
From: Daniel M. <sd...@bl...> - 2013-02-20 11:07:02
|
On Wed, Feb 20, 2013 at 11:32:05AM +0100, Maarten Brock wrote: > I seem to remember that the SDCC PIC14 port does not have any working > initialization of global variables. I think it generally should work. I used them before and it worked fine (assuming there wasn't any optimization doing magic). Regards, //Daniel |
From: Daniel M. <sd...@bl...> - 2013-02-20 11:05:39
|
On Wed, Feb 20, 2013 at 12:05:27AM -0600, Erik Petrich wrote: > I can't answer most of your questions because I do use pic and so I am not > very familiar with that part of SDCC. However, I can answer why SDCC does > not need to call __fs2uint in the first example. SDCC's optimizations are > all at the function level. It can see that x is assigned the float value > 6116.0 and then debug is assigned a value from x. It sees that there are > no intervening instructions that could modify x's value, therefor debug is > assigned the value 6116.0 cast to unsigned int. This cast is being applied > to a known constant, so the compiler performs this cast during compilation > to avoid the overhead of calling __fs2uint during run-time. Erik, This was great advice and very appreciated. Your explanations (I didn't quote all of them) were quite helpful in narrowing down the problem a bit more. [...] > void main(void) { > volatile float x = 6116.0; > debug = x; > //debug is 6116. > } I now changed the code a bit (since my previous example added both, fs2int and int2fs), to understand better what's going on: long debug; void main(void) { long x = 100; volatile float temp; temp = x; debug = temp; // debug=100 as expected } vs long debug; void main(void) { volatile long x = 100; float temp; temp = x; debug = temp; //debug=144, not expected } The asm block "functions called" is identical expect for the additional "___slong2fs" in the not working example. I am not sure whether that is sufficient indication to consider ___slong2fs the culprit, or whether I can perform any additional tests. When starting with 100 I obtain 144, for 116 I obtain 208, not sure that this makes any sense. I guess it is time to look closer into slong2fs, but I find different source files in the sdcc tree. How can I know for sure which one is used here? Why is it not the same for all chips? Is there a way of making my debugging somewhat easier than connecting displays to an actual chip? Thanks for your help //Daniel |
From: Daniel M. <sd...@bl...> - 2013-02-20 12:08:16
|
[resent after sending from the wrong mail address earlier] On Wed, Feb 20, 2013 at 12:05:15PM +0100, Daniel Michalik wrote: > The asm block "functions called" is identical expect for the additional > "___slong2fs" in the not working example. I am not sure whether that is > sufficient indication to consider ___slong2fs the culprit, or whether I > can perform any additional tests. > > When starting with 100 I obtain 144, for 116 I obtain 208, not sure that > this makes any sense. I guess it is time to look closer into slong2fs, > but I find different source files in the sdcc tree. Exciting, I found it ... :) Maybe that's just part of the solution, but I find that sdcc/device/lib/pic14/libsdcc/ulong2fs.c if(a < HIDDEN) { do { a<<=1; exp--; } while (a < HIDDEN); } causes the problem. Right underneath I find a commented piece of code - why was it commented in the first place? I replaced the version above by the part below, without the ifdef, now my example works. #if 0 while (a < HIDDEN) { a <<= 1; exp--; } #endif I then looked in sdcc/devices/lib/_ulong2fs.c, which also uses the bottom version. Why do different devices have copies of (almost) the same code in various places? Shouldn't they all use the same to avoid situations like this? Is there any kind of unit or regression test that we can add to avoid this coming back? Since I am not a developer but merely a user of few weeks experience, can I ask someone on this list to help me by bringing in the fix (and a test if possible)? It might also be desirable to review the other implementations of the same function, as far as more exist. Kind regards, //Daniel |
From: Philipp K. K. <pk...@sp...> - 2013-02-20 15:38:32
|
On 20.02.2013 13:08, Daniel Michalik wrote: > I then looked in sdcc/devices/lib/_ulong2fs.c, which also uses the > bottom version. Why do different devices have copies of (almost) the > same code in various places? > Shouldn't they all use the same to avoid situations like this? don't know for this case. In general, we have one version of each libary function implemented in C, that should work for all ports. Then some ports have specific versions , typically in asm to be able to use some special instruction. there also are a few cases of different versions due to differences in speed. Though I really wonder why anyone should care much for speed on the PIC ports, fixing them seems a much more important thing to do to me (but I don't know much about PICs, and only very rarely work on that port). Philipp |
From: Raphael N. <rn...@we...> - 2013-02-20 18:11:20
|
Hi, >> I then looked in sdcc/devices/lib/_ulong2fs.c, which also uses the >> bottom version. Why do different devices have copies of (almost) the >> same code in various places? >> Shouldn't they all use the same to avoid situations like this? > > don't know for this case. In general, we have one version of each > libary function implemented in C, that should work for all ports. Then > some ports have specific versions , typically in asm to be able to use > some special instruction. there also are a few cases of different > versions due to differences in speed. Though I really wonder why anyone > should care much for speed on the PIC ports, fixing them seems a much > more important thing to do to me (but I don't know much about PICs, and > only very rarely work on that port). We all agree on the fact that a single-source would probably be beneficial. In support of that, the libraries for the PICs are nearly completely C, no asm used for optimization. The problem (well, one of the problems, really :-[) with the PIC libraries is that they intentionally diverged from SDCCs standard library (before "my" time already, though I did nothing to revert that). When the library started to diverge, source files were copied from devices/lib to the pic specific directories in order not to pollute the common libraries (I assume). In doing so, the build system would then have to be set-up in a way to gather source files from various (i.e., two) places, including some ../ location, which (to me, and seemingly to the original author) looked bad: not self-contained, hard to split from the rest of the library (as if that was ever going to happen), and generally possibly tricky when considering out-of-source builds as well. I am not saying that I like the way things are right now, volunteers to clean it up would be welcome though I would not have much time to review and/or apply their efforts. But as things are, this is probably not going to change any time soon. Regarding regression tests: All other ports participate in regular regression tests, and they are highly unlikely to produce bad code for such fundamental operations. Both PIC ports, however, still need more love to be able to compile (not yet talking about passing) them. Philipp has done some investigations and postulated that there seems to be not that much missing in (I think) the PIC16 (18fxxx devices) backend to get there, but again: Nobody is actively working on getting the PIC ports up to speed. I'll be happy to apply your fix if and when I can verify the bug and solution using a simulator such as gpsim (*winkwink* no need for a real device here, but you may need to change the target device to one supported by gpsim). Thanks to the OP for the report and tracking the problem down to its source. Raphael |
From: Philipp K. K. <pk...@sp...> - 2013-02-20 23:44:53
|
On 20.02.2013 19:11, Raphael Neider wrote: > The problem (well, one of the problems, really :-[) with the PIC > libraries is that they intentionally diverged from SDCCs standard > library (before "my" time already, though I did nothing to revert > that). When the library started to diverge, source files were copied > from devices/lib to the pic specific directories in order not to > pollute the common libraries (I assume). In doing so, the build system > would then have to be set-up in a way to gather source files from > various (i.e., two) places, including some ../ location, which (to me, > and seemingly to the original author) looked bad: not self-contained, > hard to split from the rest of the library (as if that was ever going > to happen), and generally possibly tricky when considering > out-of-source builds as well. Hmm, the other ports manage to do with just a few port-specific lines in the Makefile.in. E.g. when I made the r2k port, I just had to make a small change in the Makefile.in copied from the z80 port to state that I want the generic strlen() instead of a port-specific one. But maybe there's some pic-specific issue I'm overlooking here. Philipp P.S.: The mcs51 port library is a mess of its own though, seems even worse than the pics, with mcs51-specific stuff in the generic files, #ifdef'ed. |
From: Raphael N. <rn...@we...> - 2013-02-20 19:47:07
|
Hi, I had a look at the problem. Actually, if (a < HIDDEN) { do { /* something */ } while (a < HIDDEN); } is equivalent to while (a < HIDDEN) { /* something */ } and the latter saves code for one comparison. So changing that *should* not break/heal anything. I did not check that. I dug into the code generator to find what goes wrong: "a" resides in (MSB) r0x1004 ...r0x1007 (LSB), "exp" is in (MSB) r0x100B r0x100A (LSB) with a temporary in r0x1009 r0x1008. HIDDEN is 0x00800000, so a < HIDDEN is turned into !(a & 0xFF800000), which is correct. ; .line 72; "ulong2fs.c" if(a < HIDDEN) { MOVF r0x1005,W BTFSC r0x1005,7 GOTO _00114_DS_ ==> Bit 7 in the third byte of a is set ==> a is not less than HIDDEN, bail out. Great. MOVF r0x1004,W ANDLW 0xff BTFSS STATUS,2 GOTO _00114_DS_ ==> The MSB of a is not 0, bail out. Great. _00110_DS_ ; .line 74; "ulong2fs.c" a<<=1; BCF STATUS,0 BANKSEL r0x1007 RLF r0x1007,F RLF r0x1006,F RLF r0x1005,F RLF r0x1004,F ; .line 75; "ulong2fs.c" exp--; MOVLW 0xff ADDWF r0x1008,F BTFSS STATUS,0 DECF r0x1009,F ; .line 76; "ulong2fs.c" } while (a < HIDDEN); MOVF r0x1005,W BTFSS r0x1005,7 GOTO _00110_DS_ ==> This is completely bogus: We should bail out (GOTO _00114_DS_) if (a & 0x00800000) OR if (a & 0xff000000). Instead we resume the loop if any one of the conditions is false. This should read # MOVF r0x1005,W # BTFSC r0x1005,7 # condition reversed # GOTO _00114_DS_ # different label, may need to be __new_label__ to update exp from its temporary, but I dont care for now MOVF r0x1004,W ANDLW 0xff BTFSC STATUS,2 // Skip if ZERO flag == 0, i.e., result not 0? GOTO _00110_DS_ // taken when ZERO flag == 1, i.e., result was 0 ==> a & 0xFF000000 == 0, so remembering a & 0x00800000 == 0 as well, we have a < HIDDEN and stay in the loop. Great. __new_label__ # The following is caused by using exp-- instead of --exp and preserves an unused copy of the original value MOVF r0x1008,W MOVWF r0x100A MOVF r0x1009,W MOVWF r0x100B ;;100 MOVF r0x1007,W _00114_DS_ I have also had a look at the pic source (src/pic14/gen.c, genAnd(), genJumpTrueOrFalse(), and friends), but this code is so arcane (or incompletely ported from its source) that I will probably not be able to actually fix this until, say, this weekend. It may take considerably longer ... Considering that the solution you provided merely hides the bug, I'll not commit it for now. Sorry for the bad news. > Since I am not a developer but merely a user of few weeks experience, > can I ask someone on this list to help me by bringing in the fix > (and a test if possible)? It might also be desirable to review the other > implementations of the same function, as far as more exist. As stated above, the implementations are semantically equivalent (unless I am mistaken) and need not be touched except for efficiency, as the while(){} construct seems to yield slightly more compact code. Kind regards, Raphael |
From: Daniel M. <sd...@bl...> - 2013-02-24 19:31:00
|
Raphael, On Wed, Feb 20, 2013 at 08:46:58PM +0100, Raphael Neider wrote: > I had a look at the problem. Actually, [code that produces a broken long2fs] > is equivalent to [code that coincidentally works] > and the latter saves code for one comparison. So changing that > *should* not break/heal anything. Yes, you are right of course. Thanks for investigating further, I read the remainder of your post with great interest. Unfortunately this is a little bit too much for me in the moment, I wont be able to find the time to acquire the necessary background knowledge to come up with a solution for it. So I'll have to leave it like this, but I hope discovering the problem and documenting it here helped a bit. [...] > As stated above, the implementations are semantically equivalent > (unless I am mistaken) and need not be touched except for efficiency, > as the while(){} construct seems to yield slightly more compact code. Yes, I agree that this bug should not be hidden. I hope other users find these emails in case they use the function themselves, until this is fixed properly. Is there an issue tracker where this should be filed? Kind regards, //Daniel |
From: Raphael N. <rn...@we...> - 2013-02-24 22:30:14
|
Hi, > Yes, you are right of course. Thanks for investigating further, I read > the remainder of your post with great interest. Unfortunately this is a > little bit too much for me in the moment, I wont be able to find the > time to acquire the necessary background knowledge to come up with a > solution for it. So I'll have to leave it like this, but I hope > discovering the problem and documenting it here helped a bit. I hope it helped: I have committed a fix as r8441 and would highly appreciate if you could verify that the bug is gone. (I would also appreciate if you informed me that the bug persists, though that report would be somewhat less appreciated ;-).) > Is there an issue tracker where this should be filed? In fact, there is. I took the liberty of putting it there already: https://sourceforge.net/tracker/index.php?func=detail&aid=3605855&group_id=599&atid=100599# Best regards, Raphael |
From: Daniel M. <sd...@bl...> - 2013-02-24 23:02:21
|
Raphael, On Sun, Feb 24, 2013 at 11:29:37PM +0100, Raphael Neider wrote: > I hope it helped: I have committed a fix as r8441 and would highly > appreciate if you could verify that the bug is gone. (I would also > appreciate if you informed me that the bug persists, though that > report would be somewhat less appreciated ;-).) This is fantastic news. I am confirming that r8441 fixes the problem. I now see the expected behaviour with both, the old and the new implementation of ulong2fs.c. Thank you! //Daniel |