Re: [boost] [exception] RFC: patch for printing stacktraces during exceptions.

Subject: Re: [boost] [exception] RFC: patch for printing stacktraces during exceptions.
From: Brett Lentz (blentz_at_[hidden])
Date: 2012-02-27 10:41:23

On 25/02/12 12:41 +0100, Vicente J. Botet Escriba wrote:
>Le 24/02/12 15:57, Brett Lentz a écrit :
>>Hi folks!
>>
>>It's been a few months since I last mailed the list. I've been working on
>>bringing some forked changes from the Passenger project into Boost 
>>due to their
>>more general applicability.
>>
>>With some help, I've got a working patch against SVN trunk that 
>>enables one of
>>Passenger's more useful features: obtaining exception stacktraces 
>>all the way
>>through the stack.
>>
>>However, due to the size of the patch and some namespacing choices 
>>made by the
>>Phusion people, I'd like to get some feedback on this patch to 
>>ensure it's
>>suitable for being brought into Boost.
>>
>Hi,
>
>I wanted just to note that this patch concerns the libraries 
>Boost.Exception, Boost.System and Boost.Thread and that it is a 
>library without a name.
>Could it be possible to define these functionalities in independent 
>libraries?
>
>>First, there's some helper classes that Phusion has put in an "oxt" 
>>namespace
>>that I'm not really sure where they should be put or if suitable 
>>analogues
>>already exist within Boost. For the current version of the patch, 
>>these files
>>are simply put into 'boost/oxt' until I have a better location for them.
>Which libraries are using it in your patch. Maybe you can put it on 
>boost::detail::oxt?

I can do that or, as you mention later in your reply, I can use the spin lock
implementation that already exists in boost.

>>Second, the current patch uses boost::tracable_exception. I'd like 
>>to confirm
>>whether that is acceptable or whether I should move this to a different
>>location in the boost namespace.
>>
>>Finally, any other comments about this patch are welcome. I have 
>>attached both
>>the patch, and some example code that will demonstrate the functionality.
>>
>>
>I don't know what is your intention proposing this patch, but I don't 
>think the patch in its actual form could be applied to Boost. Its is 
>a big patch changing a lot of code on existing libraries without any 
>test, neither documentation.
>

My personal goal is to get Passenger to the point where it can compile against the
vanilla Boost libraries with minimal or no loss in functionality. This is a 
prerequisite for its inclusion into Fedora and is very useful for a few 
different reasons. 

Passenger is something a few different people have been trying to get into 
Fedora for quite a while now. I'm just trying to help the process along.
(For the full history, see: https://bugzilla.redhat.com/show_bug.cgi?id=470696)

I believe that the changes that Passenger currently ships in their bundled
version of Boost are useful and generally applicable. Other users of Boost can
benefit from the inclusion of features like fully traceable exceptions.

There is a little bit of documentation in boost/exception/backtrace.hpp, but
I'll see about getting some test cases created.

>* I don't see any changes on windows files. Have you an 
>implementation for windows?
>

I do not have a Windows implementation. That platform isn't the Passenger 
folks' focus, nor is it mine. If this is an issue that will prevent this 
feature from being included, I can see about adding Windows support to the patch.

>Note that the patch contains the files
>
>Index: boost/exception/tracable_exception.hpp
>
>Index: libs/thread/src/pthread/tracable_thread.cpp
>
>
>That is, the declaration is in Boost.Exception, while the 
>implementation is in Boost.Thread :(
>

I'm open to suggestions on how you think it should be implemented. Passenger is
highly multi-threaded server software, so that's the focus. If we need to make
a change to make this feature more generally useful, it's something we can
consider.

>* Is it necessary that tracable_thread inherits from thread? What 
>about adding a tracker that can be used on any thread?
>
>* Why thread_exception don't inherits now from boost::system_error? 
>This breaks the current exception interface. Is it absolutely needed 
>that thread_exception inherits from tracable_exception to provide 
>tracable exceptions?
>

The point of doing it this was was to have Boost.Thread throw exceptions that
are derived from thread_exception. Doing it another way doesn't provide the
same ability to trace the full length of the stack.

>* It seems to me that dynamic_thread_group could be a nice to have, 
>but not required to have backtraces, so it should be proposed 
>separately.
>

I'll do that.

>* There is already a spin lock implementation in Boost detail. Have 
>you see if it responds to your needs?
>

This is something I will investigate. The current page is lifted from the
Passenger code as closely to how it exists there. Our first goal was just to
get a working patch that can be applied to demonstrate the functionality. 

Now that we've got that, I want to start figuring out what issues will prevent
inclusion into Boost.

>* I propose you to make a concrete proposal for the macros LIKELY and 
>UNLIKELY to Boost.Config.

I'll do that. Being that this gets into compiler-specific and/or platform-specific 
territory, can you or anyone else comment on what minimum platforms need to be
supported for inclusion?

>* system_calls seems to me an independent thing of backtraces and 
>tracable exceptions and quite dependant on posix interface.
>

I'll take a closer look at that and separate it out, if possible.

>* It is current usage on Boost to place the implementation details in 
>the namespace detail (see all the functions prefixed by "_" and to 
>prefix any macro with BOOST_LIBNAME_.
>
>* I don't know if the license is compatible with the Boost license. 
>Could you use the Boost license?
>

Passenger is using the stock MIT License which appears to be what the Boost
License is an extension of. I can ping the Passenger guys to see if the
additional text in the second paragraph of the Boost License is acceptable to 
them. However, in my non-legal opinion, these licenses should be compatible as
they're substantially the same text.

>* Please could you make a ticket concerning the changes in
>
>Index: libs/thread/src/tss_null.cpp
>
>Index: libs/thread/src/pthread/once.cpp
>
>Index: libs/thread/src/pthread/thread.cpp
>

I will do that.

>
>HTH,
>

Thanks so much for the comments. I'll be working on making the changes you
suggest. Hopefully I'll have a new patch set to post to the list soon.

>Vicente

---Brett.