Re: [boost] [Stacktrace] review, please stop discussing non-Stacktrace issues

Subject: Re: [boost] [Stacktrace] review, please stop discussing non-Stacktrace issues
From: Antony Polukhin (antoshkka_at_[hidden])
Date: 2016-12-17 11:21:28

2016-12-17 18:31 GMT+03:00 Peter Dimov <lists_at_[hidden]>:
> Niall Douglas wrote:
>
>> 3. Please could people actually review Stacktrace's implementation code
>> instead of inferring implementation from what someone else said here about
>> something they half read in the tutorial. Specifically, is its API design
>> solid? Is the use of std::string in its public API acceptable?
>
>
> There's nothing wrong with using std::string in principle, but it's
> obviously not async safe. The operations however that return std::strings
> are also not async safe at the moment, so it's not the std::string return
> value that's the problem.

Async safety of those functions won't change.

> The documentation has an example of printing a stack trace from a SIGSEGV
> handler, which is undefined behavior as it stands. So it's not clear to me
> whether the library is supposed to be useful - in its decoding/printing
> part, not just in its capture part - in signal handlers.
>
> Changing/augmenting the interface to be async safe would be easy,
>
>    size_t frame::get_name( char* buffer, size_t size ) const;
>
> instead of, or in addition to,
>
>    std::string frame::name() const;
>
> and
>
>    void frame::print_to_fd( int fd ) const;
>
> in addition to
>
>    ostream& operator<<( ostream&, frame const & );
>
> but if the implementation can't be made async safe, this is not going to be
> of much use.

Windows implementation of those functions is definitely impossible to
make async safe, as the Windows backend uses COM. And that can not be
changed :(

> A quick look of the implementation - and the list of POSIX async safe
> functions - was not enough for me to be able to resolve this question one
> way or the other. Most of what the Linux backend is doing is already async
> safe, but not everything, addr2line has an option to demangle, so the
> dependency on core::demangle could in principle be avoided... but I'm not
> quite sure.

I'd like to avoid functions that are async-safe on one platform and
async unsafe on other. Mixing safety is a perfect way for producing
issues in user code.

> There is also an indication that the current frame interface is not the
> right one. In
>
>    std::string name = f.name();
>    if (!name.empty()) {
>        os << name;
>    } else {
>        os << f.address();
>    }
>
>    const std::size_t source_line = f.source_line();
>    if (source_line) {
>        os << " at " << f.source_file() << ':' << source_line;
>    }
>
> which is perfectly reasonable code given the current interface, addr2line is
> called three times instead of just one. And in operator<< for a stacktrace
> of depth N, addr2line will be called 3*N times instead of one.
>
> The operator<< implementations themselves can be fixed, but user code that
> does the same will obviously not be able to benefit.

Yes, I'm planing to provide optimized versions of ostream operators
after the review. Windows backend has exactly the same problem.

> I understand why it's better for the value_type of
> stacktrace::const_iterator to be 'frame' instead of just 'void*', but the
> accessors each calling addr2line is a symptom of a serious problem that the
> 'stupid' void* design:
>
> void get_frame_info( void* address, string& function, string& file, size_t&
> line );
>
> lacks.
>
> stacktrace itself could provide a function that calls a user-provided
> callback
>
> void process_frame_info( size_t index, void* address, char const* function,
> char const* file, size_t line );
>
> for each frame. This would be able to call addr2line just once, and it's
> also an async-safe interface, if the implementation can be made so.

Hm... I need to think about it, but I'm not sure that the feature is
widely useful. Most of the time people just want all the available
information and default ostream operators are OK for them. Those, who
want a different (shorter) output format are typically choosing
between outputting only function names or outputting only function
location in source file. Getting function name and function location
in source file are two different COM calls on Windows, so such
callback will not work great on Windows.

-- 
Best regards,
Antony Polukhin