gdb: don't copy entirely optimized out values in value_copy
authorSimon Marchi <simon.marchi@efficios.com>
Mon, 4 Apr 2022 21:45:59 +0000 (17:45 -0400)
committerSimon Marchi <simon.marchi@polymtl.ca>
Wed, 6 Apr 2022 20:11:13 +0000 (16:11 -0400)
commit6d088eb92ee42e05a4fbe797515229cf2acd0d99
treeb662dd8772412a70b78f3a92d9ab1d6952744b29
parent477904ca751c50d243ee3cba3f12cf75e8ba12b3
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
gdb/testsuite/gdb.python/py-value.exp
gdb/value.c