7

I have an assignment. The program is to print the sum of all command line arguments in C. I tried this code it compiles but throws an error after passed arguments in the console. Below is the code.

/* Printing sum of all command line arguments */
#include <stdio.h>

int main(int argc, char *argv[]) {
    int sum = 0, counter;

    for (counter = 1; counter <= argc; counter++) {
       sum = atoi(sum) + atoi(argv[counter]);
    }
    printf("Sum of %d command line arguments is: %d\n", argc, sum);
}

After compiling it outputs a Segmentation fault (core dumped) error. Your experience may solve my problem.

Below is my edited code:

/* Printing sum of all command line arguments*/
#include <stdio.h>
#include <stdlib.h> // Added this library file

int main (int argc, char *argv[]) {
    int sum = 0, counter;

    for (counter = 1; counter < argc; counter++) {
        // Changed the arithmetic condition
        sum = sum + atoi(argv[counter]);
        // Removed the atoi from sum variable
    }
    printf("Sum of %d command line arguments is: %d\n", argc, sum);
}
20
  • 3
    @AnudeepSyamPrasad Whoever taught you to use "stdio.h" and atoi is not "the best", but rather a charlatan. Commented Feb 1, 2018 at 14:59
  • 1
    @Mawg Incorrect recommendations to post at CR is a hot potato on meta, see for example this fresh discussion: meta.stackoverflow.com/questions/362417/… Commented Feb 1, 2018 at 15:01
  • 2
    @Lundin when your code works, post it to our sister site codereview.stackexchange.com. A fine recomendation Commented Feb 1, 2018 at 17:33
  • 3
    @BjornA. C11 7.22.1 "If the value of the result cannot be represented, the behavior is undefined." Basically if you give it anything that is not an ASCII digit, the function is guaranteed to bug out. Unlike strtol family of functions, that have 100% equivalent functionality, except they don't bug out. Commented Feb 2, 2018 at 7:32
  • 2
    @Ian atoi assumes it gets spoon-fed a null-terminated string consisting of nothing but valid digits. If it gets anything else, it will bug out. There is no point in using it since the strtol family of functions have identical functionality (and more), and also proper error handling. It has nothing to do with multi-threading. Commented Feb 2, 2018 at 7:36

5 Answers 5

12

Because you are iterating until counter == argc, you are passign a NULL pointer to atoi(), It's simple, just rely on the fact that the argv array has a NULLsentinel, and do this

/* Printing sum of all command line arguments*/
#include <stdlib.h> /* For `atoi()' */
#include <stdio.h>  /* For `printf()' */

int main(int argc, char *argv[])
{
    int sum;
    sum = 0;
    for (int counter = 1; argv[counter] != NULL; ++counter)     {
        sum += atoi(argv[counter]);
    }
    printf("Sum of %d command line arguments is: %d\n", argc, sum);
}

Note that atoi(sum) is undefined behavior because sum is an int and is not a valid pointer. While atoi() will try to dereference it. Also aoti() stands for ascii to integer and sum is already an integer so the conversion doesn't make sense.

Finally, include stdlib.h for atoi(). I know you didn't include it because I have warnings enabled on my compiler and it warned me that atoi() was implicitly defined. It might work, but just because undefined behavior is that, UNDEFINED.

Also, note that there is no way to know whether the passed argument is an integer, because atoi() cannot perform error checking. You might want to use strtol() instead and check if all the values are integers.

So ... this is how you would write a more robust version of this program

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

int main(int argc, char *argv[])
{
    int sum;
    sum = 0;
    for (int counter = 1; argv[counter] != NULL; ++counter)     {
        char *endptr;
        sum += strtol(argv[counter], &endptr, 10);
        if (*endptr != '\0') {
            fprintf(stderr, "error: the `%d-th' argument `%s', is not a valid integer\n", counter, argv[counter]);
            return EXIT_FAILURE;
        }
    }
    printf("sum of %d command line arguments is: %d\n", argc, sum);
    return EXIT_SUCCESS;
}

EDIT: To address this comment

There is a possibility that argc == 0, for instance if you execute the program through one of the exec*() functions. In that case you must check before starting the loop or argv[counter] will be one element after the last, i.e. out of bounds.

Sign up to request clarification or add additional context in comments.

24 Comments

