[Stacktrace] Second review begins today 17th Mar ends 26th Mar

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
45 messages Options
123
Reply | Threaded
Open this post in threaded view
|

[Stacktrace] Second review begins today 17th Mar ends 26th Mar

Boost - Dev mailing list
I am pleased to announce the second review of the proposed
Boost.Stacktrace library running from today until Sunday 26th.

Stacktrace is an optionally header-only library providing multiple
implementation backends. At its very simplest it lets you capture the
stack backtrace for the calling thread and to print it to a
std::ostream& of your choice. The basic_stacktrace<> class quacks
like a STL container of frame object instances. The rest of the API
pretty much follows STL design principles for the most part. Use is
therefore unsurprising.

You can find the documentation at
http://apolukhin.github.io/stacktrace/index.html and the github repo
at https://github.com/apolukhin/stacktrace.


Review guidelines
=================
This is a second review of this library with many changes implemented in
response to feedback from the previous review. My previous review report
can be found at http://lists.boost.org/boost-announce/2017/01/0486.php
where I made the following conditions for acceptance:

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.


Antony hasn't entirely met my conditions, hence this second review. He
has instead implemented this changelog:

* Reimplemented boost::stacktrace::basic_stacktrace class to accept
allocators and to have a small size
* Added outputting of the path to the binary object into the
stacktrace, when source location is not available
* Allowed to use `libbacktrace` - a defacto standard for GCC/CLANG
backtracing that does not fork and has a permissive license
* Optimized stacktrace and frames ostreaming
* Removed backends and made the default tracing to have only platform
basic functionality. Added macros to enable additional functionality.
* Fixed (theoretically) MinGW compilation
* Added functions for async-safe dumping into memory or descriptor
* Changed signal handling example to be async-signal-safe
* Allowed users to choose how many frames to skip during stack capture
* Removed `boost::stacktrace::stacktrace` functions and other
internals from printed stack traces
* Updated docs:
    * examples now use Boost.Exception
    * removed all the references to "backend"
    * reduced macro count
    * "Build, Macros and Backends" section was rewritten and became
"Configuration and Build"
    * added note that library is C++03
    * added examples on storing traces into shared memory
    * updated all the human readable dumps


It is up to the Boost community as to whether this revised library is
now acceptable without any conditions, or with conditions.

Reviews should be submitted to the developer list
(boost_at_[hidden]), preferably with '[stacktrace]' in the
subject. Or if you don't wish to for some reason or are not
subscribed to the developer list you can send them privately to me at
's_sourceforge at nedprod dot com'. If so, please let me know whether
or not you'd like your review to be forwarded to the list.

For your review you may wish to consider the following questions:
      - What is your evaluation of the design?
      - What is your evaluation of the implementation?
      - What is your evaluation of the documentation?
      - What is your evaluation of the potential usefulness of the
        library?
      - Did you try to use the library? With what compiler? Did you
        have any problems?
      - How much effort did you put into your evaluation? A glance? A
        quick reading? In-depth study?
      - Are you knowledgeable about the problem domain?

And finally, every review should attempt to answer this question:

      - Do you think the library should be accepted as a Boost
library?

Be sure to say this explicitly so that your other comments don't
obscure your overall opinion.

Even if you do not wish to give a full review any technical comment
regarding the library is welcome as part of the review period and
will help me as the review manager decide whether the library should
be accepted as a Boost library. Any questions about the use of the
library are also welcome.

Niall


--
ned Productions Limited Consulting
http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Stacktrace] Second review begins today 17th Mar ends 26th Mar

Boost - Dev mailing list
AMDG

  The documentation could really use some editing.
I'm seeing a lot of spelling and grammar errors.

In Christ,
Steven Watanabe


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Stacktrace] Second review begins today 17th Mar ends 26th Mar

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
> You can find the documentation at
> http://apolukhin.github.io/stacktrace/index.html and the github repo at
> https://github.com/apolukhin/stacktrace.

A few quick comments.

