[PATCH] Print registers not saved in the frame as "", instead of "". (was: Re: [PATCH] Consistent display of "")

Pedro Alves
2013-09-05T16:29:27Z

Hi guys,

Getting back to this, trying to make progress.

On 08/19/2013 11:24 AM, Andrew Burgess wrote:
> On 16/08/2013 7:41 PM, Pedro Alves wrote:
>> On 08/12/2013 09:01 PM, Mark Kettenis wrote:
>>>> Date: Mon, 12 Aug 2013 14:55:04 +0100
>>>> From: Pedro Alves <palves@redhat.com>
>>>>
>>>> On 08/12/2013 02:31 PM, Andrew Burgess wrote:
>>>>> On 06/08/2013 7:39 PM, Pedro Alves wrote:
>>>>>> On 08/06/2013 04:41 PM, Mark Kettenis wrote:
>>>>>>>> Date: Tue, 6 Aug 2013 14:49:03 +0100
>>>>>>>> From: "Andrew Burgess" <aburgess@broadcom.com>
>>>>>>
>>>>>>>> 3. My understanding was that values lost due to the ABI of a call site
>>>>>>>> were recorded as optimized out. For evidence I would present
>>>>>>>> dwarf2_frame_prev_register, and how DWARF2_FRAME_REG_UNDEFINED is handled.
>>>>>>>>
>>>>>>>> For these reasons I believe my patch should still be considered, what do
>>>>>>>> you think?
>>>>>>>
>>>>>>> I think that registers are either available or unavailble. A register
>>>>>>> being unavailble implies that a variable that is supposed to live in
>>>>>>> such a register may have been optimized out. Whether GDB's pseudo
>>>>>>> variables that respresent registers are considered unavailable or
>>>>>>> optimized out in that case is arguable.
>>>>>>
>>>>>> I think improving consistency as in Andrew's patch is good.
>>>>>
>>>>> Given almost a week has passed with no further feedback I plan to
>>>>> commit this patch tomorrow unless there's any further discussion to be had.
>>>>
>>>> TBC, note my opinion doesn't get to overrule Mark's. Consensus
>>>> works much better, and Mark does have deep knowledge of all
>>>> ABI/pseudo registers/etc. gdb things.
>>>> That said, Mark, if you still disagree, please counter argue,
>>>> otherwise, we'll just have to assume you do agree with the
>>>> rationales and clarifications.
>>>
>>> Can't say I agree. It simply doesn't make sense for registers to be
>>> "optimized out". I guess there are two reasons why GDB can't display
>>> the contents of a register in a frame:
>>>
>>> 1. The register contents aren't made available by the debugging
>>> interface, i.e. ptrace(2) or the remote stub doesn't tell us.
>>>
>>> 2. The register wasn't saved before calling another function.
>>>
>>> I guess after Andrew's chnages 1) would be shown as <unavailable> and
>>> 2) would become <optimized out>. But in the latter case something
>>> like <not saved> would make more sense.
>>>
>>> That said, Pedro, you're pretty much the expert for this area of GDB.
>>> So If you think Andrew should go ahead with this, feel free to ignore
>>> me.
>>
>> This is a tough call. I do agree that "optimized out" for registers
>> is a bit confusing. However, we already do print "<optimized out>" in
>> other places, such as when printing expressions, and consistency
>> is good. If we did add a distinction, I agree with Andrew that it should
>> be done in a more systematic way. However, I'm not really sure we need
>> much machinery. Wouldn't something like:
>>
>> void
>> val_print_optimized_out (const struct value *val, struct ui_file *stream)
>> {
>> if (value_lval_const (val) == lval_register)
>> fprintf_filtered (stream, _("<not saved>"));
>> else
>> fprintf_filtered (stream, _("<optimized out>"));
>> }
>>
>> work? What could be the register value cases that would print
>> "not saved" that we'd still want to print "optimized out" ?
>
> The only case I can immediately think of where this would cause a
> problem would be for computed locations, (lval_computed). The easy
> answer would be (in that case) the blame the compiler - why say the
> location is in a register if that register is volatile - but sadly I see
> this way too often.

Hmm, OK, but then lval_computed values with that change won't
ever show "<not saved>", due to the lval_register check. IOW,
we'd have to do something else in addition to lval_computed values
to make them print something other than the current <optimized out>.

