How to implement searching words in a file and then writing in another file?

Question

Im trying to make SIC assembler in c, made a small test program to search for "START" keyword and get the starting address from the assembly code file. then write them in another file as first pass. code didn't work. mind telling whats wrong with it?

#include<stdio.h>
#include<stdlib.h>
#include<string.h>

int main(int argc, char *argv[])
{
    FILE *source;
    FILE *intermediate;
    rewind(source);
    char word[10];
    unsigned int address = 0;
    char start[5] = "START";
    source = fopen(argv[1],"r");
    intermediate = fopen(argv[2],"r+");
    while(strcmp(word,start) != 0)
    {
        fscanf(source, "%s", word);
    }
    fscanf(source, "%x", address);
    fprintf(intermediate, "%x", address);
    fprintf(intermediate, "%s", word);

    return(0);
}

this is the assembly input:

  COPY   START  1000
  FIRST  STL    RETADR
  CLOOP  JSUB   RDREC
         LDA    LENGTH
         COMP   ZERO
         JEQ    ENDFIL
         JSUB   WRREC
         J      CLOOP
  ENDFIL LDA    EOF
         STA    BUFFER
         LDA    THREE
         STA    LENGTH
         JSUB   WRREC
         LDL    RETADR
         RSUB
  EOF    BYTE   C'EOF'
  THREE  WORD   3
  ZERO   WORD   0
  RETADR RESW   1
  LENGTH RESW   1
  BUFFER RESB   4096

When do you call `rewind(source)`, and when do you open the file for `source`? — Some programmer dude, Mar 21 '18 at 08:20
Also remember that `char` strings in C are really called ***null-terminated** byte strings*. That *null-terminated* bit is important, and means that a string of five characters really need the space for *six* characters. Now go back and look at your variable `start` and think about this. — Some programmer dude, Mar 21 '18 at 08:21
`fscanf(source, "%x", address);` ==> `fscanf(source, "%x", &address);` note the `&`. — Weather Vane, Mar 21 '18 at 08:24
`fscanf(source, "%s", word);` into a 10 char buffer is dangerous... buffer overflow is likely. — Support Ukraine, Mar 21 '18 at 08:24

Support Ukraine · Accepted Answer · 2018-03-21T09:35:32.197

As I and others pointed out in comments, you have several errors.

1) Use of uninitialized variables (word)

2) rewind on file not open

3) Too short arrays for a string (start)

4) Wrong arguments to fscanf (address instead of &address)

Besides that there are other problems like:

1) Risk of buffer overflow in fscanf

2) Missing handling of EOF

3) Missing close of files

After fixing the above, the code could look like:

#include<stdio.h>
#include<stdlib.h>
#include<string.h>

int main(int argc, char *argv[])
{
    FILE *source;
    FILE *intermediate;
    char word[10];
    unsigned int address = 0;
    const char start[] = "START";

    source = fopen(argv[1],"r");       // Missing check of argc and successful file open
    intermediate = fopen(argv[2],"w"); // Missing check of argc and successful file open

    do
    {
      if (fscanf(source, "%9s", word) != 1)
      {
        printf("Illegal source file. START not found\n");
        fclose(source);
        fclose(intermediate);
        exit(1);
      }
    } while(strcmp(word, start) != 0);

    fscanf(source, "%x", &address);  // Missing check of return value being 1

    fprintf(intermediate, "%x", address);
    fprintf(intermediate, "%s", word);

    fclose(source);
    fclose(intermediate);

    return(0);
}

Finally notice that the code still miss important checks like the number of arguments supplied, successful file open and return value from fscan when scanning the address.

Might as well make `start[]` a `const char start[]`, so the compiler doesn't have to make a copy of the string in writeable memory on the stack. (I know the assembler itself doesn't *have* to be efficient, and there are far worse performance issues here, like the whole idea of using `fscanf` to read one word at a time and copying to another file just to discard characters before `START` and turn `START 0x1234` into `1234START`...) Also, you could use a `do{}while()` loop to avoid a pointless compare before reading the first word, so you can drop the initializer for it. — Peter Cordes, Mar 21 '18 at 09:07
@PeterCordes Good suggestions. I have added them to the answer. Thanks. — Support Ukraine, Mar 21 '18 at 09:36
`do{}while()` is under-appreciated, IMO. If you're used to thinking in assembly, [that's how asm loops are normally written](https://stackoverflow.com/questions/47783926/why-are-loops-always-compiled-like-this), so it's a handy way to let the compiler know that your loop will always run once, even in cases where the compiler can't prove that on its own (like it often can with a `for (i=0 ; i < n; i++)`). — Peter Cordes, Mar 21 '18 at 10:22

How to implement searching words in a file and then writing in another file?

1 Answers1