1. The library should try to not #include <windows.h> unless necessary.
Constructing a stacktrace object and most of its operations besides
operator<< / to_string do not need <windows.h> and it should be possible to
avoid it. The functionality that needs <windows.h> needs to be defined in a
separate header, for example <boost/stacktrace/output.hpp>.

This allows use of the library in header-only mode while still isolating
<windows.h> use into its own source file.

For example, consider throw_with_trace in the documentation; the throw
points, which in a header-only library will be in headers, do not need
<windows.h> and should not drag it in. Only the catch point, which resides
safely in its own source file, does.

Incidentally, safe_dump_win.ipp includes <windows.h>, but does not need to.

2. There should exist a documented way to construct a stacktrace from an
array of void const*. This is currently possible via from_dump, but it's not
guaranteed to work; the format of the dump is technically an implementation
detail of the library and may change.

3. detail::try_demangle is unnecessary, as it does the same thing as
core::demangle. :-)

Apart from that, looks like a substantial improvement. More comments and a
vote possibly coming later.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Stacktrace] Second review begins today 17th Mar ends 26th Mar

Boost - Dev mailing list
> You can find the documentation at
> http://apolukhin.github.io/stacktrace/index.html and the github repo at
> https://github.com/apolukhin/stacktrace.

Question: why does the library use boost::container::vector instead of
std::vector?


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Stacktrace] Second review begins today 17th Mar ends 26th Mar

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
> You can find the documentation at
> http://apolukhin.github.io/stacktrace/index.html and the github repo
> at https://github.com/apolukhin/stacktrace.

Some Cygwin errors:

In file included from
..\..\../boost/stacktrace/detail/frame_unwind.ipp:19:0,
                 from
..\..\..\libs\stacktrace\build\..\src\addr2line.cpp:10:
..\..\../boost/stacktrace/detail/location_from_symbol.hpp:25:7: error:
'Dl_info' in namespace '::' does not name a type
     ::Dl_info dli_;
       ^

..\..\../boost/stacktrace/detail/location_from_symbol.hpp:31:14: error:
'::dladdr' has not been declared
         if (!::dladdr(addr, &dli_)) {
              ^
Cygwin doesn't have Dl_info and dladdr.

..\..\../boost/stacktrace/detail/frame_msvc.ipp:97:36: error: '<::' cannot
begin  a template-argument list [-fpermissive]

..\..\../boost/stacktrace/detail/frame_msvc.ipp:97:36: note: '<:' is an
alternate spelling for '['. Insert whitespace between '<' and '::'

Digraphs are fun. :-)


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Stacktrace] Second review begins today 17th Mar ends 26th Mar

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
> You can find the documentation at
> http://apolukhin.github.io/stacktrace/index.html and the github repo
> at https://github.com/apolukhin/stacktrace.

The heuristic that skips 3 frames (under Windows) doesn't quite work. It
seems that MSVC in whole program optimization mode ignores BOOST_NOINLINE
directives and makes inline decisions that differ in unpredictable ways
between compiles. This program, for instance:

#include <boost/stacktrace.hpp>
#include <iostream>
#include <algorithm>
#include <functional>

void g( int x )
{
    boost::stacktrace::stacktrace st( -3, -1 );
    std::cout << st << std::endl;
}

void f( int x )
{
    std::function<void(int)> h( g );
    std::for_each( &x, &x + 1, g );
}

int main()
{
    f( 1 );
}

gives this trace:

0# boost::stacktrace::detail::this_thread_frames::collect at
c:\projects\boost-git\boost\boost\stacktrace\detail\frame_msvc.ipp:48
1# g at c:\projects\testbed2017\testbed2017.cpp:9
2# main at c:\projects\testbed2017\testbed2017.cpp:20
3# __scrt_common_main_seh at
f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:259
4# BaseThreadInitThunk in kernel32
5# RtlInitializeExceptionChain in ntdll
6# RtlInitializeExceptionChain in ntdll

With the default constructor, #0, #1 and #2 disappear and nothing useful
remains.

There should probably be a way to optionally remove the +1 and +2 hardcoded
skips because passing a nonportable -3 to a size_t is not exactly a best
practice.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Stacktrace] Second review begins today 17th Mar ends 26th Mar

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
2017-03-17 19:15 GMT+03:00 Peter Dimov via Boost <[hidden email]>:

