Re: [patch 00/12] entryval#2: Fix x86_64 <optimized out> parameters, virtual tail call frames

[Eli Zaretskii <eliz at gnu dot org>]

> Date: Tue, 13 Sep 2011 21:43:06 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> re-post of the series:
> 	[patch 00/12] entryval: Fix x86_64 <optimized out> parameters, virtual tail call frames
> 	http://sourceware.org/ml/gdb-patches/2011-07/msg00430.html
> 
> particular excerpt:
> The patches are available (merged only) in GIT for more convenience at:
> 	http://sourceware.org/gdb/wiki/ArcherBranchManagement
> 	archer-jankratochvil-entryval
> 
> 
> Here is attached a diff against the previous patch series.  The changes are:

Sorry, I'm confused: am I supposed to review only the diffs in this
message and forget about the rest of the series?  If no, which parts
should I review?

For now, here are the comments about this part alone:

> +set print entry-values (both|compact|default|if-needed|no|only|preferred)
> +show print entry-values
> +  Set printing of frame arguments values at function entry.  In some cases
                     ^^^^^^^^^^^^^^^^^^^^^^
"frame arguments' values" (with the apostrophe), or "frame argument
values" (in single).

> +  GDB can determine the value of function argument which was passed by the
> +  function caller, despite the argument value may be already modified.
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
"even if the value was modified inside the called function", I think.

> +set debug tailcall
> +show debug tailcall
> +  Control display of debugging info for determining virtual tail call frames,
> +  present in inferior debug info together with the @entry values.

