cbuf with char *s

[cerr]

Hi There,

I'm using the circular buffer library from
http://svn.gumstix.com/gumstix-buildroot/branches/users/ddiall/robostix/Common/CBUF.h
in order to fiffo log messages from one thread to another where they
get sent out.
Now my problem is that i want to have a defintion like this:
volatile struct
{
unsigned int m_getIdx;
unsigned int m_putIdx;
char* m_entry[ LogQ_SIZE ];
} LogQ;
but the strings that i push in are not the same i pop out because the
content at that address changes. So I thought I coudl go ahead and
allocate memory in a global msglist array to hold on to these
messages. I attemptedf this by globally declaring
char **msglist;
and in my "push" function i would go like:
pthread_mutex_lock(&log_mtx);
temp = realloc(msglist,(CBUF_Len(LogQ)+1)*sizeof(*temp));
if (temp==NULL){
syslog(LOG_ALERT, "Error reallocating memory for msglist\n");
return;
}
msglist=temp;
msglist[CBUF_Len(LogQ)] = malloc (strlen(buf)+1);
if (msglist[CBUF_Len(LogQ)]==NULL){
syslog(LOG_ALERT, "Error allocating memory for the string in msglist
\n");
return;
}
msglist[CBUF_Len(LogQ)]=buf;
CBUF_Push( LogQ, msglist[CBUF_Len(LogQ)] );
to allocate new memory. However when I pop it from the queue i still
get the same address returned for every entry. How come? Where am i
going wrong?

Thanks,
Ron

PS: Ii'll get it figured out with pointers some time soon - Thanks
for your help!

 

[Seebs]

but the strings that i push in are not the same i pop out because the
content at that address changes.

That suggests that your problem has to do with something specific to
the "threads" you're using -- the content at a given address shouldn't
otherwise change unless you change it. Since those threads are presumably
system-specific, you may find that you're better off writing a small
example that just shows the change in contents of a pointer from one
thread to another, and posting with that to a newsgroup related to your
target OS.

On the systems I use, that wouldn't happen.

-s

 

[Antoninus Twink]

That suggests that your problem has to do with something specific to
the "threads" you're using -- the content at a given address shouldn't
otherwise change unless you change it. Since those threads are
presumably system-specific, you may find that you're better off
writing a small example that just shows the change in contents of a
pointer from one thread to another, and posting with that to a
newsgroup related to your target OS.

The OP didn't show us enough code to be certain, but it seems very
likely to me that your diagnosis is wrong. (Of course, whether or not
the diagnosis is correct, your advice is completely bogus for the usual
reasons, but let's not go down that road again.)

Let's look at part of the OP's code:

msglist[CBUF_Len(LogQ)] = malloc (strlen(buf)+1);
/* snip */
msglist[CBUF_Len(LogQ)]=buf;

It seems very probable that the OP meant to write
strcpy(msglist[CBUF_Len(LogQ)], buf);
and that this is the cause of the problem - nothing to do with threads
at all.

But on the subject of threading, the OP's code did include a volatile
variable. It's worth pointing out that a volatile qualifier isn't worth
jack shit if you're hoping it will give consistent data across multiple
threads. A lock provided by the thread library (e.g. a mutex) is the
ONLY safe way to share data between threads.

 

[cerr]

That suggests that your problem has to do with something specific to
the "threads" you're using -- the content at a given address shouldn't
otherwise change unless you change it.  Since those threads are presumably
system-specific, you may find that you're better off writing a small
example that just shows the change in contents of a pointer from one
thread to another, and posting with that to a newsgroup related to your
target OS.

On the systems I use, that wouldn't happen.

I probably should be using your system then but I'm using Linux
instead.

Okay, let me try to outline this:

#include "cbuf.h"
#define LogQ_SIZE 1024
volatile struct
{
unsigned int m_getIdx;
unsigned int m_putIdx;
char* m_entry[ LogQ_SIZE ];
} LogQ;
char **msglist;

[THREAD A]
int prs_log(int level, char *msg, ...)
{
//allocate mem like pasted in above posting
allocmem[CBUF_Len(LogQ)]=msg;
pthread_mutex_lock(&log_mtx);
CBUF_Push( LogQ, allocmem[CBUF_Len(LogQ)] );
pthread_mutex_unlock(&log_mtx);
}

[THREAD B]
void run_Logger()
{
while (1){
pthread_mutex_lock(&log_mtx);
int LogQSz=CBUF_Len(LogQ);
pthread_mutex_unlock(&log_mtx);
if(LogQSz>0){
mutex_lock(&log_mtx);
char* Data=CBUF_Pop( LogQ );
mutex_unlock(&log_mtx);
send(MySocket,Data,sizeof(Data));
}
}
}

That's pretty much what i'm doing...

Thanks,
Ron

 

[Antoninus Twink]

//allocate mem like pasted in above posting
allocmem[CBUF_Len(LogQ)]=msg;

If it really is what you pasted before, then this is almost surely the
error and not "just" a memory leak.

The code before included something like

allocmem[CBUF_Len(LogQ)] = malloc(strlen(msg) + 1);

So what you clearly mean to be doing is

strcpy(allocmem[CBUF_Len(LogQ)], msg);

not a shallow pointer copy: presumably the buffer the caller is passing
in as msg gets overwritten elsewhere in the program and this is the
cause of your difficulties.

 

[cerr]

//allocate mem like pasted in above posting
allocmem[CBUF_Len(LogQ)]=msg;

If it really is what you pasted before, then this is almost surely the
error and not "just" a memory leak.

The code before included something like

allocmem[CBUF_Len(LogQ)] = malloc(strlen(msg) + 1);

So what you clearly mean to be doing is

strcpy(allocmem[CBUF_Len(LogQ)], msg);

Okay yes, this is bringing me forward a huge step already! Thanks for
that!
But then again, how can I free() that allocated memory again once I
pop it off the queue? And also, why does following

pthread_mutex_lock(&log_mtx);
Data = CBUF_Pop( LogQ );
pthread_mutex_unlock(&log_mtx);
syslog(LOG_ALERT, "Data:%s &Data:0x%x",Data,&Data);

always print the same address? Shouldn't &Data be different now and
corolate with every item allocated in msglist?
I thought I now could "free()" the memory where &Data would be
pointing to cause it's not used anymore (been popped off the queue) -
thanks for further help!

not a shallow pointer copy: presumably the buffer the caller is passing
in as msg gets overwritten elsewhere in the program and this is the
cause of your difficulties.

Exactly! Didn't think far enough! Thanks a lot for pointing that out!

Ron

 

[Antoninus Twink]

Or by a sig_atomic_t store.

Or by lockf(2).

Hehe, I knew as soon as I hit send that someone would be along to pull
me up on the loose language. Touche.

The real point is still: volatile and locking are essentially orthogonal
things in the C world.

Ever try to _initialize_ a mutex in Win32 in a thread-safe manner? Now
that's an interesting dilemma if there ever was one.

No I haven't. What's the issue?

Perhaps we could begin discussing the limitations of the
compare-and-swap primitive?

A million x86 multithreaded developers can't be wrong!

Seriously, it seems to have served its purpose pretty well over the
years. I mean, it provides a way to implement a mutual exclusion lock.
What more do you want from a mutex?!

Or how no commercial CPU actually implements a universal load-link,
store-conditional pair.

Do you not count PowerPC, Alpha and ARM as commercial CPUs?

 

[Antoninus Twink]

But then again, how can I free() that allocated memory again once I
pop it off the queue?

You could use the free() function for that.

And also, why does following

pthread_mutex_lock(&log_mtx);
Data = CBUF_Pop( LogQ );
pthread_mutex_unlock(&log_mtx);
syslog(LOG_ALERT, "Data:%s &Data:0x%x",Data,&Data);

always print the same address?

[guess]
Because in your testing you can service the log queue quickly
enough that it never has more than one message in it?
[/guess]

Shouldn't &Data be different now and corolate with every item
allocated in msglist?

No, Data (which presumably is a char *) should be different now and
correlate with every item allocated in msglist. Try

syslog(LOG_ALERT, "Data:%s &Data:0x%x", Data, (void *) Data);

I thought I now could "free()" the memory where &Data would be
pointing to cause it's not used anymore (been popped off the queue) -

I suspect you can "free()" the memory where Data is pointing to.

 

[cerr]

On  3 Dec 2009 at 21:55, cerr wrote:

//allocate mem like pasted in above posting
allocmem[CBUF_Len(LogQ)]=msg;

If it really is what you pasted before, then this is almost surely the
error and not "just" a memory leak.

The code before included something like

allocmem[CBUF_Len(LogQ)] = malloc(strlen(msg) + 1);

So what you clearly mean to be doing is

strcpy(allocmem[CBUF_Len(LogQ)], msg);

Okay yes, this is bringing me forward a huge step already! Thanks for
that!
But then again, how can I free() that allocated memory again once I
pop it off the queue? And also, why does following

      pthread_mutex_lock(&log_mtx);
      Data = CBUF_Pop( LogQ );
      pthread_mutex_unlock(&log_mtx);
      syslog(LOG_ALERT, "Data:%s &Data:0x%x",Data,&Data);

always print the same address? Shouldn't &Data be different now and
corolate with every item allocated in msglist?
I thought I now could "free()" the memory where &Data would be
pointing to cause it's not used anymore (been popped off the queue) -
thanks for further help!

Following would do this, right?

free(msglist[CBUF_Len(LogQ)]);

 

[Seebs]

I probably should be using your system then but I'm using Linux
instead.

Huh, I haven't seen anything like this.

pthread_mutex_lock(&log_mtx);

Any discusion of pthread semantics is of no use to a very large number of
the people here. I suggest comp.unix.programmer. If you can make a minimal
example (possibly not using the cbuf stuff) showing the shift in pointer
contents between threads, that will probably help. (In my experience,
generally, it's at the point where I build the minimal reproducer that I find
my bug.)

-s

 

[Keith Thompson]

Huh, I haven't seen anything like this.


Any discusion of pthread semantics is of no use to a very large number of
the people here. I suggest comp.unix.programmer.

[...]

Or perhaps comp.programming.threads.

 

[Kaz Kylheku]

Hi There,

[ snip ]

msglist[CBUF_Len(LogQ)] = malloc (strlen(buf)+1);

Assignment to msglist[CBUF_Len(LogQ)];

if (msglist[CBUF_Len(LogQ)]==NULL){
syslog(LOG_ALERT, "Error allocating memory for the string in msglist
\n");
return;
}
msglist[CBUF_Len(LogQ)]=buf;

Another assignment to msglist[CBUF_Len(LogQ)]; you've lost
the malloc(strlen(buf+1)) object, and used the original
buf in its place.

 

[Antoninus Twink]

Or perhaps comp.programming.threads.

Even if the pair of you want to keep up this ridiculous pretence of
having killfiled me, Kaz Kylekhu has also confirmed that the problem is
nothing at all to do with threads, and everything to do with C strings
and pointers.

Yet still you persist in telling posters to **** off. Pathetic.

 

[cerr]

Huh, I haven't seen anything like this.


Any discusion of pthread semantics is of no use to a very large number of
the people here.  I suggest comp.unix.programmer.  If you can make a minimal
example (possibly not using the cbuf stuff) showing the shift in pointer
contents between threads, that will probably help.  (In my experience,
generally, it's at the point where I build the minimal reproducer that I find
my bug.)

But if I declare a pointer globally its content should be transparent
to every thread in this process, no?
I'm not getting it but - in fact - with Antonius' hints i still see
some weird behaviors e.g. like:
This is what I see (e.g.):
push "Novax PRS Program Started!" onto LogQ(4)
and then the thread starts and stuff and when it comes off the queue
it looks like:
Data: x PRS Program Started! &Data:0x80672d0
The first few bytes always seem to get lost and the very first one to
appear often is screwed-up (although, not in this case...)

The push mechanism - again - is as follows(in thread A):

temp = realloc(msglist,(CBUF_Len(LogQ)+1)*sizeof(*temp));
if (temp==NULL){
syslog(LOG_ALERT, "Error reallocating memory for msglist\n");
for (i=CBUF_Len(LogQ);i>=0;i--)
free(msglist);
free(msglist);
return;
}
msglist=temp;
msglist[CBUF_Len(LogQ)] = malloc (strlen(buf)+1);
if (msglist[CBUF_Len(LogQ)]==NULL){
syslog(LOG_ALERT, "Error allocating memory for the string in msglist
\n");
return;
}
strcpy(msglist[CBUF_Len(LogQ)],buf);
pthread_mutex_lock(&log_mtx);
CBUF_Push( LogQ, msglist[CBUF_Len(LogQ)] );
pthread_mutex_unlock(&log_mtx);
syslog(LOG_ALERT, "push \"%s\" onto LogQ(%d)",buf,CBUF_Len(LogQ));

And i pop it off the queue like this (in thread B):

pthread_mutex_lock(&log_mtx);
Data = CBUF_Pop( LogQ );
pthread_mutex_unlock(&log_mtx);
free(msglist[CBUF_Len(LogQ)]);
syslog(LOG_ALERT, "Data:%s &Data:0x%x", Data, (void *) Data);

Thanks for hints and suggestions!
Ron

 

[cerr]

Hi There,

[ snip ]

        msglist[CBUF_Len(LogQ)] = malloc (strlen(buf)+1);

Assignment to msglist[CBUF_Len(LogQ)];

        if (msglist[CBUF_Len(LogQ)]==NULL){
     syslog(LOG_ALERT, "Error allocating memory for the string in msglist
\n");
   return;
        }
   msglist[CBUF_Len(LogQ)]=buf;

Another assignment to msglist[CBUF_Len(LogQ)]; you've lost
the malloc(strlen(buf+1)) object, and used the original
buf in its place.

I've been thinking - just to safe time - to just make use of the std
namespace and its queue and string components - any comments to that
approach? I know it wouldn't be "clean" C code....but...

 

[Ben Pfaff]

I've been thinking - just to safe time - to just make use of the std
namespace and its queue and string components - any comments to that
approach? I know it wouldn't be "clean" C code....but...

It wouldn't be C code at all.

 

[Keith Thompson]

I've been thinking - just to safe time - to just make use of the std
namespace and its queue and string components - any comments to that
approach? I know it wouldn't be "clean" C code....but...

It wouldn't be C code at all. Presumably it would be C++ code.
C++ has its own newsgroups.

There are also newsgroups that discuss Unix programming
(comp.unix.programmer) and threads (comp.programming.threads).

 

[Seebs]

I've been thinking - just to safe time - to just make use of the std
namespace and its queue and string components - any comments to that
approach? I know it wouldn't be "clean" C code....but...

It wouldn't do you any good.

You were just given an EXACT identification of the error in your code,
which has nothing at all to do with threads. You responded to it by
asking whether you should change to C++.

It wouldn't do you any good. What will solve your problem is understanding
what's happening. Switching from one thing you don't understand to another
will probably not help you much.

-s

 

[cerr]

It wouldn't do you any good.

You were just given an EXACT identification of the error in your code,
which has nothing at all to do with threads.  You responded to it by
asking whether you should change to C++.

It wouldn't do you any good.  What will solve your problem is understanding
what's happening.  Switching from one thing you don't understand to another
will probably not help you much.

Exactly, I did sure get ahead of where i was before yesterday but then
again, if you look at my other post
http://groups.google.com/group/comp...cb84c1/178401a4addd2c36?#doc_1f79520da1495082
I'm still aseeing some major issues that i dont' know where they're
from. I did make the strcopy - i understand that i messed up there but
i don't know why i would "lose" the first bytes of my message as i
allocate memory in the array with "malloc (strlen(buf)+1)" - on the
other hand i think if i used std::string, this would not be happening
- same with std::queue - would probably make it easier, no? But I
anyways would like to stick with C cause the rest of the code is
written in C.

 

[Seebs]

I'm still aseeing some major issues that i dont' know where they're
from.

Okay. But the post you just quoted pointed out specifically that you were
ignoring your allocated memory and just using the same pointers to everything,
which was the actual source of your problem.

I did make the strcopy - i understand that i messed up there but
i don't know why i would "lose" the first bytes of my message as i
allocate memory in the array with "malloc (strlen(buf)+1)" - on the
other hand i think if i used std::string, this would not be happening
- same with std::queue - would probably make it easier, no?

Maybe. It seems unlikely, though.

But I
anyways would like to stick with C cause the rest of the code is
written in C.

Okay.

Then my suggestion is this: Separate this out into a test program which
doesn't use any threads or anything else, and just creates the data
structure, and then reads it to verify that you're getting the results you
want. If you aren't, then you'll have a much smaller, self-contained,
program you can use to figure out what's going wrong.

-s