>> You can find the documentation at
>> http://apolukhin.github.io/stacktrace/index.html and the github repo at
>> https://github.com/apolukhin/stacktrace.
>
>
> A few quick comments.
>
> 1. The library should try to not #include <windows.h> unless necessary.
> Constructing a stacktrace object and most of its operations besides
> operator<< / to_string do not need <windows.h> and it should be possible to
> avoid it. The functionality that needs <windows.h> needs to be defined in a
> separate header, for example <boost/stacktrace/output.hpp>.
>
> This allows use of the library in header-only mode while still isolating
> <windows.h> use into its own source file.
>
> For example, consider throw_with_trace in the documentation; the throw
> points, which in a header-only library will be in headers, do not need
> <windows.h> and should not drag it in. Only the catch point, which resides
> safely in its own source file, does.

<windows.h> is a lesser evil. The huge problem is a COM header with a
lot of things in global namespace and a bunch of unportable vendor
specific extensions. One pain of death, I'm not going to rewrite all
the COM macro-spaghetti code (but I'm open to pull requests). The only
good solution to avoid global namespace polution - is not to use the
library in header only mode on MSVC.

> Incidentally, safe_dump_win.ipp includes <windows.h>, but does not need to.

Fixed in develop

> 2. There should exist a documented way to construct a stacktrace from an
> array of void const*. This is currently possible via from_dump, but it's not
> guaranteed to work; the format of the dump is technically an implementation
> detail of the library and may change.

Where that could be useful?

> 3. detail::try_demangle is unnecessary, as it does the same thing as
> core::demangle. :-)

Thanks! Fixed in develop.

--
Best regards,
Antony Polukhin

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Stacktrace] Second review begins today 17th Mar ends 26th Mar

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
2017-03-17 19:50 GMT+03:00 Peter Dimov via Boost <[hidden email]>:

>> You can find the documentation at
>> http://apolukhin.github.io/stacktrace/index.html and the github repo
>> at https://github.com/apolukhin/stacktrace.
>
>
> Some Cygwin errors:
>
> In file included from
> ..\..\../boost/stacktrace/detail/frame_unwind.ipp:19:0,
>                 from ..\..\..\libs\stacktrace\build\..\src\addr2line.cpp:10:
> ..\..\../boost/stacktrace/detail/location_from_symbol.hpp:25:7: error:
> 'Dl_info' in namespace '::' does not name a type
>     ::Dl_info dli_;
>       ^
>
> ..\..\../boost/stacktrace/detail/location_from_symbol.hpp:31:14: error:
> '::dladdr' has not been declared
>         if (!::dladdr(addr, &dli_)) {
>              ^
> Cygwin doesn't have Dl_info and dladdr.

Why id does not define the BOOST_WINDOWS macro?


> ..\..\../boost/stacktrace/detail/frame_msvc.ipp:97:36: error: '<::' cannot
> begin  a template-argument list [-fpermissive]
>
> ..\..\../boost/stacktrace/detail/frame_msvc.ipp:97:36: note: '<:' is an
> alternate spelling for '['. Insert whitespace between '<' and '::'
>
> Digraphs are fun. :-)

Fixed in develop.

--
Best regards,
Antony Polukhin

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Stacktrace] Second review begins today 17th Mar ends 26th Mar

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Antony Polukhin wrote:
> <windows.h> is a lesser evil. The huge problem is a COM header with a lot
> of things in global namespace and a bunch of unportable vendor specific
> extensions. One pain of death, I'm not going to rewrite all the COM
> macro-spaghetti code (but I'm open to pull requests). The only good
> solution to avoid global namespace polution - is not to use the library in
> header only mode on MSVC.

The suggestion is not to rewrite the COM code. It's to not include it unless
needed.

template <class E>
void throw_with_trace(const E& e) {
    throw boost::enable_error_info(e)
        << traced(boost::stacktrace::stacktrace());
}

Here no COM or <windows.h> code is needed, but there's no way to avoid their
inclusion.

> > 2. There should exist a documented way to construct a stacktrace from an
> > array of void const*. This is currently possible via from_dump, but it's
> > not guaranteed to work; the format of the dump is technically an
> > implementation detail of the library and may change.
>
> Where that could be useful?

It's useful if one wants to capture the stack trace manually in order to
avoid a dependency on stacktrace at the capture point.
boost::throw_exception, for instance, could be made to do so.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Stacktrace] Second review begins today 17th Mar ends 26th Mar

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
2017-03-17 20:30 GMT+03:00 Peter Dimov via Boost <[hidden email]>:

