Re: backtrace(3) in Cygwin

[Corinna Vinschen <corinna-cygwin at cygwin dot com>]

Hi David,


thanks for hacking on Cygwin in the first place.

Unfortunately there are some preliminaries before I can take a closer
look, namely the copyright assignment we need from everybody who's
providing more than just trivial patches to Cygwin, see
https://cygwin.com/contrib.html, the "Before you get started" section.
Fill out https://cygwin.com/assign.txt and send it to the address given
in the pamphlet.  If you're writing code for Cygwin only in your spare
time, forget the part about your employer.

Having said that...

On Mar 15 19:51, David Stacey wrote:
> Following on from my thread on the main list [1], I have made an initial
> attempt at implementing backtrace(3) and backtrace_symbols(3). This still
> needs a little work - at the moment, the paths in the strings will be
> DOS-style rather than POSIX.

Yuk! :)

> However, I wanted to submit an early version to
> make sure you were happy with my approach before I spend more time on it.
> 
> I've also documented (in the code) the issues surrounding
> backtrace_symbols_fd(3) on Cygwin.
> 
> This is my first real submission to the programme - be kind :-)

I will.  But  submissions almost always have to jump a couple of hurdles
(i.e., see how many iterations some patches need to make it into the
official Linux kernel) , so I'm asking for your patience in return, ok?
Any criticism here is not meant personally.

Also, it's a good idea to scan over https://cygwin.com/contrib.html.
Really.

Having said *that*...

* While you're already providing code, I'm missing a bit of discussion
  first.  You didn't respond to
  https://cygwin.com/ml/cygwin/2015-03/msg00206.html at all.

  Of course, it's ideally discussed on cygwin-developers anyway, but a
  discussion should ideally preceed a code submission :}

  My main point: Only *one* implementation of the stack walk code,
  not one for generating stackdumps and one for implementing backtrace(3).

  That doesn't mean we have to stick to the current stack_info class in
  exceptions.cc, but it's certainly still a good idea to implement the
  entire stack walking stuff in a class which allows usage from both
  places.


* Your coding style doesn't match the GNU coding style, see
  https://cygwin.com/contrib.html.  For correct formatting, indent is
  your friend.  It's generating GNU coding by default.  Also, while
  I'm sympathetic with comments preceeding every line with an asterisk,
  usually comments are supposed to look like this:

  /* comment line 1
     line 2
     line 3
     line 4. */


A few more comments inline:

> int backtrace(void **buffer, int size)
> {
>     HANDLE process = GetCurrentProcess();
>     const int xp_max_frame_depth = 61;
>     if (size > xp_max_frame_depth)
>         size = xp_max_frame_depth;
> 
>     /* Ignore this function when getting the stack frames. */
>     SymInitialize(process, NULL, TRUE);

This looks a bit too simple.  Most of the time we have stripped
executables, but the symbol files are available via the debuginfo
packages.  I don't know if the symbols *have* to be available as .pdb
files, but if this functionality also understands normal symbol tables
inside of executable files we might have a chance here to use the
debuginfo files for symbol evaluation.

>     /* Now populate the string lookup table and the strings with the text
>      * describing a frame. The pointer 'frame_text' is within the buffer we
>      * have just allocated and points to the start of the next string to
>      * write.
>      */
>     if (result)
>     {
>         frame_text = (char*) (result + size);
>         for (i = 0; i < size; ++i)
>         {
>             result[i] = frame_text;
> 
>             if ((SymFromAddr(process, (DWORD64)(buffer[i]), &offset, symbol)) &&
>                 (SymGetModuleInfo64(process, symbol->Address, &module_info)))
>             {
>                 frame_text += 1 + sprintf(frame_text, "%s(%s+0x%lx) [0x%lx]",
>                     module_info.LoadedImageName, symbol->Name, (unsigned long)offset,
>                     (unsigned long)buffer[i]);
>             }
>             else
>                 frame_text += 1 + sprintf(frame_text, "[0x%lx]", (unsigned long)buffer[i]);
>         }
>         assert(frame_text < (char*)result + chars_required);

Don't use assert here.  Generate an error instead.

> /* backtrace_symbols_fd() takes the same buffer and size arguments as
>  * backtrace_symbols(), but instead of returning an array of strings to the
>  * caller, it writes the strings, one per line, to the file descriptor fd.
>  * backtrace_symbols_fd() does not call malloc(3), and so can be employed in
>  * situations where the latter function might fail.
>  */
> void backtrace_symbols_fd(void *const *buffer, int size, int fd)
> {
>     /* A Cygwin implementation of backtrace_symbols_fd(3) is going to be
>      * somewhat problematic and will demand a compromise at some point along
>      * the way. The problem is how to allocate memory for the SYMBOL_INFO
>      * structure, given that we're not supposed to use heap memory. Windows
>      * defines MAX_SYM_NAME as 2000 characters, and clearly we shouldn't go
>      * trying to allocate that much memory on the stack.

This is Windows.  What you can do to make sure not to use any existing
heap is this:  Create your own Windows heap and use this exclusively
from the C++ class implementing the stack walk.  If creating the heap
fails, we're screwed, but otherwise you can allocate memory from a
pristine heap not related to other stuff in the application.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

Attachment: pgpswcmafBqCv.pgp
Description: PGP signature