Is it possible to rephrase this in a less mysterious way?  (I cannot
suggest a new wording because I don't understand what you meant ;-)

> -If you append @code{@@entry} string to a function parameter name you get its
> +If you append @kbd{@@entry} string to a function parameter name you get its
>  value at the time the function got called.  If the value is not available an
> -error message is printed.  Entry values are available only since @value{NGCC}
> -version 4.7.
> +error message is printed.  Entry values are available only with some compilers.

I think it would be good to have a cross-reference here to where you
describe "set print entry-values".

> +the function caller, despite the argument value may be already modified by the
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Same correction as above.

> +current function and therefore different.
                                  ^^^^^^^^^
"are different"

>                                         For optimized code also the current
> +value may be possibly not available and the entry value may be still known,
> +which aids the debugging of production code.

  With optimized code, the current value could be unavailable, but the
  entry value may still be known.

> +this feature will behave in the default @code{default} setting the same way as
                                   ^^^^^^^
Lose this "default", it's redundant.

> +the compiler has to produce @samp{DW_TAG_GNU_call_site} tags.  For example for
> +@value{NGCC} additionally optimization and debugging compilation options must
> +be enabled (@option{-O -g}).

  With @value{NGCC}, you need to specify @option{-O -g} during
  compilation, to get this information.


>                             Still even with the required debug info there
> +exist many reasons why the code path analysis by @value{GDBN} may fail in
> +specific cases.

I would omit this sentence, it isn't useful.

> +@table @code
> +@item no

This table needs to be preceded by a sentence saying something like

  The @{value} parameter can be one of the following:

> +@item only
> +set print entry-values only

I think this second line should be deleted.

> +Print only parameter values from function entry point.  If value from function
> +entry point is not known while the actual value is known print at least the
                                                           ^
Comma here.

> +@smallexample
> +#0  equal (val@@entry=5)
> +#0  different (val@@entry=5)
> +#0  lost (val@@entry=5)
> +#0  born (val@@entry=<optimized out>)
> +#0  invalid (val@@entry=<optimized out>)
> +@end smallexample

This example is identical to the previous one and does not show the
effect of the `preferred' setting.

> +@item if-needed
> +Print actual parameter values.  If actual parameter value is not known while
> +value from function entry point is known print at least the entry point value
                                           ^
A comma is missing.  Also, what do you mean by "at least"?

> +@smallexample
> +#0  equal (val@@entry=5)
> +#0  different (val@@entry=5)
> +#0  lost (val@@entry=5)
> +#0  born (val=10)
> +#0  invalid (val@@entry=<optimized out>)
> +@end smallexample

I think this example should show almost all values NOT @entry.

> +Always print both the actual parameter value and its value from function entry
> +point.  Still print both even if one of them or both are @code{<optimized out>}.

  Always print both the actual parameter value and its value from
  function entry point, even if values of one or both are not
  available due to compiler optimizations.

> +@item compact

I cannot say I like the "compact" name.  How about "both-if-known"
(and "both-always" for the previous one)?

> +Print the actual parameter value if it is know and also its value from
> +function entry point if it is known.  If neither is known print for the actual
> +value @code{<optimized out>}.  If not in MI mode (@pxref{GDB/MI}) and if both
> +values are known and they are equal print the shortened
              ^^^^^^^^^^^^^^^^^^^^^^^^
"known and identical".  And a comma after that.

> +@item default
> +Always print the actual parameter value.  Print also its value from function
> +entry point but only if it is known.  If not in MI mode (@pxref{GDB/MI}) and if
              ^
Comma.

> +both values are known and they are equal print the shortened
> +@code{param=param@@entry=VALUE} notation.

Not quite clear how this differs from the previous one.  Maybe it
would be better to say "like compact, but...".

> +@item show print entry-values
> +Show printing of frame arguments values at function entry.

"Show the method being used for printing of..."

> +Function @code{B} can call function @code{C} by its very last statement.  In
                                                ^^
"in"

> +instead.  Such use of a jump instruction is called tail call.

"@dfn{tail call}" -- you are introducing new terminology.

> +During execution of function @code{C} there will remain no indication it has
> +been tail called from function @code{B}.

  During execution of function @code{C}, there will be no indication
  in the generated code that it was tail-called from @code{B}.

> +function @code{B} which tail calls function @code{C} then @value{GDBN} sees as
                           ^^^^^^^^^^
"tail-calls", and a comma before "then".

> +the caller of function @code{C} the function @code{A}.

  then @value{GDBN} will see @code{A} as the caller of @{C}.

>                                                       @value{GDBN} can in
> +some cases search all the possible code paths and if it determintes there
> +exists an unambiguous code path it will create call frames for it (in this case
> +a frame with @code{$pc} in function @code{B}).  The virtual return address into
> +such tail call frame will be pointing pointing right after the jump instruction
> +(as if it would be a call instructions).

  However, in some cases @value{GDBN} can determine that @code{C} was
  tail-called from @code{B}, and will then create fictitious call
  frame for that, with the return address set up as if @code{B} called
  @code{C} normally.

> +the compiler has to produce @samp{DW_TAG_GNU_call_site} tags.  For example for
> +@value{NGCC} additionally optimization and debugging compilation options must
> +be enabled (@option{-O -g}).  Still even with the required debug info there
> +exist many reasons why the code path analysis by @value{GDBN} may fail in
> +specific cases.

Please modify this as I did with a similar text above.

> +@kbd{info frame} command (@pxref{Frame Info}) will indicate the tail call frame
> +kind by text @code{tail call frame}.

An example would be good.

> +The detection of all the possible code path executions can find them ambiguous.
> +There is no execution history stored (possible @ref{Reverse Execution} is never
> +used for this purpose) and the last known caller could have reached the known
> +callee by multiple different jump sequences.  In such case @value{GDBN} still
> +tries to show at least all the unambiguous top tail callers and all the
> +unambiguous bottom tail calees, if any.

I don't understand what this means in practice.  If you show an
example of this, I could suggest some rewording.

> +@kbd{set verbose} command (@pxref{Messages/Warnings, ,Optional Warnings and
> +Messages}.) can show some reasons why the complete code path analysis did not
> +succeed in a specific case.

Ditto.  Also, @pxref should not have a period at its end, before the
closing parenthesis, it generates such punctuation automatically (so
the above will have 2 periods in the manual).

> +@table @code
> +@item set debug tailcall
> +@kindex set debug tailcall
> +When set to on, enables tail calls analysis messages printing.  It will show
> +all the possible valid tail calls code paths it has considered.  It will also
> +print the intersection of them with the final unambiguous (possibly partial or
> +even empty) code path result.

Example, please!  Some rewording is in order, but I need an example to
suggest it.

> +@item gdb.TAILCALL_FRAME
> +A frame representing a tail call.  Tail calls are used if the last statement of
> +an inferior function is a call of a (usually different) function.  Such call
> +immediately followed by return instruction is in optimized code converted to
> +just one jump instruction.  @value{GDBN} can in some cases guess such jump has
> +been executed and it creates a @code{gdb.TAILCALL_FRAME} for it.

Instead of all this description, just add a cross-reference to where
tail-calls are described.

> +  add_setshow_enum_cmd ("entry-values", class_stack,
> +			print_entry_values_choices, &print_entry_values,
> +			_("Set printing of frame arguments values at function "

I suggest

  Set printing of function arguments at function entry.

> +GDB can print in some cases besides frame arguments values also the values\n\
> +they had at function entry (marked as `NAME@entry').  The value itself and/or\n\
> +the entry value may be <optimized out>.  Which of this current or entry\n\
> +values get printed in which case can be set by this option."),

 GDB can sometimes determine the values of function arguments at entry,
 in addition to their current values.  This option tells GDB whether
 to print the current value, the value at entry (marked as val@entry),
 or both.  Note that one or both of these values may be <optimized out>.

Thanks.