>> You can find the documentation at
>> http://apolukhin.github.io/stacktrace/index.html and the github repo
>> at https://github.com/apolukhin/stacktrace.
>
>
> The heuristic that skips 3 frames (under Windows) doesn't quite work. It
> seems that MSVC in whole program optimization mode ignores BOOST_NOINLINE
> directives and makes inline decisions that differ in unpredictable ways
> between compiles. This program, for instance:
>
> #include <boost/stacktrace.hpp>
> #include <iostream>
> #include <algorithm>
> #include <functional>
>
> void g( int x )
> {
>    boost::stacktrace::stacktrace st( -3, -1 );
>    std::cout << st << std::endl;
> }
>
> void f( int x )
> {
>    std::function<void(int)> h( g );
>    std::for_each( &x, &x + 1, g );
> }
>
> int main()
> {
>    f( 1 );
> }
>
> gives this trace:
>
> 0# boost::stacktrace::detail::this_thread_frames::collect at
> c:\projects\boost-git\boost\boost\stacktrace\detail\frame_msvc.ipp:48
> 1# g at c:\projects\testbed2017\testbed2017.cpp:9
> 2# main at c:\projects\testbed2017\testbed2017.cpp:20
> 3# __scrt_common_main_seh at
> f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:259
> 4# BaseThreadInitThunk in kernel32
> 5# RtlInitializeExceptionChain in ntdll
> 6# RtlInitializeExceptionChain in ntdll
>
> With the default constructor, #0, #1 and #2 disappear and nothing useful
> remains.
>
> There should probably be a way to optionally remove the +1 and +2 hardcoded
> skips because passing a nonportable -3 to a size_t is not exactly a best
> practice.

Just removed the skips on Windows in develop. If there's no reliable
way of skipping frames, it's better to show more info rather than skip
a most useful frame with user defined function.

--
Best regards,
Antony Polukhin

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Stacktrace] Second review begins today 17th Mar ends 26th Mar

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Antony Polukhin wrote:

> > Cygwin doesn't have Dl_info and dladdr.
>
> Why id does not define the BOOST_WINDOWS macro?

Cygwin is probably considered a POSIX platform. Which it is.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Stacktrace] Second review begins today 17th Mar ends 26th Mar

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
2017-03-18 13:40 GMT+03:00 Peter Dimov via Boost <[hidden email]>:

> Antony Polukhin wrote:
>>
>> <windows.h> is a lesser evil. The huge problem is a COM header with a lot
>> of things in global namespace and a bunch of unportable vendor specific
>> extensions. One pain of death, I'm not going to rewrite all the COM
>> macro-spaghetti code (but I'm open to pull requests). The only good solution
>> to avoid global namespace polution - is not to use the library in header
>> only mode on MSVC.
>
>
> The suggestion is not to rewrite the COM code. It's to not include it unless
> needed.
>
> template <class E>
> void throw_with_trace(const E& e) {
>    throw boost::enable_error_info(e)
>        << traced(boost::stacktrace::stacktrace());
> }
>
> Here no COM or <windows.h> code is needed, but there's no way to avoid their
> inclusion.

How it could be done?

>> > 2. There should exist a documented way to construct a stacktrace from an
>> > > array of void const*. This is currently possible via from_dump, but it's >
>> > not guaranteed to work; the format of the dump is technically an >
>> > implementation detail of the library and may change.
>>
>> Where that could be useful?
>
>
> It's useful if one wants to capture the stack trace manually in order to
> avoid a dependency on stacktrace at the capture point.
> boost::throw_exception, for instance, could be made to do so.

std::thread has no constructor from std::thread::native_handle_t and
I'd like to follow that pattern. If you've done something using
platform specific methods - you're on your own.

But you still can use the boost::stacktrace::frame to decode frames one by one:

void* data[10]; // captured frames
// ...
for (void* f: data) {
    std::cout << boost::stacktrace::frame(f);
}

--
Best regards,
Antony Polukhin

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Stacktrace] Second review begins today 17th Mar ends 26th Mar

