optimization, file reading and general screen questions in C

[chocsplease]

Hi,

I continue to work on my new version of the cli info command as a way of re-learning c and how the Amiga does things.

The current output is below -

SMBFS0:   488Gb     235Gb      252Gb  48%     0   Read and Write PCbackup
PC0:      Unable to read disk Dos type 42414400
CF0:      No Disk Present
RAM:      625Kb     625Kb        0b  100%     0   Read and Write Ram Disk
DH0:       94Mb      91Mb     2649Kb  97%     0   Read and Write WB
DF0:      837Kb    3416b       834Kb   0%     0   Read and Write Empty
CC0:      No Disk Present
DF1:      No Disk Present
DH1:     3471Mb    3353Mb      118Mb  96%     0   Read and Write Games
DH2:     3567Mb    3487Mb       80Mb  97%     0   Read and Write Apps

Available Volumes:
PCbackup Mounted
Ram Disk Mounted
Games1 Mounted
Aps Mounted
Empty Mounted
WB Mounted

It still needs work, but I have an odd issue with the size of my executable, compared with the original which for me is - 1980 bytes, mine is 40900!

Ok I may not be a brilliant programmer, but I'm not that bad!

I'm compiling with - gcc -lamiga -noixemil -O2 info64.c - at present I don't care that the executable is a.out

My includes are -

#include <proto/dos.h>
#include <proto/exec.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <exec/memory.h>

I'm using strcat, printf, AllocVec memcpy etc so think I need all of the above. I get errors if I don't include them.

Anyone any ideas? I must be pulling something in that is causing this but don't know what, or does gcc default to including debug code?

Moving on from that can anyone suggest how to do the following, if you can point me at an example that would be great.

• read the number of colours on the current screen
• find out how much red,green and blue each colour has
• find out the size of a file, I think this is via ExamineFH, but I am not 100% on what I need to pass it
• find out what the user has placed on the command line, now this is somehow by using ReadArgs, but I am at a loss as to how the template works.

Finally if I allocate a chunk of memory, can I set it up to act like an array of strings? Would inserting nulls at the end of the 'lines' and using strlen be sufficient (I suspect not).

Many thanks for any help that you can give.

[Thorham]

Originally Posted by chocsplease

It still needs work, but I have an odd issue with the size of my executable, compared with the original which for me is - 1980 bytes, mine is 40900!

Try not to use the C run time. Use AOS calls and your own functions instead. That way you also don't have to include things like stdio.h.

strcat: Write it yourself. Very easy.
printf: Use dos.library VPrintf.
AllocVec: Is an exec.library function, is fine as it is.
memcpy: Use exec.library CopyMem.

Originally Posted by chocsplease

Finally if I allocate a chunk of memory, can I set it up to act like an array of strings?

Yes, just make a simple unsigned char pointer array.

Originally Posted by chocsplease

Would inserting nulls at the end of the 'lines' and using strlen be sufficient (I suspect not).

Yes. A C string is always 0 terminated, so you can do that. Any extra zeros won't be counted by strlen of course. Also, write your own strlen to avoid using the C run time (very easy).

[thomas]

Originally Posted by chocsplease

It still needs work, but I have an odd issue with the size of my executable, compared with the original which for me is - 1980 bytes, mine is 40900!

I guess that GCC adds a lot of debug information to the exe file. Try -s option to remove it.

I'm using strcat, printf, AllocVec memcpy etc so think I need all of the above. I get errors if I don't include them.

You should avoid stdio and stdlib. There are replacements in AmigaOS, for example Printf instead of printf, CopyMem instead of memcpy, RawDoFmt instead of sprintf.

[alkis]

Ultimately, you want to reach the point where:

m68k-amigaos-gcc -nostartfiles -nodefaultlibs -nostdlib -s -static -noixemul -o executablename obj1.o obj2.o ...

compiles and links.

