Bug 19808 - python unwinder documentation could use some love
Summary: python unwinder documentation could use some love
Status: RESOLVED FIXED
Alias: None
Product: gdb
Classification: Unclassified
Component: python (show other bugs)
Version: unknown
: P2 normal
Target Milestone: 8.3
Assignee: Tom Tromey
URL:
Keywords:
Depends on:
Blocks: 19923
  Show dependency treegraph
 
Reported: 2016-03-11 17:53 UTC by Tom Tromey
Modified: 2018-09-10 13:54 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Tromey 2016-03-11 17:53:23 UTC
The python unwinder documentation is a bit unclear, IMO, and could use
some updates.

The documentation for create_unwind_info says:

     'sp, pc, special'
          'frame_id_build_special (FRAME_ID.sp, FRAME_ID.pc,
          FRAME_ID.special)'

     'sp, pc'
          'frame_id_build (FRAME_ID.sp, FRAME_ID.pc)'

          This is the most common case.

     'sp'
          'frame_id_build_wild (FRAME_ID.sp)'


This doesn't really mean anything unless you already know quite a bit
about the gdb frame internals.  I think that rather than refer to internal
functions, this text should explain the meaning of these different calls.
(Especially since I think two of these are generally uninteresting.)


Nothing in the docs explains the meaning of the parameters to the frame id.
It should at least mention that these are intended to be the "unchanging"
versions, say the CFA and the start PC of the function.

(Also, gdb should just export a generic frame id object so that all users
don't have to invent their own.  This could be put in unwinder.py.)
Comment 1 Andrew Dinn 2016-04-08 15:29:24 UTC
I found a few other things undocumented in the unwinder API that would be worth mentioning.

1) values returned by pending_frame.read_register

passing an illegal register

pending_frame.read_register throws an Exception if you pass an illegal register name for the pending frame's architecture.


retrieving an undefined register

if you pass a legal register name pending_frame.read_register always returns a lazy gdb.Value even when the register's value is not available for retrieval. in the latter case when you try to use the returned gdb.Value it throws an Exception.


what is the type of a returned gdb.Value

the type of the gdb.Value returned by pending_frame.read_register is normally the gdb.Type with name 'long long'. However, for a program counter register (e.g. rip on x86) it is the type with name 'void (*) (void)' and for a stack pointer or frame pointer register (rsp or rbp on x86) it is the type with name 'void *'. this is significant because the type determines the way that automatic conversions to python ints/longs and comparisons with python ints/longs are performed.

[n.b. I am not sure whether/how this generalises to other architectures e.g. whether for, say, AArch64 values for register rfp have type 'void *']

2) safety of gdb operations invoked within the unwinder __call__ method

[this is probably a bug that should be fixed rather than a 'feature' to be documented but documentation might help implementors to diagnose and deal with errors]

unwinder implementations are advised to do as little work as possible inside the unwinder __call__ method. a registered unwinder may be called in circumstances where the entry frame to the stack frame hierarchy is not yet established. in such circumstances invocations of certain gdb-supplied python methods below __call__ may lead gdb to attempt to re-establish the stack frame hierarchy. this can have two fatal consequences:

firstly, it may cause repeated recursive entry into the unwinder until python detects a potential stack overflow and abandons the original __call__.

secondly, and much worse, gdb may reinitialise it's current stack hierarchy, freeing the memory used to store the base frame that underlies the pending_frame argument. as a consequence gdb will most likely crash with an internal error when __call__ returns.

[The following may or may not be a useful addition to the documentation depending upon whether the bug is fixed or not]

for example, in the OpenJDK unwinder calling

  gdb.parse_and_eval("CodeCache::_heap")

falls foul of both the recursion and the frame freeing issues when the entry frame has not been established.

similarly, executing

  ip = pending_frame.read_register('rip')
  if ip < hi_water:
    . . .

falls foul of the frame freeing issue. confusingly, this only happens because the gdb.Value appears first on the left. reversing the comparison and the operands to

  if hi_water >= ip:
    . . .

bypasses the problem.

the OpenJDK unwinder avoids the recursion problem by detecting recursive entry into the unwinder's __call__ method and backing out to return None from the top level entry. this allows the standard gdb unwinders to establish a proper frame stack.

the OpenJDK unwinder avoids the comparison problem by converting register values to python longs (thereby also forcing early rather than lazy evaluation) before performing comparisons.
Comment 2 cvs-commit@gcc.gnu.org 2018-09-10 13:50:37 UTC
The master branch has been updated by Tom Tromey <tromey@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=e7b5068cc22f1a5707a9fd0310234b8d7300f227

commit e7b5068cc22f1a5707a9fd0310234b8d7300f227
Author: Tom Tromey <tom@tromey.com>
Date:   Sat Sep 8 13:25:34 2018 -0600

    Update Python unwinder documentation
    
    PR python/19808 points out a few issues in the Python unwinder
    documentation.  This patch update the documentation for
    create_unwind_info and read_register to address the issues noted, and
    adds a cautionary note about writing an unwinder.
    
    gdb/doc/ChangeLog
    2018-09-10  Tom Tromey  <tom@tromey.com>
    
    	PR python/19808:
    	* python.texi (Unwinding Frames in Python): Rewrite
    	create_unwind_info documentation.  Update read_register
    	documentation and add a note about unwinder caution.
Comment 3 Tom Tromey 2018-09-10 13:54:20 UTC
I think this is fixed enough; but if not, please open a new
bug with more details.