2

I am working on a program to read from a txt file and sort the strings.

data.txt:

jk ef ab cd bc gh fg ij hi de 

Here is my code:

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

int cmp(const void *p1, const void *p2) {
    return strcmp(*(const char **)p1,  *(const char **)p2);
}

int main() {
    FILE *f = fopen("data.txt", "r");
    char s[255][255];
    char tmp[255];
    int n = 0;

    while (!feof(f)) {
        fscanf(f, "%s", tmp);
        strcpy(s[n], tmp);
        n++;
    }

    fclose(f);

    qsort(s, n, sizeof(char *), cmp);

    int i = 0;
    for (; i < n; i++) {
        printf("%s ", s[i]);
    }

    return EXIT_SUCCESS;
} 

I ran the code on Ubuntu and it breaks on a segfault. Believe this segfault happened in qsort and I could not figure out why.

Anyone can give me some suggestions?

chqrlie
  • 131,814
  • 10
  • 121
  • 189
Bo Yuan
  • 107
  • 1
  • 12
  • 3
    1) `sizeof(char *)` --> `sizeof(*s)` – BLUEPIXY Sep 07 '17 at 07:14
  • I tried but this does not solve it. Still seeing segfault. – Bo Yuan Sep 07 '17 at 07:16
  • 3
    2) `return strcmp( *(const char **) p1, *(const char **) p2);` --> `return strcmp( (const char *) p1, (const char *) p2);` – BLUEPIXY Sep 07 '17 at 07:16
  • 5
    3) [Why is “while ( !feof (file) )” always wrong?](https://stackoverflow.com/questions/5431941/why-is-while-feof-file-always-wrong) – BLUEPIXY Sep 07 '17 at 07:17
  • 2
    Compile with all warnings and debug info (e.g. `gcc -Wall -Wextra -g` with [GCC](http://gcc.gnu.org/)....) then **use the debugger** (e.g. `gdb`). Read carefully the documentation of [qsort(3)](http://man7.org/linux/man-pages/man3/qsort.3.html). Use [C dynamic memory allocation](https://en.wikipedia.org/wiki/C_dynamic_memory_allocation) – Basile Starynkevitch Sep 07 '17 at 07:20
  • 1
    @BLUEPIXY: Re 2.: Are you sure? – alk Sep 07 '17 at 07:22
  • @alk I think so. – BLUEPIXY Sep 07 '17 at 07:23
  • @BLUEPIXY: `qsort()` passes to the compare function pointers to the array's elements. The array's elements are `char[255]`. So `qsort()` compare function gets passed in two `char(*)[255]`. – alk Sep 07 '17 at 07:24
  • 1
    @alk Eventually that head address is passed to the comparison function. E.g like `strcmp(s[X], s[Y])` – BLUEPIXY Sep 07 '17 at 07:25
  • 1
    @alk The compare function will be passed pointers that point to the beginning of the elements. Each element contains a string, so casting the pointers to `(char*)` yields correct results. – Klas Lindbäck Sep 07 '17 at 07:40

4 Answers4

7

The comparison function is incorrect, as you are sorting an array of arrays of char, you can pass the pointers to the elements to strcmp directly:

int cmp(const void *p1, const void *p2) {
    return strcmp(p1, p2);
}

Note however that your parsing loop is also incorrect: feof() is not the correct way to check for end of file. Use this instead:

  n = 0;
  while (n < 255 && fscanf(f, "%254s", s[n]) == 1) {
      n++;
  }

The qsort invokation should specify the size of the array element:

  qsort(s, n, sizeof(*s), cmp);

Here is a corrected version:

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

int cmp(const void *p1, const void *p2) {
    return strcmp(p1, p2);
}

int main(void) {
    FILE *f = fopen("data.txt", "r");
    char s[255][255];
    char tmp[255];
    int n = 0;

    if (f == NULL)
        return EXIT_FAILURE;

    while (n < 255 && fscanf(f, "%254s", s[n]) == 1) {
        n++;
    }
    fclose(f);

    qsort(s, n, sizeof(*s), cmp);

    for (int i = 0; i < n; i++) {
        printf("%s ", s[i]);
    }
    printf("\n");

    return EXIT_SUCCESS;
}
chqrlie
  • 131,814
  • 10
  • 121
  • 189
5

Many people gave a good answer.

Here is how you could have found it by yourself, step by step, with standard GNU tools:

We suppose the source file is named q.c.

Compile with debugging symbols (note that no need to have a Makefile here):

% make CFLAGS=-g q
cc -g  q.c  -o q

Now, run the program with a debugger (gdb):

% gdb q
(gdb) run
Starting program: /usr/home/fenyo/tmp/qs/q
Program received signal SIGSEGV, Segmentation fault.
0x00000008009607a6 in strcmp () from /lib/libc.so.7

Now look at the stack frame:

(gdb) where
#0  0x00000008009607a6 in strcmp () from /lib/libc.so.7
#1  0x00000000004009b5 in cmp (p1=0x7ffffffeeb60, p2=0x7ffffffeeb88) at q.c:8
#2  0x000000080093b834 in qsort () from /lib/libc.so.7
#3  0x0000000000400af5 in main () at q.c:26

So your problem is in a call to your function cmp by the qsort library, that calls strcmp with bad pointers.

So we go up from one stack frame, to be at your cmp function level:

(gdb) up
#1  0x00000000004009b3 in cmp (p1=0x7ffffffeeb60, p2=0x7ffffffeeb88) at q.c:8
8           return strcmp( *(const char **) p1,  *(const char **) p2);

We look at the type of p1:

(gdb) ptype p1
type = void *

Since p1 is a pointer, we examine its content displaying 10 first bytes:

(gdb) print (*(char *) p1)@10
$43 = "jk\000\000\000\000\000\000\000"

So we discover it is a null terminated string containing jk.

So your cast is invalid: *(const char **) p1.

This should have been (const char*) p1.

We change the cast and then it works.

Alexandre Fenyo
  • 4,526
  • 1
  • 17
  • 24
2

qsort() passes to the compare function two pointers to the array's elements.

The array's elements are of type char[255]. So qsort()'s compare function gets passed in two char(*)[255].

So it should look like

int cmp(const void *p1, const void *p2)
{
  const char (*ps1)[255] = p1;
  const char (*ps2)[255] = p2;

  return strcmp(*ps1, *ps2);
}
alk
  • 69,737
  • 10
  • 105
  • 255
  • Would be nasty function if OP wanted to change array size, right? – machine_1 Sep 07 '17 at 07:36
  • _warning: initialization discards 'const' qualifier from pointer tar get type [-Wdiscarded-qualifiers]_ – BLUEPIXY Sep 07 '17 at 07:37
  • `const char (*ps1)[255] = p1;` makes the [same warning](https://ideone.com/Ubo0r7). – BLUEPIXY Sep 07 '17 at 21:13
  • BTW, `&s[X][0]` is passed as an argument of the comparison function. It has no type information as it is passed as a void pointer. You try to reproduce the type but eventually that information is not used. You try to reproduce the type but eventually that information is not used. (Passed to `strcmp` as `const char *`) So it's just a form. It may be useful when using `strncmp` like `strncmp(*ps1, *ps2, sizeof(*ps1))`. However, since it means that it is useful as it is broken as C-String, it will be different from OP's desire. – BLUEPIXY Sep 07 '17 at 22:23
0
char s[255][255];

This smells very bad. Use C dynamic memory allocation.

Consider instead a heap-allocated pointer to an array of heap-allocated strings.

qsort(s, n , sizeof(char *), cmp);

Read carefully the documentation of qsort(3). With a char s[255][255] your call to qsort is very wrong.

Anyone can give me some suggestions?

Read more carefully a good C programming book and the documentation of every function you are using (even strcmp(3)). Look also at some C reference and glance into the C11 specification n1570.

Compile with all warnings and debug info, i.e. gcc -Wall -Wextra -g with GCC (read more about Invoking GCC). Use the debugger gdb (read about Debugging with GDB)

PS. We won't do your homework.

Basile Starynkevitch
  • 223,805
  • 18
  • 296
  • 547
  • Can you recommend a good shorter guide to learn the _basics_ of GDB first before moving on to bigger things than _Debugging with GDB_? – J...S Sep 08 '17 at 15:20
  • No. That thing has an introductory chapter. Read it. Also, tons of tutorials on GDB available elsewhere. GIYF. – Basile Starynkevitch Sep 08 '17 at 15:33