bugGnash - The GNU Flash player - Bugs: bug #19906, segfault in logging

 
 

bug #19906: segfault in logging

Submitted by:  Tomas Groth <tgc>
Submitted on:  Sat 19 May 2007 02:01:53 PM UTC  
 
Category: coreSeverity: 4 - Important
Release: NoneStatus: Fixed
Privacy: PublicAssigned to: Sandro Santilli <strk>
Open/Closed: Closed

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Fri 25 May 2007 09:43:25 AM UTC, comment #12:

I haven't seen the crash since the patch was committed, so i assume it's fixed.

Tomas Groth <tgc>
Mon 21 May 2007 07:09:07 AM UTC, comment #11:

I committed that patch finally. At least the segfault is fixed.

Sandro Santilli <strk>
Project MemberIn charge of this item.
Sun 20 May 2007 06:59:11 AM UTC, comment #10:

Ok, would a log(any, arg, type) work for you then ?
It would be semantically equivalent of current 'dbglogfile << any << arg << type << endl' but force specifying exact boundaries for a call, making locking more manageable.

I still think internationalization would suffer, but we're looking for a compromise here (and we can keep both interfaces)

Sandro Santilli <strk>
Project MemberIn charge of this item.
Sun 20 May 2007 01:29:46 AM UTC, comment #9:

Cout doesn't write timestamps with each line, nor does it optionally write to a disk file. Using printf() style output in C++ programs is just plain gross. To me add .c_str() is much messier than looking for %s.

Rob Savoye <rsavoye>
Project Administrator
Sat 19 May 2007 11:25:44 PM UTC, comment #8:

Just another drawback of output operator jumped to my mind while about to sleep: internationalization !

Different languages construct phrases placing elements in different order. So, think about:

cout << "subject verb" << object;

And:

printf("subject verb %s", object);

The latter is more eas to translate, as the translator could
move the %s part everywhere !!

This hitted my attention while thinking about the log(any, argument, here) proposal, which would make translation an hard job... not a problem if log(any, thing, here) would be only used for temporary debugging (not in releases) but again, I don't see why not using std::cout in that case...

Sandro Santilli <strk>
Project MemberIn charge of this item.
Sat 19 May 2007 11:03:28 PM UTC, comment #7:

The patch seems to fix the segfault, but since it didn't always appear, i can't say for certain it's fixed, but it does look like it.

Tomas Groth <tgc>
Sat 19 May 2007 11:00:04 PM UTC, comment #6:

Last message before going to sleep.

My proposal is to commit this patch and work on a new interface
for logging which accept arbitrary arguments.
The new interface would look like this:

log_*(any, argument, here);

The '*' would follow any existing logging function name.
The advantage of it would be not having to deal with
printf-like formats.

To avoid the clashes it could be a member of LogFile so
migration is smooth.

NOTE that this would require defining a templated function
for any number of arguments we want to pass to the log()
method, which could be maken more maintainable by using
metaprogramming tecniques, which boost provide.

See: http://www.boost.org/libs/preprocessor/doc/index.html

Of course I'm suggesting we commit my patch before 0.8.0
and work on the more general logging facilities after it.

Comments welcome.

Sandro Santilli <strk>
Project MemberIn charge of this item.
Sat 19 May 2007 09:56:40 PM UTC, comment #5:

Sorry, reattached (previous patch failed printing log_debug messages)

(file #12806)

Sandro Santilli <strk>
Project MemberIn charge of this item.
Sat 19 May 2007 09:52:06 PM UTC, comment #4:

The LogFile::operator<< interface has these drawbacks:

1) A debugging line takes multiple function calls, thus making it hard to manage locking or labelling.

2) It doesn't extend std::ostream::operator<<, thus isn't useful to use pre-existing output operators.

Attached is a patch that makes LogFile::operator<< methods private and expose a new LogFile::log() method, intended to be called by the existing log_* functions, which are semantically defined and documented.

The need expressed by rob is not bothering about arguments type.
I think we might implement this with some templates magic, altought I don't have a plan currently.

So, please review the patch. Most importantly see if it fixes the thread-safety issue.

