0a appending w/strcpy

[Grant Austin]

I have a linked list where each node holds a couple numbers a char
and a const char array. I use strcpy to take an char array
argument passed to my insert function to copy the string into
the new node.

To some of these entries 0a(line feed?) is being appended.
I've looked through the faq and googled but haven't found much
info that applies.

The code follows this post(in a bit I'll split it into multiple
files for use in a larger project).

Regards,
Newbie Grant

<-------------------------------

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


struct motNode {
struct motNode * next;
unsigned int opCode;
char type;
unsigned int numArgs;
char opName[];
};
typedef struct motNode motList;
motList *mot_head, * mot_cur;


int mot_insert(unsigned int opCode, char type, unsigned int numArgs,
const char * opName){
mot_cur = (motList *)malloc(sizeof(motList));
mot_cur->opCode = opCode;
mot_cur->type = type;
mot_cur->numArgs= numArgs;
strcpy(mot_cur->opName, opName);
mot_cur->next = mot_head;
mot_head = mot_cur;
}

mot_dump(){
while(mot_cur){
printf("%s\n",mot_cur->opName);
mot_cur = mot_cur->next;
}
}

generate_mot(){
mot_head = NULL;
mot_insert(0,'r',0,"hlt");
mot_insert(1,'r',3,"add");
mot_insert(2,'r',3,"sub");
mot_insert(3,'r',3,"mul");
mot_insert(4,'r',3,"div");
mot_insert(5,'r',3,"mod");
mot_insert(6,'r',2,"move");
mot_insert(7,'r',3,"and");
mot_insert(8,'r',3,"or");
mot_insert(9,'r',3,"xor");
mot_insert(10,'r',2,"com");
mot_insert(11,'r',3,"sll");
mot_insert(12,'r',3,"srl");
mot_insert(13,'r',3,"sra");
mot_insert(14,'r',3,"jr");
mot_insert(15,'r',1,"rdr");
mot_insert(16,'r',1,"prr");
mot_insert(17,'r',1,"prh");

/* I format */
mot_insert(18,'i',2,"li");
mot_insert(19,'i',3,"addi");
mot_insert(20,'i',3,"subi");
mot_insert(21,'i',3,"muli");
mot_insert(22,'i',3,"divi");
mot_insert(23,'i',3,"modi");
mot_insert(24,'i',3,"lwb");
mot_insert(25,'i',3,"swb");

/* J format */
mot_insert(26,'j',2,"lwa");
mot_insert(27,'i',2,"swa");
mot_insert(28,'i',1,"j");
mot_insert(29,'i',1,"jal");
mot_insert(30,'i',1,"jeq");
mot_insert(31,'i',1,"jne");
mot_insert(32,'i',1,"jlt");
mot_insert(33,'i',1,"jle");
mot_insert(34,'i',1,"jgt");
mot_insert(35,'i',1,"jge");
}

main(int argv, char * argc[]){
FILE * infle;
int
eofl,
fcount = 2;

if(argv < 3){
fprintf(stderr,"Too few arguments\n");
fprintf(stderr,"\tusage:\tp2 <flag> <input files:minimum of one>\n");
exit(1);
}
generate_mot();
mot_dump();
do{
printf("infle :: %s\n",argc[fcount]);
if((infle = fopen(argc[fcount],"r")) == NULL){
fprintf(stderr,"Unable to open data file %s\n",argc[fcount]);
}
else{
//do work
fclose(infle);
}
fcount++;
} while(fcount < argv);

}

--------------------------------->

 

[Peter Pichler]

I have a linked list where each node holds a couple numbers a char
and a const char array. I use strcpy to take an char array
argument passed to my insert function to copy the string into
the new node.

To some of these entries 0a(line feed?) is being appended.
I've looked through the faq and googled but haven't found much
info that applies.

The code follows this post(in a bit I'll split it into multiple
files for use in a larger project).

Regards,
Newbie Grant

<-------------------------------

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

struct motNode {
struct motNode * next;
unsigned int opCode;
char type;
unsigned int numArgs;
char opName[];
};

typedef struct motNode motList;

The two declarations above could be combined into one, did you know it?
Mind you, I wouldn't do it, typedefing structures is rarely a good idea.

But the most important point and probably a source of your problem is
the declaration of your opName field. You really need to give it a size.
I am surprised it even compiles.

motList *mot_head, * mot_cur;

Globals are another thing that is almost never a good idea.

