[Stacktrace] Second review begins today 17th Mar ends 26th Mar
I am pleased to announce the second review of the proposed Boost.Stacktrace library running from today until Sunday 26th. Stacktrace is an optionally header-only library providing multiple implementation backends. At its very simplest it lets you capture the stack backtrace for the calling thread and to print it to a std::ostream& of your choice. The basic_stacktrace<> class quacks like a STL container of frame object instances. The rest of the API pretty much follows STL design principles for the most part. Use is therefore unsurprising. You can find the documentation at http://apolukhin.github.io/stacktrace/index.html and the github repo at https://github.com/apolukhin/stacktrace. Review guidelines ================= This is a second review of this library with many changes implemented in response to feedback from the previous review. My previous review report can be found at http://lists.boost.org/boost-announce/2017/01/0486.php where I made the following conditions for acceptance: 1. That the documentation more clearly set out what Stacktrace does and especially does not do, and that the default facilities provided are those of the Minimum Viable Product described above i.e. no source file nor line number discovery, no external dependencies on libraries not always provided on the target platform, no invocation of child processes, no async safety during stacktrace parsing. Stacktrace ought to be pared back to the minimum viable product featureset with no external dependencies at all. 2. That suitable predefined macros opt in to additional functionality such as serialisation of offset translated backtraces, retrieval of source code path and line number etc. Enabling these may introduce dependencies on third party libraries not always present on a system such as libunwind, or on third party helper utilities such as addr2line. I would recommend that the APIs the use of which are dependent on such third party items are entirely compiled out of Stacktrace when the macro enabling them is not defined. 3. That the documentation describe ONLY the default minimum viable product functionality in its tutorial and quick start. All extra functionality enabled by macros is to be relocated into a separate documentation section away from the main documentation e.g. "Advanced/optional features". 4. That APIs made available only when a given macro has turned them on are documented as such in the API reference docs. Antony hasn't entirely met my conditions, hence this second review. He has instead implemented this changelog: * Reimplemented boost::stacktrace::basic_stacktrace class to accept allocators and to have a small size * Added outputting of the path to the binary object into the stacktrace, when source location is not available * Allowed to use `libbacktrace` - a defacto standard for GCC/CLANG backtracing that does not fork and has a permissive license * Optimized stacktrace and frames ostreaming * Removed backends and made the default tracing to have only platform basic functionality. Added macros to enable additional functionality. * Fixed (theoretically) MinGW compilation * Added functions for async-safe dumping into memory or descriptor * Changed signal handling example to be async-signal-safe * Allowed users to choose how many frames to skip during stack capture * Removed `boost::stacktrace::stacktrace` functions and other internals from printed stack traces * Updated docs: * examples now use Boost.Exception * removed all the references to "backend" * reduced macro count * "Build, Macros and Backends" section was rewritten and became "Configuration and Build" * added note that library is C++03 * added examples on storing traces into shared memory * updated all the human readable dumps It is up to the Boost community as to whether this revised library is now acceptable without any conditions, or with conditions. Reviews should be submitted to the developer list (boost_at_[hidden]), preferably with '[stacktrace]' in the subject. Or if you don't wish to for some reason or are not subscribed to the developer list you can send them privately to me at 's_sourceforge at nedprod dot com'. If so, please let me know whether or not you'd like your review to be forwarded to the list. For your review you may wish to consider the following questions: - What is your evaluation of the design? - What is your evaluation of the implementation? - What is your evaluation of the documentation? - What is your evaluation of the potential usefulness of the library? - Did you try to use the library? With what compiler? Did you have any problems? - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? - Are you knowledgeable about the problem domain? And finally, every review should attempt to answer this question: - Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion. Even if you do not wish to give a full review any technical comment regarding the library is welcome as part of the review period and will help me as the review manager decide whether the library should be accepted as a Boost library. Any questions about the use of the library are also welcome. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
AMDG The documentation could really use some editing. I'm seeing a lot of spelling and grammar errors. In Christ, Steven Watanabe
You can find the documentation at http://apolukhin.github.io/stacktrace/index.html and the github repo at https://github.com/apolukhin/stacktrace.
A few quick comments. 1. The library should try to not #include <windows.h> unless necessary. Constructing a stacktrace object and most of its operations besides operator<< / to_string do not need <windows.h> and it should be possible to avoid it. The functionality that needs <windows.h> needs to be defined in a separate header, for example <boost/stacktrace/output.hpp>. This allows use of the library in header-only mode while still isolating <windows.h> use into its own source file. For example, consider throw_with_trace in the documentation; the throw points, which in a header-only library will be in headers, do not need <windows.h> and should not drag it in. Only the catch point, which resides safely in its own source file, does. Incidentally, safe_dump_win.ipp includes <windows.h>, but does not need to. 2. There should exist a documented way to construct a stacktrace from an array of void const*. This is currently possible via from_dump, but it's not guaranteed to work; the format of the dump is technically an implementation detail of the library and may change. 3. detail::try_demangle is unnecessary, as it does the same thing as core::demangle. :-) Apart from that, looks like a substantial improvement. More comments and a vote possibly coming later.
You can find the documentation at http://apolukhin.github.io/stacktrace/index.html and the github repo at https://github.com/apolukhin/stacktrace.
Question: why does the library use boost::container::vector instead of std::vector?
2017-03-17 19:15 GMT+03:00 Peter Dimov via Boost <boost@lists.boost.org>:
You can find the documentation at http://apolukhin.github.io/stacktrace/index.html and the github repo at https://github.com/apolukhin/stacktrace.
A few quick comments.
1. The library should try to not #include <windows.h> unless necessary. Constructing a stacktrace object and most of its operations besides operator<< / to_string do not need <windows.h> and it should be possible to avoid it. The functionality that needs <windows.h> needs to be defined in a separate header, for example <boost/stacktrace/output.hpp>.
This allows use of the library in header-only mode while still isolating <windows.h> use into its own source file.
For example, consider throw_with_trace in the documentation; the throw points, which in a header-only library will be in headers, do not need <windows.h> and should not drag it in. Only the catch point, which resides safely in its own source file, does.
<windows.h> is a lesser evil. The huge problem is a COM header with a lot of things in global namespace and a bunch of unportable vendor specific extensions. One pain of death, I'm not going to rewrite all the COM macro-spaghetti code (but I'm open to pull requests). The only good solution to avoid global namespace polution - is not to use the library in header only mode on MSVC.
Incidentally, safe_dump_win.ipp includes <windows.h>, but does not need to.
Fixed in develop
2. There should exist a documented way to construct a stacktrace from an array of void const*. This is currently possible via from_dump, but it's not guaranteed to work; the format of the dump is technically an implementation detail of the library and may change.
Where that could be useful?
3. detail::try_demangle is unnecessary, as it does the same thing as core::demangle. :-)
Thanks! Fixed in develop. -- Best regards, Antony Polukhin
Antony Polukhin wrote:
<windows.h> is a lesser evil. The huge problem is a COM header with a lot of things in global namespace and a bunch of unportable vendor specific extensions. One pain of death, I'm not going to rewrite all the COM macro-spaghetti code (but I'm open to pull requests). The only good solution to avoid global namespace polution - is not to use the library in header only mode on MSVC.
The suggestion is not to rewrite the COM code. It's to not include it unless needed. template <class E> void throw_with_trace(const E& e) { throw boost::enable_error_info(e) << traced(boost::stacktrace::stacktrace()); } Here no COM or <windows.h> code is needed, but there's no way to avoid their inclusion.
2. There should exist a documented way to construct a stacktrace from an array of void const*. This is currently possible via from_dump, but it's not guaranteed to work; the format of the dump is technically an implementation detail of the library and may change.
Where that could be useful?
It's useful if one wants to capture the stack trace manually in order to avoid a dependency on stacktrace at the capture point. boost::throw_exception, for instance, could be made to do so.
2017-03-18 13:40 GMT+03:00 Peter Dimov via Boost <boost@lists.boost.org>:
Antony Polukhin wrote:
<windows.h> is a lesser evil. The huge problem is a COM header with a lot of things in global namespace and a bunch of unportable vendor specific extensions. One pain of death, I'm not going to rewrite all the COM macro-spaghetti code (but I'm open to pull requests). The only good solution to avoid global namespace polution - is not to use the library in header only mode on MSVC.
The suggestion is not to rewrite the COM code. It's to not include it unless needed.
template <class E> void throw_with_trace(const E& e) { throw boost::enable_error_info(e) << traced(boost::stacktrace::stacktrace()); }
Here no COM or <windows.h> code is needed, but there's no way to avoid their inclusion.
How it could be done?
2. There should exist a documented way to construct a stacktrace from an
array of void const*. This is currently possible via from_dump, but it's > not guaranteed to work; the format of the dump is technically an > implementation detail of the library and may change.
Where that could be useful?
It's useful if one wants to capture the stack trace manually in order to avoid a dependency on stacktrace at the capture point. boost::throw_exception, for instance, could be made to do so.
std::thread has no constructor from std::thread::native_handle_t and I'd like to follow that pattern. If you've done something using platform specific methods - you're on your own. But you still can use the boost::stacktrace::frame to decode frames one by one: void* data[10]; // captured frames // ... for (void* f: data) { std::cout << boost::stacktrace::frame(f); } -- Best regards, Antony Polukhin
Antony Polukhin via Boost wrote:
template <class E> void throw_with_trace(const E& e) { throw boost::enable_error_info(e) << traced(boost::stacktrace::stacktrace()); }
Here no COM or <windows.h> code is needed, but there's no way to avoid their inclusion.
How it could be done?
std::thread has no constructor from std::thread::native_handle_t and I'd like to follow that pattern. If you've done something using
By splitting stacktrace.hpp and frame.hpp into two headers, one with the parts that do not need <windows.h>, the other with the rest. The code above would then be able to only include the first header. Picking some names at random, stacktrace.hpp stacktrace_def.hpp stacktrace_impl.hpp frame.hpp frame_def.hpp frame_impl.hpp stacktrace_def.hpp frame_def.hpp stacktrace_impl.hpp frame_impl.hpp although currently stacktrace has no impl parts, so it could be simplified to stacktrace.hpp stacktrace_def.hpp frame_impl.hpp stacktrace_def.hpp frame_def.hpp frame.hpp frame_def.hpp frame_impl.hpp platform specific methods - you're on your own. This makes no sense to me, especially since frame is constructible from void const*. stacktrace is just an array of frames.
2017-03-18 14:11 GMT+03:00 Peter Dimov via Boost <boost@lists.boost.org>:
Antony Polukhin via Boost wrote:
template <class E> void throw_with_trace(const E& e) { throw boost::enable_error_info(e) << traced(boost::stacktrace::stacktrace()); }
Here no COM or <windows.h> code is needed, but there's no way to avoid > their inclusion.
How it could be done?
By splitting stacktrace.hpp and frame.hpp into two headers, one with the parts that do not need <windows.h>, the other with the rest. The code above would then be able to only include the first header. Picking some names at random,
stacktrace.hpp stacktrace_def.hpp stacktrace_impl.hpp
frame.hpp frame_def.hpp frame_impl.hpp
stacktrace_def.hpp frame_def.hpp
stacktrace_impl.hpp frame_impl.hpp
although currently stacktrace has no impl parts, so it could be simplified to
stacktrace.hpp stacktrace_def.hpp frame_impl.hpp
stacktrace_def.hpp frame_def.hpp
frame.hpp frame_def.hpp frame_impl.hpp
windows.h is a minor problem of a single platform. It already has an ultimate fix - use the non header-only version of the library. Most of the Boost users do not care about windows.h inclusion, those who care can link with the library. I'm not going to uglify the library by doubling/tripling the headers count and I'm definitely not going to make it's understanding harder just to satisfy the minority of users that use win platform + use MSVC + do not like windows.h header + do not wish to link + somehow wish to use only the non functional COM part of the library (that contains only a single CaptureStackBackTrace call).
std::thread has no constructor from std::thread::native_handle_t and I'd like to follow that pattern. If you've done something using
platform specific methods - you're on your own.
This makes no sense to me, especially since frame is constructible from void const*. stacktrace is just an array of frames.
It also increases the risk that people will capture the frames in a wrong way: * It's very easy to do it wrong on Windows, because it has multiple APIs for doing so and only one of them is safe. Moreover, the bad APIs have more examples and docs, so it's easier to choose wrongly. * It's terribly easy to do it wrong on POSIX, because it has documentation only on a bad API. -- Best regards, Antony Polukhin
Antony Polukhin wrote:
windows.h is a minor problem of a single platform.
windows.h is not a minor problem at all. It defines one million macros.
It already has an ultimate fix - use the non header-only version of the library.
This is not a decision that a library developer can make. If I'm writing a header-only library (that does nothing Windows-specific), I absolutely CANNOT afford to include <windows.h>, which means that I can't include stacktrace.hpp. Whether BOOST_STACKTRACE_LINK is defined is not up to me.
This makes no sense to me, especially since frame is constructible from void const*. stacktrace is just an array of frames.
It also increases the risk that people will capture the frames in a wrong way:
Not quite seeing that risk, given that the default constructor is much easier to use. You have to go out of your way to capture manually, for which presumably you have reasons.
2017-03-18 17:39 GMT+03:00 Peter Dimov via Boost <boost@lists.boost.org>:
Antony Polukhin wrote:
windows.h is a minor problem of a single platform.
windows.h is not a minor problem at all. It defines one million macros.
It already has an ultimate fix - use the non header-only version of the library.
This is not a decision that a library developer can make. If I'm writing a header-only library (that does nothing Windows-specific), I absolutely CANNOT afford to include <windows.h>, which means that I can't include stacktrace.hpp. Whether BOOST_STACKTRACE_LINK is defined is not up to me.
I've tried to move code around and separate the headers and I've failed. Headers+docs on non MS platforms start to look ugly, code does not compile separately from non COM parts or just becomes useless or COM headers leak everywhere. I'd appreciate a pull request, this issue is beyond my fixing capabilities. -- Best regards, Antony Polukhin
2017-03-18 17:39 GMT+03:00 Peter Dimov via Boost <boost@lists.boost.org>:
Antony Polukhin wrote:
windows.h is a minor problem of a single platform.
windows.h is not a minor problem at all. It defines one million macros.
dbgeng.h defines 10M macros. Getting rid of windows.h solves only small part of the problem. Looks like the right solution would be not to minify headers that use the windbh.h/dbgeng.h but rather write a correct forward declarations of windbh.h/dbgeng.h stuff, just like Boost.WinAPI does. My COM knowledge is limited, so I'd *really appreciate* some help with dbgeng.h. This will also fix multiple MinGW issues and will allow a better MinGW (and even Clang on Win) stacktracing out of the box. I'll take care of windows.h stuff soon. -- Best regards, Antony Polukhin
On 19/03/2017 06:38, Antony Polukhin via Boost wrote:
2017-03-18 17:39 GMT+03:00 Peter Dimov via Boost <boost@lists.boost.org>:
Antony Polukhin wrote:
windows.h is a minor problem of a single platform.
windows.h is not a minor problem at all. It defines one million macros.
dbgeng.h defines 10M macros. Getting rid of windows.h solves only small part of the problem.
Looks like the right solution would be not to minify headers that use the windbh.h/dbgeng.h but rather write a correct forward declarations of windbh.h/dbgeng.h stuff, just like Boost.WinAPI does.
My COM knowledge is limited, so I'd *really appreciate* some help with dbgeng.h. This will also fix multiple MinGW issues and will allow a better MinGW (and even Clang on Win) stacktracing out of the box.
I'll take care of windows.h stuff soon.
Putting on my review manager hat, here is my position on <windows.h>: 1. The size of windows.h is Microsoft's fault, not the code which includes it. 2. *Gratuitous* includes of windows.h is a showstopper for a high quality library, so including windows.h just for one or two APIs is a bad thing. 3. If however a library really makes extensive use of windows.h and if the user prefers the header only edition of the library, then including all of windows.h is *the price they pay*. They have the choice: either go header only and include windows.h, or don't go header only and don't include it. 4. In case anyone thinks I'm being unreasonably lax here, nobody much complains about header only inclusion of <sys/unistd.h> etc. That's because the size of <windows.h> is *Microsoft's fault*. It's the cost they impose on every Windows dev. We library developers are not at fault. Blame Microsoft. 5. I think it's highly unreasonable to ask Antony to remove windows.h from the headers only build. The Windows backend unavoidably uses COM with all that magic compiler-specific GUID attribute markup and such fun, and the headers brought in are auto generated from COM IDL and hand tuned by Microsoft. Hacking that out just to tick a box for some people will produce a much more brittle, unreliable and hard to maintain library. I as review manager certainly will not stop Stacktrace entering Boost on this point. If anything, if Antony ruins the library just to eliminate <windows.h> inclusion, I'd actually recommend its rejection. I appreciate that this will not be a popular opinion, but I'm the review manager and that's my judgement on it. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 3/19/2017 5:34 AM, Niall Douglas via Boost wrote:
On 19/03/2017 06:38, Antony Polukhin via Boost wrote:
2017-03-18 17:39 GMT+03:00 Peter Dimov via Boost <boost@lists.boost.org>:
Antony Polukhin wrote:
windows.h is a minor problem of a single platform.
windows.h is not a minor problem at all. It defines one million macros.
dbgeng.h defines 10M macros. Getting rid of windows.h solves only small part of the problem.
Looks like the right solution would be not to minify headers that use the windbh.h/dbgeng.h but rather write a correct forward declarations of windbh.h/dbgeng.h stuff, just like Boost.WinAPI does.
My COM knowledge is limited, so I'd *really appreciate* some help with dbgeng.h. This will also fix multiple MinGW issues and will allow a better MinGW (and even Clang on Win) stacktracing out of the box.
I'll take care of windows.h stuff soon.
Putting on my review manager hat, here is my position on <windows.h>:
1. The size of windows.h is Microsoft's fault, not the code which includes it.
2. *Gratuitous* includes of windows.h is a showstopper for a high quality library, so including windows.h just for one or two APIs is a bad thing.
3. If however a library really makes extensive use of windows.h and if the user prefers the header only edition of the library, then including all of windows.h is *the price they pay*. They have the choice: either go header only and include windows.h, or don't go header only and don't include it.
If a library makes extensive use of windows.h in a header file which only includes functionality which needs windows.h, then naturally an end-user including that header file gets windows.h included. But if I am using some libraries header file, which has a number of constructs I would like to use, none of which needs windows.h, I do not want windows.h included. I realize that this is sometimes difficult for a library developer to do, but it should be a relatively easy task compared to the high level C++ development that most Boost library developers actually do. Is splitting header files into particular functionality really that hard ? if the library developer offers specific header files for windows.h functionality and functionality not using windows.h, while still providing a general header file which includes both, then I as an end-user can at least choose whether windows.h is going to be included or not depending on what header file I include. BTW substitute "any-ridiculously-large-header-file-which-includes-endless-nondistinct-macros-and-constructs" for windows.h and the argument above is exactly the same. In other words I am in 100% agreement with Peter even while I realize that separating windows.h functionality in stacktrace may be some work for Antony. But given that the windows.h behemoth is Microsoft's fault the general principle of C++, of not "using" what one does not need, still applies.
4. In case anyone thinks I'm being unreasonably lax here, nobody much complains about header only inclusion of <sys/unistd.h> etc. That's because the size of <windows.h> is *Microsoft's fault*. It's the cost they impose on every Windows dev. We library developers are not at fault. Blame Microsoft.
5. I think it's highly unreasonable to ask Antony to remove windows.h from the headers only build. The Windows backend unavoidably uses COM with all that magic compiler-specific GUID attribute markup and such fun, and the headers brought in are auto generated from COM IDL and hand tuned by Microsoft. Hacking that out just to tick a box for some people will produce a much more brittle, unreliable and hard to maintain library.
Peter did not suggest not using windows.h itself AFAICS. He just suggested that stacktrace functionality which does not need it be segregated into specific headers which do not include it. Furthermore if stacktrace still has the general header, and promotes the use of it, rather than specific headers, that's fine as long as the end-user has a choice. OTOH if stacktrace functionality which appears not to need the constructs in windows.h still does need it under the covers in the header only version of the library, then I understand Antony's objection.
I as review manager certainly will not stop Stacktrace entering Boost on this point. If anything, if Antony ruins the library just to eliminate <windows.h> inclusion, I'd actually recommend its rejection. I appreciate that this will not be a popular opinion, but I'm the review manager and that's my judgement on it.
Niall
Peter did not suggest not using windows.h itself AFAICS. He just suggested that stacktrace functionality which does not need it be segregated into specific headers which do not include it. Furthermore if stacktrace still has the general header, and promotes the use of it, rather than specific headers, that's fine as long as the end-user has a choice. OTOH if stacktrace functionality which appears not to need the constructs in windows.h still does need it under the covers in the header only version of the library, then I understand Antony's objection.
If you choose the null backend, then yes windows.h should not be included. If you choose the non-null backend, which needs to open up a COM session with the Debug Engine to debug the calling executable, I think windows.h (and the COM headers which are also massive) pretty hard to avoid. And for the record, I too feel little love for windows.h, though they do provide NOMINMAX and WIN32_LEAN_AND_MEAN to make the problem less awful. One HUGE win with C++ Modules will be that we can finally work around windows.h once and for all. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 3/19/2017 7:30 PM, Niall Douglas via Boost wrote:
Peter did not suggest not using windows.h itself AFAICS. He just suggested that stacktrace functionality which does not need it be segregated into specific headers which do not include it. Furthermore if stacktrace still has the general header, and promotes the use of it, rather than specific headers, that's fine as long as the end-user has a choice. OTOH if stacktrace functionality which appears not to need the constructs in windows.h still does need it under the covers in the header only version of the library, then I understand Antony's objection.
If you choose the null backend, then yes windows.h should not be included.
If you choose the non-null backend, which needs to open up a COM session with the Debug Engine to debug the calling executable, I think windows.h (and the COM headers which are also massive) pretty hard to avoid.
Choosing only the windbg backend needs COM. I am still arguing along with Peter that merely choosing the windbg backend, or having it automatically chosen for me when I am using VC++, should not bring in windows.h until it is actually needed by the code I am invoking either in the stacktrace or frame classes. I can see using stacktrace where I am just passing already generated stacktrace or frame objects and once COM is needed to produce stacktraces or frames I see no reason why windows.h must be included in header files that do not need that functionality which generated the appropriate information anymore.
And for the record, I too feel little love for windows.h, though they do provide NOMINMAX and WIN32_LEAN_AND_MEAN to make the problem less awful. One HUGE win with C++ Modules will be that we can finally work around windows.h once and for all.
Niall
On Sun, Mar 19, 2017 at 6:42 PM, Edward Diener via Boost < boost@lists.boost.org> wrote:
On 3/19/2017 7:30 PM, Niall Douglas via Boost wrote:
Peter did not suggest not using windows.h itself AFAICS. He just
suggested that stacktrace functionality which does not need it be segregated into specific headers which do not include it. Furthermore if stacktrace still has the general header, and promotes the use of it, rather than specific headers, that's fine as long as the end-user has a choice. OTOH if stacktrace functionality which appears not to need the constructs in windows.h still does need it under the covers in the header only version of the library, then I understand Antony's objection.
If you choose the null backend, then yes windows.h should not be included.
If you choose the non-null backend, which needs to open up a COM session with the Debug Engine to debug the calling executable, I think windows.h (and the COM headers which are also massive) pretty hard to avoid.
Choosing only the windbg backend needs COM. I am still arguing along with Peter that merely choosing the windbg backend, or having it automatically chosen for me when I am using VC++, should not bring in windows.h until it is actually needed by the code I am invoking either in the stacktrace or frame classes.
+1 Emil
Choosing only the windbg backend needs COM. I am still arguing along with Peter that merely choosing the windbg backend, or having it automatically chosen for me when I am using VC++, should not bring in windows.h until it is actually needed by the code I am invoking either in the stacktrace or frame classes.
I can see using stacktrace where I am just passing already generated stacktrace or frame objects and once COM is needed to produce stacktraces or frames I see no reason why windows.h must be included in header files that do not need that functionality which generated the appropriate information anymore.
So, you're looking for the stacktrace and frame classes to not provide implementations of those member functions which rely on windows.h? I assume you then want an additional header file e.g. "stacktrace_extra.hpp" which provides implementations for those? If you do want this, one technique I've used in AFIO is for the public class to be mostly virtual member functions so header only includes don't drag in tons of implementation. Filing system operations are much heavier than Stacktrace's though, for AFIO a virtual function call is immaterial. I suppose a performance caring Stacktrace user would simply include the kitchen sink and rely on "override final" to eliminate the overhead. Is this what you are looking for? Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Niall Douglas wrote:
So, you're looking for the stacktrace and frame classes to not provide implementations of those member functions which rely on windows.h?
I assume you then want an additional header file e.g. "stacktrace_extra.hpp" which provides implementations for those?
Something like that. There are three possible ways to do it, and I haven't yet settled on one, or I'd have issued a pull request, as Antony asked. 1. #include "stacktrace.hpp" // as today, includes everything #include "stacktrace_def.hpp" // stacktrace without op<< or to_string In header-only libraries that don't use op<< or to_string, include the second header instead of the first. 2. #include "stackrace/stacktrace.hpp" // without op<< or to_string #include "stacktrace/output.hpp" // op<<, to_string Straightforward split into two headers, the difference is only that you need to explicitly include output.hpp for op<<. 3. #include "stacktrace.hpp" // declarations of everything, no definitions, no windows.h/COM #define BOOST_STACKTRACE_IMPLEMENTATION #include "stacktrace.hpp" // definitions, using windows.h, COM The usual way of packaging a non-header-only library in headers. The user defines BOOST_STACKTRACE_IMPLEMENTATION in one and only one .cpp file. No BOOST_STACKTRACE_LINK macro is necessary - if you link with the library, you just don't need to do the aforementioned. At the moment, I tend towards the third alternative, because it's a known practice and replaces the already existing BOOST_STRACKTRACE_LINK mechanism, so it doesn't introduce additional configuration complexity. Ideally, in (3), if you don't use op<< or to_string (or frame::name and so on), you won't need to include the implementation. So if you use a header-only library that includes stack traces in its exceptions, but you ignore these stack traces because you know nothing about the stacktrace library and don't use it, you won't have to pay the price if linking/including the implementation. In implementation terms, this_thread_frames::collect will have to be header-only so that the stacktrace constructors do not trigger link errors, the rest not.
2017-03-20 2:42 GMT+01:00 Edward Diener via Boost <boost@lists.boost.org>: On 3/19/2017 7:30 PM, Niall Douglas via Boost wrote:
Peter did not suggest not using windows.h itself AFAICS. He just
suggested that stacktrace functionality which does not need it be segregated into specific headers which do not include it. Furthermore if stacktrace still has the general header, and promotes the use of it, rather than specific headers, that's fine as long as the end-user has a choice. OTOH if stacktrace functionality which appears not to need the constructs in windows.h still does need it under the covers in the header only version of the library, then I understand Antony's objection.
If you choose the null backend, then yes windows.h should not be included.
If you choose the non-null backend, which needs to open up a COM session with the Debug Engine to debug the calling executable, I think windows.h (and the COM headers which are also massive) pretty hard to avoid.
Choosing only the windbg backend needs COM. I am still arguing along with Peter that merely choosing the windbg backend, or having it automatically chosen for me when I am using VC++, should not bring in windows.h until it is actually needed by the code I am invoking either in the stacktrace or frame classes.
I can see using stacktrace where I am just passing already generated stacktrace or frame objects and once COM is needed to produce stacktraces or frames I see no reason why windows.h must be included in header files that do not need that functionality which generated the appropriate information anymore.
Maybe this could be fixed by providing two windows backends: the current one, plus another one that offers a limited functionality but does not include the COM stuff? This would give a control to the users over the trade-off. Regards, &rzej;
2017-03-20 4:42 GMT+03:00 Edward Diener via Boost <boost@lists.boost.org>:
<...> and once COM is needed to produce stacktraces or frames <...>
It is required to produce anything useful from frame/stacktrace. Following operations use COM (on MSVC only): * ostreaming stacktrace * ostreaming frame * getting function name from frame * getting source file location from frame * getting line in source file from frame Now everyone here agrees that windows.h and COM is bad. But extracting out the windows.h/COM - is beyond my abilities. I'm not a lazy-bad-ass that does not wish to remove windows.h just because I do not care. It is because I've failed to do so. Such removal requires that all the BOOST_STACKTRACE_*_tuning_macro+BOOST_FOCEINLINE+BOOST_NOINLINE+skip_frames stuff must cooperate well on multiple platforms in header only and link-library modes. Also I can not see how you're going to use the library without 95% of functionality - I just do not understand what functions to move in what places. Moreover, as I understand at least two reviewers would like to have absolutely different functionality to depend or not on windows.h I'd gladly merge all the changes if some hero would produce a right pull request. I've failed to produce such. What is more important - moving away windows.h is a non breaking change that does not change the library interface (API). So all the messing with headers, moving code and attributes around could be done any time later. Moreover, windows.h never was a stopper for inclusion. 'find -name "*.hpp" -print0 | xargs -0 grep "windows\.h"' yields 14 libraries that use windows.h in header files. Now, when everyone agrees that windows.h is bad but nobody has the will or knowledge to fix it, let's review the remaining parts of the library. -- Best regards, Antony Polukhin
On Mon, Mar 20, 2017 at 12:25 PM, Antony Polukhin via Boost < boost@lists.boost.org> wrote:
What is more important - moving away windows.h is a non breaking change that does not change the library interface (API).
Except if the library interface needs to change to allow for the windows-specific components to be decoupled. Emil
Antony Polukhin wrote:
It is required to produce anything useful from frame/stacktrace. Following operations use COM (on MSVC only): * ostreaming stacktrace * ostreaming frame * getting function name from frame * getting source file location from frame * getting line in source file from frame
Yes, and the request is to not include COM/windows.h in programs or translation units that don't do any of the above. Please take a look in my message in which I outline three alternative approaches to achieve this aim; maybe one of those would sit well with you.
Also I can not see how you're going to use the library without 95% of functionality...
As already stated: - A header-only library constructs a stack trace, f.ex. to include it when throwing an exception - A user of this header-only library ignores the stack trace because he is not familiar with the stacktrace library and is unaware of the existence of stack traces The goal here is to not include COM/windows.h in this user's code, or require him to link with libboost_stacktrace. If this is not done, the authors of header-only libraries can't painlessly transition to using stacktrace to include stack traces in their exceptions because their users will complain about windows.h/COM or link errors. And we want libraries to use stacktrace to include stack traces in their exceptions, because this is a good thing that some of their users will appreciate.
On Sat, Mar 18, 2017 at 7:17 AM, Antony Polukhin via Boost < boost@lists.boost.org> wrote:
windows.h is a minor problem of a single platform.
I disagree, and it shouldn't be difficult to avoid it. One option is to split the platform-specific code in a windows-specific stack-trace header. Another is to (if possible) hide the offending code behind a template which doesn't trip the compiler if the user doesn't instantiate it, and requires the user to manually include windows.h if he does.
Most of the Boost users do not care about windows.h inclusion
Most Boost users are not qualified to judge what Boost libraries should and shouldn't do. Consider that e.g. bind.hpp won't even make use of MSVC platform-specific calling conventions, the motivation being to prevent unsuspecting programmers from accidentally writing platform-specific code. Emil
You can find the documentation at http://apolukhin.github.io/stacktrace/index.html and the github repo at https://github.com/apolukhin/stacktrace.
Some Cygwin errors: In file included from ..\..\../boost/stacktrace/detail/frame_unwind.ipp:19:0, from ..\..\..\libs\stacktrace\build\..\src\addr2line.cpp:10: ..\..\../boost/stacktrace/detail/location_from_symbol.hpp:25:7: error: 'Dl_info' in namespace '::' does not name a type ::Dl_info dli_; ^ ..\..\../boost/stacktrace/detail/location_from_symbol.hpp:31:14: error: '::dladdr' has not been declared if (!::dladdr(addr, &dli_)) { ^ Cygwin doesn't have Dl_info and dladdr. ..\..\../boost/stacktrace/detail/frame_msvc.ipp:97:36: error: '<::' cannot begin a template-argument list [-fpermissive] ..\..\../boost/stacktrace/detail/frame_msvc.ipp:97:36: note: '<:' is an alternate spelling for '['. Insert whitespace between '<' and '::' Digraphs are fun. :-)
2017-03-17 19:50 GMT+03:00 Peter Dimov via Boost <boost@lists.boost.org>:
You can find the documentation at http://apolukhin.github.io/stacktrace/index.html and the github repo at https://github.com/apolukhin/stacktrace.
Some Cygwin errors:
In file included from ..\..\../boost/stacktrace/detail/frame_unwind.ipp:19:0, from ..\..\..\libs\stacktrace\build\..\src\addr2line.cpp:10: ..\..\../boost/stacktrace/detail/location_from_symbol.hpp:25:7: error: 'Dl_info' in namespace '::' does not name a type ::Dl_info dli_; ^
..\..\../boost/stacktrace/detail/location_from_symbol.hpp:31:14: error: '::dladdr' has not been declared if (!::dladdr(addr, &dli_)) { ^ Cygwin doesn't have Dl_info and dladdr.
Why id does not define the BOOST_WINDOWS macro?
..\..\../boost/stacktrace/detail/frame_msvc.ipp:97:36: error: '<::' cannot begin a template-argument list [-fpermissive]
..\..\../boost/stacktrace/detail/frame_msvc.ipp:97:36: note: '<:' is an alternate spelling for '['. Insert whitespace between '<' and '::'
Digraphs are fun. :-)
Fixed in develop. -- Best regards, Antony Polukhin
Cygwin doesn't have Dl_info and dladdr.
These are the changes required: C:\Projects\boost-git\boost\libs\stacktrace>git diff diff --git a/include/boost/stacktrace/detail/frame_unwind.ipp b/include/boost/st index 8cb8e2a..37b7a7e 100644 --- a/include/boost/stacktrace/detail/frame_unwind.ipp +++ b/include/boost/stacktrace/detail/frame_unwind.ipp @@ -130,7 +130,7 @@ std::string to_string(const frame* frames, std::size_t size) std::string frame::name() const { -#ifndef BOOST_WINDOWS +#if !defined(BOOST_WINDOWS) && !defined(__CYGWIN__) ::Dl_info dli; const bool dl_ok = !!::dladdr(addr_, &dli); if (dl_ok && dli.dli_sname) { diff --git a/include/boost/stacktrace/detail/location_from_symbol.hpp b/include/ index 7faf26a..d20b0d6 100644 --- a/include/boost/stacktrace/detail/location_from_symbol.hpp +++ b/include/boost/stacktrace/detail/location_from_symbol.hpp @@ -12,7 +12,7 @@ # pragma once #endif -#ifndef BOOST_WINDOWS +#if !defined(BOOST_WINDOWS) && !defined(__CYGWIN__) # include <dlfcn.h> #else # include <boost/detail/winapi/dll.hpp> @@ -21,7 +21,7 @@ namespace boost { namespace stacktrace { namespace detail { class location_from_symbol { -#ifndef BOOST_WINDOWS +#if !defined(BOOST_WINDOWS) && !defined(__CYGWIN__) ::Dl_info dli_; public:
2017-03-18 14:48 GMT+03:00 Peter Dimov via Boost <boost@lists.boost.org>:
Cygwin doesn't have Dl_info and dladdr.
These are the changes required:
C:\Projects\boost-git\boost\libs\stacktrace>git diff diff --git a/include/boost/stacktrace/detail/frame_unwind.ipp b/include/boost/st index 8cb8e2a..37b7a7e 100644 --- a/include/boost/stacktrace/detail/frame_unwind.ipp +++ b/include/boost/stacktrace/detail/frame_unwind.ipp @@ -130,7 +130,7 @@ std::string to_string(const frame* frames, std::size_t size)
std::string frame::name() const { -#ifndef BOOST_WINDOWS +#if !defined(BOOST_WINDOWS) && !defined(__CYGWIN__) ::Dl_info dli; const bool dl_ok = !!::dladdr(addr_, &dli); if (dl_ok && dli.dli_sname) { diff --git a/include/boost/stacktrace/detail/location_from_symbol.hpp b/include/ index 7faf26a..d20b0d6 100644 --- a/include/boost/stacktrace/detail/location_from_symbol.hpp +++ b/include/boost/stacktrace/detail/location_from_symbol.hpp @@ -12,7 +12,7 @@ # pragma once #endif
-#ifndef BOOST_WINDOWS +#if !defined(BOOST_WINDOWS) && !defined(__CYGWIN__) # include <dlfcn.h> #else # include <boost/detail/winapi/dll.hpp> @@ -21,7 +21,7 @@ namespace boost { namespace stacktrace { namespace detail {
class location_from_symbol { -#ifndef BOOST_WINDOWS +#if !defined(BOOST_WINDOWS) && !defined(__CYGWIN__) ::Dl_info dli_;
public:
Fixed in develop. Thanks for the patch! -- Best regards, Antony Polukhin
You can find the documentation at http://apolukhin.github.io/stacktrace/index.html and the github repo at https://github.com/apolukhin/stacktrace.
The heuristic that skips 3 frames (under Windows) doesn't quite work. It seems that MSVC in whole program optimization mode ignores BOOST_NOINLINE directives and makes inline decisions that differ in unpredictable ways between compiles. This program, for instance: #include <boost/stacktrace.hpp> #include <iostream> #include <algorithm> #include <functional> void g( int x ) { boost::stacktrace::stacktrace st( -3, -1 ); std::cout << st << std::endl; } void f( int x ) { std::function<void(int)> h( g ); std::for_each( &x, &x + 1, g ); } int main() { f( 1 ); } gives this trace: 0# boost::stacktrace::detail::this_thread_frames::collect at c:\projects\boost-git\boost\boost\stacktrace\detail\frame_msvc.ipp:48 1# g at c:\projects\testbed2017\testbed2017.cpp:9 2# main at c:\projects\testbed2017\testbed2017.cpp:20 3# __scrt_common_main_seh at f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:259 4# BaseThreadInitThunk in kernel32 5# RtlInitializeExceptionChain in ntdll 6# RtlInitializeExceptionChain in ntdll With the default constructor, #0, #1 and #2 disappear and nothing useful remains. There should probably be a way to optionally remove the +1 and +2 hardcoded skips because passing a nonportable -3 to a size_t is not exactly a best practice.
2017-03-17 20:30 GMT+03:00 Peter Dimov via Boost <boost@lists.boost.org>:
You can find the documentation at http://apolukhin.github.io/stacktrace/index.html and the github repo at https://github.com/apolukhin/stacktrace.
The heuristic that skips 3 frames (under Windows) doesn't quite work. It seems that MSVC in whole program optimization mode ignores BOOST_NOINLINE directives and makes inline decisions that differ in unpredictable ways between compiles. This program, for instance:
#include <boost/stacktrace.hpp> #include <iostream> #include <algorithm> #include <functional>
void g( int x ) { boost::stacktrace::stacktrace st( -3, -1 ); std::cout << st << std::endl; }
void f( int x ) { std::function<void(int)> h( g ); std::for_each( &x, &x + 1, g ); }
int main() { f( 1 ); }
gives this trace:
0# boost::stacktrace::detail::this_thread_frames::collect at c:\projects\boost-git\boost\boost\stacktrace\detail\frame_msvc.ipp:48 1# g at c:\projects\testbed2017\testbed2017.cpp:9 2# main at c:\projects\testbed2017\testbed2017.cpp:20 3# __scrt_common_main_seh at f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:259 4# BaseThreadInitThunk in kernel32 5# RtlInitializeExceptionChain in ntdll 6# RtlInitializeExceptionChain in ntdll
With the default constructor, #0, #1 and #2 disappear and nothing useful remains.
There should probably be a way to optionally remove the +1 and +2 hardcoded skips because passing a nonportable -3 to a size_t is not exactly a best practice.
Just removed the skips on Windows in develop. If there's no reliable way of skipping frames, it's better to show more info rather than skip a most useful frame with user defined function. -- Best regards, Antony Polukhin
You can find the documentation at http://apolukhin.github.io/stacktrace/index.html and the github repo at https://github.com/apolukhin/stacktrace.
I wonder if basic_stacktrace, currently template <class Allocator> class basic_stacktrace { boost::container::vector<frame, Allocator> impl_; /*...*/ }; would be better of as template < class Container = std::vector<frame> > class basic_stacktrace { static_assert( std::is_same<typename Container::value_type, frame>::value, "The value_type of the Container parameter must be stacktrace::frame" ); Container impl_; /*...*/ }; This still supports the current functionality of replacing the allocator, but it also allows template<size_t N> using fixed_stacktrace = basic_stacktrace<std::array<frame, N>>; which gets us the previous iteration of the library. Fixed storage does have its uses, and while it's possible to use an in-place allocator to get it, this is much easier.
2017-03-18 16:18 GMT+03:00 Peter Dimov via Boost <boost@lists.boost.org>:
You can find the documentation at http://apolukhin.github.io/stacktrace/index.html and the github repo at https://github.com/apolukhin/stacktrace.
I wonder if basic_stacktrace, currently
template <class Allocator> class basic_stacktrace { boost::container::vector<frame, Allocator> impl_; /*...*/ };
would be better of as
template < class Container = std::vector<frame> > class basic_stacktrace { static_assert( std::is_same<typename Container::value_type, frame>::value, "The value_type of the Container parameter must be stacktrace::frame" );
Container impl_;
/*...*/ };
This still supports the current functionality of replacing the allocator, but it also allows
template<size_t N> using fixed_stacktrace = basic_stacktrace<std::array<frame, N>>;
which gets us the previous iteration of the library. Fixed storage does have its uses, and while it's possible to use an in-place allocator to get it, this is much easier.
This won't work, because std::array has no push_back/pop_back methods. Is boost::container::vector bothers you? I've used that container because it could be used in shred memory, so that people could store stacktrace in shared memory. Now it does not look like a good idea, so I'll probably change it to std::vector. -- Best regards, Antony Polukhin
Antony Polukhin wrote:
template<size_t N> using fixed_stacktrace = basic_stacktrace<std::array<frame, N>>;
This won't work, because std::array has no push_back/pop_back methods.
Ordinarily it won't; one would need to use boost::container::static_vector. But if we go this way, it should be specialized to work with std::array as well.
Is boost::container::vector bothers you?
boost::container::vector is not much of a problem; templating on a container just allows fixed storage without having to write a fixed storage allocator.
You can find the documentation at http://apolukhin.github.io/stacktrace/index.html and the github repo at https://github.com/apolukhin/stacktrace.
I see that frame supports construction from a function pointer: void print_signal_handler_and_exit() { void* p = reinterpret_cast<void*>(::signal(SIGSEGV, SIG_DFL)); boost::stacktrace::frame f(p); std::cout << f << std::endl; std::exit(0); } but it takes void*. On both POSIX and Windows, pointers to functions and void* have the same representation, but if we want to be strictly standard conforming, frame should take void(*)(). The standard guarantees that pointers to functions can be cast to void(*)(), but it doesn't guarantee it for void*.
2017-03-18 18:58 GMT+03:00 Peter Dimov via Boost <boost@lists.boost.org>:
You can find the documentation at http://apolukhin.github.io/stacktrace/index.html and the github repo at https://github.com/apolukhin/stacktrace. <...> On both POSIX and Windows, pointers to functions and void* have the same representation, but if we want to be strictly standard conforming, frame should take void(*)(). The standard guarantees that pointers to functions can be cast to void(*)(), but it doesn't guarantee it for void*.
In the C++ standard there is no requirement that pointer to frame is a pointer to function. C++ Standard does not mention frames at all and many OS return frames as just a void*, not void(*)(). I'll add a `template <class T> frame(T* addr)` and `frame(void(*addr)())` constructors. This must satisfy all the needs and allow to change underlying implementation for a platform where sizeof(void*) != sizoef(void(*)()) -- Best regards, Antony Polukhin
Antony Polukhin wrote:
In the C++ standard there is no requirement that pointer to frame is a pointer to function. C++ Standard does not mention frames at all and many OS return frames as just a void*, not void(*)().
That's true but you've stated that you don't want to support manually captured frames. So what the OS returns should be irrelevant in principle - it's an implementation detail that the library hides from the user. You do support frames constructed from function pointers, such as the one returned from ::signal, so this is what the constructor should take. The only reason to take void* would be to support passing the return value of dlsym to the constructor without a reinterpret_cast.
I'll add a `template <class T> frame(T* addr)`
This will do if you also constrain it to is_function<T>.
2017-03-20 23:52 GMT+03:00 Peter Dimov via Boost <boost@lists.boost.org>:
Antony Polukhin wrote:
In the C++ standard there is no requirement that pointer to frame is a pointer to function. C++ Standard does not mention frames at all and many OS return frames as just a void*, not void(*)().
That's true but you've stated that you don't want to support manually captured frames. So what the OS returns should be irrelevant in principle - it's an implementation detail that the library hides from the user.
You do support frames constructed from function pointers, such as the one returned from ::signal, so this is what the constructor should take.
The only reason to take void* would be to support passing the return value of dlsym to the constructor without a reinterpret_cast.
I'll add a `template <class T> frame(T* addr)`
This will do if you also constrain it to is_function<T>.
Good points! Agreed. -- Best regards, Antony Polukhin
On 2017-03-18 02:09, Niall Douglas via Boost wrote:
I am pleased to announce the second review of the proposed Boost.Stacktrace library running from today until Sunday 26th.
Started reading the docs. Looks very satisfying and I see wanting/using it extensively. I certainly vote for acceptance. My personal view is that the lib is very useful/needed. So, the sooner it gets wider deployment the sooner issues and niggles are to be addressed. The default deployment seems straightforward with the option for customization for adventurous. I like that. I was irked by boost::stacktrace::stacktrace() invocation though. Not a huge drama but boost::stacktrace() certainly looks far more natural to me. Started looking what there was in the boost::stacktrace namespace. It appears quite a few things could be be better encapsulated in the relevant classes. For ex., basic_stacktrace and frame comparison operators are all outside the respective classes. namespace boost { namespace stacktrace { class frame; bool operator<(const frame & lhs, const frame & rhs); bool operator>(const frame & lhs, const frame & rhs); ... However, the frame class has no implicit constructor to take advantage of such placement. So, those operators might go (and belong IMO) inside the frame class. Same IMO goes for basic_stacktrace. Functions in the boost::stacktrace namespace might become static functions inside boost::stacktrace class. Supporting functionality might go into some boost::stacktrace_detail namespace. That'd allow boost::stacktrace to be actually a class and consequently visible to the users as simply boost::stacktrace(). That'd be nice.
You can find the documentation at http://apolukhin.github.io/stacktrace/index.html and the github repo at https://github.com/apolukhin/stacktrace.
This portion of build/Jamfile.v2: lib boost_stacktrace_basic : # sources ../src/basic.cpp : # requirements <warnings>all <target-os>linux:<library>dl <link>shared:<define>BOOST_STACKTRACE_DYN_LINK=1 [ check-target-builds ../build//WinDbg : <build>no ] : # default build : # usage-requirements #<link>shared:<define>BOOST_STACKTRACE_DYN_LINK=1 ; looks a bit odd; why would building boost_stacktrace_basic require WinDbg? Probably a copy/paste error.
* Niall Douglas via Boost:
- What is your evaluation of the implementation?
I had a cursory look at the implementation. addr2line_pipe uses fork and fdopen, which are not async-signal-safe with glibc. But then, std::string is used as well, which is not async-signal-safe, either, so maybe that's okay. addr2line_pipe uses execvp, which could lead to evaluation of the PATH variable from programs which underwent an AT_SECURE transition (e.g., SUID programs), which could lead to privilege escalation issues. With inlining, a single stack frame can expand to a list of source locations (reflecting the inlineed call stack). The current design does not take that into account. Success of dladdr does not mean that it has obtained useful information. You really need to call addr2line in that case as well. Furthermore, before calling addr2line, you have to translate the address to an object-relative address (again with the help of dladdr). The current code assumes that addresses in the main object (without dli_sname) are not subject to ASLR, which is not true for position-independent executables. The libunwind unwinder does not use _Unwind_GetIPInfo, and it does not seem to deal with the function call/signal handler frame difference. The documentation should perhaps say something about dlclose. If an application formats the stack trace for human consumption, but this happens after stack unwinding, some DSOs may have been closed with dlclose, and it could be impossible to obtain symbol information at that point.
2017-03-25 16:29 GMT+03:00 Florian Weimer via Boost <boost@lists.boost.org>:
* Niall Douglas via Boost:
- What is your evaluation of the implementation?
I had a cursory look at the implementation.
addr2line_pipe uses fork and fdopen, which are not async-signal-safe with glibc. But then, std::string is used as well, which is not async-signal-safe, either, so maybe that's okay.
Methods that use fork and fdopen are marked as async-signal-unsafe.
addr2line_pipe uses execvp, which could lead to evaluation of the PATH variable from programs which underwent an AT_SECURE transition (e.g., SUID programs), which could lead to privilege escalation issues.
I'm providing an absolute path to the executable, so there must be no PATH evaluation. Am I missing something?
With inlining, a single stack frame can expand to a list of source locations (reflecting the inlineed call stack). The current design does not take that into account.
I've found no system API to get multiple locations. So the Stacktrace API is designed to have at most 1 location.
Success of dladdr does not mean that it has obtained useful information. You really need to call addr2line in that case as well. Furthermore, before calling addr2line, you have to translate the address to an object-relative address (again with the help of dladdr). The current code assumes that addresses in the main object (without dli_sname) are not subject to ASLR, which is not true for position-independent executables.
The libunwind unwinder does not use _Unwind_GetIPInfo, and it does not seem to deal with the function call/signal handler frame difference.
Hm. Looks I have to experiment a little bit more. Thanks for pointing that out.
The documentation should perhaps say something about dlclose. If an application formats the stack trace for human consumption, but this happens after stack unwinding, some DSOs may have been closed with dlclose, and it could be impossible to obtain symbol information at that point.
Ok. Good point. -- Best regards, Antony Polukhin
* Antony Polukhin:
addr2line_pipe uses execvp, which could lead to evaluation of the PATH variable from programs which underwent an AT_SECURE transition (e.g., SUID programs), which could lead to privilege escalation issues.
I'm providing an absolute path to the executable, so there must be no PATH evaluation. Am I missing something?
I'm talking about this: char prog_name[] = "addr2line"; As far as I can see, this is not just used as the argv[0] argument, but also as the program to execute.
With inlining, a single stack frame can expand to a list of source locations (reflecting the inlineed call stack). The current design does not take that into account.
I've found no system API to get multiple locations. So the Stacktrace API is designed to have at most 1 location.
addr2line can optionally resolve inlining information. The additional frames come from the DWARF data.
2017-03-27 14:19 GMT+03:00 Florian Weimer <fw@deneb.enyo.de>:
* Antony Polukhin:
addr2line_pipe uses execvp, which could lead to evaluation of the PATH variable from programs which underwent an AT_SECURE transition (e.g., SUID programs), which could lead to privilege escalation issues.
I'm providing an absolute path to the executable, so there must be no PATH evaluation. Am I missing something?
I'm talking about this:
char prog_name[] = "addr2line";
As far as I can see, this is not just used as the argv[0] argument, but also as the program to execute.
Fixed. The docs will be updated soon. -- Best regards, Antony Polukhin
On 3/17/2017 11:09 AM, Niall Douglas via Boost-users wrote:
I am pleased to announce the second review of the proposed Boost.Stacktrace library running from today until Sunday 26th.
Stacktrace is an optionally header-only library providing multiple implementation backends. At its very simplest it lets you capture the stack backtrace for the calling thread and to print it to a std::ostream& of your choice. The basic_stacktrace<> class quacks like a STL container of frame object instances. The rest of the API pretty much follows STL design principles for the most part. Use is therefore unsurprising.
You can find the documentation at http://apolukhin.github.io/stacktrace/index.html and the github repo at https://github.com/apolukhin/stacktrace.
This is my review of the stacktrace library.
For your review you may wish to consider the following questions: - What is your evaluation of the design?
The design is straightforward and easy to use.
- What is your evaluation of the implementation?
As others have said the windows.h issue should be addressed. Otherwise the implementation looks good.
- What is your evaluation of the documentation?
The doc could be expanded with some basic information about the stacktrace and frame classes and how they relate. I know the modern documentation method is some sort of tutorial with examples, and then a reference, but I always welcome some basic explanations, even in a library whose interfaces are as simple as stacktrace. So I would like to see the doc expanded with some more explanation about stacktraces and their frames.
- What is your evaluation of the potential usefulness of the library?
I think it will be useful to programmers as an alternative to visual debuggers. Its usefulness is based on its ability to show function information in its stacktracing.
- Did you try to use the library? With what compiler? Did you have any problems?
Tried with VS2014. No problems with the output. I did not try with clang or gcc on Linux or Windows.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
A quick reading and trying it out to see if it actually works as advertised.
- Are you knowledgeable about the problem domain?
I have used stacktraces in many programming environments to debug programming problems.
And finally, every review should attempt to answer this question:
- Do you think the library should be accepted as a Boost library?
I vote to accept the library. I think the library author still has some work to do, but I believe that the library is useful enough, and uses the right approach with the different backends, to be accepted.
participants (9)
-
Andrzej Krzemienski -
Antony Polukhin -
Edward Diener -
Emil Dotchevski -
Florian Weimer -
Niall Douglas -
Peter Dimov -
Steven Watanabe -
Vladimir Batov