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

Subject: Re: [boost] [Stacktrace] review, please stop discussing non-Stacktrace issues
From: Peter Dimov (lists_at_[hidden])
Date: 2016-12-17 10:31:59

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.

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.

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.

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.

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.

This is not exactly a common C++ style, but async safe code obviously has 
its own rules.