[Stacktrace] Review Manager report
From the reviews, I think there is consensus that the minimum viable
Firstly happy new year to everyone, and many thanks to all those who participated in the peer review of proposed Boost.Stacktrace before Christmas. I received the following formal reviews: 1. Vladimir Batov (YES) 2. Klemens Morgenstern (YES) 3. Andrey Semashev (NO) 4. Nat Goodspeed (YES) 5. Artyom Beilis (NO) 6. Barrett Adair (YES) Extensive comments without a formal acceptance recommendation were also made by: 1. Peter Dimov 2. Edward Diener There was a very noticeable lack of consensus on the strengths and weaknesses of the proposed library which has made summarising all these reviews into a coherent report with concrete conditions for acceptance very difficult. Antony also responded to many of the concerns throughout the review, agreeing with some points, disagreeing with others. I think this lack of consensus stemmed from multiple opinions on what a stacktrace library ought to provide: Minimum viable product: A portable backtrace() and backtrace_symbols() implementation with a C++ STL idiomatic API. The stack backtrace capture implementation must not allocate memory and be "fast". Note that backtrace_symbols() does NOT symbolise into strings containing source file name and line number. It also allocates memory (backtrace_symbols_fd() does not). Opinion 1: Portable serialisation of the stacktrace with instruction pointers converted to offsets into SO/DLL binary paths so a later external process can use those in combination with their debug symbols to retrieve source file names and line numbers. Ideally the serialisation formats supported would include those consumable by well known tooling for this purpose e.g. addr2line or llvm-symbolizer. Opinion 2: That Stacktrace can in process use debug symbols to retrieve source file names and line numbers. This could be implemented by invoking addr2line or equivalent as a child process which has the enormous advantage of being async safe, but has disadvantages in almost every other area. Or it could be implemented by one's own DWARF parser but with the huge caveat that memory could be badly corrupted by now. Opinion 3: That Stacktrace be "async safe" in that backtraces can be safely captured and parsed from a signal handler or SEH/TEH handler callback or point of C++ exception throw. *This implies no memory allocation during the execution of the handler*. product is achieved in Stacktrace though many mentioned that improvements are needed to the documentation and most of the negative feedback was on features not in the minimum viable product described above. I therefore recommend the ACCEPTANCE of proposed Boost.Stacktrace into Boost with the following conditions: 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. I'd like to congratulate Antony on getting yet another library of his into Boost. I hope some of his magic rubs off on me when I bring Outcome to review later this year! Regards, Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 01/02/17 19:14, Niall Douglas wrote:
From the reviews, I think there is consensus that the minimum viable product is achieved in Stacktrace though many mentioned that improvements are needed to the documentation and most of the negative feedback was on features not in the minimum viable product described above. I therefore recommend the ACCEPTANCE of proposed Boost.Stacktrace into Boost with the following conditions:
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.
Thanks Niall for managing the review and congrats to Antony. I have to say though that I really don't think that a macro-based solution is the right way to go when a policy based pure C++ solution is possible. Config macros tend to result in compatibility issues when defined differently in different areas of the application, and stacktraces have the potential of travelling long distances across the application code if attached to exceptions.
I have to say though that I really don't think that a macro-based solution is the right way to go when a policy based pure C++ solution is possible. Config macros tend to result in compatibility issues when defined differently in different areas of the application, and stacktraces have the potential of travelling long distances across the application code if attached to exceptions.
I did consider that option. Policy based designs are the right choice in many use cases e.g. Outcome is a policy based CRTP design. But in my experience a policy based design is inferior to a feature enable design in this use case. This is because none of the enabled features fundamentally change the core featureset even if the underlying engine has changed, rather they switch on additional features. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
2017-01-02 19:14 GMT+03:00 Niall Douglas
I'd like to congratulate Antony on getting yet another library of his into Boost. I hope some of his magic rubs off on me when I bring Outcome to review later this year!
Great thanks Niall for being the review manager! I'd like to have a mini review for a 2-5 days after I'll fix all the issues found during the review. Just to be sure, that modified version improves consensus (or at least does not make things worse). -- Best regards, Antony Polukhin
participants (3)
-
Andrey Semashev -
Antony Polukhin -
Niall Douglas