However, I've come to think there's a really simple rule to
follow here -- We should only ever print <not saved> for values
that represent machine/pseudo registers. IOW, $pc, $rax, etc.
If the debug info happens to describe a variable as being located
in some optimized out register, we should still print
<optimized out>. The previous version of the patch failed that:

(gdb) PASS: gdb.dwarf2/dw2-op-out-param.exp: continue to breakpoint: Stop in breakpt for test int_param_single_reg_loc
bt
#0 0x000000000040058f in breakpt ()
-#1 0x00000000004005a2 in int_param_single_reg_loc (operand0=<optimized out>, operand1=0xdeadbe00deadbe01, operand2=<optimized out>)
+#1 0x00000000004005a2 in int_param_single_reg_loc (operand0=<not saved>, operand1=0xdeadbe00deadbe01, operand2=<not saved>)
#2 0x0000000000400577 in main ()

It didn't really make a lot of sense. This new version doesn't have
that change anymore.

That simple rule suggests that whatever the internal representation,
we should be easily able to have a single central point where to tag
such values. In fact, I think that already exists in value_of_register.

> However, exchanging what I see as the current larger inconsistency, for
> this much smaller one seems like a good deal to me, especially if it
> gets this patch unblocked...

Alright, what do you (all) think of of this (supposedly finished) patch
on top of yours (Andrew's) then?

Force-pushed here (along with Andrew's patch) for convenience:
https://github.com/palves/gdb/commits/register_not_saved
git@github.com:palves/gdb.git register_not_saved

--------------
Subject: [PATCH] Print registers not saved in the frame as "<not saved>"
instead of "<optimized out>".

Currently we print not saved registers as <optimized out>. That's
confusing, because what this really means is that the register was not
saved in the frame before calling another function.

Before:

(gdb) p/x $rax $1 = <optimized out>
(gdb) info registers rax
rax <optimized out>

After:

(gdb) p/x $rax
$1 = <not saved>
(gdb) info registers rax
rax <not saved>

However, if for some reason the debug info describes a variable as being
in such a register, we still want to print <optimized out>. IOW, <not
saved> is reserved for inspecting registers at the machine level.
The patch uses lval_register+optimized_out to encode the latter.

frame_unwind_got_optimized creates not_lval optimized out registers,
so by default, in most cases, we'll see <optimized out>.

value_of_register is the function eval.c uses for evaluating
OP_REGISTER (again, $pc, etc.), and related bits. It isn't used for
anything else. This function makes sure to return lval_register
values. The patch makes "info registers" and the MI equivalent use it
too. I think it just makes a lot of sense, as this makes it so that
when printing machine registers ($pc, etc.), we go through a central
function.

We're likely to need a different encoding at some point, if/when we
support partially saved registers. Even then, I think
value_of_register will still be the spot to tag the intention to print
machine register values differently.

value_from_register however may also return optimized out
lval_register values, so at a couple places where we're computing a
variable's location from a dwarf expression, we convert the resulting
value away from lval_register to a regular optimized out value.

Tested on x86_64 Fedora 17

gdb/
2013-09-05 Pedro Alves <palves@redhat.com>

* cp-valprint.c (cp_print_value_fields): Adjust calls to
val_print_optimized_out.
* jv-valprint.c (java_print_value_fields): Likewise.
* p-valprint.c (pascal_object_print_value_fields): Likewise.
* dwarf2loc.c (dwarf2_evaluate_loc_desc_full) <xxxxxx>: If the register
was not saved, return a new optimized out value.
* findvar.c (address_from_register): Likewise.
* frame.c (put_frame_register): Tweak error string to say the
register was not saved, rather than optimized out.
* infcmd.c (default_print_one_register_info): Adjust call to
val_print_optimized_out. Use value_of_register instead of
get_frame_register_value.
* mi/mi-main.c (output_register): Use value_of_register instead of
get_frame_register_value.
* valprint.c (valprint_check_validity): Likewise.
(val_print_optimized_out): New value parameter. If the value is
lval_register, print <not saved> instead.
(value_check_printable, val_print_scalar_formatted): Adjust calls
to val_print_optimized_out.
* valprint.h (val_print_optimized_out): New value parameter.
* value.c (struct value) <optimized_out>: Extend comment.
(error_value_optimized_out): New function.
(require_not_optimized_out): Use it. Use a different string for
lval_register values.
* value.h (error_value_optimized_out): New declaration.

gdb/testsuite/
2013-09-05 Pedro Alves <palves@redhat.com>

* gdb.mi/mi-reg-undefined.exp (opt_out_pattern): Delete.
(not_saved_pattern): New.
Replace uses for the former with the latter.
---
gdb/cp-valprint.c | 4 ++--
gdb/dwarf2loc.c | 16 +++++++++++++---
gdb/findvar.c | 9 +++++++++
gdb/frame.c | 2 +-
gdb/infcmd.c | 4 ++--
gdb/jv-valprint.c | 4 ++--
gdb/mi/mi-main.c | 2 +-
gdb/p-valprint.c | 4 ++--
gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp | 4 ++--
gdb/testsuite/gdb.mi/mi-reg-undefined.exp | 4 ++--
gdb/valprint.c | 13 ++++++++-----
gdb/valprint.h | 3 ++-
gdb/value.c | 22 +++++++++++++++++++---
gdb/value.h | 5 +++++
14 files changed, 70 insertions(+), 26 deletions(-)

diff --git a/gdb/cp-valprint.c b/gdb/cp-valprint.c
index e83d979..1d7147c 100644
--- a/gdb/cp-valprint.c
+++ b/gdb/cp-valprint.c
@@ -298,7 +298,7 @@ cp_print_value_fields (struct type *type, struct type *real_type,
TYPE_FIELD_BITPOS (type, i),
TYPE_FIELD_BITSIZE (type, i)))
{
- val_print_optimized_out (stream);
+ val_print_optimized_out (val, stream);
}
else
{
@@ -334,7 +334,7 @@ cp_print_value_fields (struct type *type, struct type *real_type,
_("<error reading variable: %s>"),
ex.message);
else if (v == NULL)
- val_print_optimized_out (stream);
+ val_print_optimized_out (NULL, stream);
else
cp_print_static_field (TYPE_FIELD_TYPE (type, i),
v, stream, recurse + 1,
diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 55d43f1..1313e17 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -2290,11 +2290,21 @@ dwarf2_evaluate_loc_desc_full (struct type *type, struct frame_info *frame,
if (byte_offset != 0)
error (_("cannot use offset on synthetic pointer to register"));
do_cleanups (value_chain);
- if (gdb_regnum != -1)
- retval = value_from_register (type, gdb_regnum, frame);
- else
+ if (gdb_regnum == -1)
error (_("Unable to access DWARF register number %d"),
dwarf_regnum);
+ retval = value_from_register (type, gdb_regnum, frame);
+ if (value_optimized_out (retval))
+ {
+ /* This means the register has undefined value / was
+ not saved. As we're computing the location of some
+ variable etc. in the program, not a value for
+ inspecting a register ($pc, $sp, etc.), return a
+ generic optimized out value instead, so that we show
+ <optimized out> instead of <not saved>. */
+ do_cleanups (value_chain);
+ retval = allocate_optimized_out_value (type);
+ }
}
break;

diff --git a/gdb/findvar.c b/gdb/findvar.c
index 25242be..c3550b4 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -757,6 +757,15 @@ address_from_register (struct type *type, int regnum, struct frame_info *frame)
value = value_from_register (type, regnum, frame);
gdb_assert (value);

+ if (value_optimized_out (value))
+ {
+ /* This function is used while computing a location expression.
+ Complain about the value being optimized out, rather than
+ letting value_as_address complain about some random register
+ the expression depends on not being saved. */
+ error_value_optimized_out ();
+ }
+
result = value_as_address (value);
release_value (value);
value_free (value);
diff --git a/gdb/frame.c b/gdb/frame.c
index d52c26a..be1a1b1 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1140,7 +1140,7 @@ put_frame_register (struct frame_info *frame, int regnum,
frame_register (frame, regnum, &optim, &unavail,
&lval, &addr, &realnum, NULL);
if (optim)
- error (_("Attempt to assign to a value that was optimized out."));
+ error (_("Attempt to assign to a register that was not saved."));
switch (lval)
{
case lval_memory:
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index f6a5290..2bba49e 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2035,7 +2035,7 @@ default_print_one_register_info (struct ui_file *file,
}
else if (value_optimized_out (val))
{
- val_print_optimized_out (file);
+ val_print_optimized_out (val, file);
fprintf_filtered (file, "\n");
return;
}
@@ -2142,7 +2142,7 @@ default_print_registers_info (struct gdbarch *gdbarch,

default_print_one_register_info (file,
gdbarch_register_name (gdbarch, i),
- get_frame_register_value (frame, i));
+ value_of_register (i, frame));
}
}

diff --git a/gdb/jv-valprint.c b/gdb/jv-valprint.c
index 3b90e54..cb89a85 100644
--- a/gdb/jv-valprint.c
+++ b/gdb/jv-valprint.c
@@ -395,7 +395,7 @@ java_print_value_fields (struct type *type, const gdb_byte *valaddr,
else if (!value_bits_valid (val, TYPE_FIELD_BITPOS (type, i),
TYPE_FIELD_BITSIZE (type, i)))
{
- val_print_optimized_out (stream);
+ val_print_optimized_out (val, stream);
}
else
{
@@ -420,7 +420,7 @@ java_print_value_fields (struct type *type, const gdb_byte *valaddr,
struct value *v = value_static_field (type, i);

if (v == NULL)
- val_print_optimized_out (stream);
+ val_print_optimized_out (NULL, stream);
else
{
struct value_print_options opts;
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index e6e98b6..503b7bf 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -1161,7 +1161,7 @@ output_register (struct frame_info *frame, int regnum, int format,
{
struct gdbarch *gdbarch = get_frame_arch (frame);
struct ui_out *uiout = current_uiout;
- struct value *val = get_frame_register_value (frame, regnum);
+ struct value *val = value_of_register (regnum, frame);
struct cleanup *tuple_cleanup;
struct value_print_options opts;
struct ui_file *stb;
diff --git a/gdb/p-valprint.c b/gdb/p-valprint.c
index 05d4c6f..e6d4b91 100644
--- a/gdb/p-valprint.c
+++ b/gdb/p-valprint.c
@@ -629,7 +629,7 @@ pascal_object_print_value_fields (struct type *type, const gdb_byte *valaddr,
else if (!value_bits_valid (val, TYPE_FIELD_BITPOS (type, i),
TYPE_FIELD_BITSIZE (type, i)))
{
- val_print_optimized_out (stream);
+ val_print_optimized_out (val, stream);
}
else
{
@@ -657,7 +657,7 @@ pascal_object_print_value_fields (struct type *type, const gdb_byte *valaddr,
v = value_field_bitfield (type, i, valaddr, offset, val);

if (v == NULL)
- val_print_optimized_out (stream);
+ val_print_optimized_out (NULL, stream);
else
pascal_object_print_static_field (v, stream, recurse + 1,
options);
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp b/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp
index 4686648..44bf8e9 100644
--- a/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp
+++ b/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp
@@ -45,8 +45,8 @@ for {set f 0} {$f < 3} {incr f} {
set pattern_r8_r9_print "$hex"
set pattern_r8_r9_info "$hex\\s+$decimal"
} else {
- set pattern_rax_rbx_rcx_print "<optimized out>"
- set pattern_rax_rbx_rcx_info "<optimized out>"
+ set pattern_rax_rbx_rcx_print "<not saved>"
+ set pattern_rax_rbx_rcx_info "<not saved>"
set pattern_r8_r9_print "$hex"
set pattern_r8_r9_info "$hex\\s+$decimal"
}
diff --git a/gdb/testsuite/gdb.mi/mi-reg-undefined.exp b/gdb/testsuite/gdb.mi/mi-reg-undefined.exp
index 8bcbb21..cb3a716 100644
--- a/gdb/testsuite/gdb.mi/mi-reg-undefined.exp
+++ b/gdb/testsuite/gdb.mi/mi-reg-undefined.exp
@@ -52,13 +52,13 @@ mi_gdb_test "111-stack-list-frames" \
"111\\^done,stack=\\\[frame=\{level=\"0\",addr=\"$hex\",func=\"stop_frame\",.*\},frame=\{level=\"1\",addr=\"$hex\",func=\"first_frame\",.*\},frame=\{level=\"2\",addr=\"$hex\",func=\"main\",.*\}\\\]" \
"stack frame listing"

-set opt_out_pattern "<optimized out>"
+set not_saved_pattern "<not saved>"

for {set f 0} {$f < 3} {incr f} {
if {${f} == 0} {
set pattern_0_1_2 ${hex}
} else {
- set pattern_0_1_2 ${opt_out_pattern}
+ set pattern_0_1_2 ${not_saved_pattern}
}

mi_gdb_test "22${f}-data-list-register-values --thread 1 --frame ${f} x 0 1 2 8 9" \
diff --git a/gdb/valprint.c b/gdb/valprint.c
index 0f6d65e..61492cc 100644
--- a/gdb/valprint.c
+++ b/gdb/valprint.c
@@ -314,7 +314,7 @@ valprint_check_validity (struct ui_file *stream,
if (!value_bits_valid (val, TARGET_CHAR_BIT * embedded_offset,
TARGET_CHAR_BIT * TYPE_LENGTH (type)))
{
- val_print_optimized_out (stream);
+ val_print_optimized_out (val, stream);
return 0;
}

@@ -336,9 +336,12 @@ valprint_check_validity (struct ui_file *stream,
}

void
-val_print_optimized_out (struct ui_file *stream)
+val_print_optimized_out (const struct value *val, struct ui_file *stream)
{
- fprintf_filtered (stream, _("<optimized out>"));
+ if (val != NULL && value_lval_const (val) == lval_register)
+ fprintf_filtered (stream, _("<not saved>"));
+ else
+ fprintf_filtered (stream, _("<optimized out>"));
}

void
@@ -805,7 +808,7 @@ value_check_printable (struct value *val, struct ui_file *stream,
if (options->summary && !val_print_scalar_type_p (value_type (val)))
fprintf_filtered (stream, "...");
else
- val_print_optimized_out (stream);
+ val_print_optimized_out (val, stream);
return 0;
}

@@ -966,7 +969,7 @@ val_print_scalar_formatted (struct type *type,
printed, because all bits contribute to its representation. */
if (!value_bits_valid (val, TARGET_CHAR_BIT * embedded_offset,
TARGET_CHAR_BIT * TYPE_LENGTH (type)))
- val_print_optimized_out (stream);
+ val_print_optimized_out (val, stream);
else if (!value_bytes_available (val, embedded_offset, TYPE_LENGTH (type)))
val_print_unavailable (stream);
else
diff --git a/gdb/valprint.h b/gdb/valprint.h
index e7073b6..1d86ed7 100644
--- a/gdb/valprint.h
+++ b/gdb/valprint.h
@@ -160,7 +160,8 @@ extern int read_string (CORE_ADDR addr, int len, int width,
enum bfd_endian byte_order, gdb_byte **buffer,
int *bytes_read);

-extern void val_print_optimized_out (struct ui_file *stream);
+extern void val_print_optimized_out (const struct value *val,
+ struct ui_file *stream);

extern void val_print_unavailable (struct ui_file *stream);

diff --git a/gdb/value.c b/gdb/value.c
index 42a8d2f..871b57c 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -197,8 +197,13 @@ struct value
reset, be sure to consider this use as well! */
unsigned int lazy : 1;

- /* If nonzero, this is the value of a variable which does not
- actually exist in the program. */
+ /* If nonzero, this is the value of a variable that does not
+ actually exist in the program. If nonzero, and LVAL is
+ lval_register, this is a register ($pc, $sp, etc., never a
+ program variable) that has not been saved in the frame. All
+ optimized-out values are treated pretty much the same, except
+ registers have a different string representation and related
+ error strings. */
unsigned int optimized_out : 1;

/* If value is a variable, is it initialized or not. */
@@ -902,11 +907,22 @@ value_actual_type (struct value *value, int resolve_simple_types,
return result;
}

+void
+error_value_optimized_out (void)
+{
+ error (_("value has been optimized out"));
+}
+
static void
require_not_optimized_out (const struct value *value)
{
if (value->optimized_out)
- error (_("value has been optimized out"));
+ {
+ if (value->lval == lval_register)
+ error (_("register has not been saved in frame"));
+ else
+ error_value_optimized_out ();
+ }
}

static void
diff --git a/gdb/value.h b/gdb/value.h
index 98dbadf..db964e3 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -273,6 +273,11 @@ extern void set_value_lazy (struct value *value, int val);
extern int value_stack (struct value *);
extern void set_value_stack (struct value *value, int val);

+/* Throw an error complaining that the value has been optimized
+ out. */
+
+extern void error_value_optimized_out (void);
+
/* value_contents() and value_contents_raw() both return the address
of the gdb buffer used to hold a copy of the contents of the lval.
value_contents() is used when the contents of the buffer are needed
--
1.7.11.7