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!
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
--
Copyright 2009, all wrongs reversed. Peter Seebach / usenet...@seebs.net
http://www.seebs.net/log/ <-- lawsuits, religion, and funny pictures
http://en.wikipedia.org/wiki/Fair_Game_(Scientology) <-- get educated!
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.
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
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.
Or by a sig_atomic_t store.
Or by lockf(2).
And since we're now firmly off the beaten track of comp.lang.c, perhaps I
should take license to enumerate all the various lock-free algorithms that
most hardware provides. Ever try to _initialize_ a mutex in Win32 in a
thread-safe manner? Now that's an interesting dilemma if there ever was one.
Perhaps we could begin discussing the limitations of the compare-and-swap
primitive? Or how no commercial CPU actually implements a universal
load-link, store-conditional pair.
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
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?
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.
Following would do this, right?
free(msglist[CBUF_Len(LogQ)]);
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.)
Or perhaps comp.programming.threads.
--
Keith Thompson (The_Other_Keith) ks...@mib.org <http://www.ghoti.net/~kst>
Nokia
"We must do something. This is something. Therefore, we must do this."
-- Antony Jay and Jonathan Lynn, "Yes Minister"
[ 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.
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 fuck off. Pathetic.
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[i]);
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
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...
> 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.
--
Ben Pfaff
http://benpfaff.org
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).
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.lang.c/tree/browse_frm/thread/c4af50d1cacb84c1/78c9aa31780dbdb8?rnum=11&_done=%2Fgroup%2Fcomp.lang.c%2Fbrowse_frm%2Fthread%2Fc4af50d1cacb84c1%2F178401a4addd2c36%3F#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.
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.
It's not at all clear what you mean by "transparent" (pun
intended, but serious). Some pointers are opaque, some are pale
translucent pearly pink (the greenish ones are very rare and
expensive), but I can't recall ever seeing a transparent pointer.
Perhaps the good people in the other forums TO WHICH YOU HAVE
ALREADY BEEN REFERRED are better able to do pointer spectrography.
> [...]
> Thanks for hints and suggestions!
Hint: Try asking on a different forum.
Suggestion: Try asking on a different forum.
Plea: Try asking on a different forum.
--
Eric Sosman
eso...@ieee-dot-org.invalid
Perhaps, or perhaps not. POSIX defines a long list of functions which are
cancellation points, and perhaps that implies a memory barrier; or perhaps
something else in POSIX requires a memory barrier. I don't know, and this
not being comp.programming.threads or comp.unix.programmer, there aren't as
many eyeballs around to read and respond to such things. Even if two or ten
persons do, they could just as well be wrong. The benefit of topicality is
that it allows people to congregate in the most appropriate forum for a
topic, and so there are more checks & balances.
> > 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?
Unlike in POSIX you cannot statically initialize a mutex/critical section;
critical section objects must be initialized at run time. And if at the
point of initialization there isn't implicit synchronization, you have to
literally roll your own locking mechanism.
For example, my portable arc4random implementation (which is defined static
inline in a header) can't require explicitly initialization (i.e. from main,
before there are any threads), because then it wouldn't mimic the behavior
of native implementations. So you have to do contortions like the following.
(Note, however, that I could be horribly wrong about static initialization
of Win32 critical sections. I don't do much Win32 work. Again, this is why
comp.lang.c is the wrong forum).
static struct {
/* snip */
#if _WIN32
struct {
volatile LONG init;
CRITICAL_SECTION cs;
} mux;
#else
pthread_mutex_t mux;
#endif
} arc4 = { /* snip */ };
/* snip */
#if _WIN32
volatile LONG init;
do {
switch (init = InterlockedCompareExchange((LONG *)&arc4.mux.init, 1, 0)) {
case 2: /* initialized already. */
break;
case 1: /* try again. */
Sleep(1);
break;
case 0: /* must initialize. */
InitializeCriticalSection(&arc4.mux.cs);
InterlockedExchange((LONG *)&arc4.mux.init, 2);
break;
}
} while (1 == init);
EnterCriticalSection(&arc4.mux.cs);
#else
pthread_mutex_lock(&arc4.mux);
#endif
> > 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?!
Ever try to implement software transactional memory? You can't do it with
CAS, nor DCAS (though I remember a probablistic algorithm using DCAS, but
some early AMD64 chips lacked a 128-bit DCAS operation so I never followed
that avenue; I'm unsure whether that algo used dynamic memory and so
implicitly relied on the mutex inside malloc()).
> > 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?
Nope, because they only support "weak" LL-SC, which in practice makes them
as useful as DCAS. In fact, no general purpose commercial CPU can actually
implement software transactional memory without a mutex (i.e. neither
lock-free nor wait-free); and all the libraries that advertise "software
transactional memory" explicitly or implicitly rely on mutexes (in fact,
AFAIK all the libraries explicitly use mutexes, but I've seen snippets of
code on forums which implicitly rely on mutexes through blackbox functions
such as malloc()).
Yup, I followed your suggestion and put this little app together which
works just fine:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdarg.h>
#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;
char **temp;
int i=0;
int main (void)
{
char *buf="Test1";
temp = realloc(msglist,(CBUF_Len(LogQ)+1)*sizeof(*temp));
if (temp==NULL){
printf("Error reallocating memory for msglist\n");
for (i=CBUF_Len(LogQ);i>=0;i--)
free(msglist[i]);
free(msglist);
return;
}
msglist=temp;
msglist[CBUF_Len(LogQ)] = malloc (strlen(buf)+1);
if (msglist[CBUF_Len(LogQ)]==NULL){
printf("Error allocating memory for the string in msglist\n");
return;
}
strcpy(msglist[CBUF_Len(LogQ)],buf);
CBUF_Push( LogQ, msglist[CBUF_Len(LogQ)] );
printf("push \"%s\" onto LogQ(%d)\n",buf,CBUF_Len(LogQ));
printf("%s",CBUF_Pop(LogQ));
return 0;
}