int mot_insert(unsigned int opCode, char type, unsigned int numArgs,
const char * opName){
mot_cur = (motList *)malloc(sizeof(motList));

Two things here. First, do not cast the return value from maloc(). It is
not necessary in C and may hide potential problems, when you forget to
include stdlib.h. Second, you need to check the malloc()'s return value.

mot_cur->opCode = opCode;
mot_cur->type = type;
mot_cur->numArgs= numArgs;
strcpy(mot_cur->opName, opName);

Here is your problem. You copy something to opName, but how much room
you have in opName? A typical example of UB (Undefined Behaviour).

mot_cur->next = mot_head;
mot_head = mot_cur;
}

Your function is declared as returning int, so where is it? Better yet,
let it return a pointer to your new created node and get rid of a global
variable.

mot_dump(){
while(mot_cur){
printf("%s\n",mot_cur->opName);
mot_cur = mot_cur->next;
}
}

Another function that fails to return int, even though it is declared
as such (although implicitly). Also, it relies on mot_cur being set up
before calling. Why don't you set it up in the function? Then you could
get rid of another global variable. It would need a pointer to a head
as a parameter.

generate_mot(){
mot_head = NULL;
mot_insert(0,'r',0,"hlt");
mot_insert(1,'r',3,"add"); ....
mot_insert(34,'i',1,"jgt");
mot_insert(35,'i',1,"jge");
}

Yet again, where's your return?

