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

Subject: Re: [boost] [exception] RFC: patch for printing stacktraces during exceptions.
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2012-02-27 17:42:00

Le 27/02/12 16:41, Brett Lentz a écrit :
> 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.
I have no doubt of the utility of a backtrace library. The usual way is 
to propose a library for review.
>
>
> There is a little bit of documentation in 
> boost/exception/backtrace.hpp, but
> I'll see about getting some test cases created.
I read them. More like that will help to understand user interface and 
the design.
>
>> * 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.
I don't know the effort to get the code portable on windows platform, 
but don't providing it is a show-stopper to his adoption.
>
>> 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.
What I meant, is that will expect that the implementation of the file 
boost/exception/tracable_exception.hpp to be in the exception library. 
Anyway, it will be better if you can put all in a separated library, 
e.g? BackTrace
>
>> * Is it necessary that tracable_thread inherits from thread? What 
>> about adding a tracker that can be used on any thread?
Here again, it would help if you could define a tracable_thread or a 
tracker object, that doesn't need the modification of boost::thread. You 
could make traceable_thread a wrapper of boost::thread or use a thread 
specific Tracker object.
>>
>> * 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.
I guess that any library will be free make its exceptions to inherit 
from traceable_exception, and this should not imply to lost the its 
current interface. All the thread_exception, must inherit from 
boost::system_error.
>
>
>> * 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?
As this is an optimization feature, I think it will be enough to provide 
it for the compilers supporting it. The questions is if the proposed 
macros works for all the compiler proposing a similar feature.
>
>> * 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.
AFAIK, every Boost file must contain the Boost license.
>
>
> 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.
>
Please, try to convert the patch in one or several separated libraries 
or specific patches for each library. It will much easier to get at 
least some of them accepted. If you don't reach to get them included 
into Boost, you could always add them as part of Passenger.

Best,
Vicente