From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 122658 invoked by alias); 16 Mar 2015 10:07:22 -0000 Mailing-List: contact cygwin-developers-help@cygwin.com; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: cygwin-developers-owner@cygwin.com Mail-Followup-To: cygwin-developers@cygwin.com Received: (qmail 122644 invoked by uid 89); 16 Mar 2015 10:07:21 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.2 required=5.0 tests=AWL,BAYES_20,KAM_STOCKGEN autolearn=no version=3.3.2 X-HELO: calimero.vinschen.de Received: from aquarius.hirmke.de (HELO calimero.vinschen.de) (217.91.18.234) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 16 Mar 2015 10:07:16 +0000 Received: by calimero.vinschen.de (Postfix, from userid 500) id 86C34A807CA; Mon, 16 Mar 2015 11:07:13 +0100 (CET) Date: Mon, 16 Mar 2015 10:07:00 -0000 From: Corinna Vinschen To: cygwin-developers@cygwin.com Subject: Re: backtrace(3) in Cygwin Message-ID: <20150316100713.GF6096@calimero.vinschen.de> Reply-To: cygwin-developers@cygwin.com Mail-Followup-To: cygwin-developers@cygwin.com References: <5505E2DA.6090806@tiscali.co.uk> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="T6xhMxlHU34Bk0ad" Content-Disposition: inline In-Reply-To: <5505E2DA.6090806@tiscali.co.uk> User-Agent: Mutt/1.5.23 (2014-03-12) X-SW-Source: 2015-03/txt/msg00006.txt.bz2 --T6xhMxlHU34Bk0ad Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Content-length: 5760 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. >=20 > I've also documented (in the code) the issues surrounding > backtrace_symbols_fd(3) on Cygwin. >=20 > 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 =3D GetCurrentProcess(); > const int xp_max_frame_depth =3D 61; > if (size > xp_max_frame_depth) > size =3D xp_max_frame_depth; >=20 > /* 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 =3D (char*) (result + size); > for (i =3D 0; i < size; ++i) > { > result[i] =3D frame_text; >=20 > if ((SymFromAddr(process, (DWORD64)(buffer[i]), &offset, symb= ol)) && > (SymGetModuleInfo64(process, symbol->Address, &module_inf= o))) > { > frame_text +=3D 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 +=3D 1 + sprintf(frame_text, "[0x%lx]", (unsig= ned 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 t= he > * 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 al= ong > * the way. The problem is how to allocate memory for the SYMBOL_INFO > * structure, given that we're not supposed to use heap memory. Windo= ws > * 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 --=20 Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Maintainer cygwin AT cygwin DOT com Red Hat --T6xhMxlHU34Bk0ad Content-Type: application/pgp-signature Content-length: 819 -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJVBqtRAAoJEPU2Bp2uRE+gLaYP/1muU/VsYMKqB4n8h4neQU8j YFVR163i5fJfrWVMF1zOjsDS4EBxrEBxasdQ1ap5eP58TQrIMjRCYPxfqn/6r4cA gxQ/ZuFt9rwPFlxaDtfi+D/Go+O00G8PBzOOWXXdew62Q5SHWMWcpjcswf1h86cZ nj8aSc5r6cSwWBwMpxqPLmzG9SsqnDShtkBigpUVp3nVXK+iKUXPV1dKTVli9CLS 0hzqvdgZB4a6KfrVOVUw1vWjsVp0UUxOlDRoBwRdHpoYj88sh0JERuSeM85b7pyi EtIql5Vt3/+CnN+HKDXwEBJjMa/iuyZNj908OqBDStWFevjmNPSVOzLJtwYbgCef LfWbM8P0v9JiyqfQRF2e6aY6+7nUCA251O5s6SMKWBFhoI6FgLekmPIPc+GYWAK4 h+Qkp7RNlFLnVg3814jxpfwFeMwfWTVuz+uAqw82bQ6d/wfSUc4U0qzc+rWq9O+O GzKcXPVQ3gn2LQY3Iy+H+FRrWIXeG9w3vITsW7I07qK6+ugKrvmC60KCjWyouesX 9hiuiUdvUa2hc8RxqF+m3mQKcuOAP2Sk36h3a1CzCaXkbr8Ead4oGdXWdZO0Sm1N NpQ3ck8wH8z1kcf9pHo7Q3f50CvgF3TRLJX684bj+LGTxFSlyyq0V+afPr4RKeV4 qGSlCXx5o5KukJAjlcwp =GajA -----END PGP SIGNATURE----- --T6xhMxlHU34Bk0ad--