main(int argv, char * argc[]){
FILE * infle;
int
eofl,
fcount = 2;

if(argv < 3){
fprintf(stderr,"Too few arguments\n");
fprintf(stderr,"\tusage:\tp2 <flag> <input files:minimum of one>\n");
exit(1);

1 is not a portable value to be returned from main() or used in exit().
Use EXIT_SUCCESS or EXIT_FAILURE, both defined in stdlib.h, which you
already include.

}
generate_mot();
mot_dump();
do{
printf("infle :: %s\n",argc[fcount]);
if((infle = fopen(argc[fcount],"r")) == NULL){
fprintf(stderr,"Unable to open data file %s\n",argc[fcount]);
}
else{
//do work
fclose(infle);
}
fcount++;
} while(fcount < argv);

return EXIT_SUCCESS;

 

[Grant Austin]

Thanks for all of the comments, things are becoming clearer.
I'll be digesting for a while. (=

I'm creating the list as global because I will need access to it
in functions in other files. I thought that there might be quite
a bit of overhead passing the list to those functions. Now I realize
I can just pass a pointer to the head of the list.

Regards,
-G

 

[Al Bowers]

I have a linked list where each node holds a couple numbers a char
and a const char array. I use strcpy to take an char array
argument passed to my insert function to copy the string into
the new node.

To some of these entries 0a(line feed?) is being appended.
I've looked through the faq and googled but haven't found much
info that applies.

.............code snipped.......
There are many errors.
The major error is that you did not allocate storage space for the
struct that will store the string. Here is your code modified to
show one way you might want to define function mot_insert.

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

struct motNode
{
struct motNode * next;
unsigned int opCode;
char type;
unsigned int numArgs;
char *opName;
};
typedef struct motNode mot_List;

int mot_insert(mot_List **mot_head,unsigned int opCode, char type,
unsigned int numArgs, const char * opName);
void mot_dump(mot_List *mot_cur);
void generate_mot(mot_List **mot_cur);
void mot_free(mot_List **mot_cur);

int main(void)
{
mot_List *mot_head = NULL;

generate_mot(&mot_head);
mot_dump(mot_head);
mot_free(&mot_head);
return 0;
}

int mot_insert(mot_List **mot_head,unsigned int opCode, char type,
unsigned int numArgs, const char * opName)
{
mot_List *mot_cur = malloc(sizeof *mot_cur);

if(mot_cur == NULL) return 0;
if((mot_cur->opName = malloc(strlen(opName)+1)) == NULL)
{
free(mot_cur);
return 0;
}
mot_cur->opCode = opCode;
mot_cur->type = type;
mot_cur->numArgs= numArgs;
strcpy(mot_cur->opName, opName);
mot_cur->next = *mot_head;
*mot_head = mot_cur;
return 1;
}

void mot_dump(mot_List *mot_cur)
{
while(mot_cur)
{
printf("%s\n",mot_cur->opName);
mot_cur = mot_cur->next;
}
}

void generate_mot(mot_List **mot_cur)
{
mot_insert(mot_cur,0,'r',0,"hlt");
mot_insert(mot_cur,1,'r',3,"add");
mot_insert(mot_cur,2,'r',3,"sub");
mot_insert(mot_cur,3,'r',3,"mul");
mot_insert(mot_cur,4,'r',3,"div");
mot_insert(mot_cur,5,'r',3,"mod");
mot_insert(mot_cur,6,'r',2,"move");
mot_insert(mot_cur,7,'r',3,"and");
mot_insert(mot_cur,8,'r',3,"or");
mot_insert(mot_cur,9,'r',3,"xor");
mot_insert(mot_cur,10,'r',2,"com");
mot_insert(mot_cur,11,'r',3,"sll");
mot_insert(mot_cur,12,'r',3,"srl");
mot_insert(mot_cur,13,'r',3,"sra");
mot_insert(mot_cur,14,'r',3,"jr");
mot_insert(mot_cur,15,'r',1,"rdr");
mot_insert(mot_cur,16,'r',1,"prr");
mot_insert(mot_cur,17,'r',1,"prh");

/* I format */
mot_insert(mot_cur,18,'i',2,"li");
mot_insert(mot_cur,19,'i',3,"addi");
mot_insert(mot_cur,20,'i',3,"subi");
mot_insert(mot_cur,21,'i',3,"muli");
mot_insert(mot_cur,22,'i',3,"divi");
mot_insert(mot_cur,23,'i',3,"modi");
mot_insert(mot_cur,24,'i',3,"lwb");
mot_insert(mot_cur,25,'i',3,"swb");

/* J format */
mot_insert(mot_cur,26,'j',2,"lwa");
mot_insert(mot_cur,27,'i',2,"swa");
mot_insert(mot_cur,28,'i',1,"j");
mot_insert(mot_cur,29,'i',1,"jal");
mot_insert(mot_cur,30,'i',1,"jeq");
mot_insert(mot_cur,31,'i',1,"jne");
mot_insert(mot_cur,32,'i',1,"jlt");
mot_insert(mot_cur,33,'i',1,"jle");
mot_insert(mot_cur,34,'i',1,"jgt");
mot_insert(mot_cur,35,'i',1,"jge");
}

void mot_free(mot_List **mot_cur)
{
mot_List *tmp;

for( ; *mot_cur; *mot_cur = tmp)
{
tmp = (*mot_cur)->next;
free(*mot_cur);
}
return;
}

 

[Chris Torek]

struct motNode {
struct motNode * next;
unsigned int opCode;
char type;
unsigned int numArgs;
char opName[];
};

typedef struct motNode motList;

The two declarations above could be combined into one, did you know it?
Mind you, I wouldn't do it, typedefing structures is rarely a good idea.

Also, rather than combining them, if one really likes typedefs, one
should write the typedef first:

typedef struct X stX;

because that allows the alias to occur inside the structure:

struct X {
stX *next; /* linked list of struct X */
...
};

The "combo thing", "typedef struct X { ... } stX;", gives you the
worst of both worlds: not only do you have an not-strictly-necessary
alias just to avoid typing the letters "ruct" (in this case);
you cannot even use it inside the braces.

But the most important point and probably a source of your problem is
the declaration of your opName field. You really need to give it a size.
I am surprised it even compiles.

This kind of declaration is a new C99-only "feature" called a
"flexible array member". The effect is as if you declared the
array with size zero (if C had zero-sized objects, which it does
not even if it should [in my opinion]).

Two things here. First, do not cast the return value from maloc(). It is
not necessary in C and may hide potential problems, when you forget to
include stdlib.h. Second, you need to check the malloc()'s return value.

And if one is going to use the new C99-only feature, one must also add
space for the name:

Here is your problem. You copy something to opName, but how much room
you have in opName? A typical example of UB (Undefined Behaviour).

Indeed. Hence:

mot_cur = malloc(sizeof *mot_cur) + strlen(opName) + 1;

Note that a string whose strlen() is L requires L+1 bytes to store,
because one byte is needed for the '\0' marking the end.

As the FAQ notes, most (though not necessarily all) C89 compilers
allow you to use undefined behavior usefully by making the array
have size 1:

struct S {
...
char name[1];
};
...
struct S *p = malloc(sizeof *p + strlen(name));
...
strcpy(p->name, name);

In this case, the "+1" for the terminating '\0' is handled by giving
the array a size of 1. Unfortunately, the undefined behavior aspect
means you cannot count on this working on other systems, even if
it works on yours -- so you have to decide whether any savings you
find by using this so-called "struct hack" is worth the loss of
portability. (I have used it myself, in code that is not meant to
be strictly portable, in the data structures that describe potential
and ongoing DMA operations in devices. The NetBSD / FreeBSD
device-DMA model is based on this same idea, and in fact, the names
in a bus_dmamap_t are the ones I suggested. DMA areas are described
by a sequence of <ds_addr,ds_len> pairs, stored in an array allocated
via the "struct hack".)

 

[Grant Austin]

.text snipped...

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

struct motNode
{
struct motNode * next;
unsigned int opCode;
char type;
unsigned int numArgs;
char *opName;
};
typedef struct motNode mot_List;

int mot_insert(mot_List **mot_head,unsigned int opCode, char type,
unsigned int numArgs, const char * opName);
void mot_dump(mot_List *mot_cur);
void generate_mot(mot_List **mot_cur);
void mot_free(mot_List **mot_cur);

int main(void)
{
mot_List *mot_head = NULL;

generate_mot(&mot_head);
mot_dump(mot_head);
mot_free(&mot_head);
return 0;
}

....code snipped...

Thanks for the suggestions & code. With all of the help
I'm getting here I might have a chance at not sucking in a couple
years.

I'm not sure I understand what is meant by mot_List **mot_cur
and mot_List *mot_cur. Is **mot_cur a reference to a pointer? Is *mot_cur
the value of the pointer?


Thanks,
Grant

 

[Grant Austin]

..snip

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

struct motNode {
struct motNode * next;
unsigned int opCode;
char type;
unsigned int numArgs;
char opName[];
};

typedef struct motNode motList;

The two declarations above could be combined into one, did you know it?

....snip...

Not that I'm going to do it but do you mean you can do something
like this?:

typedef struct motNode{
struct motNode * next;
unsigned int opCode;
..
..
} motList;


-G

 

[Peter Pichler]

.snip

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

struct motNode {
struct motNode * next;
unsigned int opCode;
char type;
unsigned int numArgs;
char opName[];
};

typedef struct motNode motList;

The two declarations above could be combined into one, did you know it?

...snip...

Not that I'm going to do it but do you mean you can do something
like this?:

typedef struct motNode{
struct motNode * next;
unsigned int opCode;
..
..
} motList;

Yes, that's what I meant. But read Chris Torek's post first, before dashing
off to do it ;-)