Boost - Dev mailing list
Antony Polukhin via Boost wrote:

> > template <class E>
> > void throw_with_trace(const E& e) {
> >    throw boost::enable_error_info(e)
> >        << traced(boost::stacktrace::stacktrace());
> > }
> >
> > Here no COM or <windows.h> code is needed, but there's no way to avoid
> > their inclusion.
>
> How it could be done?

By splitting stacktrace.hpp and frame.hpp into two headers, one with the
parts that do not need <windows.h>, the other with the rest. The code above
would then be able to only include the first header. Picking some names at
random,

stacktrace.hpp
    stacktrace_def.hpp
    stacktrace_impl.hpp

frame.hpp
    frame_def.hpp
    frame_impl.hpp

stacktrace_def.hpp
    frame_def.hpp

stacktrace_impl.hpp
    frame_impl.hpp

although currently stacktrace has no impl parts, so it could be simplified
to

stacktrace.hpp
    stacktrace_def.hpp
    frame_impl.hpp

stacktrace_def.hpp
    frame_def.hpp

frame.hpp
    frame_def.hpp
    frame_impl.hpp

> std::thread has no constructor from std::thread::native_handle_t and I'd
> like to follow that pattern. If you've done something using
platform specific methods - you're on your own.

This makes no sense to me, especially since frame is constructible from void
const*. stacktrace is just an array of frames.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Stacktrace] Second review begins today 17th Mar ends 26th Mar

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
> > > Cygwin doesn't have Dl_info and dladdr.

These are the changes required:

C:\Projects\boost-git\boost\libs\stacktrace>git diff
diff --git a/include/boost/stacktrace/detail/frame_unwind.ipp
b/include/boost/st
index 8cb8e2a..37b7a7e 100644
--- a/include/boost/stacktrace/detail/frame_unwind.ipp
+++ b/include/boost/stacktrace/detail/frame_unwind.ipp
@@ -130,7 +130,7 @@ std::string to_string(const frame* frames, std::size_t
size)