(file #12805)

Sandro Santilli <strk>
Project MemberIn charge of this item.
Sat 19 May 2007 08:30:16 PM UTC, comment #3:

To be honest, I dislike printf() style debugging in C++ programs, so in a way I'd prefer to see us use dbglogfile << more than all the log_() functions. We can derive other classes from the base class to have different output streams instead of all the log_() functions. It's not a practical short term goal, but someday I'd prefer to eliminate log_().

Rob Savoye <rsavoye>
Project Administrator
Sat 19 May 2007 08:24:15 PM UTC, comment #2:

More info on this task:

The problem is that some files are still using operator<< of LogFile, which would still be non thread-safe if we move locking
to the log_* functions.

A plan could be:

1) Forbid use of operator<< making them private methods of the LogFile class
2) Provide a public log(level, fmt, ...) methods of LogFile, locking a private io_mutex
3) Updating any user so that operator<< use is substituted by appropriate call to log_*()
4) Have log_*() functions call LogFile::log(level, fmg, ...)

Sandro Santilli <strk>
Project MemberIn charge of this item.
Sat 19 May 2007 08:11:38 PM UTC, comment #1:

Took a look, it seems all operator<< use a mutex, except
one, the templated:

template <class T>
LogFile& operator << (const T& any)

Generally speaking... I'd have the log_* functions use
the mutex, rather then the operator<<, which should likely
never be directly invoked.

Using the mutex from the templated function won't be easy
as the io_mutex is defined in the .cpp file, not the .h one.

Feel like trying ? (remove all scoped_lock(io_mutex) from operator<< and add to log_*)

Sandro Santilli <strk>
Project MemberIn charge of this item.
Sat 19 May 2007 02:01:53 PM UTC, original submission:

I'm working on NetStreamFfmpeg, and are using log_debug() (a lot) to debug, and sometimes i get segfaults. I'm using threads, so maybe it's not thread-safe.

Backtrace:
[Switching to Thread 47507331171776 (LWP 613)]
0x00002b3523102665 in raise () from /lib/libc.so.6
(gdb) bt
#0 0x00002b3523102665 in raise () from /lib/libc.so.6
#1 0x00002b352310391e in abort () from /lib/libc.so.6
#2 0x00002b3523137f96 in __libc_message () from /lib/libc.so.6
#3 0x00002b352313cf3d in malloc_printerr () from /lib/libc.so.6
#4 0x00002b352313e576 in free () from /lib/libc.so.6
#5 0x00002b3522e62afc in std::string::assign () from /usr/lib/libstdc++.so.6
#6 0x00002b3522b3b0cc in gnash::LogFile::operator<< (this=0x2b3522c95840,
str=0x2b3522b76964 "DEBUG: ")
at /home/tomas/gnash/gnash/libbase/log.cpp:595
#7 0x00002b3522b3b428 in gnash::log_debug (
fmt=0x2b352293fa88 "breaking loop, queue size: %ld, current: %lf, video: %lf") at /home/tomas/gnash/gnash/libbase/log.cpp:210
#8 0x00002b35228ba09a in gnash::NetStreamFfmpeg::advance (this=0x7e0470)
at /home/tomas/gnash/gnash/server/asobj/NetStreamFfmpeg.cpp:1102
#9 0x00002b35227ee942 in gnash::video_stream_instance::advance (this=0x7da5e0)
at /home/tomas/gnash/gnash/server/video_stream_instance.cpp:126

Tomas Groth <tgc>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #12806:  LogFile.patch added by strk (18KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by rsavoye (Posted a comment)
  • -unavailable- added by strk (Posted a comment)
  • -unavailable- added by tgc (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 10 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 25 May 2007 09:43:25 AM UTCtgcStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Mon 21 May 2007 07:09:07 AM UTCstrkStatusNone=>Ready For Test
    Sun 20 May 2007 01:29:46 AM UTCrsavoyeSeverity5 - Blocker=>4 - Important
    Sat 19 May 2007 11:25:44 PM UTCstrkCarbon-Copy-=>Added jgilmore
    Sat 19 May 2007 11:00:04 PM UTCstrkSeverity3 - Normal=>5 - Blocker
      Assigned toNone=>strk
    Sat 19 May 2007 09:56:40 PM UTCstrkAttached File-=>Added LogFile.patch, #12806
    Sat 19 May 2007 09:54:32 PM UTCstrkAttached File#12805=>Removed
    Sat 19 May 2007 09:52:06 PM UTCstrkAttached File-=>Added LogFile.patch, #12805

    Back to the top


    Powered by Savane 3.1-cleanup1