Re: [boost] [Boost] [Stacktrace] review
On Wed, Dec 14, 2016 at 3:43 AM, Niall Douglas <s_sourceforge@nedprod.com> wrote: For your review you may wish to consider the following questions:
- What is your evaluation of the design?
I'm generally pleased with the design. There's a documentation section "Exceptions with stacktrace" that discusses techniques for incorporating stacktrace data into your own exceptions. I see a real possibility for integration with Boost.Exception, perhaps even a mechanism to make BOOST_THROW_EXCEPTION embed the stacktrace at the throw point. This is not an objection to accepting the library, but rather a feature request for when we do. I do have a question about use of a template parameter to constrain the number of entries, rather than a runtime parameter. Presumably that's necessary to permit certain backend implementations to be async-signal-safe? (Whatever the rationale, a note in the documentation explaining it would be nice.) My objection to that is that apparently, with the present design, if I want to write a function that accepts any specialization of basic_stacktrace, that must itself be a template function. If I'm correct about the reason for using a template parameter, would it be reasonable to split the class into a non-template base class supporting all the methods, with a template subclass that provides the storage? The base class could store a (pointer, length) passed by the subclass constructor. Without having looked at the implementation, I expect that you wouldn't even need virtual-function dispatch: the only indirection would be through the data buffer pointer. That would even permit a subclass that provides dynamic storage, for async-signal-unsafe use. Then a processing function could accept reference-to-base-class and handle any stacktrace instance.
- What is your evaluation of the implementation?
Sorry, I haven't looked at the implementation.
- What is your evaluation of the documentation?
Generally good. It could stand a cleanup pass; there are a few typos here and there. I do like that each method explicitly states both complexity and async signal safety. The description of the nullary frame() constructor says that source_line() will return both empty string and 0.
- What is your evaluation of the potential usefulness of the library?
VERY! I want this.
- Did you try to use the library? With what compiler? Did you have any problems?
No, I haven't tried to use it. (A deadline is coming up fast.)
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
I read through the documentation.
- Are you knowledgeable about the problem domain?
I have occasionally used platform-specific tactics to try to achieve the same thing, on a couple different platforms. Each time it's been painful enough to make me wish that this problem were solved, once and for all.
And finally, every review should attempt to answer this question:
- Do you think the library should be accepted as a Boost library?
Yes I do. I would like to see the non-template base class, but that's not a showstopper: I would rather have this library without than not have it at all. As I mentioned, I would like to see integration with Boost.Exception. That just seems like a natural next step. I see further opportunities for integration with Boost.Context, Boost.Fiber...
Nat Goodspeed wrote:
There's a documentation section "Exceptions with stacktrace" that discusses techniques for incorporating stacktrace data into your own exceptions. I see a real possibility for integration with Boost.Exception, ...
Yes, this jumped out at me too. The whole point of Boost.Exception is to carry arbitrary data along with the exception, so an example that uses Boost.Exception instead of the custom 'traced' type should be given.
... perhaps even a mechanism to make BOOST_THROW_EXCEPTION embed the stacktrace at the throw point.
This sounds like a pretty good idea, although I'm not sure if we can enable it by default as people have historically been very sensitive to changes to BOOST_THROW_EXCEPTION. But an opt-in macro should be fine and very useful as one would automatically get stack traces from every use of BOOST_THROW_EXCEPTION. Hello Java.
On 12/15/16 02:27, Peter Dimov wrote:
Nat Goodspeed wrote:
... perhaps even a mechanism to make BOOST_THROW_EXCEPTION embed the stacktrace at the throw point.
This sounds like a pretty good idea, although I'm not sure if we can enable it by default as people have historically been very sensitive to changes to BOOST_THROW_EXCEPTION. But an opt-in macro should be fine and very useful as one would automatically get stack traces from every use of BOOST_THROW_EXCEPTION. Hello Java.
I've not taken a look at the proposed Stacktrace library, but from what I know obtaining a stacktrace is typically a rather expensive operation. I expect it to be no less expensive than throwing the exception. That's probably too much to pay for by default. Plus, it adds one more dependency on another library via such a core component as BOOST_THROW_EXCEPTION. I think, a new macro in a separate library (i.e. not in throw_exception) would be more preferable.
On Wed, Dec 14, 2016 at 3:44 PM, Andrey Semashev <andrey.semashev@gmail.com> wrote:
On 12/15/16 02:27, Peter Dimov wrote:
Nat Goodspeed wrote:
... perhaps even a mechanism to make BOOST_THROW_EXCEPTION embed the
stacktrace at the throw point.
This sounds like a pretty good idea, although I'm not sure if we can enable it by default as people have historically been very sensitive to changes to BOOST_THROW_EXCEPTION. But an opt-in macro should be fine and very useful as one would automatically get stack traces from every use of BOOST_THROW_EXCEPTION. Hello Java.
I've not taken a look at the proposed Stacktrace library, but from what I know obtaining a stacktrace is typically a rather expensive operation. I expect it to be no less expensive than throwing the exception. That's probably too much to pay for by default. Plus, it adds one more dependency on another library via such a core component as BOOST_THROW_EXCEPTION.
I think, a new macro in a separate library (i.e. not in throw_exception) would be more preferable.
I think that the stack trace should be embedded by default by boost::throw_exception (not by BOOST_THROW_EXCEPTION), with a new configuration macro that can be used to disable this new behavior. I think that performance concerns are unwarranted in this case, since throwing exceptions is expensive already; it is generally not a good idea to assume anything about its performance. And stack trace is very useful bit of information to have. That said, I am much more concerned about added dependencies, because boost/exception/exception.hpp (which boost/throw_exception.hpp includes) is carefully designed to have NO dependencies. I would be open to providing special hooks into the boost::exception class to support stack trace (similarly to how there is special storage for __FILE__ and __LINE__), as long as the dependencies make sense. Emil Emil
On 12/15/16 03:12, Emil Dotchevski wrote:
I think that the stack trace should be embedded by default by boost::throw_exception (not by BOOST_THROW_EXCEPTION), with a new configuration macro that can be used to disable this new behavior. I think that performance concerns are unwarranted in this case, since throwing exceptions is expensive already; it is generally not a good idea to assume anything about its performance.
I disagree. Even though throwing an exception is not something that should be on the hot path, performance still matters because the code might have to be fast while processing failures as well.
On 15/12/2016 12:44, Andrey Semashev wrote:
On 12/15/16 02:27, Peter Dimov wrote:
This sounds like a pretty good idea, although I'm not sure if we can enable it by default as people have historically been very sensitive to changes to BOOST_THROW_EXCEPTION. But an opt-in macro should be fine and very useful as one would automatically get stack traces from every use of BOOST_THROW_EXCEPTION. Hello Java.
I've not taken a look at the proposed Stacktrace library, but from what I know obtaining a stacktrace is typically a rather expensive operation. I expect it to be no less expensive than throwing the exception. That's probably too much to pay for by default. Plus, it adds one more dependency on another library via such a core component as BOOST_THROW_EXCEPTION.
Capturing the stack to later perform a trace is relatively cheap and fast (though perhaps memory wasteful, though you usually wouldn't care about that unless the exception was due to running out of memory). It's interpreting that data into a human-readable trace that's the slow part. And I've previously asked about this and been assured that this library does do this in two separate phases to reduce performance impact at the trace site. Also, I'm not sure about other compilers, but with MSVC thrown exceptions always carry a stack trace with them anyway; it's possible to extract the stack trace from any exception with some platform-dependent code (without any decoration on the throw -- and depending on build options, it can even work for null references and other CPU exceptions, although some misguided people dislike that). I haven't looked at this library in detail yet so I don't know if it takes advantage of this or not.
On Wed, Dec 14, 2016 at 6:01 PM, Gavin Lambert <gavinl@compacsort.com> wrote:
On 15/12/2016 12:44, Andrey Semashev wrote:
On 12/15/16 02:27, Peter Dimov wrote:
This sounds like a pretty good idea, although I'm not sure if we can enable it by default as people have historically been very sensitive to changes to BOOST_THROW_EXCEPTION. But an opt-in macro should be fine and very useful as one would automatically get stack traces from every use of BOOST_THROW_EXCEPTION. Hello Java.
I've not taken a look at the proposed Stacktrace library, but from what I know obtaining a stacktrace is typically a rather expensive operation. I expect it to be no less expensive than throwing the exception. That's probably too much to pay for by default. Plus, it adds one more dependency on another library via such a core component as BOOST_THROW_EXCEPTION.
Capturing the stack to later perform a trace is relatively cheap and fast
+1
(though perhaps memory wasteful, though you usually wouldn't care about that unless the exception was due to running out of memory).
This is not a concern. Throwing any exception type may result in throwing std::bad_alloc instead. Emil
On 15 Dec 2016 at 15:01, Gavin Lambert wrote:
I've not taken a look at the proposed Stacktrace library, but from what I know obtaining a stacktrace is typically a rather expensive operation. I expect it to be no less expensive than throwing the exception. That's probably too much to pay for by default. Plus, it adds one more dependency on another library via such a core component as BOOST_THROW_EXCEPTION.
For a capture depth of seven frames, it costs about 30 microseconds on Microsoft Windows. Stack backtracing is inherently cache unfriendly. That is a lot more than throwing an exception. The people on SG14 have tried quite hard to demonstrate that throwing exceptions is slower than alternatives, but it has proven surprisingly hard to conclusively prove it in real world code. I am sure the compiler vendors would say "I told you so ..."
Capturing the stack to later perform a trace is relatively cheap and fast (though perhaps memory wasteful, though you usually wouldn't care about that unless the exception was due to running out of memory). It's interpreting that data into a human-readable trace that's the slow part.
Multiple orders of magnitude difference yes.
And I've previously asked about this and been assured that this library does do this in two separate phases to reduce performance impact at the trace site.
Could you take a closer look at the actual implementation code please?
Also, I'm not sure about other compilers, but with MSVC thrown exceptions always carry a stack trace with them anyway; it's possible to extract the stack trace from any exception with some platform-dependent code (without any decoration on the throw -- and depending on build options, it can even work for null references and other CPU exceptions, although some misguided people dislike that).
I was not aware that this is the case except when /EHa is used. Indeed, I had thought one of the big optimisations of /EHs was precisely the fact that backtraces are *not* captured, thus making C++ exception throws vastly faster than Win32 structured exception throws and the point of using /EHs. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 15/12/2016 20:46, Niall Douglas wrote:
Also, I'm not sure about other compilers, but with MSVC thrown exceptions always carry a stack trace with them anyway; it's possible to extract the stack trace from any exception with some platform-dependent code (without any decoration on the throw -- and depending on build options, it can even work for null references and other CPU exceptions, although some misguided people dislike that).
I was not aware that this is the case except when /EHa is used. Indeed, I had thought one of the big optimisations of /EHs was precisely the fact that backtraces are *not* captured, thus making C++ exception throws vastly faster than Win32 structured exception throws and the point of using /EHs.
I don't think so. The docs simply indicate that it reduces the locations where it captures unwind info and does some checks. This does increase performance by reducing code size (and thus the working set) and removing some conditional logic, but it comes at the cost of failing to execute some destructors if an exception does get thrown/raised. (Which is probably ok if the process is going to be terminated by an uncaught exception anyway, but less so if not.) I didn't see anything about changing the actual implementation of exceptions. The following code *does* print "caught exception" (in both Debug and Release) even when compiled with /EHsc. Granted I suppose this doesn't explicitly prove that a stack trace was captured, but I think it proves that C++ exceptions are still internally implemented as SEH exceptions, and so GetExceptionInformation (among other things) can probably still be used to extract the stack context. void badfn() { throw std::runtime_error("error"); } template<typename F> void seh_wrapper(F fn) { __try { fn(); } __except (EXCEPTION_EXECUTE_HANDLER) { std::cout << "caught exception" << std::endl; } } int main(int argc, char* argv[]) { seh_wrapper(&badfn); std::cout << "completed" << std::endl; return 0; }
On Thu, Dec 15, 2016 at 3:36 PM, Gavin Lambert <gavinl@compacsort.com> wrote:
On 15/12/2016 20:46, Niall Douglas wrote:
Also, I'm not sure about other compilers, but with MSVC thrown
exceptions always carry a stack trace with them anyway; it's possible to extract the stack trace from any exception with some platform-dependent code (without any decoration on the throw -- and depending on build options, it can even work for null references and other CPU exceptions, although some misguided people dislike that).
I was not aware that this is the case except when /EHa is used. Indeed, I had thought one of the big optimisations of /EHs was precisely the fact that backtraces are *not* captured, thus making C++ exception throws vastly faster than Win32 structured exception throws and the point of using /EHs.
I don't think so. The docs simply indicate that it reduces the locations where it captures unwind info and does some checks. This does increase performance by reducing code size (and thus the working set) and removing some conditional logic, but it comes at the cost of failing to execute some destructors if an exception does get thrown/raised. (Which is probably ok if the process is going to be terminated by an uncaught exception anyway, but less so if not.) I didn't see anything about changing the actual implementation of exceptions.
The overhead in structured exception handling comes from translating hardware exceptions to C++ exceptions. Under this (quite stupid) model, the compiler has to generate code that is able to throw C++ exceptions if any arbitrary instruction happens to crash (instead, obviously, in C++ only the throw keyword can throw and exception.) Emil
On 16/12/2016 13:04, Emil Dotchevski wrote:
The overhead in structured exception handling comes from translating hardware exceptions to C++ exceptions. Under this (quite stupid) model, the compiler has to generate code that is able to throw C++ exceptions if any arbitrary instruction happens to crash (instead, obviously, in C++ only the throw keyword can throw and exception.)
Which is what I said. I don't agree that it's necessarily stupid, though (though I do agree that it's more work). All it really boils down to though is more possible unwind state permutations (one per variable rather than one per throwable point), not really much in the way of additional code. It's not even really that much different from what it would have to do anyway in the presence of noexcept(false) constructors, which are still the majority case. It's a much better design than using signals for that, especially if your application does have a sensible way to recover from that (because you know that destructors will actually be called properly) and if the consequences of letting the application crash are worse than letting it abandon a failed task and move on. For most general-purpose applications, sure, it's usually better to just crash. As with any other feature, though, it's open to abuse and misuse by developers who don't know better, which can colour things inappropriately. (I would argue that signals are more easily misused, however, given how often people call signal-unsafe functions from a signal handler.)
On 16 Dec 2016 at 12:36, Gavin Lambert wrote:
On 15/12/2016 20:46, Niall Douglas wrote:
Also, I'm not sure about other compilers, but with MSVC thrown exceptions always carry a stack trace with them anyway; it's possible to extract the stack trace from any exception with some platform-dependent code (without any decoration on the throw -- and depending on build options, it can even work for null references and other CPU exceptions, although some misguided people dislike that).
I was not aware that this is the case except when /EHa is used. Indeed, I had thought one of the big optimisations of /EHs was precisely the fact that backtraces are *not* captured, thus making C++ exception throws vastly faster than Win32 structured exception throws and the point of using /EHs.
BTW I did some research and SEH exceptions do NOT capture stack backtraces. They do dump a CONTEXT structure, and from that you can walk a stack backtrace. This was the only legal way of getting a CONTEXT for the calling thread until Vista.
The following code *does* print "caught exception" (in both Debug and Release) even when compiled with /EHsc. Granted I suppose this doesn't explicitly prove that a stack trace was captured, but I think it proves that C++ exceptions are still internally implemented as SEH exceptions, and so GetExceptionInformation (among other things) can probably still be used to extract the stack context.
We have to be careful here. There is the SEH unwind mechanism and there is SEH itself. Absolutely everything unwinding a stack uses the former for any language, even Visual Basic, and hence C++ too. The latter is merely a client of the universal unwind mechanism, same as C++ exceptions is also implemented as a client of that mechanism. That's why throwing a C++ exception will get caught by a SEH exception handler, though without knowledge of the internal SEH codes MSVCRT uses you couldn't do much with that. Point is, it's universal and interoperable for all programming languages which can invert control flow (LLVM and GCC have a similar universal unwind framework). I also looked up some benchmarks. A C++ and SEH exception throw and catch costs about 2 microseconds on x86 on MSVC back in 2011 on a fairly low end CPU for the time. x86 SEH still uses the old setjmp/longjmp mechanism i.e. each stack frame pushes its unwind handler and pops it off the stack after i.e. it is not a "zero runtime cost" exceptions implementation. For x64, SEH unwind can now use table based unwinds i.e. zero runtime overhead exception, which SEH exposes as "vectored exception handling APIs". I didn't find any benchmarks online for what a throw and catch is on MSVC under x64, but it is likely to still be under 2 microseconds. I'm just about to go back onto minding newborn duty so I can't write the benchmark myself, but if anyone could quickly throw together a benchmark comparing C++ to SEH exception throw and catch on VS2015 it would be appreciated (just make sure the optimiser hasn't elided the code entirely). Just to remind everyone, a seven deep stack backtrace costs about 30 microseconds using the very optimised NT kernel backtrace routine. It is therefore a significant order of magnitude overhead addition to the cost of throwing and catching exceptions. Personally speaking, I would not make backtracing on exception throw default on therefore. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On Fri, Dec 16, 2016 at 11:05 AM, Niall Douglas <s_sourceforge@nedprod.com> wrote:
I also looked up some benchmarks. A C++ and SEH exception throw and catch costs about 2 microseconds on x86 on MSVC back in 2011 on a fairly low end CPU for the time. x86 SEH still uses the old setjmp/longjmp mechanism i.e. each stack frame pushes its unwind handler and pops it off the stack after i.e. it is not a "zero runtime cost" exceptions implementation.
The added run-time cost of SEH is not because it uses a different throw mechanism but because under SEH the compiler must assume that absolutely any CPU instruction may result in throwing a C++ exception. In MSVC this adds overhead at every function entry and exit, even functions that do not use the throw keyword (as a side note, all his overhead isn't critical because it can be avoided by inline, which one presumably would do anyway in critical functions.) Emil
On Wed, Dec 14, 2016 at 11:46 PM, Niall Douglas <s_sourceforge@nedprod.com> wrote:
On 15 Dec 2016 at 15:01, Gavin Lambert wrote:
I've not taken a look at the proposed Stacktrace library, but from what I know obtaining a stacktrace is typically a rather expensive operation. I expect it to be no less expensive than throwing the exception. That's probably too much to pay for by default. Plus, it adds one more dependency on another library via such a core component as BOOST_THROW_EXCEPTION.
For a capture depth of seven frames, it costs about 30 microseconds on Microsoft Windows. Stack backtracing is inherently cache unfriendly. That is a lot more than throwing an exception.
The people on SG14 have tried quite hard to demonstrate that throwing exceptions is slower than alternatives, but it has proven surprisingly hard to conclusively prove it in real world code. I am sure the compiler vendors would say "I told you so ..."
+1.
Capturing the stack to later perform a trace is relatively cheap and fast (though perhaps memory wasteful, though you usually wouldn't care about that unless the exception was due to running out of memory). It's interpreting that data into a human-readable trace that's the slow part.
Multiple orders of magnitude difference yes.
+1 With regards to integration into boost::throw_exception, obviously we only need to capture the data not format it. Emil
On 12/15/16 05:01, Gavin Lambert wrote:
On 15/12/2016 12:44, Andrey Semashev wrote:
On 12/15/16 02:27, Peter Dimov wrote:
This sounds like a pretty good idea, although I'm not sure if we can enable it by default as people have historically been very sensitive to changes to BOOST_THROW_EXCEPTION. But an opt-in macro should be fine and very useful as one would automatically get stack traces from every use of BOOST_THROW_EXCEPTION. Hello Java.
I've not taken a look at the proposed Stacktrace library, but from what I know obtaining a stacktrace is typically a rather expensive operation. I expect it to be no less expensive than throwing the exception. That's probably too much to pay for by default. Plus, it adds one more dependency on another library via such a core component as BOOST_THROW_EXCEPTION.
Capturing the stack to later perform a trace is relatively cheap and fast (though perhaps memory wasteful, though you usually wouldn't care about that unless the exception was due to running out of memory). It's interpreting that data into a human-readable trace that's the slow part. And I've previously asked about this and been assured that this library does do this in two separate phases to reduce performance impact at the trace site.
If that's the case then it would be nice to have some benchmark results before we impose that cost on everyone using BOOST_THROW_EXCEPTION. From my understanding (please, correct me if I'm wrong), even if you don't interpret the stacktrace, just to obtain it one has to iterate through the stack frames up to the beginning of the stack or some limit (which is not currently specified in BOOST_THROW_EXCEPTION). This has a linear complexity on the call depth. Throwing the exception, OTOH, is linear on the number of stack frames that need to be unrolled, plus other overhead such as exception construction and automatic variable destruction. Arguably, the latter complexity is typically less than the former, or the same in the worst case.
On 12/14/16 3:27 PM, Peter Dimov wrote:
This sounds like a pretty good idea, although I'm not sure if we can enable it by default as people have historically been very sensitive to changes to BOOST_THROW_EXCEPTION. But an opt-in macro should be fine and very useful as one would automatically get stack traces from every use of BOOST_THROW_EXCEPTION. Hello Java.
I think that there has been a lot of confusion about what BOOST_THROW_EXCEPTION is suppose to do. My understanding was that it was a macro intended to support the writing of portable code that could run on platforms which didn't support exceptions or where the user / author didn't want to use the exception mechanism so he could redefine the macro. Lot's of libraries used this idiom to decouple their libraries from the the selection of exception mechanism. With the acceptance of Boost.Exception, this BOOST_THROW_EXCEPTION was repurposed to call boost.exception. Obviously not what previous users of BOOST_THROW_EXCEPTION intended or expected. This made many libraries dependent on the new library boost.exception which had multiple repercussions - one of which is that boost.exception has now become a "core library" - not sure what that is supposed to mean. Now it's being suggested that BOOST_THROW_EXCEPTION be repurposed/enhanced yet again? Is this really a great idea? Wouldn't it better just to encourage authors of library authors to use their own code to throw exceptions and implement this code in the way most suitable for their own libraries? Of course the stack trace library might want to address this subject in the documentation and perhaps provide it's own macros or wrappers to facilitate the usage of this facility in other library code but the responsibility for a library implementation would still remain with the library author/maintainer. Robert Ramey
Robert Ramey wrote:
This made many libraries dependent on the new library boost.exception which had multiple repercussions - one of which is that boost.exception has now become a "core library" - not sure what that is supposed to mean.
No, throw_exception is its own mini-library now. It depends on nothing but Assert and Config. C:\Projects\boost-git\boost>dist\bin\boostdep.exe throw_exception Primary dependencies for throw_exception: assert: <boost/current_function.hpp> from <boost/throw_exception.hpp> config: <boost/config.hpp> from <boost/throw_exception.hpp> <boost/detail/workaround.hpp> from <boost/throw_exception.hpp>
On Wed, Dec 14, 2016 at 5:47 PM, Peter Dimov <lists@pdimov.com> wrote:
Robert Ramey wrote:
This made many libraries dependent on the new library boost.exception
which had multiple repercussions - one of which is that boost.exception has now become a "core library" - not sure what that is supposed to mean.
No, throw_exception is its own mini-library now. It depends on nothing but Assert and Config.
Also its dependence on Assert and Config has nothing to do with Exception. Emil
On 12/14/16 5:47 PM, Peter Dimov wrote:
Robert Ramey wrote:
This made many libraries dependent on the new library boost.exception which had multiple repercussions - one of which is that boost.exception has now become a "core library" - not sure what that is supposed to mean.
No, throw_exception is its own mini-library now. It depends on nothing but Assert and Config.
C:\Projects\boost-git\boost>dist\bin\boostdep.exe throw_exception Primary dependencies for throw_exception:
assert: <boost/current_function.hpp> from <boost/throw_exception.hpp>
config: <boost/config.hpp> from <boost/throw_exception.hpp> <boost/detail/workaround.hpp> from <boost/throw_exception.hpp>
I meant that every library which used BOOST_THROW_EXCEPTION now depends upon boost/throw_exception. Am I wrong about that?
On Wed, Dec 14, 2016 at 10:03 PM, Robert Ramey <ramey@rrsd.com> wrote:
On 12/14/16 5:47 PM, Peter Dimov wrote:
Robert Ramey wrote:
This made many libraries dependent on the new library boost.exception
which had multiple repercussions - one of which is that boost.exception has now become a "core library" - not sure what that is supposed to mean.
No, throw_exception is its own mini-library now. It depends on nothing but Assert and Config.
C:\Projects\boost-git\boost>dist\bin\boostdep.exe throw_exception Primary dependencies for throw_exception:
assert: <boost/current_function.hpp> from <boost/throw_exception.hpp>
config: <boost/config.hpp> from <boost/throw_exception.hpp> <boost/detail/workaround.hpp> from <boost/throw_exception.hpp>
I meant that every library which used BOOST_THROW_EXCEPTION now depends upon boost/throw_exception. Am I wrong about that?
The macro BOOST_THROW_EXCEPTION did not exist prior to Boost Exception. It was introduced so that the __FILE__ and __LINE__ of the throw are captured in the exception object. This new functionality does not depend on the Boost Exception submodule. You should look at how the git submodules are structured: boost/throw_exception.hpp is in its own minimodule which also includes boost/exception/exception.hpp. Emil
Robert Ramey wrote:
I meant that every library which used BOOST_THROW_EXCEPTION now depends upon boost/throw_exception. Am I wrong about that?
Given that BOOST_THROW_EXCEPTION and boost::throw_exception are defined in boost/throw_exception.hpp, yes, obviously. It has never been otherwise.
On Wed, Dec 14, 2016 at 7:53 PM, Robert Ramey <ramey@rrsd.com> wrote:
I think that there has been a lot of confusion about what BOOST_THROW_EXCEPTION is suppose to do.
My understanding was that it was a macro intended to support the writing of portable code that could run on platforms which didn't support exceptions or where the user / author didn't want to use the exception mechanism so he could redefine the macro. Lot's of libraries used this idiom to decouple their libraries from the the selection of exception mechanism.
I admit that my usage of BOOST_THROW_EXCEPTION is fairly recent, but that use case is not even mentioned: http://www.boost.org/doc/libs/1_62_0/libs/exception/doc/BOOST_THROW_EXCEPTIO... What I want from BOOST_THROW_EXCEPTION is to embed the current source file, line number and function name in the thrown exception -- which is the closest I can get to a "stack trace" with current portable facilities.
Now it's being suggested that BOOST_THROW_EXCEPTION be repurposed/enhanced yet again?
I suggested that there is potential for integration between (the proposed) Boost.Stacktrace and Boost.Exception. I'm flexible about exactly what form that integration will take. I rather like Emil's suggestion of embedding the stacktrace from within boost::throw_exception, since it can do that. It need not happen within the BOOST_THROW_EXCEPTION macro.
On 12/14/16 6:06 PM, Nat Goodspeed wrote:
On Wed, Dec 14, 2016 at 7:53 PM, Robert Ramey <ramey@rrsd.com> wrote:
I think that there has been a lot of confusion about what BOOST_THROW_EXCEPTION is suppose to do.
My understanding was that it was a macro intended to support the writing of portable code that could run on platforms which didn't support exceptions or where the user / author didn't want to use the exception mechanism so he could redefine the macro. Lot's of libraries used this idiom to decouple their libraries from the the selection of exception mechanism.
I admit that my usage of BOOST_THROW_EXCEPTION is fairly recent, but that use case is not even mentioned: http://www.boost.org/doc/libs/1_62_0/libs/exception/doc/BOOST_THROW_EXCEPTIO...
Right. Because the semantics of BOOST_THROW_EXCEPTION were changed and the documentation updated to the new semantics. The history of the current version only goes back to 2009 so it's not visible now. Robert Ramey
On Wed, Dec 14, 2016 at 10:14 PM, Robert Ramey <ramey@rrsd.com> wrote:
On 12/14/16 6:06 PM, Nat Goodspeed wrote:
On Wed, Dec 14, 2016 at 7:53 PM, Robert Ramey <ramey@rrsd.com> wrote:
I think that there has been a lot of confusion about what
BOOST_THROW_EXCEPTION is suppose to do.
My understanding was that it was a macro intended to support the writing of portable code that could run on platforms which didn't support exceptions or where the user / author didn't want to use the exception mechanism so he could redefine the macro. Lot's of libraries used this idiom to decouple their libraries from the the selection of exception mechanism.
I admit that my usage of BOOST_THROW_EXCEPTION is fairly recent, but that use case is not even mentioned: http://www.boost.org/doc/libs/1_62_0/libs/exception/doc/BOOS T_THROW_EXCEPTION.html
Right. Because the semantics of BOOST_THROW_EXCEPTION were changed and the documentation updated to the new semantics.
False, the semantics of BOOST_THROW_EXCEPTION have never changed. Emil
On 12/15/16 5:37 AM, Peter Dimov wrote:
Robert Ramey wrote:
Because the semantics of BOOST_THROW_EXCEPTION were changed and the documentation updated to the new semantics.
BOOST_THROW_EXCEPTION did not exist before Boost.Exception, so no, it wasn't changed.
Sorry, my mistake. It wasn't BOOST_THROW_EXCEPTION. It was boost::throw_exception whose semantics were changed in an unanticipated and surprising way. It seems to me that it might be that this was being proposed again. Maybe my concerns are overblown as those who got burned the first time by this policy have already eliminated dependency on boost::throw_exception. Robert Ramey
Robert Ramey wrote:
It wasn't BOOST_THROW_EXCEPTION. It was boost::throw_exception whose semantics were changed in an unanticipated and surprising way. It seems to me that it might be that this was being proposed again. Maybe my concerns are overblown as those who got burned the first time by this policy have already eliminated dependency on boost::throw_exception.
The whole point of using a central infrastructure for throwing exceptions (and asserts) is for us to be able to (carefully and after due deliberation, and in the interest of our users) apply changes to it which magically affect all Boost libraries without their maintainers having to do anything. Last time the effect of the changes has been to make all Boost-emitted exceptions Boost.Exception enabled and, if the macro is used, to contain the file/line/function. This has a number of benefits and the downsides have been kept to a minimum. It's normal for changes to the default behavior of such a critical piece to be controversial and we won't make them without a consensus. Which is why I originally said
This sounds like a pretty good idea, although I'm not sure if we can enable it by default as people have historically been very sensitive to changes to BOOST_THROW_EXCEPTION. But an opt-in macro should be fine and very useful as one would automatically get stack traces from every use of BOOST_THROW_EXCEPTION.
On 12/15/16 9:37 AM, Peter Dimov wrote:
Robert Ramey wrote:
It wasn't BOOST_THROW_EXCEPTION. It was boost::throw_exception whose semantics were changed in an unanticipated and surprising way. It seems to me that it might be that this was being proposed again. Maybe my concerns are overblown as those who got burned the first time by this policy have already eliminated dependency on boost::throw_exception.
The whole point of using a central infrastructure for throwing exceptions (and asserts) is for us to be able to (carefully and after due deliberation, and in the interest of our users) apply changes to it which magically affect all Boost libraries without their maintainers having to do anything.
I understand the motivation for this. Still it's a very bad idea. It changes the behavior of code in a silent, undocumented and sometimes surprising way. It couples one library to another in a way which can make the library inconvenient to use with other libraries.
Last time the effect of the changes has been to make all Boost-emitted exceptions Boost.Exception enabled and, if the macro is used, to contain the file/line/function. This has a number of benefits and the downsides have been kept to a minimum.
Again - a matter of opinion. A typical program these days will use libraries from several sources as well as internal code. It's going to be difficult to make an exception design/policy for one's application when it is set from within the library to a module whose semantics can and have changed without notice.
It's normal for changes to the default behavior of such a critical piece to be controversial and we won't make them without a consensus. Which is why I originally said
This sounds like a pretty good idea, although I'm not sure if we can enable it by default as people have historically been very sensitive to changes to BOOST_THROW_EXCEPTION. But an opt-in macro should be fine and very useful as one would automatically get stack traces from every use of BOOST_THROW_EXCEPTION.
LOL - well, I'm just illustrating your point here. It's really a much deeper point here. What is a boost library? Is it necessary part of something large - the "Boost Libraries" or does it stand on it's own - the boost serialization library. The former view is natural to developers who think in terms of "releasing" a humongous product. Its natural to want to couple things like exception policy, testing, build, etc. etc. as a more or less monolithic system. It "seems" to make things easier. But it doesn't really - not in our context. It engenders all the disputes like we constantly have about testing, deployment, documentation, build systems etc. This is one thing that the standard C++ library does better. There is less coupling between individual components. (Of course the standard library has it's own issues but we're not touching on them here.) Getting back to the main point, the boost policy for exception handling has been short sighted and the manner that it was imposed - by code ambush - not appreciated by some of us. I don't want to dwell on it since I lost that war a long time ago and I've tried to moved on. Robert Ramey
Robert Ramey wrote:
It's going to be difficult to make an exception design/policy for one's application when it is set from within the library to a module whose semantics can and have changed without notice.
It's obviously going to be much more difficult to do so if everyone just goes and does his own thing.
On Thu, Dec 15, 2016 at 11:26 AM, Robert Ramey <ramey@rrsd.com> wrote:
On 12/15/16 9:37 AM, Peter Dimov wrote:
Robert Ramey wrote:
It wasn't BOOST_THROW_EXCEPTION. It was boost::throw_exception whose
semantics were changed in an unanticipated and surprising way. It seems to me that it might be that this was being proposed again. Maybe my concerns are overblown as those who got burned the first time by this policy have already eliminated dependency on boost::throw_exception.
The whole point of using a central infrastructure for throwing exceptions (and asserts) is for us to be able to (carefully and after due deliberation, and in the interest of our users) apply changes to it which magically affect all Boost libraries without their maintainers having to do anything.
I understand the motivation for this. Still it's a very bad idea. It changes the behavior of code in a silent, undocumented and sometimes surprising way. It couples one library to another in a way which can make the library inconvenient to use with other libraries.
These are false claims -- the change you're talking about went through the review process and it was immediately reflected in the documentation of boost::throw_exception, even though it didn't break existing code. There was no "coupling" to other libraries but to a single header with zero dependencies, which is now included in the boost::throw_exception submodule. Specifically, if you want to use boost::throw_exception, the Boost Exception submodule is NOT necessary.
It's normal for changes to the default behavior of such a critical piece
to be controversial and we won't make them without a consensus. Which is why I originally said
This sounds like a pretty good idea, although I'm not sure if we can
enable it by default as people have historically been very sensitive to changes to BOOST_THROW_EXCEPTION. But an opt-in macro should be fine and very useful as one would automatically get stack traces from every use of BOOST_THROW_EXCEPTION.
LOL - well, I'm just illustrating your point here.
It's really a much deeper point here. What is a boost library? Is it necessary part of something large - the "Boost Libraries" or does it stand on it's own - the boost serialization library. The former view is natural to developers who think in terms of "releasing" a humongous product. Its natural to want to couple things like exception policy, testing, build, etc. etc. as a more or less monolithic system. It "seems" to make things easier. But it doesn't really - not in our context.
I agree 200% with your concern about coupling, so let's discuss that, rather than throwing around unsubstantiated claims. Any additions to boost::throw_exception should be self-contained. Even basic Boost components like function or shared_ptr should be off limits. The boost::exception type (which, again, does not depend on anything, boost/exception/exception.hpp does not include any headers) currently holds an int and a char const * for BOOST_THROW_EXCEPTION to store __LINE__ and __FILE__. The cost of adding the ability of boost::exception to carry a stack trace would be one more pointer (note: for this purpose it's possible to use Boost Exception's ability to transport arbitrary data in any exception, but that would make boost::throw_exception dependent on Boost Exception, which I've worked very hard to avoid so far.) That pointer can remain unused if the user chooses to turn off the automatic capture of the stack trace. Emil
On 12/15/16 12:21 PM, Emil Dotchevski wrote:
On Thu, Dec 15, 2016 at 11:26 AM, Robert Ramey <ramey@rrsd.com> wrote:
On 12/15/16 9:37 AM, Peter Dimov wrote:
Robert Ramey wrote:
It wasn't BOOST_THROW_EXCEPTION. It was boost::throw_exception whose
semantics were changed in an unanticipated and surprising way. It seems to me that it might be that this was being proposed again. Maybe my concerns are overblown as those who got burned the first time by this policy have already eliminated dependency on boost::throw_exception.
The whole point of using a central infrastructure for throwing exceptions (and asserts) is for us to be able to (carefully and after due deliberation, and in the interest of our users) apply changes to it which magically affect all Boost libraries without their maintainers having to do anything.
I understand the motivation for this. Still it's a very bad idea. It changes the behavior of code in a silent, undocumented and sometimes surprising way. It couples one library to another in a way which can make the library inconvenient to use with other libraries.
These are false claims -- the change you're talking about went through the review process and it was immediately reflected in the documentation of boost::throw_exception,
Hmmm - When this occurred I wondered how such a thing could get through the review process as I was totally surprised by it. My recollection was that when I looked back at the mailing list, it only received a very small number of reviews - like 2 or 3/. Unfortunately, the announcement of acceptance didn't include a summary of how many reviews were received and I can't find the mailing list entries to verify this. even though it didn't break existing code. It added a ton of included code to all my header files. I think the number was 5000 lines. I'm sure it did something - otherwise I wouldn't have noticed it.
There was no "coupling" to other libraries but to a single header with zero dependencies, which is now included in the boost::throw_exception submodule.
that's what coupling is. Specifically, if you want to use boost::throw_exception, the
Boost Exception submodule is NOT necessary.
well, I was using boost::throw_exception and I was got a whole bunch of new stuff I didn't ask for. It seems that it's in there to support other users who do use boost exception. Which raises the question of why that is my problem. After not getting any traction I did post a patch which would permit support boost exception without having to make any changes in boost::throw_exception. It turned out I had accidentally "re-invented" nested exceptions which were in the standard library. This would have been a good solution but it was never incorporated.
It's really a much deeper point here. What is a boost library? Is it necessary part of something large - the "Boost Libraries" or does it stand on it's own - the boost serialization library. The former view is natural to developers who think in terms of "releasing" a humongous product. Its natural to want to couple things like exception policy, testing, build, etc. etc. as a more or less monolithic system. It "seems" to make things easier. But it doesn't really - not in our context.
I agree 200% with your concern about coupling, so let's discuss that, rather than throwing around unsubstantiated claims.
Any additions to boost::throw_exception should be self-contained. Even basic Boost components like function or shared_ptr should be off limits.
The boost::exception type (which, again, does not depend on anything, boost/exception/exception.hpp does not include any headers) currently holds an int and a char const * for BOOST_THROW_EXCEPTION to store __LINE__ and __FILE__. The cost of adding the ability of boost::exception to carry a stack trace would be one more pointer (note: for this purpose it's possible to use Boost Exception's ability to transport arbitrary data in any exception, but that would make boost::throw_exception dependent on Boost Exception, which I've worked very hard to avoid so far.) That pointer can remain unused if the user chooses to turn off the automatic capture of the stack trace.
OK - I looked at boost/exception/exception.hpp and it only adds 500 lines of header code to every file which invokes boost::throw_exception. A big improvement over the original. But my point is really that adding something like this in jamming into my library makes me responsible for something that I have to invest significant time to understand. It increases the scope of my task and provides me with no benefit. I don't think you have the right to make a decision to do this - especially without previous discussion. We went through this the last time though. As a practical matter, it's not really an issue any more. Robert Ramey
On Thu, Dec 15, 2016 at 1:24 PM, Robert Ramey <ramey@rrsd.com> wrote:
It added a ton of included code to all my header files. I think the number was 5000 lines. I'm sure it did something - otherwise I wouldn't have noticed it.
False, it's 522 lines: https://github.com/boostorg/throw_exception/blob/develop/include/boost/excep... .
There was no "coupling" to other libraries but to a single header with zero
dependencies, which is now included in the boost::throw_exception submodule.
that's what coupling is.
Yes, boost/throw_exception.hpp is coupled with boost/exception/exception.hpp (would you be happier if I copy the contents of exception.hpp and paste it in throw_exception.hpp?) No, the throw_exception module is not coupled with the exception module, because boost/exception/exception.hpp resides in the throw_exception module and not in the Boost Exception module: throw_exception: https://github.com/boostorg/throw_exception. Boost Exception: https://github.com/boostorg/exception
Specifically, if you want to use boost::throw_exception, the
Boost Exception submodule is NOT necessary.
well, I was using boost::throw_exception and I was got a whole bunch of new stuff I didn't ask for.
Let's be specific: you got a base class for your exception types, for free, which admittedly you didn't ask for.
It seems that it's in there to support other users who do use boost exception. Which raises the question of why that is my problem. After not getting any traction I did post a patch which would permit support boost exception without having to make any changes in boost::throw_exception.
I'm extremely interested in this, I do want to implement Boost Exception non-intrusively if possible. Can I see code?
OK - I looked at boost/exception/exception.hpp and it only adds 500 lines of header code to every file which invokes boost::throw_exception. A big improvement over the original.
The original was ~400 lines.
But my point is really that adding something like this in jamming into my library makes me responsible for something that I have to invest significant time to understand.
Your choice. Back then you overreacted, made changes to Serialization to not call throw_exception, which broke BOOST_NO_EXCEPTIONS builds for users of Serialization (for which you blamed me). And then, yes, to fix that mess you had to invest time to understand how throw_exception works.
As a practical matter, it's not really an issue any more.
If you don't want to talk about this issue any more, stop making false claims. Emil
On 12/15/16 3:33 PM, Emil Dotchevski wrote:
It seems that it's in there to support other users who do use boost exception. Which raises the question of why that is my problem. After not getting any traction I did post a patch which would permit support boost exception without having to make any changes in boost::throw_exception.
I'm extremely interested in this, I do want to implement Boost Exception non-intrusively if possible. Can I see code?
This was discussed in a thread http://boost.2283326.n4.nabble.com/Boost-and-exceptions-td4631441i140.html#a...
On Thu, Dec 15, 2016 at 4:53 PM, Robert Ramey <ramey@rrsd.com> wrote:
On 12/15/16 3:33 PM, Emil Dotchevski wrote:
It seems that it's in there to support other users who do use boost
exception. Which raises the question of why that is my problem. After not getting any traction I did post a patch which would permit support boost exception without having to make any changes in boost::throw_exception.
I'm extremely interested in this, I do want to implement Boost Exception non-intrusively if possible. Can I see code?
This was discussed in a thread
http://boost.2283326.n4.nabble.com/Boost-and-exceptions- td4631441i140.html#a4631898
As I pointed out in my response there, this is not equivalent to what Boost Exception does. You are catching one exception object and throwing another, specifically you catch(...) and then throw a new object of the unrelated type exception_wrapper<>. To emulate the semantics of Boost Exception non-intrusively, you have to be able to do something like this: catch(...) { //somehow access the current exception object and associate arbitrary data with it throw; //re-throw the original exception object, not a new one. } With Boost Exception this works like so: catch( boost::exception & e ) { e << my_error_info(....); throw; } The problem is that this only works for exceptions of types that derive from boost::exception. The boost::throw_exception class template does NOTHING more than inject boost::exception as a base to the type used to instantiate it, and only if it isn't a base already. Emil
On Thu, Dec 15, 2016 at 11:21 PM, Emil Dotchevski <emildotchevski@gmail.com> wrote:
On Thu, Dec 15, 2016 at 11:26 AM, Robert Ramey <ramey@rrsd.com> wrote:
The boost::exception type (which, again, does not depend on anything, boost/exception/exception.hpp does not include any headers) currently holds an int and a char const * for BOOST_THROW_EXCEPTION to store __LINE__ and __FILE__. The cost of adding the ability of boost::exception to carry a stack trace would be one more pointer (note: for this purpose it's possible to use Boost Exception's ability to transport arbitrary data in any exception, but that would make boost::throw_exception dependent on Boost Exception, which I've worked very hard to avoid so far.) That pointer can remain unused if the user chooses to turn off the automatic capture of the stack trace.
The problem is not the pointer per se. The problem is that your proposal implies a dependency on the code that fills that pointer, the Stacktrace library. Robert expands on the dependency injection and I'm worried by performance degradation caused by this change. I must say I'm not really buying the "automatic stacktrace from exception" advantage. Really, the source file and line, combined with sufficient logging, were quite enough for me for years; if there were cases I wished to have a backtrace, they were rare enough I don't remember one of them. So please, just define a separate macro with this new feature and leave BOOST_THROW_EXCEPTION as is. The config macro to disable the stacktrace doesn't satisfy me. First, because it is a way to opt out whereas I believe such a feature should be an opt in as it is expensive. Second, because it doesn't work with compiled binaries (e.g. distributed in package distros and Boost installers). Third, I suspect this feature could be useful in a subset of source code, e.g. a particular set of function calls or a scope, while not so useful in other places of the application. For instance, one might want to temporarily enable it to debug a particular place of code without affecting the rest of the program. For that the feature should be configurable in runtime. What I'm thinking is that there could be a hook that would allow a user-defined function to be called on the exception that is about to be thrown. By default no hook is installed, so throwing an exception works exactly as BOOST_THROW_EXCEPTION currently works. Boost.Stacktrace could provide its hook, which the user could install to enable automatic stacktraces. The important part is that this is user's choice and that it decouples throwing the exception from Boost.Stacktrace.
On 16/12/2016 12:11, Andrey Semashev wrote:
The config macro to disable the stacktrace doesn't satisfy me. First, because it is a way to opt out whereas I believe such a feature should be an opt in as it is expensive. Second, because it doesn't work with compiled binaries (e.g. distributed in package distros and Boost installers). Third, I suspect this feature could be useful in a subset of source code, e.g. a particular set of function calls or a scope, while not so useful in other places of the application. For instance, one might want to temporarily enable it to debug a particular place of code without affecting the rest of the program. For that the feature should be configurable in runtime.
I'm curious whether your answer would change if your first two objections were invalid, ie. if it were not expensive and if it could be used for optimised and stripped binaries. (Regarding that latter point, that's what both Nat and I [in the pre-review] were talking about as a feature request, that the raw trace could be serialised and decoded later on a separate machine with access to the corresponding symbol files. This practice is not uncommon in Windows with minidumps and pdb files.)
On Thu, Dec 15, 2016 at 3:45 PM, Gavin Lambert <gavinl@compacsort.com> wrote:
On 16/12/2016 12:11, Andrey Semashev wrote:
The config macro to disable the stacktrace doesn't satisfy me. First, because it is a way to opt out whereas I believe such a feature should be an opt in as it is expensive. Second, because it doesn't work with compiled binaries (e.g. distributed in package distros and Boost installers). Third, I suspect this feature could be useful in a subset of source code, e.g. a particular set of function calls or a scope, while not so useful in other places of the application. For instance, one might want to temporarily enable it to debug a particular place of code without affecting the rest of the program. For that the feature should be configurable in runtime.
I'm curious whether your answer would change if your first two objections were invalid, ie. if it were not expensive and if it could be used for optimised and stripped binaries.
(Regarding that latter point, that's what both Nat and I [in the pre-review] were talking about as a feature request, that the raw trace could be serialised and decoded later on a separate machine with access to the corresponding symbol files. This practice is not uncommon in Windows with minidumps and pdb files.)
+1 Emil
On 12/16/16 02:45, Gavin Lambert wrote:
On 16/12/2016 12:11, Andrey Semashev wrote:
The config macro to disable the stacktrace doesn't satisfy me. First, because it is a way to opt out whereas I believe such a feature should be an opt in as it is expensive. Second, because it doesn't work with compiled binaries (e.g. distributed in package distros and Boost installers). Third, I suspect this feature could be useful in a subset of source code, e.g. a particular set of function calls or a scope, while not so useful in other places of the application. For instance, one might want to temporarily enable it to debug a particular place of code without affecting the rest of the program. For that the feature should be configurable in runtime.
I'm curious whether your answer would change if your first two objections were invalid, ie. if it were not expensive and if it could be used for optimised and stripped binaries.
If obtaining the backtrace was cheap and of constant complexity, that would certainly be less of a problem. The source file location is a great example of that: it is cheap to obtain and transport in the exception, yet it provides one with a lot of information. But with stacktraces somehow I don't believe this is the case (wrt. the "cheap" part anyway). Regarding the optimized and stripped binaries, I was actually talking about a different thing, I think. For instance, if BOOST_THROW_EXCEPTION attaches a stacktrace by default then Boost.Log library binaries distributed in my favorite Linux packages also do that for all exceptions the library throws. There is no way the user can disable that without recompiling the library. But you touched another interesing subject: does the library have the capability of loading debug symbols to interpret the backtrace? Because if it doesn't then the attached stacktraces will be nearly useless for stripped binaries (which they all are when distributed in packages).
(Regarding that latter point, that's what both Nat and I [in the pre-review] were talking about as a feature request, that the raw trace could be serialised and decoded later on a separate machine with access to the corresponding symbol files. This practice is not uncommon in Windows with minidumps and pdb files.)
I think, with load address randomization, the stacktrace alone is not enough for that. You'd need a core dump of the process to interpret the backtrace.
On 16 Dec 2016 at 12:37, Andrey Semashev wrote:
But you touched another interesing subject: does the library have the capability of loading debug symbols to interpret the backtrace? Because if it doesn't then the attached stacktraces will be nearly useless for stripped binaries (which they all are when distributed in packages).
Are you saying this information is not in Stacktrace's documentation? Or it is insufficiently clear?
(Regarding that latter point, that's what both Nat and I [in the pre-review] were talking about as a feature request, that the raw trace could be serialised and decoded later on a separate machine with access to the corresponding symbol files. This practice is not uncommon in Windows with minidumps and pdb files.)
I think, with load address randomization, the stacktrace alone is not enough for that. You'd need a core dump of the process to interpret the backtrace.
No, you just need the offset into each module. I did suggest to Antony in my pre-manager review that he needed a serialisation mechanism which dumps out module paths plus offsets in a format consumable by tooling such as addr2line. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 12/16/16 22:05, Niall Douglas wrote:
On 16 Dec 2016 at 12:37, Andrey Semashev wrote:
But you touched another interesing subject: does the library have the capability of loading debug symbols to interpret the backtrace? Because if it doesn't then the attached stacktraces will be nearly useless for stripped binaries (which they all are when distributed in packages).
Are you saying this information is not in Stacktrace's documentation? Or it is insufficiently clear?
I have not read the docs yet.
On Thu, Dec 15, 2016 at 3:11 PM, Andrey Semashev <andrey.semashev@gmail.com> wrote:
On Thu, Dec 15, 2016 at 11:21 PM, Emil Dotchevski <emildotchevski@gmail.com> wrote:
On Thu, Dec 15, 2016 at 11:26 AM, Robert Ramey <ramey@rrsd.com> wrote:
The boost::exception type (which, again, does not depend on anything, boost/exception/exception.hpp does not include any headers) currently holds an int and a char const * for BOOST_THROW_EXCEPTION to store __LINE__ and __FILE__. The cost of adding the ability of boost::exception to carry a stack trace would be one more pointer (note: for this purpose it's possible to use Boost Exception's ability to transport arbitrary data in any exception, but that would make boost::throw_exception dependent on Boost Exception, which I've worked very hard to avoid so far.) That pointer can remain unused if the user chooses to turn off the automatic capture of the stack trace.
The problem is not the pointer per se. The problem is that your proposal implies a dependency on the code that fills that pointer, the Stacktrace library.
I am not proposing a dependency of throw_exception on Starktrace, but removing a minimal amount of code from Stacktrace (e.g. something that packs everything into a memory buffer) and putting it into throw_exception. I do think that logically, this code should be left in a separate header, but it has to be written without any Boost dependencies. If that is not possible, I agree that the integration is a bad idea.
Robert expands on the dependency injection and I'm worried by performance degradation caused by this change.
I admit I worry about coupling more than I do about the performance. I am speculating, but I suspect that in my own uses emitting exceptions has to be something like 1000x slower than it is to maybe show in my profiling. Perhaps I'm wrong. I must say I'm not really buying the "automatic stacktrace from
exception" advantage. Really, the source file and line, combined with sufficient logging, were quite enough for me for years; if there were cases I wished to have a backtrace, they were rare enough I don't remember one of them.
So please, just define a separate macro with this new feature and leave BOOST_THROW_EXCEPTION as is.
First, let's again clarify: there is no reason to change the BOOST_THROW_EXCEPTION macro. The stack trace can be captured by the boost::throw_exception function template. The problem is that users who want take advantage of the stack trace (I understand you are not one of them) have no control over the library level code that uses boost::throw_exception. That said, if you agree that ultimately it is the user who should be able to decide whether or not "good" Boost libraries embed stack trace in exceptions they emit, the argument wouldn't be whether or not capturing the stack trace is integrated into boost::throw_exception, but whether or not it is enabled by default. Either way, the boost::exception class (defined in boost/exception/exception.hpp) should provide a hook for that integration in order to avoid coupling between boost::throw_exception and Stacktrace.
The config macro to disable the stacktrace doesn't satisfy me. First, because it is a way to opt out whereas I believe such a feature should be an opt in as it is expensive.
I think that how expensive it is needs to be investigated and discussed. Either way it is a compromise.
What I'm thinking is that there could be a hook that would allow a user-defined function to be called on the exception that is about to be thrown. By default no hook is installed, so throwing an exception works exactly as BOOST_THROW_EXCEPTION currently works. Boost.Stacktrace could provide its hook, which the user could install to enable automatic stacktraces. The important part is that this is user's choice and that it decouples throwing the exception from Boost.Stacktrace.
Do you mean a run-time hook or a compile-time hook? I think this should be compile-time hook in order to avoid the perils of dynamic initialization in complex multi-thread programs. But we already have that compile-time hook, it's called boost::throw_exception. Emil
On 12/16/16 03:00, Emil Dotchevski wrote:
I am not proposing a dependency of throw_exception on Starktrace, but removing a minimal amount of code from Stacktrace (e.g. something that packs everything into a memory buffer) and putting it into throw_exception. I do think that logically, this code should be left in a separate header, but it has to be written without any Boost dependencies. If that is not possible, I agree that the integration is a bad idea.
I'm not sure what else Boost.Stacktrace does; it looks like it means moving the bulk of Stacktrace into the throw_exception submodule. I don't think that is a reasonable solution in terms of code management. Also, I'm not sure what APIs Stacktrace uses behind the scene, does it depend on additional third party libraries to obtain the backtrace?
I must say I'm not really buying the "automatic stacktrace from exception" advantage. Really, the source file and line, combined with sufficient logging, were quite enough for me for years; if there were cases I wished to have a backtrace, they were rare enough I don't remember one of them.
So please, just define a separate macro with this new feature and leave BOOST_THROW_EXCEPTION as is.
First, let's again clarify: there is no reason to change the BOOST_THROW_EXCEPTION macro. The stack trace can be captured by the boost::throw_exception function template.
BOOST_THROW_EXCEPTION calls boost::throw_exception, so by changing the latter you also change the former.
The problem is that users who want take advantage of the stack trace (I understand you are not one of them) have no control over the library level code that uses boost::throw_exception.
True.
That said, if you agree that ultimately it is the user who should be able to decide whether or not "good" Boost libraries embed stack trace in exceptions they emit,
Yes, it should be primarilly the user's decision. Although I can see library developers also wishing to avoid this feature, at least selectively. That is why I propose a new macro for this.
the argument wouldn't be whether or not capturing the stack trace is integrated into boost::throw_exception, but whether or not it is enabled by default. Either way, the boost::exception class (defined in boost/exception/exception.hpp) should provide a hook for that integration in order to avoid coupling between boost::throw_exception and Stacktrace.
The way I see it, boost::exception already provides everything needed, which is the ability to attach arbitrary data to the exception. I don't think adding more special case capability to it is a wise solution in the long term.
The config macro to disable the stacktrace doesn't satisfy me. First, because it is a way to opt out whereas I believe such a feature should be an opt in as it is expensive.
I think that how expensive it is needs to be investigated and discussed. Either way it is a compromise.
Yes, I think some performance numbers should be presented before making the change to throw_exception. Also, if we go with the run-time hook approach (with no hook by default), the performance issue won't be so pressing.
What I'm thinking is that there could be a hook that would allow a user-defined function to be called on the exception that is about to be thrown. By default no hook is installed, so throwing an exception works exactly as BOOST_THROW_EXCEPTION currently works. Boost.Stacktrace could provide its hook, which the user could install to enable automatic stacktraces. The important part is that this is user's choice and that it decouples throwing the exception from Boost.Stacktrace.
Do you mean a run-time hook or a compile-time hook?
A run-time hook. I described why a compile-time hook doesn't work for me (the config macro is equivalent to a compile-time hook with regard to the points I made previously).
I think this should be compile-time hook in order to avoid the perils of dynamic initialization in complex multi-thread programs. But we already have that compile-time hook, it's called boost::throw_exception.
The run-time hook can be tricky to implement, that is true, but in the simplest form it should be doable. The implementation details are of course discussable, but below is something more concrete to get things started. Introduce a process-wide singleton pointer, which is a pointer to the function that will augment the exception about to be thrown: typedef void (*exception_augmenter)(boost::exception&); The augmenter will be able to use Boost.Exception API to add any data to the exception. The pointer can be updated atomically. If it deems necessary, a thread-local and module-local copy can be maintained. The default value of null means no augmenter and throwing the exception works as BOOST_THROW_EXCEPTION currently does. The tricky part is to maintain the singleton. On Windows one can use a named semaphore to store the pointer as the semaphore state (see https://github.com/boostorg/sync/blob/develop/include/boost/sync/detail/wait... for an example, Boost.Interprocess also has something similar). On POSIX systems it might be enough to just mark the pointer with default visibility. A solution with shared memory (shmget/shmat/etc.) also seems possible. The user will be able to install his (or Boost.Stacktrace's) augmenter through the API provided by throw_exception submodule and thus enable stacktraces in exceptions.
On 12/16/16 1:20 AM, Andrey Semashev wrote:
I think this should be compile-time hook in order to avoid the perils of dynamic initialization in complex multi-thread programs. But we already have that compile-time hook, it's called boost::throw_exception.
The run-time hook can be tricky to implement, that is true, but in the simplest form it should be doable. The implementation details are of course discussable, but below is something more concrete to get things started.
Introduce a process-wide singleton pointer,
<snip> This illustrates what the issue is and where we need to be moving. For many C++ programs and libraries, the exception is a convenient way to handle errors and for this reason is widely used. Traditionally, library authors have documented exceptions that they threw and let user programs decide to catch and handle them. To deal with platforms which didn't provide exceptions we threw through boost::throw_exception. So far so good. But as we make bigger and more complex programs using mulitple libraries from different sources, this becomes somewhat of a problem to keep track of. And users would like to have systematic way of "unifying" them. So I'm thinking the real solution is to develop customs and idioms for usage by library authors which would permit users to specify the way they would like to see exceptions handled. For example, the boost serialization library uses the traditional method of defining exceptions and calling boost::throw_exception. But this doesn't let users control behavior. My more recent effort - safe_numerics, (in the boost library incubator) specifies a template parameter which refers to an error policy and includes several options - invoke exception, invoke specified function, etc... Something along these lines is what I'm now thinking is where we should be headed. When I did it for safe_numerics, I wasn't thinking of any general solution or approach to usage of exceptions inside of library code, it's just came naturally from the fact that pretty much the whole safe_numerics is to deal with errors with a library. To summarize, what is need is to come up with a "best practices" recommendation to library authors as to how to deal with exceptional conditions detected from withing a library. Were this to occur and be convincing to library authors, the whole question of what boost::throw_exception should do wouldn't have to be agreed upon as it would be in the hands of the library user to decide. Robert Ramey
-----Original Message----- From: Boost [mailto:boost-bounces@lists.boost.org] On Behalf Of Robert Ramey Sent: 16 December 2016 16:47 To: boost@lists.boost.org Subject: Re: [boost] [Boost] [Stacktrace] review
On 12/16/16 1:20 AM, Andrey Semashev wrote:
I think this should be compile-time hook in order to avoid the perils of dynamic initialization in complex multi-thread programs. But we already have that compile-time hook, it's called boost::throw_exception.
The run-time hook can be tricky to implement, that is true, but in the simplest form it should be doable. The implementation details are of course discussable, but below is something more concrete to get things started.
Introduce a process-wide singleton pointer,
<snip>
This illustrates what the issue is and where we need to be moving.
For many C++ programs and libraries, the exception is a convenient way to handle errors and for this reason is widely used. Traditionally, library authors have documented exceptions that they threw and let user programs decide to catch and handle them. To deal with platforms which didn't provide exceptions we threw through boost::throw_exception. So far so good. But as we make bigger and more complex programs using mulitple libraries from different sources, this becomes somewhat of a problem to keep track of. And users would like to have systematic way of "unifying" them. So I'm thinking the real solution is to develop customs and idioms for usage by library authors which would permit users to specify the way they would like to see exceptions handled.
For example, the boost serialization library uses the traditional method of defining exceptions and calling boost::throw_exception. But this doesn't let users control behavior. My more recent effort - safe_numerics, (in the boost library incubator) specifies a template parameter which refers to an error policy and includes several options - invoke exception, invoke specified function, etc... Something along these lines is what I'm now thinking is where we should be headed. When I did it for safe_numerics, I wasn't thinking of any general solution or approach to usage of exceptions inside of library code, it's just came naturally from the fact that pretty much the whole safe_numerics is to deal with errors with a library.
To summarize, what is need is to come up with a "best practices" recommendation to library authors as to how to deal with exceptional conditions detected from withing a library.
Were this to occur and be convincing to library authors, the whole question of what boost::throw_exception should do wouldn't have to be agreed upon as it would be in the hands of the library user to decide.
John Maddock gave serious thought to this when devising the Policies concept for Boost.Math (after some mutterings from me). This allows people to be all C-ish and ignore just bad input thru to throwing C++ exceptions, and control other things like accuracy and precision too. It seems to work well (except that many people need reminding that if you don't catch exceptions, you don't get any info about when went wrong - it just ends with a muted phut :-). Downside is that it makes the code harder to write and much more confusing to read. This too might be a model for other libraries. Paul --- Paul A. Bristow Prizet Farmhouse Kendal UK LA8 8AB +44 (0) 1539 561830
On 16 Dec 2016 at 8:46, Robert Ramey wrote:
This illustrates what the issue is and where we need to be moving.
For many C++ programs and libraries, the exception is a convenient way to handle errors and for this reason is widely used. Traditionally, library authors have documented exceptions that they threw and let user programs decide to catch and handle them. To deal with platforms which didn't provide exceptions we threw through boost::throw_exception. So far so good. But as we make bigger and more complex programs using mulitple libraries from different sources, this becomes somewhat of a problem to keep track of. And users would like to have systematic way of "unifying" them. So I'm thinking the real solution is to develop customs and idioms for usage by library authors which would permit users to specify the way they would like to see exceptions handled.
Isn't that the whole point of proposed Boost.Outcome? I appreciate the explanation was a very long and tedious document as posted here some weeks ago, but once my paternity leave ends I'll be back onto investing an hour per morning until the docs look like the feedback from here requested. (and just for reference, Outcome provides an optional method of hooking when an errored state is created. One hook provided generates a stack backtrace embedded with the errored state) Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On Fri, Dec 16, 2016 at 8:46 AM, Robert Ramey <ramey@rrsd.com> wrote:
For many C++ programs and libraries, the exception is a convenient way to handle errors and for this reason is widely used. Traditionally, library authors have documented exceptions that they threw and let user programs decide to catch and handle them. To deal with platforms which didn't provide exceptions we threw through boost::throw_exception. So far so good. But as we make bigger and more complex programs using mulitple libraries from different sources, this becomes somewhat of a problem to keep track of. And users would like to have systematic way of "unifying" them. So I'm thinking the real solution is to develop customs and idioms for usage by library authors which would permit users to specify the way they would like to see exceptions handled.
This is missing the point of using exceptions. Beyond a mechanism for reporting errors, exceptions enforce postconditions: a function will either succeed or it will not return. Specifically, note that even under BOOST_NO_EXCEPTIONS, boost::throw_exception is NOT allowed to return, or else it would be useless to library developers (because they couldn't use it to enforce postconditions.) There might be exceptions to this, but whether a function throws or not should not be configurable by the user. Emil
On 12/16/16 12:48 PM, Emil Dotchevski wrote:
On Fri, Dec 16, 2016 at 8:46 AM, Robert Ramey <ramey@rrsd.com> wrote:
For many C++ programs and libraries, the exception is a convenient way to handle errors and for this reason is widely used. Traditionally, library authors have documented exceptions that they threw and let user programs decide to catch and handle them. To deal with platforms which didn't provide exceptions we threw through boost::throw_exception. So far so good. But as we make bigger and more complex programs using mulitple libraries from different sources, this becomes somewhat of a problem to keep track of. And users would like to have systematic way of "unifying" them. So I'm thinking the real solution is to develop customs and idioms for usage by library authors which would permit users to specify the way they would like to see exceptions handled.
This is missing the point of using exceptions. Beyond a mechanism for reporting errors, exceptions enforce postconditions:
failing a post condition would be an error
a function will either succeed or it will not return.
not necessarily bool f() { if ... invoke_error return failure or ignore error ... return success } if invoke error is mapped to throw exception then it will never return. If it's mapped to something else - like emitting an error message or invoking a user specified call back then it won't throw an exception. Specifically, note that even under
BOOST_NO_EXCEPTIONS, boost::throw_exception is NOT allowed to return, or else it would be useless to library developers Right - it's already useless to library developers
(because they couldn't use it to enforce postconditions.)
There might be exceptions to this, but whether a function throws or not should not be configurable by the user.
In general, a library writer is not the best person to decide how to handle various exception conditions. In many simple cases it doesn't matter. In others it's obvious that there is no way to make one size fits all. Refer to previously noted examples Reflecting on this, I think that this difference of point of view is source of our difference of opinion. I don't believe that I can really anticipate what the use is going to want to do when a exception condition occurs. This leads to trying to guess. Which in turn requires the user to work his own code library or application to accommodate his code to the decision I, as a library writer, I've made on his behalf. Robert Ramey
On Sat, Dec 17, 2016 at 12:54 AM, Robert Ramey <ramey@rrsd.com> wrote:
On 12/16/16 12:48 PM, Emil Dotchevski wrote:
a function will either succeed or it will not return.
not necessarily
bool f() { if ... invoke_error return failure or ignore error ... return success }
if invoke error is mapped to throw exception then it will never return. If it's mapped to something else - like emitting an error message or invoking a user specified call back then it won't throw an exception.
That results in a really horrible API with dual error reporting mechanisms.
On Fri, Dec 16, 2016 at 2:05 PM, Andrey Semashev <andrey.semashev@gmail.com> wrote:
On Sat, Dec 17, 2016 at 12:54 AM, Robert Ramey <ramey@rrsd.com> wrote:
On 12/16/16 12:48 PM, Emil Dotchevski wrote:
a function will either succeed or it will not return.
not necessarily
bool f() { if ... invoke_error return failure or ignore error ... return success }
if invoke error is mapped to throw exception then it will never return. If it's mapped to something else - like emitting an error message or invoking a user specified call back then it won't throw an exception.
That results in a really horrible API with dual error reporting mechanisms.
Indeed. Again, enforcing postconditions is the main benefit of throwing. Specifically: if( condition-that-code-below-cant-deal-with ) boost::throw_exception(my_error()); //safe to assume no error occurred because boost::throw_exception does nor return. The call to throw_exception is not merely reporting the error but also protecting the scope that follows. Emil
On 12/16/16 2:53 PM, Emil Dotchevski wrote:
On Fri, Dec 16, 2016 at 2:05 PM, Andrey Semashev <andrey.semashev@gmail.com> wrote:
On Sat, Dec 17, 2016 at 12:54 AM, Robert Ramey <ramey@rrsd.com> wrote:
On 12/16/16 12:48 PM, Emil Dotchevski wrote:
a function will either succeed or it will not return.
not necessarily
bool f() { if ... invoke_error return failure or ignore error ... return success }
if invoke error is mapped to throw exception then it will never return. If it's mapped to something else - like emitting an error message or invoking a user specified call back then it won't throw an exception.
That results in a really horrible API with dual error reporting mechanisms.
That's a matter of opinion. It's just an example. There are other ones cited above which are better - but they're part of something a lot more larger and not suitable for pasting here. The real point is that we can't say apriori that that boost::throw_exception should do a certain thing and no other thing.
Indeed. Again, enforcing postconditions is the main benefit of throwing. Specifically:
if( condition-that-code-below-cant-deal-with ) boost::throw_exception(my_error()); //safe to assume no error occurred because boost::throw_exception does nor return.
The call to throw_exception is not merely reporting the error but also protecting the scope that follows.
There are cases when that's not the only choice. A floating point calculation could result in a NaN. In some cases one might want to pass it on and other cases one might want to trap it as an exception. If you don't like this example, it's easy to conjure up other ones. One user might want to invoke save and invoke a stack trace while another doesn't want to do this for dependencies or other reasons. The real point is that from where we work here, we can't know what the library user wants/needs. It's better to factor this out and leave it to him. Robert Ramey
Emil
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On Fri, Dec 16, 2016 at 3:51 PM, Robert Ramey <ramey@rrsd.com> wrote:
On 12/16/16 2:53 PM, Emil Dotchevski wrote:
On Fri, Dec 16, 2016 at 2:05 PM, Andrey Semashev < andrey.semashev@gmail.com> wrote:
On Sat, Dec 17, 2016 at 12:54 AM, Robert Ramey <ramey@rrsd.com> wrote:
On 12/16/16 12:48 PM, Emil Dotchevski wrote:
a function will either succeed or it will not return.
not necessarily
bool f() { if ... invoke_error return failure or ignore error ... return success }
if invoke error is mapped to throw exception then it will never return.
If
it's mapped to something else - like emitting an error message or
invoking a
user specified call back then it won't throw an exception.
That results in a really horrible API with dual error reporting mechanisms.
That's a matter of opinion. It's just an example. There are other ones cited above which are better - but they're part of something a lot more larger and not suitable for pasting here.
The real point is that we can't say apriori that that boost::throw_exception should do a certain thing and no other thing.
What we can say is that no matter what else it does, boost::throw_exception will not return, because that would be a breaking change. It should also be obvious that if I want to throw an exception, the function I call may not return.
Indeed. Again, enforcing postconditions is the main benefit of throwing.
Specifically:
if( condition-that-code-below-cant-deal-with ) boost::throw_exception(my_error()); //safe to assume no error occurred because boost::throw_exception does nor return.
The call to throw_exception is not merely reporting the error but also protecting the scope that follows.
There are cases when that's not the only choice. A floating point calculation could result in a NaN. In some cases one might want to pass it on and other cases one might want to trap it as an exception.
You maybe confusing the terms. There are system exceptions, which you seem to be referring to ("trapping" is part of the terminology used in the floating point IEEE standard), also known as asynchronous exceptions because they may occur at any instruction. This has nothing whatsoever to do with the exception handling mechanism defined by the C++ standard. I am not saying that it is impossible to let the user decide if library functions throw or use some other error reporting mechanism, only that if the user is given that choice, it makes C++ exceptions useless to the library developer, because he must always define behavior for the error branch, like a C programmer must. This is error-prone and can be costly, both in terms of development effort and run-time overhead. Emil
On 12/16/16 4:32 PM, Emil Dotchevski wrote:
I am not saying that it is impossible to let the user decide if library functions throw or use some other error reporting mechanism, only that if the user is given that choice, it makes C++ exceptions useless to the library developer, because he must always define behavior for the error branch, like a C programmer must. This is error-prone and can be costly, both in terms of development effort and run-time overhead.
Emil
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
The long and short of this is: a) boost::throw_exception can do whatever it's developer (you) wants it to. b) stacktrace can do whatever it's developer want's it to. c) Library X developers can either accept these choices or they can permit their users to specify error handling through a parameter. We can't and wouldn't want to inhibit them from making the best choice in the context of the library they are writing. There's no conflict here and nothing has to be agreed upon. Robert Ramey
On 12/16/16 2:05 PM, Andrey Semashev wrote:
On Sat, Dec 17, 2016 at 12:54 AM, Robert Ramey <ramey@rrsd.com> wrote:
On 12/16/16 12:48 PM, Emil Dotchevski wrote:
a function will either succeed or it will not return.
not necessarily
bool f() { if ... invoke_error return failure or ignore error ... return success }
if invoke error is mapped to throw exception then it will never return. If it's mapped to something else - like emitting an error message or invoking a user specified call back then it won't throw an exception.
That results in a really horrible API with dual error reporting mechanisms.
FYI - it's not a dual reporting mechanism. It's leaving the choice of reporting/handling mechanism to the library user. Once he selects it, that one is it. Robert Ramey
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On 12/17/16 02:53, Robert Ramey wrote:
On 12/16/16 2:05 PM, Andrey Semashev wrote:
On Sat, Dec 17, 2016 at 12:54 AM, Robert Ramey <ramey@rrsd.com> wrote:
On 12/16/16 12:48 PM, Emil Dotchevski wrote:
a function will either succeed or it will not return.
not necessarily
bool f() { if ... invoke_error return failure or ignore error ... return success }
if invoke error is mapped to throw exception then it will never return. If it's mapped to something else - like emitting an error message or invoking a user specified call back then it won't throw an exception.
That results in a really horrible API with dual error reporting mechanisms.
FYI - it's not a dual reporting mechanism. It's leaving the choice of reporting/handling mechanism to the library user. Once he selects it, that one is it.
It is a dual reporting mechanism from the standpoint of interface definition. If the function uses exceptions to report errors, I expect it to _not_ use error codes and such. In particular, the return value can be used to return generated data or void. I do not have to examine the returned error code. If the function uses error codes, I expect it to not throw. I can use that function in noexcept contexts and do not have to wrap it in try/catch. If the function definition enables both reporting mechanisms, I have to assume the error can be communicating both ways. Saying it will communicate error one way and not the other in the docs is not sufficient because it leaves room for error (both on the user's side and the function developer's side). The policy has to be expressed on the language level, by the function interface definition, leaving no room for interpretation.
Quick update on the Stacktrace library: * I've fixed a few issues with COM objects, that were highlighted in private emails. Those fixed do not affect library interface or functionality * Docs were slightly updated (better English in Motivation section, added missing name into the Acknowledgements section) If you wish to see the library source codes without those fixes (just as they were at the beginning of the Boost review), you can use the link: https://github.com/apolukhin/stacktrace/tree/boost_review TODOs so far: * At github one of the users was complaining about the performance of ostreaming operations. There is a way to significantly speedup ostreaming without changing the interface of the library. I'll do the optimization after the Boost Review. * Improve docs for the "Build, Macros and Backends" sections (say that library is using system headers, no 3rd party libraries must be installed or manually linked) * Add a note to the docs that library is header only by default and usable in C++98/C++03. -- Best regards, Antony Polukhin
On Fri, Dec 16, 2016 at 1:20 AM, Andrey Semashev <andrey.semashev@gmail.com> wrote:
On 12/16/16 03:00, Emil Dotchevski wrote:
I am not proposing a dependency of throw_exception on Starktrace, but removing a minimal amount of code from Stacktrace (e.g. something that packs everything into a memory buffer) and putting it into throw_exception. I do think that logically, this code should be left in a separate header, but it has to be written without any Boost dependencies. If that is not possible, I agree that the integration is a bad idea.
I'm not sure what else Boost.Stacktrace does; it looks like it means moving the bulk of Stacktrace into the throw_exception submodule. I don't think that is a reasonable solution in terms of code management.
Maybe not. What I'm saying is that 1) automatic capture of stack trace is very useful and 2) any possible integration into boost::throw_exception requires careful consideration. Let's see the actual code before we say it is not acceptable.
Also, I'm not sure what APIs Stacktrace uses behind the scene, does it depend on additional third party libraries to obtain the backtrace?
I am not sure either, I'm assuming it needs only depend on system libraries. Obviously if that's not the case the integration into throw_exception is a no-go.
the argument wouldn't be whether or not capturing the
stack trace is integrated into boost::throw_exception, but whether or not it is enabled by default. Either way, the boost::exception class (defined in boost/exception/exception.hpp) should provide a hook for that integration in order to avoid coupling between boost::throw_exception and Stacktrace.
The way I see it, boost::exception already provides everything needed, which is the ability to attach arbitrary data to the exception. I don't think adding more special case capability to it is a wise solution in the long term.
This can be said about __FILE__ and __LINE__ which BOOST_THROW_EXCEPTION captures automatically. The reason why it's done is that 1) the cost in terms of time and space is considered negligible and 2) it is potentially critical for debugging. The reason why it Stacktrace would need special handling rather than go through the usual Boost Exception machinery is also the same as why there is special handling (members in boost::exception): to avoid coupling with the rest of the Boost Exception machinery, which is considerably heavier and bigger than boost/exception/exception.hpp.
The run-time hook can be tricky to implement, that is true, but in the
simplest form it should be doable. The implementation details are of course discussable, but below is something more concrete to get things started.
Introduce a process-wide singleton pointer, which is a pointer to the function that will augment the exception about to be thrown:
typedef void (*exception_augmenter)(boost::exception&);
Consider dynamically loaded libraries. Also consider multiple libraries wanting different hooks. A process-wide pointer is very difficult to do in the general case, and I suspect it'd drag a lot more dependencies than capturing the stacktrace would. Consider that boost::throw_exception must support very very old compilers.
Besides, throwing exceptions and capturing stack trace are thread-local operations, while global pointer would require synchronization with the initialization code.
The augmenter will be able to use Boost.Exception API to add any data to
the exception.
The pointer can be updated atomically. If it deems necessary, a thread-local and module-local copy can be maintained. The default value of null means no augmenter and throwing the exception works as BOOST_THROW_EXCEPTION currently does.
The tricky part is to maintain the singleton. On Windows one can use a named semaphore to store the pointer as the semaphore state (see https://github.com/boostorg/sync/blob/develop/include/boost/ sync/detail/waitable_timer.hpp#L121 for an example, Boost.Interprocess also has something similar). On POSIX systems it might be enough to just mark the pointer with default visibility. A solution with shared memory (shmget/shmat/etc.) also seems possible.
Are you proposing to include <windows.h> in boost/throw_exception.hpp? That's a no-go. Making it not header-only module? Also a no-go.
Emil
On 12/16/16 22:32, Emil Dotchevski wrote:
On Fri, Dec 16, 2016 at 1:20 AM, Andrey Semashev <andrey.semashev@gmail.com> wrote:
the argument wouldn't be whether or not capturing the
stack trace is integrated into boost::throw_exception, but whether or not it is enabled by default. Either way, the boost::exception class (defined in boost/exception/exception.hpp) should provide a hook for that integration in order to avoid coupling between boost::throw_exception and Stacktrace.
The way I see it, boost::exception already provides everything needed, which is the ability to attach arbitrary data to the exception. I don't think adding more special case capability to it is a wise solution in the long term.
This can be said about __FILE__ and __LINE__ which BOOST_THROW_EXCEPTION captures automatically. The reason why it's done is that 1) the cost in terms of time and space is considered negligible and 2) it is potentially critical for debugging.
The reason why it Stacktrace would need special handling rather than go through the usual Boost Exception machinery is also the same as why there is special handling (members in boost::exception): to avoid coupling with the rest of the Boost Exception machinery, which is considerably heavier and bigger than boost/exception/exception.hpp.
The backtrace itself is not a lightweght object, so the overhead of the generic Boost.Exception machinery is probably not significant. And if that overhead is significant then maybe it could be optimized instead. After all, why have it if we don't want to use it. Also, note that the source location is always injected into the exception (well, as long as you use BOOST_THROW_EXCEPTION or a similar macro), while the backtrace is optional.
The run-time hook can be tricky to implement, that is true, but in the
simplest form it should be doable. The implementation details are of course discussable, but below is something more concrete to get things started.
Introduce a process-wide singleton pointer, which is a pointer to the function that will augment the exception about to be thrown:
typedef void (*exception_augmenter)(boost::exception&);
Consider dynamically loaded libraries.
That is a separate can of worms, which can ruin your day even without the global pointer. The global hook doesn't make it worse per se.
Also consider multiple libraries wanting different hooks.
That would be problematic to support - both with run-time and compile-time hooks (the latter is difficult because of ODR violations). I'm not sure this level of flexibility would be useful either.
A process-wide pointer is very difficult to do in the general case, and I suspect it'd drag a lot more dependencies than capturing the stacktrace would.
The most complicated part is Windows-specific implementation, and it only requires a few functions from WinAPI. Marking the pointer visible on POSIX is trivial.
Consider that boost::throw_exception must support very very old compilers.
How old? I mean, it only has to support the oldest compiler Boost libraries support.
Besides, throwing exceptions and capturing stack trace are thread-local operations, while global pointer would require synchronization with the initialization code.
That's a valid concern, but I think it can be mitigated by caching the pointer. We could also accept the limitation that the augmentor has to be installed once at the beginning of the program and simply not support its multithreaded modification.
Are you proposing to include <windows.h> in boost/throw_exception.hpp? That's a no-go.
No. Use approach similar to Boost.WinAPI on Windows (i.e. re-declare WinAPI components).
Making it not header-only module? Also a no-go.
No.
On Fri, Dec 16, 2016 at 12:24 PM, Andrey Semashev <andrey.semashev@gmail.com
wrote:
On 12/16/16 22:32, Emil Dotchevski wrote:
On Fri, Dec 16, 2016 at 1:20 AM, Andrey Semashev < andrey.semashev@gmail.com> wrote:
the argument wouldn't be whether or not capturing the
stack trace is integrated into boost::throw_exception, but whether or not it is enabled by default. Either way, the boost::exception class (defined in boost/exception/exception.hpp) should provide a hook for that integration in order to avoid coupling between boost::throw_exception and Stacktrace.
The way I see it, boost::exception already provides everything needed, which is the ability to attach arbitrary data to the exception. I don't think adding more special case capability to it is a wise solution in the long term.
This can be said about __FILE__ and __LINE__ which BOOST_THROW_EXCEPTION captures automatically. The reason why it's done is that 1) the cost in terms of time and space is considered negligible and 2) it is potentially critical for debugging.
The reason why it Stacktrace would need special handling rather than go through the usual Boost Exception machinery is also the same as why there is special handling (members in boost::exception): to avoid coupling with the rest of the Boost Exception machinery, which is considerably heavier and bigger than boost/exception/exception.hpp.
The backtrace itself is not a lightweght object
Yes it is. It's basically a list of pointers and we can cap it by default at, say, 10. Just to be clear, I'm talking about adding the equivalent of something like this to the boost::exception class: void (*stack_trace_[10])();
so the overhead of the generic Boost.Exception machinery is probably not significant. And if that overhead is significant then maybe it could be optimized instead. After all, why have it if we don't want to use it.
The cost of using the generic Boost Exception machinery is not limited to speed and space but also coupling. The boost::throw_exception function template is not coupled with any Boost libraries, while Boost Exception itself uses e.g. shared_ptr and a few others. That said, even the runtime cost of the generic Boost Exception machinery is probably an order of magnitude bigger than storing 10 pointers.
Consider that boost::throw_exception must
support very very old compilers.
How old? I mean, it only has to support the oldest compiler Boost libraries support.
Yes, which is very old. :) Anyway, the point is that boost::throw_exception must remain extremely lightweight in terms of dependencies AND it must be very conservative in terms of using C++ features. Specifically, I am hinting that std synchronization facilities are off-limits (though to be fair capturing the stack trace can be commented-out for old compilers.)
Are you proposing to include <windows.h> in boost/throw_exception.hpp?
That's a no-go.
No. Use approach similar to Boost.WinAPI on Windows (i.e. re-declare WinAPI components).
Making it not header-only module? Also a no-go.
No.
Requiring to link the Windows kernel? That's a breaking change, too. Emil
On Sat, Dec 17, 2016 at 12:11 AM, Emil Dotchevski <emildotchevski@gmail.com> wrote:
On Fri, Dec 16, 2016 at 12:24 PM, Andrey Semashev <andrey.semashev@gmail.com
wrote:
The backtrace itself is not a lightweght object
Yes it is. It's basically a list of pointers and we can cap it by default at, say, 10.
Just to be clear, I'm talking about adding the equivalent of something like this to the boost::exception class:
void (*stack_trace_[10])();
What happens when the backtrace exceeds this limit? Would it be possible to attach backtraces larger than that? Adding an array of pointers to the base class of exceptions is also not free. Whether one uses stacktraces or not, all Boost exceptions become 80 bytes larger regardless. For comparison, std::runtime_error in gcc 6 takes 16 bytes (not counting the string, if it's dynamically allocated and not stored in-place). Not that I have thousands exceptions stored somewhere, but that doesn't look like a reasonable tradeoff.
so the overhead of the generic Boost.Exception machinery is probably not significant. And if that overhead is significant then maybe it could be optimized instead. After all, why have it if we don't want to use it.
The cost of using the generic Boost Exception machinery is not limited to speed and space but also coupling. The boost::throw_exception function template is not coupled with any Boost libraries, while Boost Exception itself uses e.g. shared_ptr and a few others. That said, even the runtime cost of the generic Boost Exception machinery is probably an order of magnitude bigger than storing 10 pointers.
Thing is, the 10 pointers affect everyone, including those not using the backtraces. They also affect boost::exception interface making it more like a swiss knife with a bunch of case-specific APIs. That means everyone using boost::exception are coupled with whatever is present with those APIs. You described the 10 pointers, but I don't think you would expose that array as is from boost::exception - if only for type safety, you would want to wrap it into a class or something. That class will have some methods to work with the stacktrace, and so on. That is beside the fact that given there is Boost.Stacktrace library, that class doesn't really belong to Boost.Exception or throw_exception.
How old? I mean, it only has to support the oldest compiler Boost libraries support.
Yes, which is very old. :) Anyway, the point is that boost::throw_exception must remain extremely lightweight in terms of dependencies AND it must be very conservative in terms of using C++ features. Specifically, I am hinting that std synchronization facilities are off-limits
Intrinsics are still available.
Requiring to link the Windows kernel? That's a breaking change, too.
But you do have to link something to obtain the backtrace. You're not planning to implement it inline in the header, for all platforms, do you? :) Anyway, if everything's off limits then the only thing I can suggest is to leave throw_exception as is and create a separate macro/function with support for backtraces.
On Fri, Dec 16, 2016 at 1:59 PM, Andrey Semashev <andrey.semashev@gmail.com> wrote:
On Sat, Dec 17, 2016 at 12:11 AM, Emil Dotchevski <emildotchevski@gmail.com> wrote:
On Fri, Dec 16, 2016 at 12:24 PM, Andrey Semashev < andrey.semashev@gmail.com
wrote:
The backtrace itself is not a lightweght object
Yes it is. It's basically a list of pointers and we can cap it by default at, say, 10.
Just to be clear, I'm talking about adding the equivalent of something like this to the boost::exception class:
void (*stack_trace_[10])();
What happens when the backtrace exceeds this limit? Would it be possible to attach backtraces larger than that?
We capture the top 10, or maybe the top 32. This puts a limit to both the space and speed overhead of capturing the stack trace.
Adding an array of pointers to the base class of exceptions is also not free. Whether one uses stacktraces or not, all Boost exceptions become 80 bytes larger regardless. For comparison, std::runtime_error in gcc 6 takes 16 bytes (not counting the string, if it's dynamically allocated and not stored in-place). Not that I have thousands exceptions stored somewhere, but that doesn't look like a reasonable tradeoff.
In C++, it is unspecified how much memory is required to throw an exception. By definition, throwing an exception may require a memory allocation, which might fail with std::bad_alloc. That said, I've never seen this in practice, I don't think we should be concerned.
so the overhead of the generic Boost.Exception machinery is probably not significant. And if that overhead is significant then maybe it could be optimized instead. After all, why have it if we don't want to use it.
The cost of using the generic Boost Exception machinery is not limited to speed and space but also coupling. The boost::throw_exception function template is not coupled with any Boost libraries, while Boost Exception itself uses e.g. shared_ptr and a few others. That said, even the runtime cost of the generic Boost Exception machinery is probably an order of magnitude bigger than storing 10 pointers.
Thing is, the 10 pointers affect everyone, including those not using the backtraces.
True, so do capturing __FILE__ and __LINE__. I'm not suggesting that capturing the stack trace by default is free, only that it is the better compromise (pending looking at the actual implementation).
They also affect boost::exception interface making it more like a swiss knife with a bunch of case-specific APIs.
Not really, like __FILE__ and __LINE__, the data captured by the stack trace would be available through the usual boost::get_error_info interface; the fact that there is special handling of this data is an implementation detail not exposed in the API.
That means everyone using boost::exception are coupled with whatever is present with those APIs. You described the 10 pointers, but I don't think you would expose that array as is from boost::exception - if only for type safety, you would want to wrap it into a class or something.
No, it would be an array of pointers, or maybe just an array of bytes that a boost::get_error_info specialization will know how to deal with. That part of Boost Exception may be coupled with Stacktrace, that's fine.
Requiring to link the Windows kernel? That's a breaking change, too.
But you do have to link something to obtain the backtrace. You're not planning to implement it inline in the header, for all platforms, do you? :)
I don't know, but I wouldn't exclude that as a possibility. If the goal is to make this as lightweight as possible so that the stacktrace can be reasonably captured by default, why not? As a general rule, I think that it is permissible to use compiler-specific APIs that don't require explicit linker parameters. Emil
On 15 Dec 2016 at 16:00, Emil Dotchevski wrote:
Do you mean a run-time hook or a compile-time hook?
I think this should be compile-time hook in order to avoid the perils of dynamic initialization in complex multi-thread programs. But we already have that compile-time hook, it's called boost::throw_exception.
Firstly thanks to all for the detailed discussion. Bringing it back to the review of Stacktrace which this is (and not of Exception, another discussion thread would suit that), I am seeing the following options proposed for Stacktrace: Option 1: That Stacktrace implement a macro like BOOST_STACKTRACE_THROW_EXCEPTION() which reimplements BOOST_THROW_EXCEPTION() but with muxed in stacktrace. Option 2: That Stacktrace optionally hook some compile time hook in Boost.Exception to mux in stacktrace. Option 3: That Stacktrace optionally hook some runtime hook in Boost.Exception to mux in stacktrace. Implied in all the above is that the exception type stacktrace mux in framework as described in the current Stacktrace docs is considered bad form and should be deleted? Can people interested vote on one of the options above, or propose their own option, and say whether they want the documented mux in mechanism removed or not? Thanks, Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 12/16/16 11:05 AM, Niall Douglas wrote:
Option 1: That Stacktrace implement a macro like BOOST_STACKTRACE_THROW_EXCEPTION() which reimplements BOOST_THROW_EXCEPTION() but with muxed in stacktrace.
Option 2: That Stacktrace optionally hook some compile time hook in Boost.Exception to mux in stacktrace.
Option 3: That Stacktrace optionally hook some runtime hook in Boost.Exception to mux in stacktrace.
Option 3: My suggestion was to ignore the whole issue and assign it some other library/component. A couple libraries use the idiom similar to template MyTemplate<class T some_type, class E error_handler = boost::throw_exception> This decouples the error handling from the functionality of the library itself and permits the user to unificate his error handling across libraries. It also avoids the necessity of coordinating error handling across libraries in time and space. In this particular case stacktrace would provide the mechanism to invoke/serialization or whatever a stack trace and it would be job of the library/application which uses it to decide when to invoke it. Robert Ramey
On 12/16/16 22:05, Niall Douglas wrote:
On 15 Dec 2016 at 16:00, Emil Dotchevski wrote:
Do you mean a run-time hook or a compile-time hook?
I think this should be compile-time hook in order to avoid the perils of dynamic initialization in complex multi-thread programs. But we already have that compile-time hook, it's called boost::throw_exception.
Firstly thanks to all for the detailed discussion. Bringing it back to the review of Stacktrace which this is (and not of Exception, another discussion thread would suit that), I am seeing the following options proposed for Stacktrace:
Option 1: That Stacktrace implement a macro like BOOST_STACKTRACE_THROW_EXCEPTION() which reimplements BOOST_THROW_EXCEPTION() but with muxed in stacktrace.
Option 2: That Stacktrace optionally hook some compile time hook in Boost.Exception to mux in stacktrace.
Option 3: That Stacktrace optionally hook some runtime hook in Boost.Exception to mux in stacktrace.
Option 3 or 3+1 (i.e. the runtime hook is used by a new macro) look the most preferable to me.
Implied in all the above is that the exception type stacktrace mux in framework as described in the current Stacktrace docs is considered bad form and should be deleted?
Frankly, I haven't yet reviewed Stacktrace, although now that I entered the discussion I probably should. I'll try to do that on the weekend. For now it seems unnecessary to have two mechanisms for injecting stacktraces. Given that we already have Boost.Exception, it seems preferable to use its facilities.
On Fri, Dec 16, 2016 at 11:05 AM, Niall Douglas <s_sourceforge@nedprod.com> wrote:
On 15 Dec 2016 at 16:00, Emil Dotchevski wrote:
Do you mean a run-time hook or a compile-time hook?
I think this should be compile-time hook in order to avoid the perils of dynamic initialization in complex multi-thread programs. But we already have that compile-time hook, it's called boost::throw_exception.
Firstly thanks to all for the detailed discussion. Bringing it back to the review of Stacktrace which this is (and not of Exception, another discussion thread would suit that), I am seeing the following options proposed for Stacktrace:
Option 1: That Stacktrace implement a macro like BOOST_STACKTRACE_THROW_EXCEPTION() which reimplements BOOST_THROW_EXCEPTION() but with muxed in stacktrace.
Option 2: That Stacktrace optionally hook some compile time hook in Boost.Exception to mux in stacktrace.
Option 3: That Stacktrace optionally hook some runtime hook in Boost.Exception to mux in stacktrace.
Implied in all the above is that the exception type stacktrace mux in framework as described in the current Stacktrace docs is considered bad form and should be deleted?
Can people interested vote on one of the options above, or propose their own option, and say whether they want the documented mux in mechanism removed or not?
It seems that there is some disagreement on how costly each of the options is. I think that before a vote we should look at actual code, actual proposed changes, and actual costs in terms of coupling and speed/space. This shouldn't be difficult to do, given that Stacktrace already exists. Emil
Niall Douglas wrote:
Bringing it back to the review of Stacktrace which this is (and not of Exception, another discussion thread would suit that), I am seeing the following options proposed for Stacktrace:
Option 1: That Stacktrace implement a macro like BOOST_STACKTRACE_THROW_EXCEPTION() which reimplements BOOST_THROW_EXCEPTION() but with muxed in stacktrace.
Option 2: That Stacktrace optionally hook some compile time hook in Boost.Exception to mux in stacktrace.
Option 3: That Stacktrace optionally hook some runtime hook in Boost.Exception to mux in stacktrace.
This is proving to be a distraction from the Stacktrace review and I think that we need to postpone this discussion for when Stacktrace is accepted. I, for one, don't back any of those options. Stacktrace itself shouldn't hook anything. boost::throw_exception should provide some sort of a mechanism via which the user should (easily) be able to include a stack trace into the exceptions.
Implied in all the above is that the exception type stacktrace mux in framework as described in the current Stacktrace docs is considered bad form and should be deleted?
That's not a problem as far as I'm concerned, but it would be nice if the Boost.Exception way is documented as well.
This [the potential integration with throw_exception] is proving to be a distraction from the Stacktrace review and I think that we need to postpone this discussion for when Stacktrace is accepted.
Although, on second thought, there is one pertinent question that seems to have come up. Specifically, if the library should be extended to allow me to capture the trace on my own using RtlCaptureStackBackTrace or ::backtrace, then at some later point use the rest of the Stacktrace functionality. And additionally, can I do that for ::backtrace captures even when Stacktrace is using a different backend (unwind) as its default.
From a quick look at the code, it seems that the library cannot currently use more than one backend at the same time because they are all named 'class backend'.
As a note, the documentation states that backtrace is a POSIX function, but it seems to be Linux-specific. I can't find it in the POSIX standard. I doubt that unwind is also standard POSIX. Does the library support OS X?
On 17 Dec 2016 at 0:50, Peter Dimov wrote:
Bringing it back to the review of Stacktrace which this is (and not of Exception, another discussion thread would suit that), I am seeing the following options proposed for Stacktrace: [snip] This is proving to be a distraction from the Stacktrace review and I think that we need to postpone this discussion for when Stacktrace is accepted.
I agree. 1. Can those talking about Boost.Exception please move their discussion to a new thread and stop distracting from the Stacktrace review thread? I'm not saying it's not valuable discussion, just please start a new thread. 2. I have seen repeated comments by more than one person of "Stacktrace would need to have XXX functionality to do ...". Please consider reviewing Stacktrace as presented before making more of these comments. About 80% of those comments Stacktrace already has that functionality and you are wasting time proving a rationale for them being implemented because it's implemented already. For the 20% of the time Stacktrace doesn't have something, I need to see a formal review from you saying "Stacktrace needs YYY functionality to do ..." 3. Please could people actually review Stacktrace's implementation code instead of inferring implementation from what someone else said here about something they half read in the tutorial. Specifically, is its API design solid? Is the use of std::string in its public API acceptable? Is its stacktrace capturing implementation malloc free? That sort of thing. These are the sorts of non-hand-waving review feedback we need. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Niall Douglas wrote:
3. Please could people actually review Stacktrace's implementation code instead of inferring implementation from what someone else said here about something they half read in the tutorial. Specifically, is its API design solid? Is the use of std::string in its public API acceptable?
There's nothing wrong with using std::string in principle, but it's obviously not async safe. The operations however that return std::strings are also not async safe at the moment, so it's not the std::string return value that's the problem. The documentation has an example of printing a stack trace from a SIGSEGV handler, which is undefined behavior as it stands. So it's not clear to me whether the library is supposed to be useful - in its decoding/printing part, not just in its capture part - in signal handlers. Changing/augmenting the interface to be async safe would be easy, size_t frame::get_name( char* buffer, size_t size ) const; instead of, or in addition to, std::string frame::name() const; and void frame::print_to_fd( int fd ) const; in addition to ostream& operator<<( ostream&, frame const & ); but if the implementation can't be made async safe, this is not going to be of much use. A quick look of the implementation - and the list of POSIX async safe functions - was not enough for me to be able to resolve this question one way or the other. Most of what the Linux backend is doing is already async safe, but not everything, addr2line has an option to demangle, so the dependency on core::demangle could in principle be avoided... but I'm not quite sure. There is also an indication that the current frame interface is not the right one. In std::string name = f.name(); if (!name.empty()) { os << name; } else { os << f.address(); } const std::size_t source_line = f.source_line(); if (source_line) { os << " at " << f.source_file() << ':' << source_line; } which is perfectly reasonable code given the current interface, addr2line is called three times instead of just one. And in operator<< for a stacktrace of depth N, addr2line will be called 3*N times instead of one. The operator<< implementations themselves can be fixed, but user code that does the same will obviously not be able to benefit. I understand why it's better for the value_type of stacktrace::const_iterator to be 'frame' instead of just 'void*', but the accessors each calling addr2line is a symptom of a serious problem that the 'stupid' void* design: void get_frame_info( void* address, string& function, string& file, size_t& line ); lacks. stacktrace itself could provide a function that calls a user-provided callback void process_frame_info( size_t index, void* address, char const* function, char const* file, size_t line ); for each frame. This would be able to call addr2line just once, and it's also an async-safe interface, if the implementation can be made so. This is not exactly a common C++ style, but async safe code obviously has its own rules.
2016-12-17 18:31 GMT+03:00 Peter Dimov <lists@pdimov.com>:
Niall Douglas wrote:
3. Please could people actually review Stacktrace's implementation code instead of inferring implementation from what someone else said here about something they half read in the tutorial. Specifically, is its API design solid? Is the use of std::string in its public API acceptable?
There's nothing wrong with using std::string in principle, but it's obviously not async safe. The operations however that return std::strings are also not async safe at the moment, so it's not the std::string return value that's the problem.
Async safety of those functions won't change.
The documentation has an example of printing a stack trace from a SIGSEGV handler, which is undefined behavior as it stands. So it's not clear to me whether the library is supposed to be useful - in its decoding/printing part, not just in its capture part - in signal handlers.
Changing/augmenting the interface to be async safe would be easy,
size_t frame::get_name( char* buffer, size_t size ) const;
instead of, or in addition to,
std::string frame::name() const;
and
void frame::print_to_fd( int fd ) const;
in addition to
ostream& operator<<( ostream&, frame const & );
but if the implementation can't be made async safe, this is not going to be of much use.
Windows implementation of those functions is definitely impossible to make async safe, as the Windows backend uses COM. And that can not be changed :(
A quick look of the implementation - and the list of POSIX async safe functions - was not enough for me to be able to resolve this question one way or the other. Most of what the Linux backend is doing is already async safe, but not everything, addr2line has an option to demangle, so the dependency on core::demangle could in principle be avoided... but I'm not quite sure.
I'd like to avoid functions that are async-safe on one platform and async unsafe on other. Mixing safety is a perfect way for producing issues in user code.
There is also an indication that the current frame interface is not the right one. In
std::string name = f.name(); if (!name.empty()) { os << name; } else { os << f.address(); }
const std::size_t source_line = f.source_line(); if (source_line) { os << " at " << f.source_file() << ':' << source_line; }
which is perfectly reasonable code given the current interface, addr2line is called three times instead of just one. And in operator<< for a stacktrace of depth N, addr2line will be called 3*N times instead of one.
The operator<< implementations themselves can be fixed, but user code that does the same will obviously not be able to benefit.
Yes, I'm planing to provide optimized versions of ostream operators after the review. Windows backend has exactly the same problem.
I understand why it's better for the value_type of stacktrace::const_iterator to be 'frame' instead of just 'void*', but the accessors each calling addr2line is a symptom of a serious problem that the 'stupid' void* design:
void get_frame_info( void* address, string& function, string& file, size_t& line );
lacks.
stacktrace itself could provide a function that calls a user-provided callback
void process_frame_info( size_t index, void* address, char const* function, char const* file, size_t line );
for each frame. This would be able to call addr2line just once, and it's also an async-safe interface, if the implementation can be made so.
Hm... I need to think about it, but I'm not sure that the feature is widely useful. Most of the time people just want all the available information and default ostream operators are OK for them. Those, who want a different (shorter) output format are typically choosing between outputting only function names or outputting only function location in source file. Getting function name and function location in source file are two different COM calls on Windows, so such callback will not work great on Windows. -- Best regards, Antony Polukhin
Antony Polukhin wrote:
Windows implementation of those functions is definitely impossible to make async safe, as the Windows backend uses COM. And that can not be changed :(
I'm not sure that the concept of async safety exists on Windows. Windows doesn't have signals. Or, stated differently, everything is async safe on Windows.
I'd like to avoid functions that are async-safe on one platform and async unsafe on other. Mixing safety is a perfect way for producing issues in user code.
Well, at least some of the code will be correct then, as opposed to none today.
Yes, I'm planing to provide optimized versions of ostream operators after the review. Windows backend has exactly the same problem.
The problem is that the frame interface has these inherent performance problems. You could optimize op<<, but any user code that doesn't use op<< is still going to have issues. This is an indication that the provided interface is not quite right.
Getting function name and function location in source file are two different COM calls on Windows, so such callback will not work great on Windows.
The cost of initializing and shutting down COM each time dwarfs calling GetNameByOffset. It shouldn't even be a contest. It also occurs to me that it would be more natural for the backend interface to use size_t get_name(char* buffer, size_t size) instead of string name(), because all the low-level APIs the backends use actually already use the former interface, or something closer to it. Again, not very C++ friendly, but it probably should be at least an option for people who don't want to touch the C/C++ runtime when printing their stack trace, as it could be in an unpredictable state after a crash.
2016-12-17 19:37 GMT+03:00 Peter Dimov <lists@pdimov.com>:
Antony Polukhin wrote:
Windows implementation of those functions is definitely impossible to make async safe, as the Windows backend uses COM. And that can not be changed :(
I'm not sure that the concept of async safety exists on Windows. Windows doesn't have signals. Or, stated differently, everything is async safe on Windows.
Actually, it's the other way around: everything is async-signal unsafe https://msdn.microsoft.com/en-us/en-en/library/xdkz3x12.aspx : "Do not use any function that generates a system call (for example, _getcwd or time)."
Yes, I'm planing to provide optimized versions of ostream operators after the review. Windows backend has exactly the same problem.
The problem is that the frame interface has these inherent performance problems. You could optimize op<<, but any user code that doesn't use op<< is still going to have issues. This is an indication that the provided interface is not quite right.
This could be optimized and cached, but in that case library won't be header only and it will get dependencies on boost_thread. Most of the people won't like it. Optimized ostream operators seem to be a lesser evil.
Getting function name and function location in source file are two different COM calls on Windows, so such callback will not work great on Windows.
The cost of initializing and shutting down COM each time dwarfs calling GetNameByOffset. It shouldn't even be a contest.
It also occurs to me that it would be more natural for the backend interface to use size_t get_name(char* buffer, size_t size) instead of string name(), because all the low-level APIs the backends use actually already use the former interface, or something closer to it. Again, not very C++ friendly, but it probably should be at least an option for people who don't want to touch the C/C++ runtime when printing their stack trace, as it could be in an unpredictable state after a crash.
Probably I'm missing some point, but async-signal-safety means almost the same thing. If function is not async-signal-safe you shall not use it in unpredictable states after crash. Anyway, if you do not wish to use C++ runtime after a crash, you definitely would not like to use COM in that place (so std::string in the interface does not make a big difference). -- Best regards, Antony Polukhin
Antony Polukhin wrote:
Actually, it's the other way around: everything is async-signal unsafe https://msdn.microsoft.com/en-us/en-en/library/xdkz3x12.aspx :
"Do not use any function that generates a system call (for example, _getcwd or time)."
There are no signals on Windows. They just don't exist. The signal function is provided by the C runtime and emulates signals. The closest thing to a signal are the Ctrl+C and Ctrl+Break keyboard interrupts, and they spawn a new thread to call the handler, instead of reusing one as a POSIX signal would do.
Anyway, if you do not wish to use C++ runtime after a crash, you definitely would not like to use COM in that place (so std::string in the interface does not make a big difference).
The runtime and COM have nothing in common. If your program crashes somewhere in the runtime, the runtime may be unusable from that point on, but this does not affect COM at all. And if you crash somewhere in a COM component, that particular component may or may not be usable from that point on, but this does not necessarily affect COM as a whole. You can still use COM.
2016-12-17 20:47 GMT+03:00 Peter Dimov <lists@pdimov.com>:
Antony Polukhin wrote:
Actually, it's the other way around: everything is async-signal unsafe https://msdn.microsoft.com/en-us/en-en/library/xdkz3x12.aspx :
"Do not use any function that generates a system call (for example, _getcwd or time)."
There are no signals on Windows. They just don't exist. The signal function is provided by the C runtime and emulates signals.
The closest thing to a signal are the Ctrl+C and Ctrl+Break keyboard interrupts, and they spawn a new thread to call the handler, instead of reusing one as a POSIX signal would do.
Ok, but the signal documentation does not allow to use a lot of functions in signal handlers (syscalls, malloc, printf...). Am I allowed to use COM functions in signal handlers? -- Best regards, Antony Polukhin
Antony Polukhin wrote:
Ok, but the signal documentation does not allow to use a lot of functions in signal handlers (syscalls, malloc, printf...). Am I allowed to use COM functions in signal handlers?
The signal handlers are fake, they aren't really signal handlers. A crash generates a structured exception on Windows, and the runtime calls the appropriate signal handler when handling that exception. I'm not 100% sure what can be done in a SEH handler, but calling Windows API functions is certainly possible, and this specific COM use - loading an inproc handler and calling it - should be fine too. (In some rare situations, such as crashes in constructors of static objects in DLLs, the loader lock may cause a deadlock, but that's life.)
On 17 Dec 2016 at 20:22, Peter Dimov wrote:
Ok, but the signal documentation does not allow to use a lot of functions in signal handlers (syscalls, malloc, printf...). Am I allowed to use COM functions in signal handlers?
The signal handlers are fake, they aren't really signal handlers. A crash generates a structured exception on Windows, and the runtime calls the appropriate signal handler when handling that exception. I'm not 100% sure what can be done in a SEH handler, but calling Windows API functions is certainly possible, and this specific COM use - loading an inproc handler and calling it - should be fine too. (In some rare situations, such as crashes in constructors of static objects in DLLs, the loader lock may cause a deadlock, but that's life.)
Reentering the COM machinery is definitely not safe, it deadlocks randomly depending on the threading model used. I have found that if you preload the COM object and precall all the functions you'll call from the async handler, you preexecute all the delay loading and lazy binding code. When the async handler is called, it will then not reenter the COM machinery, thus avoiding deadlocks. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Niall Douglas wrote:
Reentering the COM machinery is definitely not safe, it deadlocks randomly depending on the threading model used.
I have found that if you preload the COM object and precall all the functions you'll call from the async handler, you preexecute all the delay loading and lazy binding code. When the async handler is called, it will then not reenter the COM machinery, thus avoiding deadlocks.
This is not COM, but the loader lock. If you try to load a DLL from DllMain, it deadlocks. This in C++ terms means that constructors of static objects in a DLL can't load DLLs, so in our case, would not be able to resolve stack traces. This is an acceptable limitation. That said, I'm actually not sure whether the debug engine is in-process.
2016-12-18 17:03 GMT+03:00 Peter Dimov <lists@pdimov.com>:
Niall Douglas wrote:
Reentering the COM machinery is definitely not safe, it deadlocks randomly depending on the threading model used.
I have found that if you preload the COM object and precall all the functions you'll call from the async handler, you preexecute all the delay loading and lazy binding code. When the async handler is called, it will then not reenter the COM machinery, thus avoiding deadlocks.
This is not COM, but the loader lock. If you try to load a DLL from DllMain, it deadlocks. This in C++ terms means that constructors of static objects in a DLL can't load DLLs, so in our case, would not be able to resolve stack traces. This is an acceptable limitation.
That said, I'm actually not sure whether the debug engine is in-process.
The bad thing is that any printf like function, kernel calls (including multithreading) or malloc are not async signal handler safe. I'm quite sure that COM functions allocate memory and do some synchronization. It is possible to make an async-signal-safe stacktrace printing on Linux... But on Windows it seems impossible. If anyone knows a way to move data from signal handler into a usual environment I can create a safe_print() function that * will do safe printing for Linux/POSIX * will do some extremely heavy machinery of moving the stacktrace outside the signal handler and printing it for windows. Any ideas on how to do that? -- Best regards, Antony Polukhin
Antony Polukhin wrote:
The bad thing is that any printf like function, kernel calls (including multithreading) or malloc are not async signal handler safe. I'm quite sure that COM functions allocate memory and do some synchronization.
We can't speak of "COM functions" as such. Every COM function does its own thing. In the in-process case, COM is basically just an ABI. What Dbgeng.dll does is what it does. It certainly doesn't allocate using (the program's) malloc though. And if it does synchronize, it does it with its own CRITICAL_SECTIONs, and those can be used in a structured exception handler. If Dbgeng.dll itself crashes, it would most likely be unable to resolve its own crash, of course. For async safe printing itself, you should use write/WriteFile. Something like void frame::print_to_fd( int fd ); // POSIX void frame::print_to_fd( void* fd ); // Windows, void* is HANDLE Now the user will be able to pass GetStdHandle( STD_ERROR_HANDLE ) for stderr output, or a file handle for log output.
2016-12-18 20:38 GMT+03:00 Peter Dimov <lists@pdimov.com>:
Antony Polukhin wrote:
The bad thing is that any printf like function, kernel calls (including multithreading) or malloc are not async signal handler safe. I'm quite sure that COM functions allocate memory and do some synchronization.
We can't speak of "COM functions" as such. Every COM function does its own thing. In the in-process case, COM is basically just an ABI. What Dbgeng.dll does is what it does. It certainly doesn't allocate using (the program's) malloc though. And if it does synchronize, it does it with its own CRITICAL_SECTIONs, and those can be used in a structured exception handler.
Those are very generous assumptions. As long as we have no strict documentation or source codes of the component - it is UB to use it in async-signal-handler. -- Best regards, Antony Polukhin
Antony Polukhin wrote:
As long as we have no strict documentation or source codes of the component - it is UB to use it in async-signal-handler.
Yes, obviously. It's not clear though what do Linux users gain from the decision to keep their backend async-unsafe just because Windows is not strictly documented. And, FWIW, Windows is pretty resilient. Here for instance I crash in an APC that is called by the kernel, and it works, even though I can't see g() in the trace probably because Windows has handled the exception and rethrown it: #include <boost/stacktrace.hpp> #include <iostream> #include <signal.h> #include <windows.h> static void handler( int sig ) { boost::stacktrace::stacktrace st; std::cout << st << std::endl; _exit( 3 ); } VOID CALLBACK g( ULONG_PTR ) { std::cout << "g\n"; *(int*)0 = 0; } static void f() { std::cout << "f\n"; QueueUserAPC( g, GetCurrentThread(), 0 ); SleepEx( 1000, TRUE ); } int main() { signal( SIGSEGV, handler ); f(); }
On 17 Dec 2016 at 19:47, Peter Dimov wrote:
Antony Polukhin wrote:
Actually, it's the other way around: everything is async-signal unsafe https://msdn.microsoft.com/en-us/en-en/library/xdkz3x12.aspx :
"Do not use any function that generates a system call (for example, _getcwd or time)."
There are no signals on Windows. They just don't exist. The signal function is provided by the C runtime and emulates signals.
There seems to be a lot of muddled discussion here. Of course Windows has signals, as already referred to by myself earlier it's called vectored exception handling which is exactly the same as a signal implementation. In an exception handler you cannot call any async unsafe routine such as anything in MSVCRT nor anything implemented by kernel32.dll in userspace. As on POSIX, almost all syscalls implemented entirely in kernel space are safe. With respect to Stacktrace, that means that if you installed a SIGSEGV handler and wanted to print a stacktrace from it before fatal exit, you have exactly the same limitations on Windows as on POSIX. You can't call printf, you can't call malloc. You can't use ostream. This is why I hinted at the usage of std::string in Stacktrace's API. As far as I can tell, this API design choice precludes the use of Stacktrace in any SIGSEGV handlers. Antony makes the valid point that on Windows there are race problems with the DbgHelp library, in fact not only is it not async-unsafe, it's also thread-unsafe. Trying to symbolise a stacktrace from multiple threads concurrently on Windows will therefore misoperate. Is this okay with the Boost community or do you feel it is a problem? Specifically: 1. Should Stacktrace be async safe so it can be used to print a stacktrace from a signal/exception handler? 2. Is its lack of thread safety on Windows a problem? Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Niall Douglas wrote:
In an exception handler you cannot call any async unsafe routine such as anything in MSVCRT nor anything implemented by kernel32.dll in userspace. As on POSIX, almost all syscalls implemented entirely in kernel space are safe.
Thanks Niall. Do you know which Windows API functions are safe and which aren't? I couldn't find a list anywhere.
Antony makes the valid point that on Windows there are race problems with the DbgHelp library, in fact not only is it not async-unsafe, it's also thread-unsafe.
He doesn't use DbgHelp in the Windows backend though, he uses Dbgeng.h. This is not the same thing, I think?
Of course Windows has signals, as already referred to by myself earlier it's called vectored exception handling which is exactly the same as a signal implementation.
Not quite. A signal immediately suspends the thread and calls the handler in it. Windows exception handling, in contrast, unwinds the stack. So if the kernel crashes somewhere deep, it can unwind itself to a usable state before the program gets to handle the exception. Or at least that's my understanding.
On 17 Dec 2016 at 20:32, Peter Dimov wrote:
In an exception handler you cannot call any async unsafe routine such as anything in MSVCRT nor anything implemented by kernel32.dll in userspace. As on POSIX, almost all syscalls implemented entirely in kernel space are safe.
Thanks Niall. Do you know which Windows API functions are safe and which aren't? I couldn't find a list anywhere.
I don't think there is even a list internal to Microsoft. There are various user compiled lists around the internet, the ReactOS team also have a good list somewhere on their mailing list. I've seen my code which worked perfectly on Win7 rarely and randomly deadlock on Win10 and one time, vice versa. WOW64 also has a very different safe list to native Win32. There are some obvious things not to call: anything which obviously runs code in userspace.
Antony makes the valid point that on Windows there are race problems with the DbgHelp library, in fact not only is it not async-unsafe, it's also thread-unsafe.
He doesn't use DbgHelp in the Windows backend though, he uses Dbgeng.h. This is not the same thing, I think?
Woohoo! That is amazing news, and congrats to Antony for getting Dbgeng working. Last month when I prereviewed Stacktrace I mentioned that DbgHelp was a steaming pile of poo and that I had had much more reliable experience with the thoroughly superior Dbgeng. Antony asked for some example code because Dbgeng is barely documented, I no longer had access to the code I wrote many years ago which used it. I tried to cobble something together and I made some progress over what Antony had, but I ran out of time due to needing to mind the recent new baby. I'm not actually sure what he changed from what I sent him, mine and his look very close, I must have missed something very small. I hadn't realised Antony figured it out and had assumed he was still on DbgHelp, and the fact he's using Dbgeng makes Stacktrace much superior to 99% of the windows stack trace implementations out there. Particular benefits include: * Dbgeng understands non-native stack frames, so mixed .NET, WinRT and C++/CLI stack traces just work. * Dbgeng is threadsafe. * Dbgeng doesn't randomly fail and randomly work next time you call it.
Of course Windows has signals, as already referred to by myself earlier it's called vectored exception handling which is exactly the same as a signal implementation.
Not quite. A signal immediately suspends the thread and calls the handler in it. Windows exception handling, in contrast, unwinds the stack. So if the kernel crashes somewhere deep, it can unwind itself to a usable state before the program gets to handle the exception. Or at least that's my understanding.
You're missing a few steps. 1. RaiseException() is like kill() and starts the signal handling process with parameters. Hardware exceptions raise an exception at the immediately point of ocurrance i.e. inside any locks etc. 2. Any installed vectored exception handlers are like sigaction() except they are called for all exception codes. The handler returns whether it handled it or whether to keep searching. Vectored exception handlers are process wide. 3. If still unhandled, the Thread Information Block (TIB) for the thread where the exception occurred is asked for the current thread-local TEH (Table Exception Handling) on x64 or SEH (Structured Exception Handling) on x86. A search for all handlers installed for all code in the stack until the point of exception are called in reverse order. Each handler may handle the exception, or say to keep searching. 4. Every thread is always begun with a default exception handler which opens that famous dialog box and terminates the process, so if your exception reaches this the right thing happens. 5. C++ exceptions are implemented 100% a client of the same TEH and SEH framework. In fact they are simply a RaiseException(0xE06D7363, ...). If you dive into the implementation of __CxxThrowException() you'll see that. What's very important to note is that all this occurs without unwinding the stack and at the point of the exception with any locks still held. This is the source of the reentrancy which causes the deadlocks if you call any userspace implemented code from an exception handler. The reason it doesn't unwind is because a handler may choose to restart execution of the failed operation. There was a very clever commercial C++ object-to-disk system a very long time ago which serialised C++ objects out to disk and removed the RAM storage. When the C++ program faulted on accessing them, the SEH handler would deserialise the object back into RAM and restart the failed instruction. Worked beautifully. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
2016-12-18 16:56 GMT+03:00 Niall Douglas <s_sourceforge@nedprod.com>: <...>
Antony makes the valid point that on Windows there are race problems with the DbgHelp library, in fact not only is it not async-unsafe, it's also thread-unsafe.
He doesn't use DbgHelp in the Windows backend though, he uses Dbgeng.h. This is not the same thing, I think?
Woohoo!
That is amazing news, and congrats to Antony for getting Dbgeng working. Last month when I prereviewed Stacktrace I mentioned that DbgHelp was a steaming pile of poo and that I had had much more reliable experience with the thoroughly superior Dbgeng. Antony asked for some example code because Dbgeng is barely documented, I no longer had access to the code I wrote many years ago which used it. I tried to cobble something together and I made some progress over what Antony had, but I ran out of time due to needing to mind the recent new baby. I'm not actually sure what he changed from what I sent him, mine and his look very close, I must have missed something very small.
I've took Nialls code that uses COM as a base and mixed it with the code I already had that collects stacktrace without COM and DbgHelp. Some hammering produced a class that *probably* async-safely collects stacktrace and can output it using async-unsafe COM. -- Best regards, Antony Polukhin
On Sat, Dec 17, 2016 at 9:17 PM, Niall Douglas <s_sourceforge@nedprod.com> wrote:
Specifically:
1. Should Stacktrace be async safe so it can be used to print a stacktrace from a signal/exception handler?
That depends on whether any backend supports decoding the stacktrace in signal handlers. Note that if we also want to load debug symbols from a separate file while doing this, the task seems to become impossible.
2. Is its lack of thread safety on Windows a problem?
Yes, I think that is a serious problem. Though I'm not sure if Boost.Stacktrace can do something about it, except to introduce a global lock (sigh).
On 18 Dec 2016 at 0:29, Andrey Semashev wrote:
1. Should Stacktrace be async safe so it can be used to print a stacktrace from a signal/exception handler?
That depends on whether any backend supports decoding the stacktrace in signal handlers. Note that if we also want to load debug symbols from a separate file while doing this, the task seems to become impossible.
If you preload the symbols and prebind the COM functions you'll use in the async unsafe handler, I've found decoding the stacktrace to be reliable in the past on Windows. On POSIX launching a subprocess and parsing its output is async safe. I have to admit I've always launched addr2line or llvm-symbolizer. It's nasty and slow but it works.
2. Is its lack of thread safety on Windows a problem?
Yes, I think that is a serious problem. Though I'm not sure if Boost.Stacktrace can do something about it, except to introduce a global lock (sigh).
Thankfully as I just recently discovered Stacktrace is using Dbgeng not Dbghelp. That means it's threadsafe (yay!). Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
2016-12-18 0:29 GMT+03:00 Andrey Semashev <andrey.semashev@gmail.com>:
On Sat, Dec 17, 2016 at 9:17 PM, Niall Douglas <s_sourceforge@nedprod.com> wrote:
Specifically:
1. Should Stacktrace be async safe so it can be used to print a stacktrace from a signal/exception handler?
That depends on whether any backend supports decoding the stacktrace in signal handlers. Note that if we also want to load debug symbols from a separate file while doing this, the task seems to become impossible.
Spawning a new subprocess in signal handler (or at the beginning of the program) would be OK for you? If I'll came up with an async_safe_print(stacktrace()); function that may fix the broken example in docs, provide much more motivation for having non allocating stacktrace class.
2. Is its lack of thread safety on Windows a problem?
Yes, I think that is a serious problem. Though I'm not sure if Boost.Stacktrace can do something about it, except to introduce a global lock (sigh).
It is thread safe now (was not thread safe 2 months ago). -- Best regards, Antony Polukhin
On 12/18/16 20:15, Antony Polukhin wrote:
2016-12-18 0:29 GMT+03:00 Andrey Semashev <andrey.semashev@gmail.com>:
On Sat, Dec 17, 2016 at 9:17 PM, Niall Douglas <s_sourceforge@nedprod.com> wrote:
Specifically:
1. Should Stacktrace be async safe so it can be used to print a stacktrace from a signal/exception handler?
That depends on whether any backend supports decoding the stacktrace in signal handlers. Note that if we also want to load debug symbols from a separate file while doing this, the task seems to become impossible.
Spawning a new subprocess in signal handler (or at the beginning of the program) would be OK for you? If I'll came up with an async_safe_print(stacktrace()); function that may fix the broken example in docs, provide much more motivation for having non allocating stacktrace class.
If that is only possible by forking a process then I'm not sure that accomplishes much. I mean, if there is a non-signal-safe formatting function for stacktraces, there's no problem to call `fork()` yourself in the signal handler, use that formatting function in the child process, and pipe its result back to the main process. I guess, a helper function that does this would be useful, just make sure to explicitly document that it forks. Also, the non-forking formatting implementation is still needed for all cases other than signal handlers.
2016-12-18 21:37 GMT+03:00 Andrey Semashev <andrey.semashev@gmail.com>:
On 12/18/16 20:15, Antony Polukhin wrote:
2016-12-18 0:29 GMT+03:00 Andrey Semashev <andrey.semashev@gmail.com>:
On Sat, Dec 17, 2016 at 9:17 PM, Niall Douglas <s_sourceforge@nedprod.com> wrote:
Specifically:
1. Should Stacktrace be async safe so it can be used to print a stacktrace from a signal/exception handler?
That depends on whether any backend supports decoding the stacktrace in signal handlers. Note that if we also want to load debug symbols from a separate file while doing this, the task seems to become impossible.
Spawning a new subprocess in signal handler (or at the beginning of the program) would be OK for you? If I'll came up with an async_safe_print(stacktrace()); function that may fix the broken example in docs, provide much more motivation for having non allocating stacktrace class.
If that is only possible by forking a process then I'm not sure that accomplishes much. I mean, if there is a non-signal-safe formatting function for stacktraces, there's no problem to call `fork()` yourself in the signal handler, use that formatting function in the child process, and pipe its result back to the main process. I guess, a helper function that does this would be useful, just make sure to explicitly document that it forks. Also, the non-forking formatting implementation is still needed for all cases other than signal handlers.
It's not that simple. After a fork() you are still in the async-signal-processing mode. You have to call one of the exec* functions to safely print the backtrace. Windows has no fork(). Looks like the only way to do that safely on Windows, is to create a process before the signal, make a pipe and wait for data on a pipe. As soon as the signal appears - dump data into the pipe and die. The child process have to output the content... it seems to me that such out-of-the-box functionality won't make all the users happy. They would definitely want to tune multiple things that I'm failing to predict. -- Best regards, Antony Polukhin
On 19 Dec 2016 at 1:26, Antony Polukhin wrote:
Windows has no fork(). Looks like the only way to do that safely on Windows, is to create a process before the signal, make a pipe and wait for data on a pipe. As soon as the signal appears - dump data into the pipe and die. The child process have to output the content... it seems to me that such out-of-the-box functionality won't make all the users happy. They would definitely want to tune multiple things that I'm failing to predict.
The NT kernel has a fork() implementation: RtlCloneUserProcess() You should note that much Win32 code is highly unreliable after being forked, only Nt*() calls are reliable but if you called NtTerminateProcess() on the clone when you're done so no Win32 code executes it should work. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 12/19/16 01:26, Antony Polukhin wrote:
2016-12-18 21:37 GMT+03:00 Andrey Semashev <andrey.semashev@gmail.com>:
On 12/18/16 20:15, Antony Polukhin wrote:
2016-12-18 0:29 GMT+03:00 Andrey Semashev <andrey.semashev@gmail.com>:
On Sat, Dec 17, 2016 at 9:17 PM, Niall Douglas <s_sourceforge@nedprod.com> wrote:
Specifically:
1. Should Stacktrace be async safe so it can be used to print a stacktrace from a signal/exception handler?
That depends on whether any backend supports decoding the stacktrace in signal handlers. Note that if we also want to load debug symbols from a separate file while doing this, the task seems to become impossible.
Spawning a new subprocess in signal handler (or at the beginning of the program) would be OK for you? If I'll came up with an async_safe_print(stacktrace()); function that may fix the broken example in docs, provide much more motivation for having non allocating stacktrace class.
If that is only possible by forking a process then I'm not sure that accomplishes much. I mean, if there is a non-signal-safe formatting function for stacktraces, there's no problem to call `fork()` yourself in the signal handler, use that formatting function in the child process, and pipe its result back to the main process. I guess, a helper function that does this would be useful, just make sure to explicitly document that it forks. Also, the non-forking formatting implementation is still needed for all cases other than signal handlers.
It's not that simple. After a fork() you are still in the async-signal-processing mode. You have to call one of the exec* functions to safely print the backtrace.
Ah, right.
Windows has no fork(). Looks like the only way to do that safely on Windows, is to create a process before the signal, make a pipe and wait for data on a pipe. As soon as the signal appears - dump data into the pipe and die. The child process have to output the content... it seems to me that such out-of-the-box functionality won't make all the users happy. They would definitely want to tune multiple things that I'm failing to predict.
Yes, if the only way to do that is fork+exec then that is not usable for me, not as the default implementation of the stacktrace decoding algorithm. If such functionality is provided, it should be strictly optional and disabled by default (i.e. on Windows don't start the helper process unless the user explicitly requests that in runtime). Let the decoding algorithm itself be not-signal-safe but in-process.
Andrey Semashev wrote:
Yes, if the only way to do that is fork+exec then that is not usable for me, not as the default implementation of the stacktrace decoding algorithm. If such functionality is provided, it should be strictly optional and disabled by default (i.e. on Windows don't start the helper process unless the user explicitly requests that in runtime). Let the decoding algorithm itself be not-signal-safe but in-process.
Why is spawning a process to do the decoding unacceptable? I'm sorry if you already explained this in a previous message.
On 12/19/16 17:07, Peter Dimov wrote:
Andrey Semashev wrote:
Yes, if the only way to do that is fork+exec then that is not usable for me, not as the default implementation of the stacktrace decoding algorithm. If such functionality is provided, it should be strictly optional and disabled by default (i.e. on Windows don't start the helper process unless the user explicitly requests that in runtime). Let the decoding algorithm itself be not-signal-safe but in-process.
Why is spawning a process to do the decoding unacceptable?
I pointed out this in my review. One reason is because of security considerations. If the library executes a foreign executable with path lookup, it is possible to put a malicious executable with the parent process permissions. The parent process can be a daemon, running with elevated permissions, so this could potentially be devastating. This is more so a problem if the user of Boost.Stacktrace is not suspecting that the library executes a process under the hood. In general, as an application developer, I always want to be in control when my application forks and what other processes it executes. If that happens, I want that to happen in very few and specific places of my code. In those places I can carefully setup environment and permissions for the process to run, as well as specify the full path to the executable, to reduce the possibility of a breach.
On 12/19/16 18:58, Andrey Semashev wrote:
On 12/19/16 17:07, Peter Dimov wrote:
Andrey Semashev wrote:
Yes, if the only way to do that is fork+exec then that is not usable for me, not as the default implementation of the stacktrace decoding algorithm. If such functionality is provided, it should be strictly optional and disabled by default (i.e. on Windows don't start the helper process unless the user explicitly requests that in runtime). Let the decoding algorithm itself be not-signal-safe but in-process.
Why is spawning a process to do the decoding unacceptable?
I pointed out this in my review. One reason is because of security considerations. If the library executes a foreign executable with path lookup, it is possible to put a malicious executable with the parent
...it is possible to *run* a malicious executable...
process permissions. The parent process can be a daemon, running with elevated permissions, so this could potentially be devastating. This is more so a problem if the user of Boost.Stacktrace is not suspecting that the library executes a process under the hood.
In general, as an application developer, I always want to be in control when my application forks and what other processes it executes. If that happens, I want that to happen in very few and specific places of my code. In those places I can carefully setup environment and permissions for the process to run, as well as specify the full path to the executable, to reduce the possibility of a breach.
Andrey Semashev wrote:
Why is spawning a process to do the decoding unacceptable?
I pointed out this in my review. One reason is because of security considerations. If the library executes a foreign executable with path lookup, it is possible to put a malicious executable with the parent process permissions. The parent process can be a daemon, running with elevated permissions, so this could potentially be devastating. This is more so a problem if the user of Boost.Stacktrace is not suspecting that the library executes a process under the hood.
OK... but can you give some specific examples, on Linux and Windows, as to how such an attack would work? Complete with actual directory and file names.
On 12/19/16 19:09, Peter Dimov wrote:
Andrey Semashev wrote:
Why is spawning a process to do the decoding unacceptable?
I pointed out this in my review. One reason is because of security considerations. If the library executes a foreign executable with path lookup, it is possible to put a malicious executable with the parent process permissions. The parent process can be a daemon, running with elevated permissions, so this could potentially be devastating. This is more so a problem if the user of Boost.Stacktrace is not suspecting that the library executes a process under the hood.
OK... but can you give some specific examples, on Linux and Windows, as to how such an attack would work? Complete with actual directory and file names.
Suppose someone placed $HOME/bin/addr2line of the following content: #!/bin/bash if [ $UID -eq 0 ] then echo Hello, root! else /usr/bin/addr2line "$@" fi And then there is your application that is linked with Boost.Stacktrace and at some point it executes `frame::source_file()` or `frame::source_line()` (for example, while processing an exception with attached stacktrace). It would work seemingly fine when executed under your user, but it will execute malicious code when executed under `sudo`. And there may be valid reasons to want to run your program under `sudo` at times. I'm less knowledgeable about Windows innerworkings, but I think something similar is possible there as well. Also, I'm not a security expert, so maybe there are other exploits. Bottom line: I just don't want a potential security issue where none is required. No fork+exec = no problem.
Andrey Semashev wrote:
Suppose someone placed $HOME/bin/addr2line of the following content:
That's only going to work if $HOME/bin is on the path before /usr/bin, which seems not very prudent from a security perspective. The user can type 'addr2line' (or anything else in /usr/bin such as 'ls') himself, after all. Hello root. So it's not that easy. In general, if the attacker has write access to a directory in $PATH, things are already not very secure. This also applies to Windows, because $PATH is searched for DLLs, although it has very low priority, so you need to find a DLL that the program attempts to load but isn't present in the system directories. That said, Stacktrace should probably not use $PATH at all for locating its helper process. On POSIX, execvp is not async safe anyway, so using /usr/bin/addr2line directly may be better. And on Windows, the helper would generally be installed along with the program - it won't be a system utility such as addr2line. In which case it would be spawned from the directory of the program using a full path. And if the attacker has write access to the directory of the program, there are many other things he can do, such as replacing the program itself, or adding a rogue .DLL there.
On 12/19/16 20:04, Peter Dimov wrote:
Andrey Semashev wrote:
Suppose someone placed $HOME/bin/addr2line of the following content:
That's only going to work if $HOME/bin is on the path before /usr/bin, which seems not very prudent from a security perspective. The user can type 'addr2line' (or anything else in /usr/bin such as 'ls') himself, after all. Hello root. So it's not that easy.
Well, $HOME/bin is before /usr/bin on my system. And I suppose, it is expected to be looked up first, precisely to be able to override system binaries with your own. Similar to how /usr/local/bin is expected to be looked up before /usr/bin.
In general, if the attacker has write access to a directory in $PATH, things are already not very secure. This also applies to Windows, because $PATH is searched for DLLs, although it has very low priority, so you need to find a DLL that the program attempts to load but isn't present in the system directories.
I think, a process could also modify the system-wide PATH variable, at least on some Windows versions. IIRC, some installers (Visual Studio? Intel compiler? I can't remember) did that at some point. I don't know if that's the case now.
That said, Stacktrace should probably not use $PATH at all for locating its helper process. On POSIX, execvp is not async safe anyway, so using /usr/bin/addr2line directly may be better.
That is somewhat better. It does exclude PATH from the lookup, that's for sure. But consider that the application could use a custom build of Boost and be installed in /opt. What if the application also wants to use a custom addr2line from its /opt/my_app/bin? It's possible to patch the path in Boost.Stacktrace, but that adds some maintenance burden of having to maintain the patch. A config macro perhaps? A runtime option? The full path does not solve the problem of setting up environment for the process though. E.g. I don't have a way to drop privileges of root before executing addr2line. Environment setup is also problematic - one has to configure and restore environment before and after every call of Boost.Stacktrace that executes a process.
And on Windows, the helper would generally be installed along with the program - it won't be a system utility such as addr2line. In which case it would be spawned from the directory of the program using a full path.
That imposes a requirement on the user's application layout. I'd say, it should be configurable as well. It should probably be a runtime configuration in order to support portable applications (i.e. those not requiring installation in a particular directory). Security issues aside, there is also a matter of controlling the child process. What if it hangs? Or crashes? How does it affect the master application? What if the master crashes (i.e. how can you ensure you don't leave a zombie process)? What will you do if you exhaust the limit of running processes in the system (i.e. if the app itself is not able to spawn its processes because of a bunch of running addr2line)? Then, there are performance implications of running a process for each function call. These kind of questions are difficult to answer in a generic library, which is also not focused on process management. I'm really not convinced that I have to execute a process to be able to decode a stacktrace. That may be the only way to do that from a signal handler, but in a general context that is really not the expected behavior of the library. IMO, the signal handler case is special enough to warrant a special API for that, and it may involve forking and executing a process. That API should likely include the necessary means to configure the process before `exec`ing the new process, including the full path, permissions, a running timeout and what not. For other cases an in-process solution should be available.
Andrey Semashev wrote:
Well, $HOME/bin is before /usr/bin on my system.
This means that if an attacker gains access to $HOME he can override /usr/bin/anything, unless you watch $HOME/bin like a hawk.
I think, a process could also modify the system-wide PATH variable, at least on some Windows versions. IIRC, some installers (Visual Studio? Intel compiler? I can't remember) did that at some point. I don't know if that's the case now.
You need root for that.
That is somewhat better. It does exclude PATH from the lookup, that's for sure. But consider that the application could use a custom build of Boost and be installed in /opt. What if the application also wants to use a custom addr2line from its /opt/my_app/bin?
It wouldn't be hard for Stacktrace to provide a function to set the actual path to addr2line, if we agree that spawning a helper process is acceptable in principle.
The full path does not solve the problem of setting up environment for the process though. E.g. I don't have a way to drop privileges of root before executing addr2line.
Dropping privileges could perhaps work for addr2line which takes a filename rather than a PID but it wouldn't work for utilities that are PID-based such as atos. If the program is root, the utility would also need root in order to inspect its address space and locate the module based on the address. I think.
Security issues aside, there is also a matter of controlling the child process. What if it hangs? Or crashes? How does it affect the master application? What if the master crashes (i.e. how can you ensure you don't leave a zombie process)? What will you do if you exhaust the limit of running processes in the system (i.e. if the app itself is not able to spawn its processes because of a bunch of running addr2line)?
Very similar questions can be posed towards the current Windows implementation. There isn't that much of a difference between using a helper process and doing the same work in-process. In fact, if your process has just crashed, it's better to do the work from another process, because its memory would be intact. (COM by the way isolates the client from whether the server is in-process or not, so the debug engine can well spawn a process under the hood, and we wouldn't be any wiser. Nor should we be.)
Then, there are performance implications of running a process for each function call.
That's what I tried to address with my API suggestions.
On 12/19/16 21:42, Peter Dimov wrote:
Andrey Semashev wrote:
I think, a process could also modify the system-wide PATH variable, at least on some Windows versions. IIRC, some installers (Visual Studio? Intel compiler? I can't remember) did that at some point. I don't know if that's the case now.
You need root for that.
But installers always execute as root on Windows, don't they? (I believe, the term is "with elevated privileges") The idea is that the user installed something that modified system PATH so that it can later be used to inject a malicious process into your application. Also, disabling UAC is a popular practice, I think. :p
The full path does not solve the problem of setting up environment for the process though. E.g. I don't have a way to drop privileges of root before executing addr2line.
Dropping privileges could perhaps work for addr2line which takes a filename rather than a PID but it wouldn't work for utilities that are PID-based such as atos. If the program is root, the utility would also need root in order to inspect its address space and locate the module based on the address. I think.
Aren't there capabilities that could be granted to the child process? If not, that is unfortunate. Not just because dropping privileges won't work on all platforms, but also because the user will have to depend on the library implementation details to know if it can work. I suppose, this can be somewhat mitigated in the backend reported if it is capable to decode the stacktrace with reduced privileges.
Security issues aside, there is also a matter of controlling the child process. What if it hangs? Or crashes? How does it affect the master application? What if the master crashes (i.e. how can you ensure you don't leave a zombie process)? What will you do if you exhaust the limit of running processes in the system (i.e. if the app itself is not able to spawn its processes because of a bunch of running addr2line)?
Very similar questions can be posed towards the current Windows implementation. There isn't that much of a difference between using a helper process and doing the same work in-process.
In case of COM, handling the out-of-process COM objects is Windows' job. The application itself need not handle surrogate processes running those objects, nor does it have to specify path to those process executables or the COM object's dll. You are quite right in that COM hides most of that complexity, and that answers many of the questions above.
In fact, if your process has just crashed, it's better to do the work from another process, because its memory would be intact.
That is true, but I reiterate that handling a crash (or a signal) is special enough to warrant a special solution. At this point performance doesn't really matter, and the host process is pretty much doomed anyway, so the issues affecting its survival are off the table. By the way, since we're discussing crash handling, I have to say that I'm currently using gdb for this purpose. The signal handler in my process does fork and exec gdb, which saves the backtraces of all threads of the application to a file. If compared to Boost.Stacktrace, this approach has significant benefits, such as that gdb loads debug symbols, if present on the system, and that it dumps all threads instead of just one. Of course, gdb is much heavier than Boost.Stacktrace, but still this is something that I'd like to be achievable with the library.
Then, there are performance implications of running a process for each function call.
That's what I tried to address with my API suggestions.
I don't think that fully solves the performance issue because it still executes a process per stacktrace entry. In my example with gdb above, all decoding is done in one process, and I have to wonder how Boost.Stacktrace doing the same amount of work would compare to that.
On 19 Dec 2016 at 23:14, Andrey Semashev wrote:
I think, a process could also modify the system-wide PATH variable, at least on some Windows versions. IIRC, some installers (Visual Studio? Intel compiler? I can't remember) did that at some point. I don't know if that's the case now.
You need root for that.
But installers always execute as root on Windows, don't they? (I believe, the term is "with elevated privileges") The idea is that the user installed something that modified system PATH so that it can later be used to inject a malicious process into your application.
I think we're getting onto non-Stacktrace review topics once more. Again, I have no problem with discussion being moved to a thread not entitled "Stacktrace review".
In fact, if your process has just crashed, it's better to do the work from another process, because its memory would be intact.
That is true, but I reiterate that handling a crash (or a signal) is special enough to warrant a special solution. At this point performance doesn't really matter, and the host process is pretty much doomed anyway, so the issues affecting its survival are off the table.
There are long established, high quality open source crash reporter libraries and why anyone would roll their own solution instead of just using one of those would be beyond me. I've used CrashRpt in years gone by and it worked great. On Windows for years you've been able to manually generate a dump file by calling MinidumpWriteDump() but the big gotcha is you need to launch a separate process to call it on your crashed process. One evil way to achieve this is via Powershell bundled with Windows since Win7 which can dump your process for you. Back to Stacktrace. I think I am beginning to see a consensus emerge: that Stacktrace should be able to serialise the stacktrace of an about to be terminated process to disc in an async-safe way in a format which other tooling such as addr2line or llvm-symbolizer can consume in combination with the debug symbols to produce an accurate source and line number stacktrace. Stacktrace does not need to parse the stacktrace in an async-safe way. Is this consensus acceptable to the Boost community? If so, Stacktrace would need to figure out the load address of each DLL/SO/EXE after ASLR and convert the stacktrace pointers into offsets before writing them in text to a file without using any CRT functions nor malloc for llvm-symbolizer to consume. It's a big ask from Antony though doable, this is why I am asking if this is what the Boost community wants as a precondition, or is it too much of an ask, or not enough? Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 12/20/2016 09:11 AM, Niall Douglas wrote:
... Stacktrace should be able to serialise the stacktrace of an about to be terminated process to disc in an async-safe way in a format which other tooling such as addr2line or llvm-symbolizer can consume in combination with the debug symbols to produce an accurate source and line number stacktrace. Stacktrace does not need to parse the stacktrace in an async-safe way.
The need to have and to use an additional tool to actually see the stack seems like a considerable complication. To me personally printing out the stack-trace to the log was a quick and light-weight (but often sufficient) clue where to start my investigation. How is the suggested procedure better compared to the established on Linux -- retrieve the core of the failed app, run it with gdb, look at the debug symbols, stack-trace, etc.? I suspect a similar/same mechanism to already exist on Windows as well, right?
Think stack trace of release builds where the debug symbols have been stripped out. This is common and the default in the Microsoft world. It allows for the smallest, fastest and more secure execution while at the same time allowing for access to the stacktrace. It is accomplished by storing the debug symbols outside of the artifact, in a PDB file. A stack trace is no longer an collection of strings but rather a collection of relative virtual addresses. This information as well as artifact load address can be used with the PDB to reconstruct that stack trace. Microsoft have symbol servers which not only contain the artifact, dll or exe, but also the PDB file. PDB file can vary in size to ones that are small that provide method and line numbers to verbose that holds a lot more debug information. Since PE, portable executable, file format is mostly standard between Windows and Linux environments couldn't Linux support this functionality via using dwarf debug symbols while Microsoft uses PDB. gcc: -gdwarf ... All that having been said, I do believe stacktrace should be included in its present form. I just wish the author would acknowledge this use case that is the value of stack traces for release builds and is open to future revisions towards it. I also hope the community doesn't dismiss this library because it doesn't do everything but would look at it as a valuable first step. Okay Okay 10 steps. ________________________________ From: Boost <boost-bounces@lists.boost.org> on behalf of Vladimir Batov <Vladimir.Batov@constrainttec.com> Sent: Monday, December 19, 2016 6:42:11 PM To: boost@lists.boost.org Subject: Re: [boost] [Stacktrace] review, please stop discussing non-Stacktrace issues On 12/20/2016 09:11 AM, Niall Douglas wrote:
... Stacktrace should be able to serialise the stacktrace of an about to be terminated process to disc in an async-safe way in a format which other tooling such as addr2line or llvm-symbolizer can consume in combination with the debug symbols to produce an accurate source and line number stacktrace. Stacktrace does not need to parse the stacktrace in an async-safe way.
The need to have and to use an additional tool to actually see the stack seems like a considerable complication. To me personally printing out the stack-trace to the log was a quick and light-weight (but often sufficient) clue where to start my investigation. How is the suggested procedure better compared to the established on Linux -- retrieve the core of the failed app, run it with gdb, look at the debug symbols, stack-trace, etc.? I suspect a similar/same mechanism to already exist on Windows as well, right? _______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On 12/20/16 01:11, Niall Douglas wrote:
Back to Stacktrace. I think I am beginning to see a consensus emerge: that Stacktrace should be able to serialise the stacktrace of an about to be terminated process to disc in an async-safe way in a format which other tooling such as addr2line or llvm-symbolizer can consume in combination with the debug symbols to produce an accurate source and line number stacktrace. Stacktrace does not need to parse the stacktrace in an async-safe way.
Is this consensus acceptable to the Boost community?
Sorry for repeating myself, but in my review and several replies in the discussion I suggested that the library should provide two interfaces for decoding a stacktrace. One for use in normal context, outside signal handler, and the other for signal handlers. The latter has to be signal-safe and can fork the process and reuse the former interface internally. It could also be used to save the decoded stacktrace to a file. This approach would allow to obtain decoded stacktrace in every context of a program. What you describe is quite different. It doesn't allow to decode the stacktrace in a live process, but provides a way to obtain it postmortem. I'm not sure how useful that would be, given that it's already possible to produce a core dump and examine that. In my practice though I prefer decoded stacktraces to core dumps because it doesn't require me to have the exact same binaries on my system as those on the client's. That is most likely not limited with binaries shipped with my application. If I was developing a crash handler with Stacktrace, I would prefer it produce a decoded backtrace instead of intermediate output that needs further resolving using the involved binaries.
Andrey Semashev wrote:
For other cases an in-process solution should be available.
I don't disagree that a built-in solution would be nice, but addr2line is basically a wrapper around libbfd, which is GPL.
After this discussion, I can suggest the following changes to the library: - the part that captures a stack trace and the part that resolves it to function/file/line should be separate, and usable without each other. - ideally, both ought to be async safe, or as async safe as possible on the target architecture. - there should be a low-level API that is async safe, and a high level API that is not necessarily such. - macOS should be supported. - the library should not #include <windows.h>. The part of the Windows backend that needs <windows.h> should be a separately-compiled library. What I can suggest as a low-level API is: // capture typedef void (*stacktrace_frame)(); // a bit on the pedantic side, but why not size_t stacktrace_capture( stacktrace_frame buffer[], size_t size ); // resolution void stracktrace_resolve_frame( stacktrace_frame address, char function[], size_t fnsize, char file[], size_t filesize, size_t & line ); typedef void (*stacktrace_resolve_callback)( stacktrace_frame address, char const * function, char const * file, size_t line ); void stacktrace_resolve( stacktrace_frame const[] trace, size_t size, char function[], size_t fnsize, char file[], size_t filesize, stacktrace_resolve_callback callback ); On Windows, I see stacktrace_capture as header-only and async-safe, and stacktrace_resolve as separately compiled and maybe-mostly-safe, as today. On Mac OS X, stacktrace_capture can use ::backtrace and stacktrace_resolve can use the atos utility instead of addr2line. https://developer.apple.com/legacy/library/documentation/Darwin/Reference/Ma...
2016-12-18 20:26 GMT+03:00 Peter Dimov <lists@pdimov.com>:
After this discussion, I can suggest the following changes to the library:
- the part that captures a stack trace and the part that resolves it to function/file/line should be separate, and usable without each other.
Already.
- ideally, both ought to be async safe, or as async safe as possible on the target architecture.
I'm trying to unify behavior rather than provoking people to write unportable code. Both must be as async safe, if *all* the platforms allow to do that.
- there should be a low-level API that is async safe, and a high level API that is not necessarily such.
There's no way to get function location in async safe way onWindows.
- macOS should be supported.
OK.
- the library should not #include <windows.h>. The part of the Windows backend that needs <windows.h> should be a separately-compiled library.
I'd rather take the boost/winapi path.
What I can suggest as a low-level API is:
// capture
typedef void (*stacktrace_frame)(); // a bit on the pedantic side, but why not
size_t stacktrace_capture( stacktrace_frame buffer[], size_t size );
That's doable. But how are you going to use it?
// resolution
void stracktrace_resolve_frame( stacktrace_frame address, char function[], size_t fnsize, char file[], size_t filesize, size_t & line );
What if the user wants only the function name?
typedef void (*stacktrace_resolve_callback)( stacktrace_frame address, char const * function, char const * file, size_t line );
void stacktrace_resolve( stacktrace_frame const[] trace, size_t size, char function[], size_t fnsize, char file[], size_t filesize, stacktrace_resolve_callback callback );
On Windows, I see stacktrace_capture as header-only and async-safe, and stacktrace_resolve as separately compiled and maybe-mostly-safe, as today.
As today - it is unsafe. We may assume the safety, but there's no guarantee. Moreover, I'm pretty sure that it is unsafe.
On Mac OS X, stacktrace_capture can use ::backtrace and stacktrace_resolve can use the atos utility instead of addr2line.
glibc ::backtrace() implementation is not async-signal-safe. So probably I will need to discover another way of doing that thing.
https://developer.apple.com/legacy/library/documentation/Darwin/Reference/Ma...
-- Best regards, Antony Polukhin
Antony Polukhin wrote:
2016-12-18 20:26 GMT+03:00 Peter Dimov <lists@pdimov.com>:
What I can suggest as a low-level API is:
// capture
typedef void (*stacktrace_frame)(); // a bit on the pedantic side, but why not
size_t stacktrace_capture( stacktrace_frame buffer[], size_t size );
That's doable. But how are you going to use it?
The majority of users will just construct a stacktrace high-level object, as today, which will use this in the constructor to capture. But people who'd rather go lower level can use this directly, and people who'd rather go even lower can capture a stack trace via other means, without depending on Stacktrace for the capture part. throw_exception, for instance, may reimplement the capture part, because it doesn't need to resolve.
// resolution
void stracktrace_resolve_frame( stacktrace_frame address, char function[], size_t fnsize, char file[], size_t filesize, size_t & line );
What if the user wants only the function name?
You can check for filesize < 2. Or for fnsize < 2.
As today - it is unsafe. We may assume the safety, but there's no guarantee. Moreover, I'm pretty sure that it is unsafe.
There are different levels of unsafety in practice, even though purists may claim that undefined is undefined and that's that. There's a whole spectrum from "almost always works" to "rarely works, if ever". If we're going to play the pedantry game, everything after a crash is already undefined behavior anyway. In practice, most important is to avoid the C/C++ runtime not just because this or that is not documented async-safe, but because a crash may be a symptom of random memory damage. Holding the loader lock is pretty much a guaranteed loss, apart from that, I'd estimate our chances as above average.
I'd rather take the boost/winapi path.
That's an option but, frankly, a separately compiled library is not that problematic, as long as you avoid autolinking it unless needed. That is, a simple capture should not attempt to link it. This is why I thought separating the capture and resolution into two headers worthwhile.
On 19/12/2016 06:26, Peter Dimov wrote:
- the part that captures a stack trace and the part that resolves it to function/file/line should be separate, and usable without each other.
- ideally, both ought to be async safe, or as async safe as possible on the target architecture.
Agreed for capturing. But I don't think there should be any requirement to make resolving a trace be async-safe. There's rarely going to be anything useful you could do with such a trace that would also be async-safe; it'd be better to require that this be done in regular context instead. And I doubt all (any?) platforms would be capable of doing so, anyway.
Gavin Lambert wrote:
But I don't think there should be any requirement to make resolving a trace be async-safe. There's rarely going to be anything useful you could do with such a trace that would also be async-safe; it'd be better to require that this be done in regular context instead.
::write is async-safe. You can write it to a file or stderr or a pipe or a socket.
On 19/12/2016 11:57, Peter Dimov wrote:
::write is async-safe. You can write it to a file or stderr or a pipe or a socket.
Yes, but most application code would want to do that via a common logging library or something which probably isn't async-safe. I'm not saying it couldn't be done, just that it increases the risk that application writers will do it wrong, like they've been getting signal handlers wrong for many years as it is.
Gavin Lambert wrote:
On 19/12/2016 11:57, Peter Dimov wrote:
::write is async-safe. You can write it to a file or stderr or a pipe or a socket.
Yes, but most application code would want to do that via a common logging library or something which probably isn't async-safe.
I'm not saying it couldn't be done, just that it increases the risk that application writers will do it wrong, like they've been getting signal handlers wrong for many years as it is.
Yeah, well, some/many will get it wrong, but how does it increase the risk? Currently the risk stands at 100% getting it wrong.
In
std::string name = f.name(); if (!name.empty()) { os << name; } else { os << f.address(); }
const std::size_t source_line = f.source_line(); if (source_line) { os << " at " << f.source_file() << ':' << source_line; }
which is perfectly reasonable code given the current interface, addr2line is called three times instead of just one. And in operator<< for a stacktrace of depth N, addr2line will be called 3*N times instead of one.
The same problem exists in the Windows backend, where the COM machinery is initialized/deinitialized on each call, and GetLineByOffset is called twice, each time discarding half of the result.
On 17 Dec 2016 at 17:31, Peter Dimov wrote:
3. Please could people actually review Stacktrace's implementation code instead of inferring implementation from what someone else said here about something they half read in the tutorial. Specifically, is its API design solid? Is the use of std::string in its public API acceptable?
There's nothing wrong with using std::string in principle, but it's obviously not async safe. The operations however that return std::strings are also not async safe at the moment, so it's not the std::string return value that's the problem.
The documentation has an example of printing a stack trace from a SIGSEGV handler, which is undefined behavior as it stands. So it's not clear to me whether the library is supposed to be useful - in its decoding/printing part, not just in its capture part - in signal handlers.
Thanks Peter, you did a great job covering my own concerns here. I can tell you it is entirely possible to write a stacktrace printer which is async safe on POSIX. I've done it before. The Windows case is much harder to achieve, you basically need to bring in non system libraries because DbgHelp is crap. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On Thu, Dec 15, 2016 at 8:59 AM, Robert Ramey <ramey@rrsd.com> wrote:
On 12/15/16 5:37 AM, Peter Dimov wrote:
Robert Ramey wrote:
Because the semantics of BOOST_THROW_EXCEPTION were changed and the
documentation updated to the new semantics.
BOOST_THROW_EXCEPTION did not exist before Boost.Exception, so no, it wasn't changed.
Sorry, my mistake.
It wasn't BOOST_THROW_EXCEPTION. It was boost::throw_exception whose semantics were changed in an unanticipated and surprising way. It seems to me that it might be that this was being proposed again. Maybe my concerns are overblown as those who got burned the first time by this policy have already eliminated dependency on boost::throw_exception.
We can't objectively discuss how unanticipated or surprising anyone thinks a code change is, but the change to boost::throw_exception (so many years ago) did not break existing code, the only observable difference is that any exception emitted by boost::throw_exception can now be caught as boost::exception. Emil
On 15/12/2016 13:53, Robert Ramey wrote:
I think that there has been a lot of confusion about what BOOST_THROW_EXCEPTION is suppose to do.
My understanding was that it was a macro intended to support the writing of portable code that could run on platforms which didn't support exceptions or where the user / author didn't want to use the exception mechanism so he could redefine the macro. Lot's of libraries used this idiom to decouple their libraries from the the selection of exception mechanism.
Shouldn't a macro with that intent be defined in core/no_exceptions_support.hpp instead? (There currently doesn't appear to be a throw macro there, presumably because boost::throw_exception was sufficient?)
With the acceptance of Boost.Exception, this BOOST_THROW_EXCEPTION was repurposed to call boost.exception. Obviously not what previous users of BOOST_THROW_EXCEPTION intended or expected. This made many libraries dependent on the new library boost.exception which had multiple repercussions - one of which is that boost.exception has now become a "core library" - not sure what that is supposed to mean.
It doesn't really look like (at least in current code) that BOOST_THROW_EXCEPTION has any function unless you want to capture the error_info automatically. Otherwise you can just always use boost::throw_exception instead. If you want to use BOOST_THROW_EXCEPTION without depending on Boost.Exception, it looks like you can define BOOST_EXCEPTION_DISABLE, although at the cost of losing the current_exception and error_info wrappers. This is independent of BOOST_NO_EXCEPTIONS, which controls whether exceptions are thrown at all.
On 16/12/2016 02:39, Peter Dimov wrote:
Gavin Lambert wrote:
Shouldn't a macro with that intent be defined in core/no_exceptions_support.hpp instead?
This file didn't exist at the time boost/throw_exception.hpp was written.
I know that; I'm just saying that perhaps adding such a macro there (simpler than throw_exception) might address Robert's concerns. I'm not personally worried about it though.
On Wed, Dec 14, 2016 at 2:42 PM, Nat Goodspeed <nat@lindenlab.com> wrote:
On Wed, Dec 14, 2016 at 3:43 AM, Niall Douglas <s_sourceforge@nedprod.com> wrote:
- Do you think the library should be accepted as a Boost library?
Yes I do. I would like to see the non-template base class, but that's not a showstopper: I would rather have this library without than not have it at all.
As I mentioned, I would like to see integration with Boost.Exception. That just seems like a natural next step.
I see further opportunities for integration with Boost.Context, Boost.Fiber...
I forgot to mention another potential enhancement of interest. This is a feature request post-acceptance. I would like to be able to: * serialize a stacktrace from a running program without available symbol files * deserialize that stacktrace on a different system that has symbol files for the original program * display symbolic output (function names, source files, line numbers) for the original stacktrace. For this usage, the serialization format could optimize for space rather than readability. I suspect that the prerequisites for that use case would vary by platform, but the functionality would be *very* useful.
2016-12-15 5:15 GMT+03:00 Nat Goodspeed <nat@lindenlab.com>:
On Wed, Dec 14, 2016 at 2:42 PM, Nat Goodspeed <nat@lindenlab.com> wrote: <...> I forgot to mention another potential enhancement of interest. This is a feature request post-acceptance.
I would like to be able to:
* serialize a stacktrace from a running program without available symbol files * deserialize that stacktrace on a different system that has symbol files for the original program
Probably you already could do those two bulletins http://apolukhin.github.io/stacktrace/boost_stacktrace/getting_started.html#...
* display symbolic output (function names, source files, line numbers) for the original stacktrace.
This is simple to do on POSIX and I will need further investigation on how to do it on Windows. If it's possible - I'll add the functionality after the review. -- Best regards, Antony Polukhin
2016-12-14 22:42 GMT+03:00 Nat Goodspeed <nat@lindenlab.com>: <...>
I do have a question about use of a template parameter to constrain the number of entries, rather than a runtime parameter. Presumably that's necessary to permit certain backend implementations to be async-signal-safe? (Whatever the rationale, a note in the documentation explaining it would be nice.)
Yes, this is for async-signal-safety + it gives an exception safety + it removes all the allocator related stuff from the stacktrace. I'll add a note.
My objection to that is that apparently, with the present design, if I want to write a function that accepts any specialization of basic_stacktrace, that must itself be a template function.
Yes, you're right. But in my head - you'll be using the same default stacktrace with the same template parameter all over the place. So it's mostly for detecting problems (you'll get link time error while attempting to link libraries that exchange stacktraces with different default parameters) and for interoperability with libraries, that may accept/produce stacktraces that differ from the current.
If I'm correct about the reason for using a template parameter, would it be reasonable to split the class into a non-template base class supporting all the methods, with a template subclass that provides the storage? The base class could store a (pointer, length) passed by the subclass constructor. Without having looked at the implementation, I expect that you wouldn't even need virtual-function dispatch: the only indirection would be through the data buffer pointer.
There is a detail::backend class, that could be easily transformed to such class... But I'd rather not, as it seems to be an overengineering.
That would even permit a subclass that provides dynamic storage, for async-signal-unsafe use.
Could you please give an example, where such class could be useful? Thank you for the review! -- Best regards, Antony Polukhin
On 15/12/2016 19:52, Antony Polukhin wrote:
2016-12-14 22:42 GMT+03:00 Nat Goodspeed <nat@lindenlab.com>:
My objection to that is that apparently, with the present design, if I want to write a function that accepts any specialization of basic_stacktrace, that must itself be a template function.
Yes, you're right. But in my head - you'll be using the same default stacktrace with the same template parameter all over the place. So it's mostly for detecting problems (you'll get link time error while attempting to link libraries that exchange stacktraces with different default parameters) and for interoperability with libraries, that may accept/produce stacktraces that differ from the current.
That makes me nervous, as I imagine that library code will want to transport stack traces around (imagine exception_ptr with stack traces, or future/thread/context/coroutine/fiber integration). But the max required depth can't be known to a library as it depends on the application code that uses the library (and indeed can get quite deep with some of those constructs, eg. when making a future ready fires a continuation that synchronously makes another future ready, and so on). Application-configurable macros could solve some of that but it still feels problematic to me.
participants (11)
-
Andrey Semashev -
Antony Polukhin -
Emil Dotchevski -
Gavin Lambert -
Jarrad Waterloo -
Nat Goodspeed -
Niall Douglas -
Paul A. Bristow -
Peter Dimov -
Robert Ramey -
Vladimir Batov