[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
|

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.

I see that frame supports construction from a function pointer:

void print_signal_handler_and_exit() {
    void* p = reinterpret_cast<void*>(::signal(SIGSEGV, SIG_DFL));
    boost::stacktrace::frame f(p);
    std::cout << f << std::endl;
    std::exit(0);
}

but it takes void*.

On both POSIX and Windows, pointers to functions and void* have the same
representation, but if we want to be strictly standard conforming, frame
should take void(*)(). The standard guarantees that pointers to functions
can be cast to void(*)(), but it doesn't guarantee it for void*.


_______________________________________________
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
On 2017-03-18 02:09, Niall Douglas via Boost wrote:
> I am pleased to announce the second review of the proposed
> Boost.Stacktrace library running from today until Sunday 26th.

Started reading the docs. Looks very satisfying and I see wanting/using
it extensively. I certainly vote for acceptance. My personal view is
that the lib is very useful/needed. So, the sooner it gets wider
deployment the sooner issues and niggles are to be addressed.

The default deployment seems straightforward with the option for
customization for adventurous. I like that. I was irked by
boost::stacktrace::stacktrace() invocation though. Not a huge drama but
boost::stacktrace() certainly looks far more natural to me. Started
looking what there was in the boost::stacktrace namespace. It appears
quite a few things could be be better encapsulated in the relevant
classes. For ex., basic_stacktrace and frame comparison operators are
all outside the respective classes.

namespace boost {
   namespace stacktrace {
     class frame;

     bool operator<(const frame & lhs, const frame & rhs);
     bool operator>(const frame & lhs, const frame & rhs);
     ...

However, the frame class has no implicit constructor to take advantage
of such placement. So, those operators might go (and belong IMO) inside
the frame class. Same IMO goes for basic_stacktrace. Functions in the
boost::stacktrace namespace might become static functions inside
boost::stacktrace class. Supporting functionality might go into some
boost::stacktrace_detail namespace. That'd allow boost::stacktrace to be
actually a class and consequently visible to the users as simply
boost::stacktrace(). That'd be nice.








_______________________________________________
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.

This portion of build/Jamfile.v2:

lib boost_stacktrace_basic
   : # sources
     ../src/basic.cpp
   : # requirements
     <warnings>all
     <target-os>linux:<library>dl
     <link>shared:<define>BOOST_STACKTRACE_DYN_LINK=1
     [ check-target-builds ../build//WinDbg : <build>no ]
   : # default build
   : # usage-requirements
     #<link>shared:<define>BOOST_STACKTRACE_DYN_LINK=1
   ;

looks a bit odd; why would building boost_stacktrace_basic require WinDbg?
Probably a copy/paste error.


_______________________________________________
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
On Sat, Mar 18, 2017 at 7:17 AM, Antony Polukhin via Boost <
[hidden email]> wrote:

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

I disagree, and it shouldn't be difficult to avoid it. One option is to
split the platform-specific code in a windows-specific stack-trace header.
Another is to (if possible) hide the offending code behind a template which
doesn't trip the compiler if the user doesn't instantiate it, and requires
the user to manually include windows.h if he does.


> Most of
> the Boost users do not care about windows.h inclusion
>

Most Boost users are not qualified to judge what Boost libraries should and
shouldn't do. Consider that e.g. bind.hpp won't even make use of MSVC
platform-specific calling conventions, the motivation being to prevent
unsuspecting programmers from accidentally writing platform-specific code.

Emil

_______________________________________________
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 17:39 GMT+03:00 Peter Dimov via Boost <[hidden email]>:

> 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.

I've tried to move code around and separate the headers and I've
failed. Headers+docs on non MS platforms start to look ugly, code does
not compile separately from non COM parts or just becomes useless or
COM headers leak everywhere. I'd appreciate a pull request, this issue
is beyond my fixing capabilities.

--
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 17:39 GMT+03:00 Peter Dimov via Boost <[hidden email]>:
> 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.

dbgeng.h defines 10M macros. Getting rid of windows.h solves only
small part of the problem.

Looks like the right solution would be not to minify headers that use
the windbh.h/dbgeng.h but rather write a correct forward declarations
of windbh.h/dbgeng.h stuff, just like Boost.WinAPI does.

My COM knowledge is limited, so I'd *really appreciate* some help with
dbgeng.h. This will also fix multiple MinGW issues and will allow a
better MinGW (and even Clang on Win) stacktracing out of the box.

I'll take care of windows.h stuff soon.

--
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
On 19/03/2017 06:38, Antony Polukhin via Boost wrote:

> 2017-03-18 17:39 GMT+03:00 Peter Dimov via Boost <[hidden email]>:
>> 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.
>
> dbgeng.h defines 10M macros. Getting rid of windows.h solves only
> small part of the problem.
>
> Looks like the right solution would be not to minify headers that use
> the windbh.h/dbgeng.h but rather write a correct forward declarations
> of windbh.h/dbgeng.h stuff, just like Boost.WinAPI does.
>
> My COM knowledge is limited, so I'd *really appreciate* some help with
> dbgeng.h. This will also fix multiple MinGW issues and will allow a
> better MinGW (and even Clang on Win) stacktracing out of the box.
>
> I'll take care of windows.h stuff soon.

Putting on my review manager hat, here is my position on <windows.h>:

1. The size of windows.h is Microsoft's fault, not the code which
includes it.

2. *Gratuitous* includes of windows.h is a showstopper for a high
quality library, so including windows.h just for one or two APIs is a
bad thing.

3. If however a library really makes extensive use of windows.h and if
the user prefers the header only edition of the library, then including
all of windows.h is *the price they pay*. They have the choice: either
go header only and include windows.h, or don't go header only and don't
include it.

4. In case anyone thinks I'm being unreasonably lax here, nobody much
complains about header only inclusion of <sys/unistd.h> etc. That's
because the size of <windows.h> is *Microsoft's fault*. It's the cost
they impose on every Windows dev. We library developers are not at
fault. Blame Microsoft.

5. I think it's highly unreasonable to ask Antony to remove windows.h
from the headers only build. The Windows backend unavoidably uses COM
with all that magic compiler-specific GUID attribute markup and such
fun, and the headers brought in are auto generated from COM IDL and hand
tuned by Microsoft. Hacking that out just to tick a box for some people
will produce a much more brittle, unreliable and hard to maintain library.

I as review manager certainly will not stop Stacktrace entering Boost on
this point. If anything, if Antony ruins the library just to eliminate
<windows.h> inclusion, I'd actually recommend its rejection.
I appreciate that this will not be a popular opinion, but I'm the review
manager and that's my judgement on it.

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
On 3/19/2017 5:34 AM, Niall Douglas via Boost wrote:

> On 19/03/2017 06:38, Antony Polukhin via Boost wrote:
>> 2017-03-18 17:39 GMT+03:00 Peter Dimov via Boost <[hidden email]>:
>>> 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.
>>
>> dbgeng.h defines 10M macros. Getting rid of windows.h solves only
>> small part of the problem.
>>
>> Looks like the right solution would be not to minify headers that use
>> the windbh.h/dbgeng.h but rather write a correct forward declarations
>> of windbh.h/dbgeng.h stuff, just like Boost.WinAPI does.
>>
>> My COM knowledge is limited, so I'd *really appreciate* some help with
>> dbgeng.h. This will also fix multiple MinGW issues and will allow a
>> better MinGW (and even Clang on Win) stacktracing out of the box.
>>
>> I'll take care of windows.h stuff soon.
>
> Putting on my review manager hat, here is my position on <windows.h>:
>
> 1. The size of windows.h is Microsoft's fault, not the code which
> includes it.
>
> 2. *Gratuitous* includes of windows.h is a showstopper for a high
> quality library, so including windows.h just for one or two APIs is a
> bad thing.
>
> 3. If however a library really makes extensive use of windows.h and if
> the user prefers the header only edition of the library, then including
> all of windows.h is *the price they pay*. They have the choice: either
> go header only and include windows.h, or don't go header only and don't
> include it.

If a library makes extensive use of windows.h in a header file which
only includes functionality which needs windows.h, then naturally an
end-user including that header file gets windows.h included. But if I am
using some libraries header file, which has a number of constructs I
would like to use, none of which needs windows.h, I do not want
windows.h included. I realize that this is sometimes difficult for a
library developer to do, but it should be a relatively easy task
compared to the high level C++ development that most Boost library
developers actually do. Is splitting header files into particular
functionality really that hard ? if the library developer offers
specific header files for windows.h functionality and functionality not
using windows.h, while still providing a general header file which
includes both, then I as an end-user can at least choose whether
windows.h is going to be included or not depending on what header file I
include.

BTW substitute
"any-ridiculously-large-header-file-which-includes-endless-nondistinct-macros-and-constructs"
for windows.h and the argument above is exactly the same.

In other words I am in 100% agreement with Peter even while I realize
that separating windows.h functionality in stacktrace may be some work
for Antony. But given that the windows.h behemoth is Microsoft's fault
the general principle of C++, of not "using" what one does not need,
still applies.

>
> 4. In case anyone thinks I'm being unreasonably lax here, nobody much
> complains about header only inclusion of <sys/unistd.h> etc. That's
> because the size of <windows.h> is *Microsoft's fault*. It's the cost
> they impose on every Windows dev. We library developers are not at
> fault. Blame Microsoft.
>
> 5. I think it's highly unreasonable to ask Antony to remove windows.h
> from the headers only build. The Windows backend unavoidably uses COM
> with all that magic compiler-specific GUID attribute markup and such
> fun, and the headers brought in are auto generated from COM IDL and hand
> tuned by Microsoft. Hacking that out just to tick a box for some people
> will produce a much more brittle, unreliable and hard to maintain library.

Peter did not suggest not using windows.h itself AFAICS. He just
suggested that stacktrace functionality which does not need it be
segregated into specific headers which do not include it. Furthermore if
stacktrace still has the general header, and promotes the use of it,
rather than specific headers, that's fine as long as the end-user has a
choice. OTOH if stacktrace functionality which appears not to need the
constructs in windows.h still does need it under the covers in the
header only version of the library, then I understand Antony's objection.

>
> I as review manager certainly will not stop Stacktrace entering Boost on
> this point. If anything, if Antony ruins the library just to eliminate
> <windows.h> inclusion, I'd actually recommend its rejection.
> I appreciate that this will not be a popular opinion, but I'm the review
> manager and that's my judgement on it.
>
> Niall
>



_______________________________________________
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
> Peter did not suggest not using windows.h itself AFAICS. He just
> suggested that stacktrace functionality which does not need it be
> segregated into specific headers which do not include it. Furthermore if
> stacktrace still has the general header, and promotes the use of it,
> rather than specific headers, that's fine as long as the end-user has a
> choice. OTOH if stacktrace functionality which appears not to need the
> constructs in windows.h still does need it under the covers in the
> header only version of the library, then I understand Antony's objection.

If you choose the null backend, then yes windows.h should not be included.

If you choose the non-null backend, which needs to open up a COM session
with the Debug Engine to debug the calling executable, I think windows.h
(and the COM headers which are also massive) pretty hard to avoid.

And for the record, I too feel little love for windows.h, though they do
provide NOMINMAX and WIN32_LEAN_AND_MEAN to make the problem less awful.
One HUGE win with C++ Modules will be that we can finally work around
windows.h once and for all.

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
On 3/19/2017 7:30 PM, Niall Douglas via Boost wrote:

>> Peter did not suggest not using windows.h itself AFAICS. He just
>> suggested that stacktrace functionality which does not need it be
>> segregated into specific headers which do not include it. Furthermore if
>> stacktrace still has the general header, and promotes the use of it,
>> rather than specific headers, that's fine as long as the end-user has a
>> choice. OTOH if stacktrace functionality which appears not to need the
>> constructs in windows.h still does need it under the covers in the
>> header only version of the library, then I understand Antony's objection.
>
> If you choose the null backend, then yes windows.h should not be included.
>
> If you choose the non-null backend, which needs to open up a COM session
> with the Debug Engine to debug the calling executable, I think windows.h
> (and the COM headers which are also massive) pretty hard to avoid.

Choosing only the windbg backend needs COM. I am still arguing along
with Peter that merely choosing the windbg backend, or having it
automatically chosen for me when I am using VC++, should not bring in
windows.h until it is actually needed by the code I am invoking either
in the stacktrace or frame classes.

I can see using stacktrace where I am just passing already generated
stacktrace or frame objects and once COM is needed to produce
stacktraces or frames I see no reason why windows.h must be included in
header files that do not need that functionality which generated the
appropriate information anymore.

>
> And for the record, I too feel little love for windows.h, though they do
> provide NOMINMAX and WIN32_LEAN_AND_MEAN to make the problem less awful.
> One HUGE win with C++ Modules will be that we can finally work around
> windows.h once and for all.
>
> Niall
>



_______________________________________________
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
On Sun, Mar 19, 2017 at 6:42 PM, Edward Diener via Boost <
[hidden email]> wrote:

> On 3/19/2017 7:30 PM, Niall Douglas via Boost wrote:
>
>> Peter did not suggest not using windows.h itself AFAICS. He just
>>> suggested that stacktrace functionality which does not need it be
>>> segregated into specific headers which do not include it. Furthermore if
>>> stacktrace still has the general header, and promotes the use of it,
>>> rather than specific headers, that's fine as long as the end-user has a
>>> choice. OTOH if stacktrace functionality which appears not to need the
>>> constructs in windows.h still does need it under the covers in the
>>> header only version of the library, then I understand Antony's objection.
>>>
>>
>> If you choose the null backend, then yes windows.h should not be included.
>>
>> If you choose the non-null backend, which needs to open up a COM session
>> with the Debug Engine to debug the calling executable, I think windows.h
>> (and the COM headers which are also massive) pretty hard to avoid.
>>
>
> Choosing only the windbg backend needs COM. I am still arguing along with
> Peter that merely choosing the windbg backend, or having it automatically
> chosen for me when I am using VC++, should not bring in windows.h until it
> is actually needed by the code I am invoking either in the stacktrace or
> frame classes.
>

+1

Emil

_______________________________________________
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
> Choosing only the windbg backend needs COM. I am still arguing along
> with Peter that merely choosing the windbg backend, or having it
> automatically chosen for me when I am using VC++, should not bring in
> windows.h until it is actually needed by the code I am invoking either
> in the stacktrace or frame classes.
>
> I can see using stacktrace where I am just passing already generated
> stacktrace or frame objects and once COM is needed to produce
> stacktraces or frames I see no reason why windows.h must be included in
> header files that do not need that functionality which generated the
> appropriate information anymore.

So, you're looking for the stacktrace and frame classes to not provide
implementations of those member functions which rely on windows.h?

I assume you then want an additional header file e.g.
"stacktrace_extra.hpp" which provides implementations for those?

If you do want this, one technique I've used in AFIO is for the public
class to be mostly virtual member functions so header only includes
don't drag in tons of implementation. Filing system operations are much
heavier than Stacktrace's though, for AFIO a virtual function call is
immaterial. I suppose a performance caring Stacktrace user would simply
include the kitchen sink and rely on "override final" to eliminate the
overhead.

Is this what you are looking for?

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
In reply to this post by Boost - Dev mailing list
2017-03-20 2:42 GMT+01:00 Edward Diener via Boost <[hidden email]>:

On 3/19/2017 7:30 PM, Niall Douglas via Boost wrote:

>
>> Peter did not suggest not using windows.h itself AFAICS. He just
>>> suggested that stacktrace functionality which does not need it be
>>> segregated into specific headers which do not include it. Furthermore if
>>> stacktrace still has the general header, and promotes the use of it,
>>> rather than specific headers, that's fine as long as the end-user has a
>>> choice. OTOH if stacktrace functionality which appears not to need the
>>> constructs in windows.h still does need it under the covers in the
>>> header only version of the library, then I understand Antony's objection.
>>>
>>
>> If you choose the null backend, then yes windows.h should not be included.
>>
>> If you choose the non-null backend, which needs to open up a COM session
>> with the Debug Engine to debug the calling executable, I think windows.h
>> (and the COM headers which are also massive) pretty hard to avoid.
>>
>
> Choosing only the windbg backend needs COM. I am still arguing along with
> Peter that merely choosing the windbg backend, or having it automatically
> chosen for me when I am using VC++, should not bring in windows.h until it
> is actually needed by the code I am invoking either in the stacktrace or
> frame classes.
>
> I can see using stacktrace where I am just passing already generated
> stacktrace or frame objects and once COM is needed to produce stacktraces
> or frames I see no reason why windows.h must be included in header files
> that do not need that functionality which generated the appropriate
> information anymore.


Maybe this could be fixed by providing two windows backends: the current
one, plus another one that offers a limited functionality but does not
include the COM stuff? This would give a control to the users over the
trade-off.

Regards,
&rzej;

_______________________________________________
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
Niall Douglas wrote:

> So, you're looking for the stacktrace and frame classes to not provide
> implementations of those member functions which rely on windows.h?
>
> I assume you then want an additional header file e.g.
> "stacktrace_extra.hpp" which provides implementations for those?

Something like that.

There are three possible ways to do it, and I haven't yet settled on one, or
I'd have issued a pull request, as Antony asked.

1.

#include "stacktrace.hpp" // as today, includes everything
#include "stacktrace_def.hpp" // stacktrace without op<< or to_string

In header-only libraries that don't use op<< or to_string, include the
second header instead of the first.

2.

#include "stackrace/stacktrace.hpp" // without op<< or to_string
#include "stacktrace/output.hpp" // op<<, to_string

Straightforward split into two headers, the difference is only that you need
to explicitly include output.hpp for op<<.

3.

#include "stacktrace.hpp" // declarations of everything, no definitions, no
windows.h/COM

#define BOOST_STACKTRACE_IMPLEMENTATION
#include "stacktrace.hpp" // definitions, using windows.h, COM

The usual way of packaging a non-header-only library in headers. The user
defines BOOST_STACKTRACE_IMPLEMENTATION in one and only one .cpp file. No
BOOST_STACKTRACE_LINK macro is necessary - if you link with the library, you
just don't need to do the aforementioned.

At the moment, I tend towards the third alternative, because it's a known
practice and replaces the already existing BOOST_STRACKTRACE_LINK mechanism,
so it doesn't introduce additional configuration complexity.

Ideally, in (3), if you don't use op<< or to_string (or frame::name and so
on), you won't need to include the implementation. So if you use a
header-only library that includes stack traces in its exceptions, but you
ignore these stack traces because you know nothing about the stacktrace
library and don't use it, you won't have to pay the price if
linking/including the implementation.

In implementation terms, this_thread_frames::collect will have to be
header-only so that the stacktrace constructors do not trigger link errors,
the rest not.


_______________________________________________
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-20 4:42 GMT+03:00 Edward Diener via Boost <[hidden email]>:
> <...> and once COM is needed to produce
> stacktraces or frames <...>

It is required to produce anything useful from frame/stacktrace.
Following operations use COM (on MSVC only):
* ostreaming stacktrace
* ostreaming frame
* getting function name from frame
* getting source file location from frame
* getting line in source file from frame


Now everyone here agrees that windows.h and COM is bad. But extracting
out the windows.h/COM - is beyond my abilities. I'm not a lazy-bad-ass
that does not wish to remove windows.h just because I do not care. It
is because I've failed to do so. Such removal requires that all the
BOOST_STACKTRACE_*_tuning_macro+BOOST_FOCEINLINE+BOOST_NOINLINE+skip_frames
stuff must cooperate well on multiple platforms in header only and
link-library modes. Also I can not see how you're going to use the
library without 95% of functionality - I just do not understand what
functions to move in what places. Moreover, as I understand at least
two reviewers would like to have absolutely different functionality to
depend or not on windows.h

I'd gladly merge all the changes if some hero would produce a right
pull request. I've failed to produce such.

What is more important - moving away windows.h is a non breaking
change that does not change the library interface (API). So all the
messing with headers, moving code and attributes around could be done
any time later. Moreover, windows.h never was a stopper for inclusion.
'find -name "*.hpp" -print0 | xargs -0 grep "windows\.h"' yields 14
libraries that use windows.h in header files.

Now, when everyone agrees that windows.h is bad but nobody has the
will or knowledge to fix it, let's review the remaining parts of the
library.


--
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 18:58 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.
<...>
> On both POSIX and Windows, pointers to functions and void* have the same
> representation, but if we want to be strictly standard conforming, frame
> should take void(*)(). The standard guarantees that pointers to functions
> can be cast to void(*)(), but it doesn't guarantee it for void*.

In the C++ standard there is no requirement that pointer to frame is a
pointer to function. C++ Standard does not mention frames at all and
many OS return frames as just a void*, not void(*)().

I'll add a `template <class T> frame(T* addr)` and
`frame(void(*addr)())` constructors. This must satisfy all the needs
and allow to change underlying implementation for a platform where
sizeof(void*) != sizoef(void(*)())

--
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
On Mon, Mar 20, 2017 at 12:25 PM, Antony Polukhin via Boost <
[hidden email]> wrote:

> What is more important - moving away windows.h is a non breaking
> change that does not change the library interface (API).


Except if the library interface needs to change to allow for the
windows-specific components to be decoupled.

Emil

_______________________________________________
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:

> In the C++ standard there is no requirement that pointer to frame is a
> pointer to function. C++ Standard does not mention frames at all and many
> OS return frames as just a void*, not void(*)().

That's true but you've stated that you don't want to support manually
captured frames. So what the OS returns should be irrelevant in principle -
it's an implementation detail that the library hides from the user.

You do support frames constructed from function pointers, such as the one
returned from ::signal, so this is what the constructor should take.

The only reason to take void* would be to support passing the return value
of dlsym to the constructor without a reinterpret_cast.

> I'll add a `template <class T> frame(T* addr)`

This will do if you also constrain it to is_function<T>.


_______________________________________________
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:

> It is required to produce anything useful from frame/stacktrace.
> Following operations use COM (on MSVC only):
> * ostreaming stacktrace
> * ostreaming frame
> * getting function name from frame
> * getting source file location from frame
> * getting line in source file from frame

Yes, and the request is to not include COM/windows.h in programs or
translation units that don't do any of the above.

Please take a look in my message in which I outline three alternative
approaches to achieve this aim; maybe one of those would sit well with you.

> Also I can not see how you're going to use the library without 95% of
> functionality...

As already stated:

- A header-only library constructs a stack trace, f.ex. to include it when
throwing an exception
- A user of this header-only library ignores the stack trace because he is
not familiar with the stacktrace library and is unaware of the existence of
stack traces

The goal here is to not include COM/windows.h in this user's code, or
require him to link with libboost_stacktrace.

If this is not done, the authors of header-only libraries can't painlessly
transition to using stacktrace to include stack traces in their exceptions
because their users will complain about windows.h/COM or link errors. And we
want libraries to use stacktrace to include stack traces in their
exceptions, because this is a good thing that some of their users will
appreciate.


_______________________________________________
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-20 23:52 GMT+03:00 Peter Dimov via Boost <[hidden email]>:

> Antony Polukhin wrote:
>
>> In the C++ standard there is no requirement that pointer to frame is a
>> pointer to function. C++ Standard does not mention frames at all and many OS
>> return frames as just a void*, not void(*)().
>
>
> That's true but you've stated that you don't want to support manually
> captured frames. So what the OS returns should be irrelevant in principle -
> it's an implementation detail that the library hides from the user.
>
> You do support frames constructed from function pointers, such as the one
> returned from ::signal, so this is what the constructor should take.
>
> The only reason to take void* would be to support passing the return value
> of dlsym to the constructor without a reinterpret_cast.
>
>> I'll add a `template <class T> frame(T* addr)`
>
>
> This will do if you also constrain it to is_function<T>.

Good points! Agreed.

--
Best regards,
Antony Polukhin

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