[outcome] some problems compiling
So, I am trying to run my first test program with Boost.Outcome. The library fails to compile with GCC 6.3.0 on MinGW on Windows 7. I have filed the issue: https://github.com/ned14/boost.outcome/issues/36 But also, I can see in file <boost/outcome/outcome_v1.0.hpp>, lines 1267 to 1279 ``` #ifdef __cplusplus namespace { #endif typedef struct _IMAGEHLP_LINE64 { unsigned long SizeOfStruct; void *Key; unsigned long LineNumber; wchar_t *FileName; unsigned long long int Address; } IMAGEHLP_LINE64, *PIMAGEHLP_LINE64; ``` 1. Why do you check for __cplusplus? Is it a copy-and-paste of some Windows headers? 2. This class and subsequent functions are defined in anonymous namespace. This means they are redefined and recompiled in each translation unit, and remain a per TU definition. Is that intent? Regards, &rzej;
On 23/05/2017 22:46, Andrzej Krzemienski via Boost wrote:
So, I am trying to run my first test program with Boost.Outcome. The library fails to compile with GCC 6.3.0 on MinGW on Windows 7. I have filed the issue: https://github.com/ned14/boost.outcome/issues/36
Mingw is not on the list of supported compilers at https://ned14.github.io/boost.outcome/prerequisites.html I refuse to support Mingw, not worth the waste of my time. I will unofficially support Mingw-w64. Outcome had been tested on mingw-w64 some time ago, but it had become minor broken since, it's now fixed on develop branch. Thanks for the bug report. A far better experience than Mingw is to use the Linux Subsystem for Windows 10. Full Ubuntu 16.04 LTS environment, but on Windows.
But also, I can see in file <boost/outcome/outcome_v1.0.hpp>, lines 1267 to 1279
``` #ifdef __cplusplus namespace { #endif
typedef struct _IMAGEHLP_LINE64 { unsigned long SizeOfStruct; void *Key; unsigned long LineNumber; wchar_t *FileName; unsigned long long int Address; } IMAGEHLP_LINE64, *PIMAGEHLP_LINE64; ```
1. Why do you check for __cplusplus? Is it a copy-and-paste of some Windows headers?
It's C source code from an all C sub-sub-library brought into the partially preprocessed edition. That particular all C sub-sub-library implements POSIX backtrace() and backtrace_symbols() for Windows.
2. This class and subsequent functions are defined in anonymous namespace. This means they are redefined and recompiled in each translation unit, and remain a per TU definition. Is that intent?
Yes. Because it's a C source file, you would get symbol collision errors otherwise. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
2017-05-24 3:23 GMT+02:00 Niall Douglas via Boost <boost@lists.boost.org>:
On 23/05/2017 22:46, Andrzej Krzemienski via Boost wrote:
So, I am trying to run my first test program with Boost.Outcome. The library fails to compile with GCC 6.3.0 on MinGW on Windows 7. I have filed the issue: https://github.com/ned14/boost.outcome/issues/36
Mingw is not on the list of supported compilers at https://ned14.github.io/boost.outcome/prerequisites.html
I refuse to support Mingw, not worth the waste of my time. I will unofficially support Mingw-w64. Outcome had been tested on mingw-w64 some time ago, but it had become minor broken since, it's now fixed on develop branch. Thanks for the bug report.
A far better experience than Mingw is to use the Linux Subsystem for Windows 10. Full Ubuntu 16.04 LTS environment, but on Windows.
But also, I can see in file <boost/outcome/outcome_v1.0.hpp>, lines 1267 to 1279
``` #ifdef __cplusplus namespace { #endif
typedef struct _IMAGEHLP_LINE64 { unsigned long SizeOfStruct; void *Key; unsigned long LineNumber; wchar_t *FileName; unsigned long long int Address; } IMAGEHLP_LINE64, *PIMAGEHLP_LINE64; ```
1. Why do you check for __cplusplus? Is it a copy-and-paste of some Windows headers?
It's C source code from an all C sub-sub-library brought into the partially preprocessed edition. That particular all C sub-sub-library implements POSIX backtrace() and backtrace_symbols() for Windows.
2. This class and subsequent functions are defined in anonymous namespace. This means they are redefined and recompiled in each translation unit, and remain a per TU definition. Is that intent?
Yes. Because it's a C source file, you would get symbol collision errors otherwise.
Can't the same effect be achieved with declaring the functions inline? And then you would only get one symbol. Regards, &rzej;
2. This class and subsequent functions are defined in anonymous namespace. This means they are redefined and recompiled in each translation unit, and remain a per TU definition. Is that intent?
Yes. Because it's a C source file, you would get symbol collision errors otherwise.
Can't the same effect be achieved with declaring the functions inline? And then you would only get one symbol.
We would then collide with anyone who later includes <windows.h>, because we are defining the exact same symbol names as windows in order to avoid including windows, so it would be an ODR violation. It's a trick to include windows without including windows which only works on MSVC by taking advantage of its lax parser. That's why on clang we just go ahead and include windows. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On Wed, May 24, 2017 at 7:28 AM, Niall Douglas wrote:
2. This class and subsequent functions are defined in anonymous namespace. This means they are redefined and recompiled in each translation unit, and remain a per TU definition. Is that intent?
Yes. Because it's a C source file, you would get symbol collision errors otherwise.
Can't the same effect be achieved with declaring the functions inline? And then you would only get one symbol.
We would then collide with anyone who later includes <windows.h>, because we are defining the exact same symbol names as windows in order to avoid including windows, so it would be an ODR violation.
It's a trick to include windows without including windows which only works on MSVC by taking advantage of its lax parser. That's why on clang we just go ahead and include windows.
Boost libraries already avoid including <windows.h> by leveraging Boost.WinAPI which provides the definitions necessary (and are added to as needed). Your use of an unnamed namespace for the types is questionable. Depending on the compiler, I think you should want to decorate them with __attribute__ ((__may_alias__)). Glen
On 24/05/2017 13:02, Glen Fernandes via Boost wrote:
On Wed, May 24, 2017 at 7:28 AM, Niall Douglas wrote:
2. This class and subsequent functions are defined in anonymous namespace. This means they are redefined and recompiled in each translation unit, and remain a per TU definition. Is that intent?
Yes. Because it's a C source file, you would get symbol collision errors otherwise.
Boost libraries already avoid including <windows.h> by leveraging Boost.WinAPI which provides the definitions necessary (and are added to as needed).
Your use of an unnamed namespace for the types is questionable. Depending on the compiler, I think you should want to decorate them with __attribute__ ((__may_alias__)).
The code in question is in a sub-sub-library which is itself completely standalone and reusable. It is Windows only because it implements a common POSIX API for Windows, so such attributes won't be useful. And that code would only ever get used if someone didn't configure a better stack backtrace printer. But I've been surprised by the number of people using winclang over MSVC, so I am open to doing better than including <windows.h> on winclang. Logged to https://github.com/ned14/boost.outcome/issues/39 Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 25/05/2017 00:31, Niall Douglas wrote:
The code in question is in a sub-sub-library which is itself completely standalone and reusable. It is Windows only because it implements a common POSIX API for Windows, so such attributes won't be useful. And that code would only ever get used if someone didn't configure a better stack backtrace printer.
Speaking of, now that StackTrace has been accepted into Boost, are you considering migrating that code to use it instead? Somewhat related, the docs for error_code_extended::backtrace() specify that the returned value must be freed. Is that referring only to the array itself or must the individual char* within the returned char** be individually freed as well? Why doesn't this just return std::vector<std::string> or some kind of smart pointer for avoidance of doubt and exception-safety? It feels like this is allowing implementation details to escape unsafely.
The code in question is in a sub-sub-library which is itself completely standalone and reusable. It is Windows only because it implements a common POSIX API for Windows, so such attributes won't be useful. And that code would only ever get used if someone didn't configure a better stack backtrace printer.
Speaking of, now that StackTrace has been accepted into Boost, are you considering migrating that code to use it instead?
As I have stated many times now, the stacktrace captured is 100% standard format and can be fed to any third party stack trace printer, including Boost.Stacktrace. The very minimal support provided out of the box is there for the majority of users who don't care about maximum information stack trace prints.
Somewhat related, the docs for error_code_extended::backtrace() specify that the returned value must be freed. Is that referring only to the array itself or must the individual char* within the returned char** be individually freed as well?
Yes, the wording is confusing now I look at it. It's fixed in develop branch. Thanks for the spot.
Why doesn't this just return std::vector<std::string> or some kind of smart pointer for avoidance of doubt and exception-safety? It feels like this is allowing implementation details to escape unsafely.
The implementation literally returns what backtrace_symbols() returns, which is a malloced char **. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Niall Douglas wrote:
As I have stated many times now, the stacktrace captured is 100% standard format and can be fed to any third party stack trace printer, including Boost.Stacktrace.
Stacktrace has two parts, one for printing, the other for capturing. The capturing part is essentially what you have, and during the Stacktrace review, I specifically insisted that it should be minimal, non-intrusive, and should not under any circumstances include <windows.h>. The idea was to make the capturing part of Stacktrace usable in header-only libraries such as, in out case, Outcome.
On 26/05/2017 02:42, Niall Douglas wrote:
Why doesn't this just return std::vector<std::string> or some kind of smart pointer for avoidance of doubt and exception-safety? It feels like this is allowing implementation details to escape unsafely.
The implementation literally returns what backtrace_symbols() returns, which is a malloced char **.
I assumed that, hence my comment about a leaking implementation detail. That's a reasonable return value for a C library like backtrace_symbols; it's bad style for a C++ library. If you don't want the expense of copying the strings individually to a std::vector<std::string>, perhaps returning a std::unique_ptr<char**, c_free> would be a reasonable compromise for that specific issue, although that still masks the array-ness so it's not ideal. The other problem with this API as it stands is that it provides no way to convey the actual number of frame strings returned. backtrace_symbols() in the underlying library makes no guarantee to null-terminate the array, as far as I can tell. raw_backtrace(), or backtrace() in the underlying library, does give you the number of frames filled but this information is not available to the caller of error_code_extended::backtrace() unless they call raw_backtrace() separately, which seems pointless. Since symbolising the trace is inherently an expensive operation anyway I don't think it's worth worrying about avoiding the copy, which is why I suggested the std::vector<std::string> return value. It avoids both issues.
On 26/05/2017 00:57, Gavin Lambert via Boost wrote:
On 26/05/2017 02:42, Niall Douglas wrote:
Why doesn't this just return std::vector<std::string> or some kind of smart pointer for avoidance of doubt and exception-safety? It feels like this is allowing implementation details to escape unsafely.
The implementation literally returns what backtrace_symbols() returns, which is a malloced char **.
I assumed that, hence my comment about a leaking implementation detail. That's a reasonable return value for a C library like backtrace_symbols; it's bad style for a C++ library.
If you don't want the expense of copying the strings individually to a std::vector<std::string>, perhaps returning a std::unique_ptr<char**, c_free> would be a reasonable compromise for that specific issue, although that still masks the array-ness so it's not ideal.
That's a fair point. Logged to https://github.com/ned14/boost.outcome/issues/45.
The other problem with this API as it stands is that it provides no way to convey the actual number of frame strings returned. backtrace_symbols() in the underlying library makes no guarantee to null-terminate the array, as far as I can tell. raw_backtrace(), or backtrace() in the underlying library, does give you the number of frames filled but this information is not available to the caller of error_code_extended::backtrace() unless they call raw_backtrace() separately, which seems pointless.
And you've just discovered an exceeding bounds bug. Thank you very much. Logged to https://github.com/ned14/boost.outcome/issues/46.
Since symbolising the trace is inherently an expensive operation anyway I don't think it's worth worrying about avoiding the copy, which is why I suggested the std::vector<std::string> return value. It avoids both issues.
Agreed. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Niall Douglas wrote:
It's a trick to include windows without including windows which only works on MSVC by taking advantage of its lax parser. That's why on clang we just go ahead and include windows.
I remember recently having to fix similar code in smart_ptr for clang: https://github.com/boostorg/smart_ptr/commit/3568e093bb42da2dcf050c659bb5cbb... You could also look at winapi for inspiration, for example here https://github.com/boostorg/winapi/blob/develop/include/boost/detail/winapi/... I _think_ that winapi works on clang.
participants (5)
-
Andrzej Krzemienski -
Gavin Lambert -
Glen Fernandes -
Niall Douglas -
Peter Dimov