last day (15 days later) » 

3:04 AM
-1
Q: Why am I getting "Invalid read of size 8" when popping linked list?

Anthony SetteI have written a function in C that pops the first node off of a linked list and makes changes accordingly so the list starts at the next node. However, this results in a segmentation fault and I am not sure why. I have tried reading through Valgrind documents and reanalyzing my functions but I ...

 
This is as minimal as I can post because it is case specific since it was generated by Valgrind. If I was able to determine the problem and create a minified example producing the same issue I wouldn't have a question. Thank you anyway.
 
It is neither complete nor verifiable.
 
@melpomene Thanks for linking, according to that page, it is complete and verifiable. I would love to share all my code and have no problem doing so but it is too many lines to expect someone to parse. According to Valgrind, those are the functions causing problems and they are all labeled. Thank you for looking but please if you cannot help me don't downvote the question and tarnish my reputation.
 
"Complete: Make sure all information necessary to reproduce the problem is included in the question itself." There isn't even a main function. I can't reproduce anything.
Oh, hello. Looks like the old MCVE page is gone; it now redirects to [reprex]. Anyway, the "verifiable" part is lacking because valgrind tells you exactly where the issue is (t_lib.c:149), but we have no way of knowing which line that corresponds to.
 
3:04 AM
@melpomene I understand why you would think that but the code that calls these functions is in a file called test01x.c which is compiled collectively using a makefile. I cannot link a test file with over 100 lines as well as a few other dependencies totaling 400-500 lines. It is not something that can be minified and stay reproducible. I am sure you would understand since you develop in C. Thanks for the advice anyway.
@melpomene that corresponds to the tcb_remove(). How can I indicate that in my post? Thanks for still trying to help.
 
I don't care about your reputation; I care about whether this is a good question (i.e. shows research effort, is clear, is useful). I don't think it shows any research effort: It's just random snippets of code and "how do I fix?". There are no attempts to debug or at least narrow down the problem. Errors from valgrind are blatantly ignored because it "does not seem to be the cause of the segmentation fault" (and you know this how?). The lack of a MCVE means the question is not useful.
I know it corresponds to tcv_remove because valgrind said so: ==366== at 0x401021: tcb_remove (t_lib.c:149). The point is that valgrind also tells you the exact line with the error, but you don't tell us which line that is.
"if you need any additional information let me know" - That's not how it works. SO has the format of one question and one or more answers. It's not a conversation.
 
lie 149 is as follows temp->next = NULL;
 