std::string frame::name() const {
-#ifndef BOOST_WINDOWS
+#if !defined(BOOST_WINDOWS) && !defined(__CYGWIN__)
     ::Dl_info dli;
     const bool dl_ok = !!::dladdr(addr_, &dli);
     if (dl_ok && dli.dli_sname) {
diff --git a/include/boost/stacktrace/detail/location_from_symbol.hpp
b/include/
index 7faf26a..d20b0d6 100644
--- a/include/boost/stacktrace/detail/location_from_symbol.hpp
+++ b/include/boost/stacktrace/detail/location_from_symbol.hpp
@@ -12,7 +12,7 @@
#   pragma once
#endif

-#ifndef BOOST_WINDOWS
+#if !defined(BOOST_WINDOWS) && !defined(__CYGWIN__)
#   include <dlfcn.h>
#else
#   include <boost/detail/winapi/dll.hpp>
@@ -21,7 +21,7 @@
namespace boost { namespace stacktrace { namespace detail {

class location_from_symbol {
-#ifndef BOOST_WINDOWS
+#if !defined(BOOST_WINDOWS) && !defined(__CYGWIN__)
     ::Dl_info dli_;

public:


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Stacktrace] Second review begins today 17th Mar ends 26th Mar

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
> You can find the documentation at
> http://apolukhin.github.io/stacktrace/index.html and the github repo
> at https://github.com/apolukhin/stacktrace.

I wonder if basic_stacktrace, currently

template <class Allocator>
class basic_stacktrace {
    boost::container::vector<frame, Allocator> impl_;
    /*...*/
};

would be better of as

template < class Container = std::vector<frame> >
class basic_stacktrace {
    static_assert( std::is_same<typename Container::value_type,
frame>::value,
        "The value_type of the Container parameter must be
stacktrace::frame" );

    Container impl_;

    /*...*/
};

This still supports the current functionality of replacing the allocator,
but it also allows

    template<size_t N> using fixed_stacktrace =
basic_stacktrace<std::array<frame, N>>;

which gets us the previous iteration of the library. Fixed storage does have
its uses, and while it's possible to use an in-place allocator to get it,
this is much easier.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Stacktrace] Second review begins today 17th Mar ends 26th Mar

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
2017-03-18 14:11 GMT+03:00 Peter Dimov via Boost <[hidden email]>:

> Antony Polukhin via Boost wrote:
>>
>> > template <class E>
>> > void throw_with_trace(const E& e) {
>> >    throw boost::enable_error_info(e)
>> >        << traced(boost::stacktrace::stacktrace());
>> > }
>> >
>> > Here no COM or <windows.h> code is needed, but there's no way to avoid >
>> > their inclusion.
>>
>> How it could be done?
>
>
> By splitting stacktrace.hpp and frame.hpp into two headers, one with the
> parts that do not need <windows.h>, the other with the rest. The code above
> would then be able to only include the first header. Picking some names at
> random,
>
> stacktrace.hpp
>    stacktrace_def.hpp
>    stacktrace_impl.hpp
>
> frame.hpp
>    frame_def.hpp
>    frame_impl.hpp
>
> stacktrace_def.hpp
>    frame_def.hpp
>
> stacktrace_impl.hpp
>    frame_impl.hpp
>
> although currently stacktrace has no impl parts, so it could be simplified
> to
>
> stacktrace.hpp
>    stacktrace_def.hpp
>    frame_impl.hpp
>
> stacktrace_def.hpp
>    frame_def.hpp
>
> frame.hpp
>    frame_def.hpp
>    frame_impl.hpp

windows.h is a minor problem of a single platform. It already has an
ultimate fix - use the non header-only version of the library. Most of
the Boost users do not care about windows.h inclusion, those who care
can link with the library. I'm not going to uglify the library by
doubling/tripling the headers count and I'm definitely not going to
make it's understanding harder just to satisfy the minority of users
that use win platform + use MSVC + do not like windows.h header + do
not wish to link + somehow wish to use only the non functional COM
part of the library (that contains only a single CaptureStackBackTrace
call).

>> std::thread has no constructor from std::thread::native_handle_t and I'd
>> like to follow that pattern. If you've done something using
>
> platform specific methods - you're on your own.
>
> This makes no sense to me, especially since frame is constructible from void
> const*. stacktrace is just an array of frames.

It also increases the risk that people will capture the frames in a wrong way:
* It's very easy to do it wrong on Windows, because it has multiple
APIs for doing so and only one of them is safe. Moreover, the bad APIs
have more examples and docs, so it's easier to choose wrongly.
* It's terribly easy to do it wrong on POSIX, because it has
documentation only on a bad API.

--
Best regards,
Antony Polukhin

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Stacktrace] Second review begins today 17th Mar ends 26th Mar

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
2017-03-18 14:48 GMT+03:00 Peter Dimov via Boost <[hidden email]>:

>> > > Cygwin doesn't have Dl_info and dladdr.
>
>
> These are the changes required:
>
> C:\Projects\boost-git\boost\libs\stacktrace>git diff
> diff --git a/include/boost/stacktrace/detail/frame_unwind.ipp
> b/include/boost/st
> index 8cb8e2a..37b7a7e 100644
> --- a/include/boost/stacktrace/detail/frame_unwind.ipp
> +++ b/include/boost/stacktrace/detail/frame_unwind.ipp
> @@ -130,7 +130,7 @@ std::string to_string(const frame* frames, std::size_t
> size)
>
>
> std::string frame::name() const {
> -#ifndef BOOST_WINDOWS
> +#if !defined(BOOST_WINDOWS) && !defined(__CYGWIN__)
>     ::Dl_info dli;
>     const bool dl_ok = !!::dladdr(addr_, &dli);
>     if (dl_ok && dli.dli_sname) {
> diff --git a/include/boost/stacktrace/detail/location_from_symbol.hpp
> b/include/
> index 7faf26a..d20b0d6 100644
> --- a/include/boost/stacktrace/detail/location_from_symbol.hpp
> +++ b/include/boost/stacktrace/detail/location_from_symbol.hpp
> @@ -12,7 +12,7 @@
> #   pragma once
> #endif
>
> -#ifndef BOOST_WINDOWS
> +#if !defined(BOOST_WINDOWS) && !defined(__CYGWIN__)
> #   include <dlfcn.h>
> #else
> #   include <boost/detail/winapi/dll.hpp>
> @@ -21,7 +21,7 @@
> namespace boost { namespace stacktrace { namespace detail {
>
> class location_from_symbol {
> -#ifndef BOOST_WINDOWS
> +#if !defined(BOOST_WINDOWS) && !defined(__CYGWIN__)
>     ::Dl_info dli_;
>
> public:
>
>

Fixed in develop. Thanks for the patch!

--
Best regards,
Antony Polukhin

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Stacktrace] Second review begins today 17th Mar ends 26th Mar

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
2017-03-18 16:18 GMT+03:00 Peter Dimov via Boost <[hidden email]>:

>> You can find the documentation at
>> http://apolukhin.github.io/stacktrace/index.html and the github repo
>> at https://github.com/apolukhin/stacktrace.
>
>
> I wonder if basic_stacktrace, currently
>
> template <class Allocator>
> class basic_stacktrace {
>    boost::container::vector<frame, Allocator> impl_;
>    /*...*/
> };
>
> would be better of as
>
> template < class Container = std::vector<frame> >
> class basic_stacktrace {
>    static_assert( std::is_same<typename Container::value_type,
> frame>::value,
>        "The value_type of the Container parameter must be stacktrace::frame"
> );
>
>    Container impl_;
>
>    /*...*/
> };
>
> This still supports the current functionality of replacing the allocator,
> but it also allows
>
>    template<size_t N> using fixed_stacktrace =
> basic_stacktrace<std::array<frame, N>>;
>
> which gets us the previous iteration of the library. Fixed storage does have
> its uses, and while it's possible to use an in-place allocator to get it,
> this is much easier.

This won't work, because std::array has no push_back/pop_back methods.

Is boost::container::vector bothers you? I've used that container
because it could be used in shred memory, so that people could store
stacktrace in shared memory. Now it does not look like a good idea, so
I'll probably change it to std::vector.

--
Best regards,
Antony Polukhin

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Stacktrace] Second review begins today 17th Mar ends 26th Mar

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Antony Polukhin wrote:

> windows.h is a minor problem of a single platform.

windows.h is not a minor problem at all. It defines one million macros.

> It already has an ultimate fix - use the non header-only version of the
> library.

This is not a decision that a library developer can make. If I'm writing a
header-only library (that does nothing Windows-specific), I absolutely
CANNOT afford to include <windows.h>, which means that I can't include
stacktrace.hpp. Whether BOOST_STACKTRACE_LINK is defined is not up to me.

> > This makes no sense to me, especially since frame is constructible from
> > void const*. stacktrace is just an array of frames.
>
> It also increases the risk that people will capture the frames in a wrong
> way:

Not quite seeing that risk, given that the default constructor is much
easier to use. You have to go out of your way to capture manually, for which
presumably you have reasons.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [Stacktrace] Second review begins today 17th Mar ends 26th Mar

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Antony Polukhin wrote:

> >    template<size_t N> using fixed_stacktrace =
> > basic_stacktrace<std::array<frame, N>>;
>
> This won't work, because std::array has no push_back/pop_back methods.

Ordinarily it won't; one would need to use boost::container::static_vector.
But if we go this way, it should be specialized to work with std::array as
well.

> Is boost::container::vector bothers you?

boost::container::vector is not much of a problem; templating on a container
just allows fixed storage without having to write a fixed storage allocator.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
123