Hi, thanks for the review.  Fixed nits, a couple comments and
responses.

On Mon 20 Apr 2015 07:27, Doug Evans <xdje42@gmail.com> writes:

> Andy Wingo <wingo@igalia.com> writes:
>
>> +(define* (has-active-frame-filters? #:optional (locus (current-progspace)))
>
> Is current-progspace the best default here?  Dunno.
> Since this function is only called without arguments
> how about removing "locus" for now.

This is an internal helper.  It should probably be called with an
argument but it is not, as you note below, and we haven't seen a problem
because the default value is the same as the one it would be called
with.  However it is the correct default, I think; below you ask what
the correct default is for apply-frame-filters, and I think it is indeed
the current progspace.  Note that this isn't really a locus but more of
a scope.  Which frame filters can apply to a backtrace?  The ones
registered on objfiles that are in the current progspace, the ones
registered on the current progspace, and the ones registered globally.
See the same-scope? procedure, which I reproduce here:

  (define (same-scope? a b)
    "Return #t if loci A and B represent the same scope, for the purposes
  of frame filter selection."
    (cond
     ;; If either locus is global, they share a scope.
     ((or (not a) (not b)) #t)
     ;; If either locus is an objfile, compare their progspaces, unless
     ;; the objfile is invalid.
     ((objfile? a) (and (objfile-valid? a)
                        (same-scope? (objfile-progspace a) b)))
     ((objfile? b) (and (objfile-valid? b)
                        (same-scope? a (objfile-progspace b))))
     ;; Otherwise they are progspaces.  If they eq?, it's the same scope.
     (else (eq? a b))))

I don't know if this should be different, but it is effectively what
Python does (see frames.py:return_list).  I guess Python also includes
filters for objfiles in other progspaces, but that sounds like the wrong
thing to me...

>> +(define* (find-frame-filter-by-name name #:optional (locus (current-progspace)))
>
> Is current-progspace the best default?  Dunno.
> How about removing a default for locus until there's a compelling choice.
>
>> +  (prune-frame-filters!)
>> +  (or (find (lambda (filter)
>> +              (and (equal? name (frame-filter-name filter))
>> +                   (eq? (frame-filter-locus filter) locus)))
>> +            *frame-filters*)
>> +      (find (lambda (filter)
>> +              (and (equal? name (frame-filter-name filter))
>> +                   (same-scope? (frame-filter-locus filter) locus)))
>> +            *frame-filters*)
>> +      (error "no frame filter found with name" name)))

You sure?  The code above will also find filters that have "same-scope?"
with the given locus, which for the current progspace includes its
objfiles and the global scope.  To me this is a fine default and in no
way limits what the function can do.  Just want to double-check here.

>> +(define* (apply-active-frame-filters stream #:optional
>> +                                     (locus (current-progspace)))
>
> Similarly, it's not clear to me what the best default for locus is.
> Also, it appears locus is currently unused (the only caller doesn't
> pass a locus).
>
>> +  "Fold the active frame filter procedures over a stream."
>> +  (fold (lambda (filter stream)
>> +          (if (and (frame-filter-enabled? filter)
>> +                   (same-scope? (frame-filter-locus filter) locus))
>> +              ((frame-filter-procedure filter) stream)
>> +              stream))
>> +        stream
>> +        *frame-filters*))

Not sure if I understand this comment.  "locus" is certainly used in the
body of the function, with its default value of (current-progspace) when
called from apply-frame-filter.  Are you suggesting that instead we bind
(let ((scope (current-progspace))) ...)  in the function?  To me the
optional argument with its default initializer does this nicely.

>> +{
>> +  struct type *type;
>> +
>> +  GDBSCM_BEGIN_TRY_CATCH_WITH_DYNWIND ()
>> +    {
>> +      struct ui_file *stb = mem_fileopen ();
>> +
>> +      make_cleanup_ui_file_delete (stb);
>> +      type = check_typedef (value_type (val));
>> +      type_print (value_type (val), "", stb, -1);
>
> pass type here instead of value_type (val)?
> [I suspect py-framefilter.c has the same bug.]

This causes scm-frame-filter-mi.exp to fail:

  Expecting: ^(-stack-list-arguments 2[
  ]+)?(\^done,stack-args=\[frame={level="0",args=\[{name="foo",type="int",value="21"},{name="bar",type="char \*",value="0x[0-9A-Fa-f]+ \\"Param\\""},{name="fb",type="foobar \*",value="0x[0-9A-Fa-f]+"},{name="bf",type="foobar"}\]},frame={level="1",args=\[\]},frame={level="2",args=\[{name="j",type="int",value="10"}\]},.*frame={level="22",args=\[\],children=\[frame={level="23",args=\[\]}\]},.*frame={level="26",args=\[{name="f",type="int",value="3"},{name="d",type="int",value="5"}\]},frame={level="27",args=\[\]}\][
  ]+[(]gdb[)] 
  [ ]*)
  -stack-list-arguments 2
  ^done,stack-args=[frame={level="0",args=[{name="foo",type="int",value="21"},{name="bar",type="char *",value="0x4008b1 \"Param\""},{name="fb",type="foobar *",value="0x601010"},{name="bf",type="struct {...}"}]},frame={level="1",args=[]},frame={level="2",args=[{name="j",type="int",value="10"}]},frame={level="3",args=[]},frame={level="4",args=[{name="j",type="int",value="9"}]},frame={level="5",args=[]},frame={level="6",args=[{name="j",type="int",value="8"}]},frame={level="7",args=[]},frame={level="8",args=[{name="j",type="int",value="7"}]},frame={level="9",args=[]},frame={level="10",args=[{name="j",type="int",value="6"}]},frame={level="11",args=[]},frame={level="12",args=[{name="j",type="int",value="5"}]},frame={level="13",args=[]},frame={level="14",args=[{name="j",type="int",value="4"}]},frame={level="15",args=[]},frame={level="16",args=[{name="j",type="int",value="3"}]},frame={level="17",args=[]},frame={level="18",args=[{name="j",type="int",value="2"}]},frame={level="19",args=[]},frame={level="20",args=[{name="j",type="int",value="1"}]},frame={level="21",args=[]},frame={level="22",args=[],children=[frame={level="23",args=[]}]},frame={level="24",args=[{name="i",type="int",value="3"}]},frame={level="25",args=[{name="j",type="int",value="3"}]},frame={level="26",args=[{name="f",type="int",value="3"},{name="d",type="int",value="5"}]},frame={level="27",args=[]}]
  (gdb) 
  FAIL: gdb.guile/scm-frame-filter-mi.exp: stack-list-arguments 2

One piece of it expects:

  {name="bf",type="foobar"}

but gets:

  {name="bf",type="struct {...}"}

Dunno.  I didn't make this change in the patch because I don't
understand what it's trying to do.  What's the right thing?  There's
another failure also, for "stack-list-arguments 2 0 3"

> Whether we *want* to strip typedefs here is a good question.
> It's not immediately obvious that we do.
> OTOH, check_typedef serves two purposes, the other being to resolve
> opaque types and I'm guessing we do want to resolve opaque types here,
> *and* we have no other function which does just this (we need to fix this).
>
> Thoughts?

I am not sure and I don't really understand this code :)  I'm happy to
improve it over time though.

I think I've covered the rest of the nits.  Attached patch is v6.

Regards,

Andy