Use the above, and for each function the compiler complains that it can not find to link either use an alternative from AmigaOS or roll your own.

[chocsplease]

Many thanks for all the replies.

Just adding -s to my compile line reduces the executable to 14k, still not great but a lot better than 40+.

However I am having a few very odd problems converting from the standard c routines to Amiga specific.

Can anyone explain the difference between the stdio c printf and the Amiga Printf? I am getting the compile warning 'Warning: Initialisation makes integer from pointer without cast' on this line :

Printf("line %s\n", buffer);

buffer is defined thus

int findsizechars (char buffer[])

As far as I can tell there are no integers involved here at all!

More seriously this code -

if (err==0) 
{
           	        
       	err= findsizechars(readstr); 
           	
}

gives

error = 16459

And nothing else when I run the executable. (err is just a plain int)

where as

err= findsizechars(readstr);

In other words no If statement - works fine (but I then have a bug which the if is supposed to cure)

[alkis]

The warning comes from the trickery gcc has to do in order to inline AmigaOS calls.

Let's take this for reference

#include <proto/dos.h>

int main(int argc, char *argv[]) {
  char buffer[] = "Alkis";
  Printf("Hello %s\n", buffer);
  return 0;
}

if you run gcc with -E you'll see the source after it gets preprocessed.

int main(int argc, char *argv[]) {
  char buffer[] = "Alkis";
  ({ULONG _tags[] = {buffer}; ({ CONST_STRPTR _VPrintf_v1 = (("Hello %s\n")); const APTR _VPrintf_v2 = ((const APTR) _tags); LONG _VPrintf_re2 = ({ register int _d1 __asm("d1"); register int _a0 __asm("a0"); register int _a1 __asm("a1"); register LONG _VPrintf_re __asm("d0"); register void *const _VPrintf_bn __asm("a6") = (DOSBase); register CONST_STRPTR _n1 __asm("d1") = _VPrintf_v1; register const APTR _n2 __asm("d2") = _VPrintf_v2; __asm volatile ("jsr a6@(-""0x3ba"":W)" : "=r" (_VPrintf_re), "=r" (_d1), "=r" (_a0), "=r" (_a1) : "r" (_VPrintf_bn), "rf"(_n1), "rf"(_n2) : "fp0", "fp1", "cc", "memory"); _VPrintf_re; }); _VPrintf_re2; });});
  return 0;
}

buffer gets assigned to ULONG _tags[] and there is your warning.
If you want the warning to go away, try casting buffer to (LONG).

or...

You can tell gcc not to inline.
you don't include <proto/dos.h> and instead you include <clib/dos_protos.h>

#include <clib/dos_protos.h>

int main(int argc, char *argv[]) {
  char buffer[] = "Alkis";
  Printf("Hello %s\n", buffer);
  return 0;
}

now, let's see what -E tells us

int main(int argc, char *argv[]) {
  char buffer[] = "Alkis";
  Printf("Hello %s\n", buffer);
  return 0;
}

right. No substitutions. Now, the program needs to link against amiga.lib to find Printf.
You don't get any warnings about that call now. You get an extra indirection call, your program calls code in amiga.lib which then calls the real Printf code in OS. But IO calls are never time critical, so that shouldn't matter.

I don't undestrand your second question, sorry. Please, provide a minimum working example source code.

[chocsplease]

Many thanks for such an extensive and useful reply, it always helps to know what's happening in the background, I had no idea the -E option even existed.

Actually looking at my code more closely, I can see the goof I made in the casting - oops.
Anyway onward...

Originally Posted by alkis

<snip>
I don't undestrand your second question, sorry. Please, provide a minimum working example source code.

To be honest neither do I! the 3 relevant functions are below.

