Re: [boost] [stacktrace] review (changing vote to NO)
1. use of -rdynamic/-export-dynamic should be noted in "bold face" regarding the use of the library.
Those flags are not required because addr2line can extract information from debug sections of the binary.
Yes.. this is exactly the issue - I don't think that addr2line should be used by default. Getting the stack trace using dladdr works very well - so no need to use add2line unless absolutely required. I also want to hear regarding inefficiency of add2line use - creating the process for each stack frame you wanted to trace. It is very inefficient so I suggest not to use one. Finally reviewing your code once again I realized following: 1. Instead of using adde2line to get the object name you can just call add2line with /proc/12345/exe (works with dynamically loaded shared objects as well - i tested it) 2. Use of addr2line for every each frame must be fixed But what is most important that I understood that you don't even use dladdr to get function names even if you can - which is very bad decision. Additionally I realized that with_trace and traced aren't parts of the library! These should be built in there by default. -------------------------- After using the library and running couple of tests on Linux with GCC.. I have to revoke my "Conditional YES" vote and change it to "NO": And the reason is following: 1. Linux/*nix backend is poor - instead of using dladdr to get function names easily as I do in the backend I written and use on CppCMS [1] you call external exe in very inefficient way. 2. I realized that the library does not provide the basic functionality like collecting strack trace on throw (what I thought was built in but I was mistaken) 3. No answer for non MSVC compilers built in. But the most important reason that the library does not feel mature enough in terms of both backends and frontends. I strongly strongly strongly recommend to continue the work on the library as it provides important functionality. The frontend should be adjusted a little to couple with common cases. The backend must be modified to not to fork/exec by default and optimize one significantly. I clearly understand you to want to use addr2line as it gives more power but it can't be used by default. I searched for a long time for a lightweight solution for converting addresses to the function names for CppCMS including trying to use debug information but couldn't find anything suitable besides dlfunc. However addr2line is by no means can be accepted as "default" solution. I also recommend to take a look on cppcms's backtrace library it is very simple and provides critical information in production environment. Regards, Artyom Beilis [1] https://sourceforge.net/p/cppcms/code/HEAD/tree/framework/trunk/booster/lib/... _______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
2016-12-25 16:22 GMT+03:00 Artyom Beilis
1. use of -rdynamic/-export-dynamic should be noted in "bold face" regarding the use of the library.
Those flags are not required because addr2line can extract information from debug sections of the binary.
Yes.. this is exactly the issue - I don't think that addr2line should be used by default. Getting the stack trace using dladdr works very well - so no need to use add2line unless absolutely required.
In my opinion forcing users to use some flags that affect code generation is not nice. However you are right, with -rdynamic/-export-dynamic there's no need in addr2line.
Finally reviewing your code once again I realized following:
1. Instead of using adde2line to get the object name you can just call add2line with /proc/12345/exe (works with dynamically loaded shared objects as well - i tested it)
Hm... Good point, but I'm not sure that /proc/self/exe is portable.
2. Use of addr2line for every each frame must be fixed
Fix for that is in my TODOs. This would be fixed.
But what is most important that I understood that you don't even use dladdr to get function names even if you can - which is very bad decision.
dladdr is used by default, and when the call fails library fallbacks to addr2line https://github.com/apolukhin/stacktrace/blob/master/include/boost/stacktrace...
Additionally I realized that with_trace and traced aren't parts of the library! These should be built in there by default.
During the Review I've received multiple requests to embed stacktraces into exceptions. And everyone was requesting to do that in a different way. I've got about 5 different incompatible ways to do that, and everyone thinks that there's only one (or at most 3) right ways to do that. So my opinion about that feature request is extreamely strong: NO. That would not happen, because there's no way to satisfy everyone needs.
3. No answer for non MSVC compilers built in.
That would be fixed.
The frontend should be adjusted a little to couple with common cases.
In what way?
The backend must be modified to not to fork/exec by default and optimize one significantly.
+1 Thanks for the review! -- Best regards, Antony Polukhin
Antony Polukhin wrote:
Additionally I realized that with_trace and traced aren't parts of the library! These should be built in there by default.
During the Review I've received multiple requests to embed stacktraces into exceptions. And everyone was requesting to do that in a different way. I've got about 5 different incompatible ways to do that, and everyone thinks that there's only one (or at most 3) right ways to do that.
There is one and only one way to do that, and it's to use Boost.Exception. That's what it's there for. Every library defining its own with_foo and with_bar exception wrappers is not the way to go. And neither is duplicating the standard exception hierarchy as in https://sourceforge.net/p/cppcms/code/HEAD/tree/framework/trunk/booster/boos...
On 12/25/16 17:37, Peter Dimov wrote:
Antony Polukhin wrote:
Additionally I realized that with_trace and traced aren't parts of the > library! These should be built in there by default.
During the Review I've received multiple requests to embed stacktraces into exceptions. And everyone was requesting to do that in a different way. I've got about 5 different incompatible ways to do that, and everyone thinks that there's only one (or at most 3) right ways to do that.
There is one and only one way to do that, and it's to use Boost.Exception. That's what it's there for.
Every library defining its own with_foo and with_bar exception wrappers is not the way to go. And neither is duplicating the standard exception hierarchy as in
https://sourceforge.net/p/cppcms/code/HEAD/tree/framework/trunk/booster/boos...
+1
Additionally I realized that with_trace and traced aren't parts of the > library! These should be built in there by default.
During the Review I've received multiple requests to embed stacktraces into exceptions. And everyone was requesting to do that in a different way. I've got about 5 different incompatible ways to do that, and everyone thinks that there's only one (or at most 3) right ways to do that.
There is one and only one way to do that, and it's to use Boost.Exception. That's what it's there for.
Every library defining its own with_foo and with_bar exception wrappers is not the way to go.
As long as there is a simple way to throw an exception such that: 1. It is derived from std::exception 2. It contains information about the stack trace It is ok for me. But this basic functionality this way or other should be present. I personally liked with_trace very much as very simple and intuitive, straight forward and does not require redesign of exception hierarchy (like I did in cppcms/booster)
And neither is duplicating the standard exception hierarchy as in
https://sourceforge.net/p/cppcms/code/HEAD/tree/framework/trunk/booster/boos...
I hadn't told that this is best approach it just one possible approach that I designed for CppCMS (not boost) I also hand't submitted it for boost review. Artyom
Artyom Beilis wrote:
As long as there is a simple way to throw an exception such that:
1. It is derived from std::exception 2. It contains information about the stack trace
It is ok for me.
This is the basic way to use Boost.Exception with Stacktrace:
#include
As long as there is a simple way to throw an exception such that:
1. It is derived from std::exception 2. It contains information about the stack trace
It is ok for me.
This is the basic way to use Boost.Exception with Stacktrace:
#include
#include #include <stdexcept> #include <iostream> typedef boost::error_info
stacktrace_info; template<class E> void throw_with_stacktrace( E const & e ) { throw boost::enable_error_info( e ) << stacktrace_info( boost::stacktrace::stacktrace() ); }
Perfect, Than this should be a part of the stackrace library by default with examples and so on. I really like the idea to have such a library in Boost. But I don't feel it is mature enough, especially the backends in their current state - and finally the backends are actually the most critical and hardest stuff to implement _correctly_ Regards, Artyom Beilis
On 2016-12-26 03:32, Artyom Beilis wrote:
... I really like the idea to have such a library in Boost.
Then IMO you should be at least voting conditional "yes".
But I don't feel it is mature enough, especially the backends in their current state - and finally the backends are actually the most critical and hardest stuff to implement _correctly_
I am not disputing that at all. To say that the lib is in the perfect shape would be a stretch. That said, IMO expecting the author to do everything right, to address all imaginable use-cases and to provide extensive documentation that would satisfy everyone is an impossible task and, therefore, IMO it should not be the expectation during a review. I can't help feeling that quite often reviewers expect everything on a silver plate with all Ts crossed and everything. And then they want more. I do not believe that's a constructive attitude. IMO it puts an immense pressure and burden on the author without giving anything in return... even an incentive to come back. The "door" is simply shut with a message "go away; you might try knocking again and going through that same humiliation again with no promises at all". Would not that be wiser to give the author an incentive to address the discussed issues, to accept the lib conditionally and to see the lib evolving, improving, finished as part of Boost? Surely, it won't give the users "everything". However, it'll give the users working "something" and will provide the author with a considerably better feedback and the desire to address that.
At the risk of going off-topic again, I'll reply once.
On Sun, Dec 25, 2016 at 10:59 PM, Vladimir Batov
On 2016-12-26 03:32, Artyom Beilis wrote:
I really like the idea to have such a library in Boost.
Then IMO you should be at least voting conditional "yes".
Acknowledging the need for a particular functionality is not the same as accepting a particular implementation.
But I don't feel it is mature enough, especially the backends in their current state - and finally the backends are actually the most critical and hardest stuff to implement _correctly_
I am not disputing that at all. To say that the lib is in the perfect shape would be a stretch. That said, IMO expecting the author to do everything right, to address all imaginable use-cases and to provide extensive documentation that would satisfy everyone is an impossible task and, therefore, IMO it should not be the expectation during a review. I can't help feeling that quite often reviewers expect everything on a silver plate with all Ts crossed and everything. And then they want more. I do not believe that's a constructive attitude. IMO it puts an immense pressure and burden on the author without giving anything in return... even an incentive to come back. The "door" is simply shut with a message "go away; you might try knocking again and going through that same humiliation again with no promises at all".
I think you misunderstand the point of a review. If you read the review policy[1], you will notice that the main point of a review is to evaluate the library, identify its strong and weak spots. By turning a blind eye on a design flaw of a presented library you're doing a disservice to the library author and the library users. Instead, by giving constructive feedback (positive and negative), especially if supported by real world experience, you help the library improvement. In fact, this (the feedback) is the most valuable reward for the author from the review. And let me say, there's nothing humiliating in having a library rejected during a review.
Would not that be wiser to give the author an incentive to address the discussed issues, to accept the lib conditionally and to see the lib evolving, improving, finished as part of Boost? Surely, it won't give the users "everything". However, it'll give the users working "something" and will provide the author with a considerably better feedback and the desire to address that.
There are things that can be fixed post-review. Usually this includes minor and technical stuff, like adding support for particular compilers, fixing tests and docs. Sometimes that includes more complex stuff, but that may warrant conditional acceptance. There are other things that cannot be fixed easily and would probably require changing library design. Those changes often affect library API and usage patterns, which warrants the need of a new review of the reworked library. In those cases it's better to reject the library so that the new, improved version is presented later. Note that missing functionality, unless considered crucial for the library domain, is typically not considered a reason for rejection. In fact, I'd say, it's usually preferable that the library does less things but it does them well. New features can be added at a later stage to the already accepted library, and if the library is designed properly, these features would fit nicely. Consider also that whenever a library is accepted into Boost, the matter of backward compatibility appears. If the accepted library is somehow seriously flawed, that flaw may be difficult and painful to fix in the future. [1]: http://www.boost.org/community/reviews.html
On 12/25/16 3:41 PM, Andrey Semashev wrote:
Consider also that whenever a library is accepted into Boost, the matter of backward compatibility appears.
How so? Certainly there is no requirement that boost libraries support older versions of C++ and/or compilers - and indeed many don't. I don't think I see what you're referring to here.
If the accepted library is somehow seriously flawed,
Then of course it should be rejected. Of course reaching a concensus as to whether it's flawed might not be so easy.
I checked this page and don't see where it says anything relevent to your points. Robert Ramey
On 2016-12-26 10:41, Andrey Semashev wrote:
... Acknowledging the need for a particular functionality is not the same as accepting a particular implementation.
Put bluntly users do not care about implementation. They care about functionality provided. I advocate giving the lib a "foot in the door". That and far better feedback from the users will be a strong incentive for the author to keep working and improving the lib. You seem to prefer all-or-nothing approach. I believe it's unrealistic and bad. You might disagree.
... IMO expecting the author to do everything right, to address all imaginable use-cases and to provide extensive documentation that would satisfy everyone is an impossible task and, therefore, IMO it should not be the expectation during a review. ... I think you misunderstand the point of a review. If you read the review policy[1], you will notice that the main point of a review is to evaluate the library, identify its strong and weak spots.
Well, I suspect it might be quite hard to "misunderstand the point of a review"... It's not the rocket science after all. I honestly do not feel like arguing what the review policy means/says or was meant to mean/say and if everyone follows it to the letter.
By turning a blind eye on a design flaw of a presented library you're doing a disservice to the library author and the library users. Instead, by giving constructive feedback (positive and negative), especially if supported by real world experience, you help the library improvement. In fact, this (the feedback) is the most valuable reward for the author from the review. And let me say, there's nothing humiliating in having a library rejected during a review.
Well, I do not get scolded often these days. So, it is quite "refreshing" to learn that I am "turning a blind eye on a design" and "doing a disservice". I thought I was merely expressing my humble opinion... but probably I got it all wrong. As for the feedback, then I do not see voting something useful "no" to be much of a feedback. Indeed, the author does get initial feedback during a review. However, not all of that feedback is constructive. Some valid, some subjective, some capricious. After it's voted out the feedback is no more. Real valuable feedback will come when the lib is in Boost. People'll start using the lib (I would) in their real projects asking for this and that.
Would not that be wiser to give the author an incentive to address the discussed issues, to accept the lib conditionally and to see the lib evolving, improving, finished as part of Boost? ... There are things that can be fixed post-review. ... There are other things that cannot be fixed easily and would probably require changing library design. Those changes often affect library API and usage patterns, which warrants the need of a new review of the reworked library.
Yes, all that sounds wonderful... on paper. In real life as a user I'll take what's available first. Then, if that is improved in the next version, I'll take it gladly. If design changes and gives me more or fixes something, I'll accept that. I do not remember Spirit transitioning from V1 to not backward-compatible V2 being re-reviewed. I can be wrong though. Everything evolves. Software more so. People adjust and move on. Please do not present it as the end of world.
In those cases it's better to reject the library so that the new, improved version is presented later.
Again, wonderful.. in theory. The reality IMO is different because 1) "improving" is better with real user feedback that you deny to the author by voting "no"; 2) "later" might never come as the review process is quite exhausting/draining; not everyone wants to experience it again; you might say "too bad for him"; I'll say it's bad for the user as well as they end up with your good intentions and no library; 3) "later" might be too late as by that time the user'll have something else in place.
... Consider also that whenever a library is accepted into Boost, the matter of backward compatibility appears. If the accepted library is somehow seriously flawed, that flaw may be difficult and painful to fix in the future.
"difficult", "painful", "backward compatibility"... Come on. I am sure you've been in the industry for a while, have "survived" many upgrades/updates, etc. We all know it's not an issue. We update, adjust, move on.
On Mon, Dec 26, 2016 at 2:58 AM, Vladimir Batov
On 2016-12-26 10:41, Andrey Semashev wrote:
... Acknowledging the need for a particular functionality is not the same as accepting a particular implementation.
Put bluntly users do not care about implementation. They care about functionality provided.
They don't until they hit the issue. I think there is a difference between generic utility library like "unordered" and domain specific library. When we talk about domain specific library - users expect that its "under-the-hood" implementation would meet the highest standards - so they may think of API. Especially when it goes to doing something cross platform. Doing the review I felt that the Windows backend is well written for MSVC but the rest was lacking and was implemented in as stop-gap solution to provide the functionality for Linux. Especially that it wasn't mentioned in any place in docs how it is actually done. (@Antony Polukhin I'm sure you worked hard on it and I apologize if it offends you somehow) But as domain specific library I think there is some way to go. And that is why the original vote was conditional yes. The problem is that the backend needs rework, now with additional issues in frontend I decided to change the vote - because in its current shape. But I do want it to be back for the review after updates and I clearly stayed so.
As for the feedback, then I do not see voting something useful "no" to be much of a feedback.
Indeed - that is why this library received lots of feedback - so the author can do fixes in design and come back.
"difficult", "painful", "backward compatibility"... Come on. I am sure you've been in the industry for a while, have "survived" many upgrades/updates, etc. We all know it's not an issue. We update, adjust, move on.
I must say the poor - or more correctly say non-existant backward compatibility of the boost libraries limits its usefulness significantly in real world industry. I've seen companies that: 1. Stuck with old versions of boost because they can't upgrade without breaking the entire code base 2. Limit themselves to very small parts of boost Artyom Beilis
participants (6)
-
Andrey Semashev -
Antony Polukhin -
Artyom Beilis -
Peter Dimov -
Robert Ramey -
Vladimir Batov