Libfxcg is by no means perfect.
I would normally just submit a pull request however there is no guarantee that affect people will be aware that it is a bug in libfxcg and not their code and how to fix it. I have long had these fixed in my fork but never bothered to really document my changes. I think having a topic will get noticed by more people so that they can too make these fixes and point out any other bugs that they may have found.
All bugs listed here affect the most recent version of libfxcg as of the time of writing this post and can be found here:
https://github.com/Jonimoose/libfxcgOf course older versions are affected as well. I know for a fact that the linker script fix affects even the old PrizmSdk 0.3 from 2011 located here
http://www.cemetech.net/news.php?id=486 I could not see if other bugs affect this version as the source is not included for it's libc and I have not made tests for it and see no need to as it is obsolete.
Lets not limit ourselves to bugs or terrible code that affect both. If you see a bug that affects only libfxcg it is best reported. Really no-one should be using the old PrizmSdk 0.3 from 2011 however should you find a bug that affects it and you want to post it go ahead it may give people another reason to upgrade.
1. fread is broken.
The way fread should work is that every time it is called the data following the read data is read the next call. However the function will always read from the beginning of the file. As of writting the file located here
https://github.com/Jonimoose/libfxcg/blob/master/libc/stdio.c says in fwrite
size_t ret = Bfile_ReadFile_OS(handle_tonative(stream->fileno), buffer, n, 0);
However it should be
size_t ret = Bfile_ReadFile_OS(handle_tonative(stream->fileno), buffer, n, -1);
The only difference is that I changed the zero to a negative one. What this means is that it continues to read where it left off.
2. Linker script does not correctly handle BSS.
When you have static variables in a library you may be surprised to find that global and static variables are not initialized. Fix this by changing
.bss : {
_bbss = . ;
*(.bss) *(COMMON);
_ebss = . ;
} >ram
To
.bss : {
_bbss = . ;
*(.bss)
*(.bss*)
*(COMMON)
_ebss = . ;
} >ram
The reason why this works is that the linker will insert things such as .bss_variable_name when compiled in a library. This puts it in the proper section and upon start-up it will be cleared just like it should.
3. The memove implementation is terrible.
Actually I had worse insults that I was tempted to put but I am sure whoever wrote it makes better code by now and I do not want to hold this against them but
void* memmove(void* destination, const void* source, size_t num) {
void* d = malloc(num);
memcpy(d, source, num);
memcpy(destination, d, num);
free(d);
return destination;
}
Yes that is right, I did not make this code up for slander purposes. It is really in there in string.c
Lets replace it with something better
void *memmove(void *dst, const void *src, size_t count){
//From dietlibc
char *a = dst;
const char *b = src;
if (src!=dst){
if (src>dst){
while (count--) *a++ = *b++;
}else{
a+=count-1;
b+=count-1;
while (count--) *a-- = *b--;
}
}
return dst;
}
The reason it is considered a bug is because if you need to copy something larger than the small heap the function will fail.
4.The exit function crashes the calculator on purpose.
void exit(int status) {
fprintf(stderr, "TERMINATED (%i)", status);
// We don't have a clean way to exit (right now), so just crash it.
((void (*)())1)(); // Unaligned instruction
}
Actually we do have a clean way to exit. When this function is called there is usually a problem so the user should be able to see the error code. To do this we print the error message and use the GetKey function, from there the user can press the menu key when they are done reading the error code so just replace it with:
void exit(int status) {
fprintf(stderr, "TERMINATED (%i)\nPress menu key to exit\n", status);
int key;
while(1)
GetKey(&key);
}
Also edit the header stdlib.h change
void exit(int status);
to
void exit(int status) __attribute__ ((noreturn));
Also while you are there fix the abs definition so that it matches the source code.
Change
long abs(long n);
To
int abs(int x);
And now add #include <stdlib.h> to the top of stdlib.c
The reason for doing this is so that when compiling stdlib.c gcc knows that this function does not return in addition to the program you compile with gcc and libfxcg that involves exit. Yes we could just duplicate the __attribute__ ((noreturn)) to also be in stdlib.c but I feel that this is a cleaner solution. Also if stdlib.c fails to compile due to you including it, that means the headers suck and need improvement which is why I fixed the abs definition.
5. You cannot actually see the errors.
I don't really consider this a bug per say, however I believe it is much better to be able to see the error messages on screen instead of sending anything from stderr to the serial port.
So replace both fwrite_term and fwrite with
static size_t fwrite_term(const void *ptr, size_t size, size_t nitems,FILE *stream,int col){
static int termXfxcg,termYfxcg;
char *buffer = alloca(1 + nitems * size);
if (buffer == NULL) {
IOERR(stream, ENOMEM);
return 0;
}
memcpy(buffer, ptr, nitems * size);
buffer[nitems * size] = '\0';
const char *outp = buffer;
char *eol;
// Loop over all lines in buffer, terminate once we've printed all lines.
do{
eol = strchr(outp, '\n');
if(eol) {
*eol = '\0';
}
// Cast to wider type for correct pointers
PrintMiniMini(&termXfxcg, &termYfxcg, outp, 0, col, 0);
// CR/LF if applicable
if(eol){
termXfxcg = 0;
termYfxcg += 10;
outp = eol + 1;
}
if(termYfxcg>=200)
termYfxcg=0;
}while (eol);
return nitems;
}
// TODO make ptr, stream restrict for optimization
size_t fwrite(const void *ptr,size_t size,size_t nitems,FILE *stream){
if (isstdstream(stream)) {
if(stream->fileno==2){
// stderr: display but red font
//return fwrite_serial(ptr, size, nitems, stream);
return fwrite_term(ptr, size, nitems, stream,TEXT_COLOR_RED);
}else if(stream->fileno==1){
// stdout: display
return fwrite_term(ptr, size, nitems, stream,TEXT_COLOR_BLACK);
}else{
// stdin..?
}
}
// TODO this must be able to fail, but how?
Bfile_WriteFile_OS(handle_tonative(stream->fileno), ptr, size * nitems);
return nitems;
}
Now in stdio.h remove the lines that say
unsigned short termx, termy;
6. fopen is also broken.
It is common knowledge that forgetting to null terminate a string that should be null terminated will cause a bug.
Look for:
Bfile_StrToName_ncpy(chars16, path, plen);
and replace it with
Bfile_StrToName_ncpy(chars16, path, plen+1);
This will null terminate the string.