Peter

 

[Peter Pichler]

Indeed. Hence:

mot_cur = malloc(sizeof *mot_cur) + strlen(opName) + 1;

ITYM moving this----------------------^ here----------------^ ;-)

Thanks for the other comments.

Peter

 

[Chris Torek]

[on using "flexible array members]

"Chris Torek" <[email protected]> wrote in message

[what happened to the message-ID? I believe it was

ITYM moving this----------------------^ here----------------^ ;-)

Er, yes. Of course, as shown here, the marks might not be all that
meaningful to readers (especially if anyone is reading this with
variable-pitched fonts), so here is the corrected line:

mot_cur = malloc(sizeof *mot_cur + strlen(opName) + 1);

Fortunately, any ANSI C compiler has to complain about:

p = malloc(n1) + n2;

(although gcc, in its default mode of compiling GNUC instead of C,
thinks that arithmetic on void pointers should move by bytes instead
of drawing a diagnostic -- so that my error produces dreadfully
wrong results instead of a diagnostic).

 

[Grant Austin]

Indeed. Hence:

mot_cur = malloc(sizeof *mot_cur) + strlen(opName) + 1;

I'm doing something like this now, thanks to Mr. Bowers:
motList *mot_cur = malloc(sizeof *mot_cur);
mot_cur->opName = malloc(strlen(opName)+1));

This looks like the same thing just longer.

(I do check what malloc returns in the actual code)

Thanks for the comments. My mind is trying to cave in on itself.
I'm going to go decompress before I take a crack at the hash table
where each node contains a list of integers.

If anyone was wondering and hadn't figured it out -- I'm writing
a one pass assembler.


Regards,
Grant

 

[Joona I Palaste]