int ReadPrefs(void) 
{
    
     BPTR fh;
     int i, err, temp;
     UBYTE readstr[10];
     UBYTE *buffer;
   

     /* Open existing file for reading */
     fh = Open("c:info64.prefs", MODE_OLDFILE);
     if (fh) /*if file exists */
    {
        /* Read a string and check for End of File */
        while (buffer = FGets(fh, readstr, 10L)) 
        {
            /* make sure there is a null at the end of the buffer*/
            readstr[3]='\0';
            /* Convert read string to uppercase */
            for ( i=0; i<4; i++ )        /* don't need to make the null upper case */
            {
                readstr[i]=toupper((unsigned char)readstr[i]);
            }
            
            /*Printf("line %s\n", readstr);*/
            
            /* now find the sizes print error if we can't */
         
           if (err==0) 
           {
                        
               err= findsizechars(readstr); 
            }
        
        }
        Close(fh);
     }

/*    err=1; <<<<<<<<<*/

     Printf("error = %d\n",err);
     


     if (err==1) prtprefformat(); 
     
    /* Printf("EOF found\n"); */
     
return err;     
}

/* find the size chars (K,M,T etc) and update the global array */

int findsizechars (char buffer[])
{
    int err=0;
    if (buffer[0]>=' ') /*ignore lines with chars less than space, cr, lf etc*/
    {
    printf("line %s\n", buffer);
        if ((buffer[1]=='=') && (buffer[2]>='A'))
        {
            switch (buffer[0])
            {
                case 'K' :
                    sizedefs[0]=buffer[2];
                    break;
                
                case 'M' :
                    sizedefs[1]=buffer[2];
                    break;
            
                case 'G':
                    sizedefs[2]=buffer[2];
                    break;
                
                case    'T':
                    sizedefs[3]=buffer[2];
                    break;
                
                case 'P':
                    sizedefs[4]=buffer[2];
                    break;
                case 'b':
                    sizedefs[5]=buffer[2];
                    break;
                
                case 'N' :  /* user wants fake values for gb,tb & pb (decimal not hex)*/
                    if (buffer[2]=='F')
                    {            /*     KB        MB         GB           TB             PB*/
                        multipliers[0]=0x2800;
                        multipliers[1]=0xa00000;
                        multipliers[2]=0x2540BE400;
                        multipliers[3]=0x9184E72A00;
                        multipliers[4]=0x16345785D8A000;
                    }
                    else if (buffer[2]!='T')
                    {
                        /*Printf("error 1\n");*/
                        err=1;
                    }
                    break;
                    
                default:
                    /*Printf("Buffer %s, error\n",buffer);*/
                    err=1;
                    break;
            }
        }
        else 
        {
            err=1;
            Printf("error 3\n");
        }
    }                
return err;
}

void prtprefformat(void)
{
    Printf("The format of the info64.prefs file is :\n\n");
    Printf("<char1>=<char2> where <char1> is any of b,K,M,G,T,P or N for byte,Kilobyte, Megabyte,Gigabyte,Terabyte,PetaByte: N is defined below. \n");
    Printf("and <char2> is the character that your language uses for these values.\n\n");
    Printf("E.g. G=Z  \n\nYou must separate each with a new line.\nYou only need to state the ones that are different from the above defaults.\n");
    Printf("The line N=F sets Giga, Tera and Peta bytes to be their decimal values (eg 1,000,000,000 bytes = 1 Giga byte) rather than the correct\n");
    Printf("binary values (ie 1,073,641,824 bytes = 1 Giga byte). Defaults to N=T, so the correct binary value.\n");
    Printf("Note: The defaults are used if the info64.prefs file is not found in c: .\n");
}

The lines in question are close to the end of the function ReadPrefs.

If I call findsizechars inside the if statement I get just Error = 16459 as output and nothing else. if I call findsizechars outside the if then the code works and I get the output in the first post.

I am completely baffled on this, I have no idea what this error is - my code does not have any lines that print it so it must be a gcc runtime error, but Google has failed me and I can't find out what it means.

