Re: [lmi] PATCH: Build fixes after unwind.cpp addition

[Vadim Zeitlin]

On Thu, 4 Mar 2021 12:23:07 +0000 Greg Chicares <gchicares@sbcglobal.net> wrote:

GC> On 2/26/21 10:25 PM, Vadim Zeitlin wrote:
GC> > 
GC> >  I've created https://github.com/let-me-illustrate/lmi/pull/172 with a
GC> > couple of small build fixes that are required to build lmi in the CI
GC> > environment, build it using autotools and/or build it using clang since
GC> > unwind.cpp addition.
GC> 
GC> [...snip details...]
GC> 
GC> >  If possible, I'd really like to merge the first two and, preferably,
GC> > three (and, ideally, of course, all 5), commits soon. Please let me know 
if
GC> > you see any problems with doing this or any questions about the changes
GC> > here.
GC> 
GC> Okay, I've done
GC>   $git fetch xanadu
GC>   $git cherry-pick ..xanadu/unwind-build-fixes
GC> and will push after my next nychthemeral test.

 Thanks!

 And now https://github.com/let-me-illustrate/lmi/actions is green again.

GC> I'll take it on faith that...
GC> 
GC>   commit 1aa86defc7e
GC>     It is unnecessary to check for GNU compiler being used for the code only
GC>     compiled when GNU Standard C++ library is, as is the case since the
GC>     parent commit.
GC> 
GC> ...although I thought it was possible to use libstdc++ with clang.

 Yes, you're right, it is indeed possible to do this, at least under Linux
and I just didn't think of this case when writing the commit message, sorry.
However it doesn't materially change anything because clang still defines
__GNUC__ and I strongly suspect that any other compiler capable of using
libstdc++ would be sufficiently compatible with gcc to define it too, so
the check for "defined __GNUC__" is still unnecessary.

 To go slightly further into this rabbit hole, a maybe better argument for
removing __GNUC__ check is that we don't actually depend on the compiler
being used here, but just on using libstdc++, which defines __cxa_throw()
etc in its ABI.

 So if we wanted, we could support clang with libstdc++ in this code. At
least compiling it with clang works in this case after fixing 2 minor
problems:

1. We need to re-add "#ifndef __clang" around the pragma disabling the
   gcc-specific -Wconditionally-supported.

2. We need to add __attribute__((__noreturn__)) to avoid a warning about
   returning from __cxa_throw() which is itself declared as "noreturn".
   Unfortunately, we can't use [[noreturn]] here because it can be used
   only with functions, and neither function pointers nor types, and, even
   more unfortunately, we have to add in 2 different places depending on
   the compiler used because clang supports it _only_ in the cxa_throw_t
   declaration but _not_ on original_cxa_throw, while gcc does exactly the
   converse.

I don't know if you're interested enough in this use case to uglify the
sources with clang-specific code, but if you're, I could test this (i.e.
not just compilation, but that it actually produces the expected results
during run-time) and submit the small PR fixing this.


GC> As for...
GC> 
GC>   commit 65d3ef327e4
GC> 
GC>      #include "unwind.hpp"
GC> 
GC>     -#if defined LMI_X86_64 && defined LMI_POSIX
GC>     +#if defined LMI_X86_64 && defined LMI_POSIX && defined __GLIBCXX__
GC> 
GC> ...I wondered whether __GLIBCXX__ is sure to be defined at this point.

 Oops, this is another point that I've completely failed to take into
account, sorry, and thanks for pointing it out!

GC> It turns out that yes, it is (iff it should be),

 Yes, I did at least test this that it worked, so it is indeed defined. But
it happens purely accidentally, AFAICS: __GLIBCXX__ is only guaranteed to
be defined after including some C++ standard library header, but we never
do this here. What we do is to include <stdlib.h> (via config.hpp and
platform_dependent.hpp) and this just happens to include <cstdlib> which
defines __GLIBCXX__ (indirectly, but this doesn't matter).

 So this could easily get broken by a future libstdc++ update and I
definitely should have thought to explicitly include some C++ header here.
I don't know which one would be the most appropriate, but the same
<cstdlib> would do fine, of course. Or we could just move the existing
inclusion of <cstdio> upwards. What would you prefer?

GC> Let me make some general comments about 'unwind.cpp'.
GC> 
GC> I added this for the proximate reason that lmi's map_lookup() throws:
GC>     "map_lookup: key '" << key << "' not found."
GC> That's just the most common cause of a general problem: exceptions
GC> lack context, so it's hard to figure out how to redress them.

 FWIW I think the real solution to this problem is to ensure that context
is available, by catching and re-throwing the exceptions as they propagate
through the abstractions boundaries. This is, of course, much more
difficult to do than doing what you did once in a single place.

GC> It would be nice to do something like that for msw builds run on
GC> native msw, but I don't want to spend time on that myself. Perhaps
GC> once we upgrade to x86_64-w64-mingw32, wx will offer something
GC> similar for GUI binaries? (Or maybe I'm remembering a segfault
GC> handler.)

 wx does provide wxStackWalker class, but it's not very useful for the code
compiled with MinGW because it doesn't have debug symbols in the
platform-native PDB format. As I wrote back in

https://lists.nongnu.org/archive/html/lmi/2016-06/msg00045.html

VZ> GC> So we'd get a stack trace if lmi crashes?
VZ> 
VZ>  Well, almost. I actually tested this wxWidgets change with lmi, after
VZ> disabling the overridden OnAssertFailure(), so I know that it does work,
VZ> but it's not as nice as with MSVC because gcc doesn't emit debug
VZ> information in the platform-standard PDB format, but you get at least the
VZ> addresses, which is better than nothing, and could be used with addr2line
VZ> (and I even thought about integrating support for this in wxWidgets
VZ> itself), and you can also use https://github.com/rainers/cv2pdb tool to
VZ> convert DWARF debug info (which is more or less the same as CodeView
VZ> format) to PDB and then you do get the real symbolic information. I did run
VZ> into bugs with cv2pdb tool and I'm not sure how wise is it to rely on it,
VZ> so I feel like addr2line approach could be a better one, even if it relies
VZ> on an external program. Anyhow, further improvements are definitely
VZ> possible, but what we have now is already better than nothing. And there
VZ> is, of course, 0 danger in having this code in wxWidgets as long as lmi
VZ> doesn't use it in any way anyhow.

Of course, 5 years wasn't enough for me to actually do anything about it
and so we still don't use addr2line in wxStackWalker when using gcc, but
I'd still gladly do it if it can be helpful.

 However another problem is that you almost surely don't want to depend on
wx (especially because lmi uses a monolithic build of wx and so can't
depend on just wxbase) in non-GUI parts of the code, so I'm not sure if
it's really going to help.

 So maybe I should provide an alternative version of unwind.cpp for MSW?
This would, of course, be much more work (I'd have to redo everything that
wxStackWalker already does), but looks like a better alternative. What do
you think?


GC> For pc-linux-gnu unit tests that use LMI_TEST_THROW(), this unwinder
GC> reports the stack trace for every exception--even intentional ones
GC> arising from LMI_TEST_THROW(). It shouldn't be hard to block that in
GC> the definition of LMI_TEST_THROW(); I just haven't felt strongly
GC> motivated to do that yet, because it causes no harm.

 I do feel strongly motivated to do this because these endless stack dumps
annoy me a lot. So I'd definitely like to do this if you (a) don't do it
yourself in the near future and (b) would accept my changes.

 Please let me know if I should do this (and/or anything else mentioned
above)!
VZ

pgpPYHMf2YA2B.pgp
Description: PGP signature