[on using "flexible array members]

"Chris Torek" <[email protected]> wrote in message

[what happened to the message-ID? I believe it was
<news:[email protected]>, anyway]

Er, yes. Of course, as shown here, the marks might not be all that
meaningful to readers (especially if anyone is reading this with
variable-pitched fonts), so here is the corrected line:

mot_cur = malloc(sizeof *mot_cur + strlen(opName) + 1);

I still prefer doing those marks with fixed-pitch fonts. That way
anyone who is using *any* fixed-pitch font, no matter what size,
what style of letters, or even what *writing system* the font is,
the marks will measure up correctly.
Unfortunately there are many who use those marks with variable-pitch
fonts, in the belief that their own newsreader is the only one in the
world, and no one ever configures it in a way that differs from theirs
in the slightest.

--
/-- Joona Palaste ([email protected]) ------------- Finland --------\
\-- http://www.helsinki.fi/~palaste --------------------- rules! --------/
"The day Microsoft makes something that doesn't suck is probably the day they
start making vacuum cleaners."
- Ernst Jan Plugge

 

[Mike Wahler]

text snipped...
...code snipped...

Thanks for the suggestions & code. With all of the help
I'm getting here I might have a chance at not sucking in a couple
years.

I'm not sure I understand what is meant by mot_List **mot_cur
and mot_List *mot_cur. Is **mot_cur a reference to a pointer? Is *mot_cur
the value of the pointer?

As declarations:

mot_List *mot_cur
.. declares a pointer to type 'mot_List'

mot_List **mot_head
.. declares a pointer to a pointer to type 'mot_List'
(a pointer is an object, just like any other type).


Given the above declarations, the expressions:

mot_cur
.. designates an object of type 'pointer to mot_List' ('mot_List *')

*mot_cur
.. designates an object of type 'mot_List'

mot_head
.. designates an object of type 'pointer to pointer to mot_List'
('mot_List **')

*mot_head
.. designates an object of type 'pointer to mot_List' ('mot_List *')

**mot_head
.. designates an object of type 'motList'.


Thus (again, given the above declarations) the expressions:

*mot_cur and **mot_head have the same type ('mot_List')

mot_cur and *mot_head have the same type ('pointer to 'mot_List', ('mot_List
*') )

And no, C does not have a 'reference' type. (C++ does, and it uses a
different
syntax, using the '&' symbol)

The 'depth' to which one can go with 'pointer to pointer to ...' is
theoritically
unlimited, but does have limits in practice, of course. The language
standard
makes recommendations of the minimum number of these 'levels', I don't
recall
how deep. But in practice, it's rare to have more that two levels, i.e.
'pointer
to pointer.'

Does that help?

-Mike

 

[Peter Pichler]

The 'depth' to which one can go with 'pointer to pointer to ...' is
theoritically unlimited, but does have limits in practice, of course.
The language standard makes recommendations of the minimum number of
these 'levels', I don't recall how deep.
6.

But in practice, it's rare to have more that two levels, i.e.
'pointer to pointer.'

Does that help?

-Mike

 

[those who know me have no need of my name]

in comp.lang.c i read:

I'm doing something like this now, thanks to Mr. Bowers:
motList *mot_cur = malloc(sizeof *mot_cur);
mot_cur->opName = malloc(strlen(opName)+1));

This looks like the same thing just longer.

it isn't the same thing though; chris' version makes the array `part of'
the structure, while yours has a pointer to the space. i.e., in effect:

struct { ...; char opName[strlen(opName)+1]; } *mot_cur;
vs
struct { ...; char *opName; } *mot_cur;

if you were to fwrite each of these you might see the difference more
readily ...

fwrite(mot_cur, sizeof *mot_cur + strlen(mot_cur.opName), 1, fp);
vs
fwrite(mot_cur, sizeof *mot_cur, 1, fp);

the former has the string in the output, while the later just has some
bytes (the pointer) without the string data -- adding strlen(mot_cur.opName)
doesn't help because the string might not be adjacent to the structure, and
besides you'd still have the pointer in the stream. to get the same data
output you would need to chase the pointer:

fwrite(mot_cur, offsetof(mot_cur, opName), 1, fp);
fwrite(mot_cur.opName, strlen(mot_cur.opName), 1, fp);

when it's a pointer you must always chase it yourself and handle all the
memory allocation (two malloc's and two free's for each instance).

when you use `the struct hack' or flexible array members you must handle
the fact that sizeof is not correct, or rather it's not sufficient. a
size_t member that records the actual size allocated can be useful.

 

[Grant Austin]

Thank you. Great explanation. Crystal Clear now.

-G

 

[Grant Austin]

.snip...

Does that help?

Yep thanks, nothing like practice to cement theory.

Regards,
-G

 

[Al Bowers]

text snipped...


...code snipped...

Thanks for the suggestions & code. With all of the help
I'm getting here I might have a chance at not sucking in a couple
years.

I'm glad you studied the code. However, after I posted it I
discovered one flaw in the function mot_free. I did not
free the memory for the additional allocation for the sting
used by the member opName. To correct this define the function:

void mot_free(mot_List **mot_cur)
{
mot_List *tmp;

for( ; *mot_cur; *mot_cur = tmp)
{
tmp = (*mot_cur)->next;
free((*mot_cur)->opName);
free(*mot_cur);
}
return;
}