Re: [boost] [stacktrace] review (changing vote to NO)

Subject: Re: [boost] [stacktrace] review (changing vote to NO)
From: Antony Polukhin (antoshkka_at_[hidden])
Date: 2016-12-25 08:45:40

2016-12-25 16:22 GMT+03:00 Artyom Beilis <artyom.beilis_at_[hidden]>:
>> > 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/detail/backend_linux.hpp#L205

> 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