The only complete answer that addresses all of the bugs - nice one.
@Bathsheba I thought you were going to say you preferred to use counter < argc
If you wanted this to be really robust, you could check for potential int overflow (more potential for UB and such effects could manifest themselves at +/- 32767). But is this getting silly now?
@ChrisTurner: No that sucks.
@Bathsheba Pah. I bet you prefer that until someone writes *argv[counter] and gets an eternal loop, while *argv[counter] == NULL would give a compiler error for comparing incompatible types. It is not a tautology, it is rugged, professional code that reduces bugs. MISRA-C bans the former dangerous style for a reason.
|
7

It is specified that argv[argc] will always be a null pointer. You loop once too many, and pass this null pointer to atoi, leading to undefined behavior.

Change your loop conditions to be counter < argc.

And sum already is an integer, you do not need to convert it to an integer with atoi. That atoi(sum) will also lead to undefined behavior as the very first iteration will pass zero to atoi, which could be seen as a null pointer as well.

Comments

6

The last element of argv is defined to be NULL, and the first one is always the program name. Therefore you can reduce your code to

#include "stdio.h"

int main(int argc, char *argv[])
{
    int sum = 0;
    for (int i = 1; argv[i]; ++i){
        sum += atoi(argv[i]);
    }
    printf("Sum of %d command line arguments is: %d\n", argc, sum);
}

In your code, the behaviour of atoi(sum) and what boils down to argv[argc] on the final iteration will be undefined.

Comments

-2

A recursive version, just for the heck of it :)

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

int main(int argc, char **argv)
{   
    static int sum;

    if (*++argv == NULL)
        return !printf("sum: %d argc %d\n", sum, argc - 1);

    sum += atoi(*argv);
    return main(argc, argv);
}

8 Comments

Just to confirm my understanding, the static int sum keeps it's value through all iterations? Also, nice :)
Thanks :) Yes, the static is used to keep adding to the same variable.
You can't call main() from within main(). The main() is special.
Aw shit, I missed that, you're right. Throw it in it's own method and pass it the arguments and you'll be good.
@IharobAlAsimi You're thinking of C++. C has no such restriction.
|
-2
/* My example. Should work, though untested. */
#include <stdio.h>

int main(int argc, char *argv[])
{
    int sum, index;    // Generally considered better form to put them on separate lines.
    sum = 0;

    if(argc > 1) {    
        for(index = 1; index < argc; index++) {
            sum += atoi(argv[index]);
        }
        printf("%d Command-line args\nSum = %d\n", argc, sum);
    } 
    else {
        printf("Not enough command-line args\n");
    }
}

Make sure to try you hardest to adhere to a specific formatting style for your code. Search for style guides if you want to define yours by name. GNU C is a good starting point. They'll make your code more readable for you, easier to debug, and help others understand what's going on in it.

Couple of corrections for your aforementioned code..

/* Your code with corrections */
#include "stdio.h"

int main(int argc, char *argv[])
{
   int sum=0,counter; 
   // 1. Normally you want to give each variable it's own line. Readability is important.
   // 2. Don't initialize on declaration either. Refer to my example.
   // 3. Always add spaces between operators and at the end of statements for better readability.
    for(counter=1;counter<=argc;counter++)
    {
       // 4. atoi(sum) is unneccessary as it interprets your int sum as a const char*, 
       // which as far as I can tell should just give you back it's ascii value. 
       // It might be undefined however, so I would remove it regardless.
       // 5. Your segfault issue is because you iterate over the end of the array.
       // You try to access the [argc] value of the array (remember 0-based
       // indexing means a size val is always 1 greater than the greatest index).
       // Make 'counter<=argc' into 'counter < argc'

       sum = atoi(sum) + atoi(argv[counter]);
    }
  printf("Sum of %d command line arguments is: %d\n", argc, sum);
}

atoi() documentation of a form.

Be forewarned, your code will still function if you input chars or strings into the command line, but the behavior will be odd. atoi() will convert the char to it's corresponding ASCII decimal character index/value.

You can also use a null value (argv[] is always terminated with a NULL value at argv[argc]) to end iteration. I prefer using the provided argc though.

Hopefully this helps you a little bit. If you don't understand anything I threw up here, comment or search it online.

Cheers!

2 Comments

You never actually explain the cause of the bug.
@John3136 Yes I did. Read the comments in his code. I explain it in #5.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.