"It is not something that can be minified and stay reproducible." Well, why the h*ck not?
I have a few ideas about things that might be wrong with your code, but I can't verify that I'm right because hey, incomplete code, not verifiable
anyway, half of your functions leak memory like a sieve
I like the comment on void tcb_add(tcb_t **first, tcb_t *second) { //works because it blatantly doesn't work
tcb_add randomly drops most of the existing list elements
but the most likely cause of the symptoms you're describing is probably tcb_remove returning a pointer to an uninitialized tcb_t when called on an empty list
 
How so? about the tcb_add?
 
("most likely" among the snippets you've posted, that is)
*first = (*first)->next; advances the root pointer whose address tcb_add is called with
so it leaks everything except the last element of the existing list
 
3:18 AM
Your last message about the remove I agree! temp is a pointer and rem is a double pointer. My intention is to return the first item in the linked but I return a pointer and that connection is severed when change the pointer temp.
Oh I see about add! I can create a temp variable!
 
you already have a temp variable, first
you just need to work with first, not *first
i.e. first = &(*first)->next
also, you seem to be modeling a queue. wouldn't a doubly-linked list be better for that?
 
Wow that line fixed a lot of my problems!
Also, a doubly-linked list may be better and that's how I started but I got confused when I previously tried to implement it.
I created another struct called queue_t with tcb_t *head and tcb_t *tail but it got difficult when I tried to add.
should I give it a go again?
 
you seem to be implementing some kind of thread scheduler
 
yes, that's exactly it!
 
I would recommend you step away from that and get familiar with memory management in C first
do simple exercises involving pointers and linked lists
 
3:29 AM
It's for a class so I can't. He kinda jumped the basics. Most people know from another class but Im taking this class early.
I definitely will after the semester is over.
 
ok, two things
ptr = malloc(...); ptr = ... is always a memory leak
 
Really?
 
you've just lost the only pointer to the block returned by malloc
 
if you don't store the return value of malloc anywhere, how are you going to free it?
and the second assignment to ptr overwrites the pointer
 
3:33 AM
I thought I needed to allocate space before assigning though?
 
assigning to what?
 
a node or a value to the variable.
 
int *ptr; int i = 42; ptr = &i; is perfectly valid
 
Oh ok.
 
int *ptr; int i = 42; *ptr = i; is an error: ptr is uninitialized, so dereferencing it has undefined behavior
int *ptr; int i = 42; ptr = malloc(sizeof (int)); *ptr = i; is valid
here ptr is initialized (it points to a dynamically allocated int), so we can store an int there
int *ptr; int i = 42; ptr = malloc(sizeof (int)); ptr = &i; is a memory leak
 
3:36 AM
Wait so why did you say this ptr = malloc(sizeof (int)); after giving it a value
 
hmm?
 
Shouldn't you allocate space then assign int?
int *ptr; ptr = malloc(sizeof (int));, int i = 42; *ptr = i;
oh wait.
 
same thing
 
I thought you said ptr = 42 I didn't realize it was i
 
ptr = 42 would be a type error (pointer on the left, number on the right)
 
3:39 AM
tcb_t *temp = malloc(sizeof(tcb_t)); is this ok?
or should it be tcb_t *temp;, temp = malloc(sizeof(tcb_t));
 
those are the same thing
 
ok
should I try using doubly linked list?
 
no, I think you can get away with a simpler construction
here's a sketch of an implementation:
 
struct node { struct node *next; ... };
 
3:41 AM
struct tcb_t {
int thread_id;
int thread_priority;
ucontext_t *thread_context;
struct tcb_t *next;
}; typedef struct tcb_t tcb_t;
This is my struct for tcb_t
 
struct node *node_create(void) { struct node *ptr = malloc(sizeof *ptr); ptr->next = NULL; ... return ptr; }
here ... represents initializing any other fields node may have
then: struct queue { struct node *head, **ptail; };
 
I need to assign fields to NULL in create??
previously I had this for queue
typedef struct tQueue_t
{
//pointers to start and end of list
tcb_t *head, *tail;
} tQueue_t;
 
that's not strictly necessary, but it will probably keep you out of trouble
in particular, it ensures you don't have uninitialized fields (assuming all nodes are created through node_create())
void queue_add(struct queue *q) { struct node *ptr = node_create(); *q->ptail = ptr; q->ptail = &ptr->next; }
that probably needs more parameters
in particular, you probably want to pass in a node to be added to the queue
but the logic for adding a new node to the queue (the last two statements) should be correct
 
should I try implementing queue then?
I am going to upload my t_lib.c
Nevermind I can't it just wants an image
 
struct node *queue_remove(struct queue *q) { struct node *ptr = q->head; if (ptr) { q->head = ptr->next; if (q->ptail == &ptr->next) { q->ptail = &q->head; } } return ptr; }
also, before you can use a queue, it has to be initialized like this: void queue_init(struct queue *q) { q->head = NULL; q->ptail = &q->ptail; }
 
3:55 AM
what does this line do?
q->ptail = &q->ptail;
 
oops
that's a typo, actually
I meant q->ptail = &q->head;
the invariant of this data structure is that ptail always points to the last next pointer in the chain
 
oh ok!
 
for an empty queue, the last pointer is q->head (which has a value of NULL)
 
why do you declare the functions as a struct?
 
I don't
struct node * is just the return type of queue_remove
 
4:01 AM
queue_add(struct queue_t *q) {
struct tbc_t *ptr = tbc_create();
*q->ptail = ptr; //did you intend for the *
q->ptail = &ptr->next;
}
It says a value of struct tcb_t * cannot be assigned to tcb_t
@melpomene oh ok!
 
did you declare ptail as a pointer to a pointer?
 
in the struct?
oh! o i didn't
quick fix
Also here is the functions I had before abandoning doubly-linked lists:
queue_t* queue_create() {
//Allocate space for new Queue
queue_t *temp = (queue_t *) calloc(1,sizeof(queue_t));
temp->head = temp->tail = NULL;
return temp;
}

void queue_add(queue_t *q, tcb_t *t) {
//If queue empty, head = tail
if(q->tail == NULL){
q->head = q->tail = t;
q->tail->next = NULL;
}

//Otherwise, append to list
else{
q->tail->next = t;
q->tail = t;
q->tail->next = NULL;
}
}

tcb_t* queue_remove(queue_t *q) {
//If list empty, do nothing
if(q->head == NULL){
return NULL;
}

//Pop node off the front of the queue, and return it
 
Linked lists are so much simpler when you have a head and tail node allocated at all times. These nodes aren't actually part of the list (i.e. they contain no data)
 
oh ok
 
I disagree with that
 
4:09 AM
woah there is 2 people hi all!
 
It reduces the amount code by half, or at least the number of checks
It's really hard to say that's not simpler
 
you get the same benefit by using pointers to pointers in your functions
 
q->ptail = &ptr->next; //this line is giving me a error about incompatable class
@ikegami im confused on your debate? you like *head and *tail? instead of **ptail
 
Sounds plausible, but having a extra level of indirect everywhere is hardly simpler.
Anthony, I was going to link to a post I wrote, but it's in Perl X_X
 
heh
 
Perl has generalized lvalues ("references"), so the pointer-to-pointer trick applies :-)
 
so which is better?
struct queue_t {
tcb_t *head, **ptail;
}; typedef struct queue_t queue_t;
struct queue_t {
tcb_t *head, *tail;
}; typedef struct queue_t queue_t;
 
@melpomene, I've used the level of indirect to which you refer in Perl: sub dive_val :lvalue { my $p = \shift; $p = \( $$p->{$_} ) for @_; $$p }
 
ah, classic
does \shift actually work? I would've written \$_[0]; shift
 
Yes, it works.
shift returns the actual scalar removed from the array.
 
4:21 AM
perl -wE "${ sub { \shift }->($_) } = 42; say" confirmed
 
Ops don't tend to make unnecessary copies
(Subs, on the other hand, ...)
:lvalue prevents a copy from being made
With aliasing: sub dive_val :lvalue { my \$r = \shift; \$r = \( $r->{$_} ) for @_; $r }
I can't wait for that feature to stop being experimental
 
@AnthonySette I wrote some tests for my queue: ideone.com/8qodlK
 
Thank you! The double pointer confused me but I wrote some stuff too with just *head and *tail. Ill test both of them with what I have
This is my full t_lib.c but now with queue's that I wrote
with all ur ideas though obviously, I couldn't have done it other wise
 

last day (15 days later) »