I've been able to code around this by changing the lines around the if to

 err= findsizechars(readstr);
           if  (err >0)
           {
               Printf("There is an error in the info64.prefs file at line \n%s \n",(long)readstr);
               break;
           }

This works as the call to findsizechars is outside the if, but I really would like to know why the other code goes nuts.

[idrougge]

if (err==0) 
           {
                        
               err= findsizechars(readstr); 
            }

Unlike Basic, a variable doesn't have any guaranteed value unless you give it one. err may be 0, -1 or 34012. So it's unlikely that the above if is ever evaluated as true and findsizechars is not called.

[matthey]

Originally Posted by idrougge

Unlike Basic, a variable doesn't have any guaranteed value unless you give it one. err may be 0, -1 or 34012. So it's unlikely that the above if is ever evaluated as true and findsizechars is not called.

Local variables start with whatever trash value is in the memory location selected to hold the variable (the problem in chocsplease's code). Global variables are initialized to zero at program start.

[idrougge]

Good catch on the global variables. Wonder why they thought that was a good idea.

[ajk]

Global variables are static, so initializing them to zero once in the beginning of the program doesn't really add any overhead. But for local, temporary variables, it might, so it's left up to the programmer. It is a little inconsistent.

[chocsplease]

Many thanks for spotting my coding goof. I can't believe I missed something so fundamental.... this used to be so easy <sigh>

Could someone help me out with the other questions I had in my original post?
I've sorted one - finding the size of a file - but I am still stumped on :

• reading the number of colours on the current screen
• finding out how much red,green and blue each colour has
• finding out what the user has placed on the command line, now this is somehow by using ReadArgs, but I am at a loss as to how the template works.

And a couple of new ones -

• Finding out if a programme is started from the cli or workbench
• Is it possible to draw a box in the cli window at the current text curser position in such a way that it will scroll with the text?

I suspect the last of these is complicated if it is even possible.

Many thanks.

[Thorham]

Originally Posted by chocsplease

reading the number of colours on the current screen
finding out how much red,green and blue each colour has

You can do this for Kickstart 3. It prints all the colors of the screen to the console:

#include <proto/intuition.h>
#include <proto/graphics.h>

#include <stdio.h>

int main(void)
{
    struct Screen *screen;
    unsigned char screenName[] = "Workbench";
    unsigned long colorTable[256 * 3];

    struct ViewPort *viewPort;
    struct ColorMap *colorMap;

    int i;
    unsigned long r, g, b;

    screen = LockPubScreen(screenName);

    if (screen)
    {
        viewPort = &screen->ViewPort;
        colorMap = viewPort->ColorMap;

        GetRGB32(colorMap, 0, colorMap->Count, colorTable);

        UnlockPubScreen(NULL, screen);

        for (i = 0; i < colorMap->Count; i++)
        {
            r = colorTable[i * 3 + 0] >> 24;
            g = colorTable[i * 3 + 1] >> 24;
            b = colorTable[i * 3 + 2] >> 24;

            printf("Color: %03d  red: %03d  green: %03d  blue: %03d\n", i, r, g, b);
        }
    }

    return 0;
}

Originally Posted by chocsplease

finding out what the user has placed on the command line

Very easy. This prints all arguments to the console:

#include <stdio.h>

int main(int argc, char *argv[])
{
    int i;

    if (argc > 0)
    {
        for (i = 0; i < argc; i++)
        {
            printf("Argument: %d = %s\n", i, argv[i]);
        }
    }

    return 0;
}

Note that I used SASC6.58 to compile both examples.

Originally Posted by chocsplease

using ReadArgs, but I am at a loss as to how the template works

Don't bother. Use argc and argv[], and print your own help text. Those templates don't seem too useful and aren't as clear as your own descriptions can be.

Originally Posted by chocsplease

Finding out if a programme is started from the cli or workbench

This isn't difficult, I just don't remember how to do it

Originally Posted by chocsplease

Is it possible to draw a box in the cli window at the current text curser position in such a way that it will scroll with the text?

Sounds difficult and very hacky. Don't do it.

[Leffmann]

Originally Posted by chocsplease

Finding out if a programme is started from the cli or workbench

The C startup will detect it and let you know by setting argc to 0, in which case you cast argv to struct WBStartup* to get more information.

[meynaf]

Originally Posted by Thorham

Don't bother. Use argc and argv[], and print your own help text. Those templates don't seem too useful and aren't as clear as your own descriptions can be.

ReadArgs is very practical and templates aren't hard to use. I will nevermore use anything else !
Gives me automatic syntax checking in command lines. I really like this function.
But okay, i'm not using C

Each template has a name and eventual modifiers.
/A indicates required - will fail if not there (e.g. "FILE/A" for required file name).
/S indicates switch - if that keyword is there, true, if not, false.
/N indicates number (decimal).
/K indicates keyword - the entry must be preceded with that text or it won't match.
/M indicates multiple entry - the function will return an array.
Other switches exist but I don't use them.
With nothing, it's normal string, null if not present.
You can specify keyword equivalences with "=".

If the user types "?" then he will see the template and is asked to enter the parameters.

When in doubt you can play with AmigaDos commands - they more or less all behave like this.

[Thorham]

Originally Posted by meynaf

But okay, i'm not using C

Yeah, I was just going to say something about that. But perhaps you're right, and they're not as useless as I thought.

[chocsplease]

Many thanks for all the replies,

I have been trying to avoid stdio out as my executable is much larger than I would expect (14+k) and it has been suggested that doing so will reduce this.

Unfortunately I have hit another guru. I know the line that's causing it I just don't know why - here's the code -

int ReadPrefs(void) 
{
    struct FileInfoBlock *fib = malloc(sizeof(struct FileInfoBlock));
     BPTR fh;
     int i,ii, err=0, temp=0,success=0;
     UBYTE readstr[10];
     UBYTE *buffer;
     unsigned long long size;
     long fileread;
     char *msg1,*msg2; /*msg1 is the default message, msg2 is the replacement if any*/
   
       if (fib == NULL)
       {
           Printf("Cannot allocate memory for prefs file\n");
           return 1;
       }
   
     /*printf("Reading file numbers.dat.\n");*/
     /* Open existing file for reading */
     fh = Open("c:info64.prefs", MODE_OLDFILE);
     if (fh) /*if file exists */
    {
        /*check file size*/
        success = ExamineFH(fh,fib);
        if (success==0) 
        {
            free (fib);
            Printf("Error - cannot get size of prefs file, using defaults\n");
            close (fh);
            return 0;
        }
        else    
        {
            size= fib->fib_Size+1;
            printf("Hurray! %ld \n",(long)size);
            free(fib);
            PrefsFile = malloc(size);
        }
        printf("OK1\n");
        fileread = Read(fh,(APTR)PrefsFile,(long)size);
        (char)PrefsFile[size]='\0'; /*put null at end of file*/
        printf("OK2\n");
        for (i=0;i<size;i++)
        {
            /*sanity check file as best as we can*/
            if ((char)PrefsFile[i]<' ')
            {
                (char)PrefsFile[i]='\0'; /*swap carridge return for null at end of eachline turn into C strings*/
                printf("[%d]",PrefsFile[i]);
            }
            else if ((char)PrefsFile[i]=='%')
            {
                Printf("Illegal character in prefs file -%% is not allowed\n");
                err=1;
                return err;
            }
            else if (i==size-1) /*if we are at the end of the file, size is file len +1*/
            {
                if ((char)PrefsFile[i]=='\\') 
                {
                    Printf("Error in Prefs file, %c is not allowed without following n\n",'\\');
                    err=1;
                    return err;
                }
                if ((char)PrefsFile[i]=='=') 
                {
                    Printf("Error in Prefs file, %c must be followed by the new message\n",'=');
                    err=1;
                    return err;
                }    
                else /* i is not equal to size-1*/ 
                {
                    if (((char)PrefsFile[i]=='\\') && (toupper((char)PrefsFile[i+1])!='N'))
                    {
                        Printf("%c is not allowed without a following n\n",'\\');
                        err=1;
                        return err;
                    }
                }
            }
            
            if (PrefsFile[i]>=' ') Printf("%c",(char)PrefsFile[i]);
            
        }
        printf("OK3\n");
        msg1=msg2=PrefsFile; /*give pointers default ot the start of the prefs file*/
        
        printf("msg1=%s/n",msg1);
        
        /*now read through prefs setting messages etc*/
        for (i=0;i<size;i++)
        {
            printf("<%d>",i);
            if ((char)PrefsFile[i]=='=')
            {
                printf("prefs file equals found\n");
                /*temp change current pos to null and store the pos*/
                (char)PrefsFile[i]='\0';
                temp=strlen(msg1);
                printf("length of msg1 is %d\n",temp);
            
                /*make msg1 lowercase*/
                for (ii=0;ii<temp;ii++)
                {
                    msg1[ii]=tolower(msg1[ii]);
                }
                printf("msg1 is now %s\n",msg1);
                msg2=&(char)PrefsFile[i+1];    /*set replacement message to 1 char after equals*/
                printf("msg2 is now %s",msg2);
                err=update_message(msg1,msg2); /*change the required message or error if its not one we know*/
                (char)PrefsFile[i]='=';        /*put equals sign back*/
                if (err!=0) 
                {
                    Printf("Formatting error in prefs file - unknown message type?\n");
                    break;            /*got error so stop*/
                }    
            }
            else if ((i<size) && ((char)PrefsFile[i]=='\0')) /*at end of line so move msg pointers to new line, i will always be < size */
            {
                printf("updating msg pointers\n");
                msg1=msg2=&(char)PrefsFile[i+1];
                printf("updated ok!\n");
            }
        }
    
        
        
         
     }
     printf("about to close file\n");
    /*close(fh); <<<<<<crashes here*/
     printf("error=%d\n",err);
     if (err==1) prtprefformat(); 
     
    /* printf("EOF found\n"); */
     
return err;     
}

The problem is the (currently commented out) close(fh); line almost at the end of the function. I know it crashes here as U get the previous printf's output and the code runs with the line commented.

This is the output up until this point -

Hurray! 18 
OK1
OK2
[0][0][0][0][0]OK3
msg1=k*k/n<0><1><2><3>updating msg pointers
updated ok!
<4><5>prefs file equals found
length of msg1 is 1
msg1 is now m
msg2 is now min update message, mess1 -m, mess2 - m
<6><7>updating msg pointers
updated ok!
<8><9>prefs file equals found
length of msg1 is 1
msg1 is now t
msg2 is now tin update message, mess1 -t, mess2 - t
<10><11>updating msg pointers
updated ok!
<12><13>prefs file equals found
length of msg1 is 1
msg1 is now p
msg2 is now pin update message, mess1 -p, mess2 - p
<14><15>updating msg pointers
updated ok!
<16>updating msg pointers
updated ok!
<17>about to close file
error=0

What I cannot work out is why I get this crash

Anyone any clues? Once again I am totally stuck.

[paraj]

There might be more problems, but you're writing one byte past the end of "PrefsFile":

                PrefsFile = malloc(size);
        }
        printf("OK1\n");
        fileread = Read(fh,(APTR)PrefsFile,(long)size);
        (char)PrefsFile[size]='\0'; /*put null at end of file*/ <---- Writing past the end of the allocated data

[alkis]

It's the buffer overflow as paraj said. (probably)

A few other things
close(fh) should be inside the if (fh) {}
Try struct FileInfoBlock __aligned fib and then pass the address of it &fib (one thing less to malloc/free)

[meynaf]

You should write Close(fh), not close(fh).