[binutils-gdb] gdb: don't copy entirely optimized out values in value_copy

[binutils-gdb] gdb: don't copy entirely optimized out values in value_copy

[Simon Marchi]

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

commit 6d088eb92ee42e05a4fbe797515229cf2acd0d99
Author: Simon Marchi <simon.marchi@efficios.com>
Date:   Mon Apr 4 17:45:59 2022 -0400

    gdb: don't copy entirely optimized out values in value_copy
    
    Bug 28980 shows that trying to value_copy an entirely optimized out
    value causes an internal error.  The original bug report involves MI and
    some Python pretty printer, and is quite difficult to reproduce, but
    another easy way to reproduce (that is believed to be equivalent) was
    proposed:
    
        $ ./gdb -q -nx --data-directory=data-directory -ex "py print(gdb.Value(gdb.Value(5).type.optimized_out()))"
        /home/smarchi/src/binutils-gdb/gdb/value.c:1731: internal-error: value_copy: Assertion `arg->contents != nullptr' failed.
    
    This is caused by 5f8ab46bc691 ("gdb: constify parameter of
    value_copy").  It added an assertion that the contents buffer is
    allocated if the value is not lazy:
    
      if (!value_lazy (val))
        {
          gdb_assert (arg->contents != nullptr);
    
    This was based on the comment on value::contents, which suggest that
    this is the case:
    
      /* Actual contents of the value.  Target byte-order.  NULL or not
         valid if lazy is nonzero.  */
      gdb::unique_xmalloc_ptr<gdb_byte> contents;
    
    However, it turns out that it can also be nullptr also if the value is
    entirely optimized out, for example on exit of
    allocate_optimized_out_value.  That function creates a lazy value, marks
    the entire value as optimized out, and then clears the lazy flag.  But
    contents remains nullptr.
    
    This wasn't a problem for value_copy before, because it was calling
    value_contents_all_raw on the input value, which caused contents to be
    allocated before doing the copy.  This means that the input value to
    value_copy did not have its contents allocated on entry, but had it
    allocated on exit.  The result value had it allocated on exit.  And that
    we copied bytes for an entirely optimized out value (i.e. meaningless
    bytes).
    
    From here I see two choices:
    
     1. respect the documented invariant that contents is nullptr only and
        only if the value is lazy, which means making
        allocate_optimized_out_value allocate contents
     2. extend the cases where contents can be nullptr to also include
        values that are entirely optimized out (note that you could still
        have some entirely optimized out values that do have contents
        allocated, it depends on how they were created) and adjust
        value_copy accordingly
    
    Choice #1 is safe, but less efficient: it's not very useful to allocate
    a buffer for an entirely optimized out value.  It's even a bit less
    efficient than what we had initially, because values coming out of
    allocate_optimized_out_value would now always get their contents
    allocated.
    
    Choice #2 would be more efficient than what we had before: giving an
    optimized out value without allocated contents to value_copy would
    result in an optimized out value without allocated contents (and the
    input value would still be without allocated contents on exit).  But
    it's more risky, since it's difficult to ensure that all users of the
    contents (through the various_contents* accessors) are all fine with
    that new invariant.
    
    In this patch, I opt for choice #2, since I think it is a better
    direction than choice #1.  #1 would be a pessimization, and if we go
    this way, I doubt that it will ever be revisited, it will just stay that
    way forever.
    
    Add a selftest to test this.  I initially started to write it as a
    Python test (since the reproducer is in Python), but a selftest is more
    straightforward.
    
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28980
    Change-Id: I6e2f5c0ea804fafa041fcc4345d47064b5900ed7

Diff:
---
 gdb/testsuite/gdb.python/py-value.exp |  3 +++
 gdb/value.c                           | 27 ++++++++++++++++++++++-----
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/gdb/testsuite/gdb.python/py-value.exp b/gdb/testsuite/gdb.python/py-value.exp
index 5d77b0ccc3c..b88c4515c8c 100644
--- a/gdb/testsuite/gdb.python/py-value.exp
+++ b/gdb/testsuite/gdb.python/py-value.exp
@@ -62,6 +62,9 @@ proc test_value_creation {} {
 
   # Test address attribute is None in a non-addressable value
   gdb_test "python print ('result = %s' % i.address)" "= None" "test address attribute in non-addressable value"
+
+  # Test creating / printing an optimized out value
+  gdb_test "python print(gdb.Value(gdb.Value(5).type.optimized_out()))"
 }
 
 # Check that we can call gdb.Value.__init__ to change a value.
diff --git a/gdb/value.c b/gdb/value.c
index 482abcee1c1..24f1151c03f 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -343,8 +343,10 @@ struct value
   LONGEST embedded_offset = 0;
   LONGEST pointed_to_offset = 0;
 
-  /* Actual contents of the value.  Target byte-order.  NULL or not
-     valid if lazy is nonzero.  */
+  /* Actual contents of the value.  Target byte-order.
+
+     May be nullptr if the value is lazy or is entirely optimized out.
+     Guaranteed to be non-nullptr otherwise.  */
   gdb::unique_xmalloc_ptr<gdb_byte> contents;
 
   /* Unavailable ranges in CONTENTS.  We mark unavailable ranges,
@@ -1725,8 +1727,10 @@ value_copy (const value *arg)
   val->stack = arg->stack;
   val->is_zero = arg->is_zero;
   val->initialized = arg->initialized;
+  val->unavailable = arg->unavailable;
+  val->optimized_out = arg->optimized_out;
 
-  if (!value_lazy (val))
+  if (!value_lazy (val) && !value_entirely_optimized_out (val))
     {
       gdb_assert (arg->contents != nullptr);
       ULONGEST length = TYPE_LENGTH (value_enclosing_type (arg));
@@ -1735,8 +1739,6 @@ value_copy (const value *arg)
       copy (arg_view, value_contents_all_raw (val));
     }
 
-  val->unavailable = arg->unavailable;
-  val->optimized_out = arg->optimized_out;
   val->parent = arg->parent;
   if (VALUE_LVAL (val) == lval_computed)
     {
@@ -4271,6 +4273,20 @@ test_insert_into_bit_range_vector ()
   }
 }
 
+static void
+test_value_copy ()
+{
+  type *type = builtin_type (current_inferior ()->gdbarch)->builtin_int;
+
+  /* Verify that we can copy an entirely optimized out value, that may not have
+     its contents allocated.  */
+  value_ref_ptr val = release_value (allocate_optimized_out_value (type));
+  value_ref_ptr copy = release_value (value_copy (val.get ()));
+
+  SELF_CHECK (value_entirely_optimized_out (val.get ()));
+  SELF_CHECK (value_entirely_optimized_out (copy.get ()));
+}
+
 } /* namespace selftests */
 #endif /* GDB_SELF_TEST */
 
@@ -4355,6 +4371,7 @@ and exceeds this limit will cause an error."),
   selftests::register_test ("ranges_contain", selftests::test_ranges_contain);
   selftests::register_test ("insert_into_bit_range_vector",
 			    selftests::test_insert_into_bit_range_vector);
+  selftests::register_test ("value_copy", selftests::test_value_copy);
 #endif
 }