Re: [PATCH 15/20] Add attribute::get_unsigned method

Simon Marchi
2020-03-30T15:57:14Z

On 2020-03-28 3:22 p.m., Tom Tromey wrote:
> This introduces a new attribute::get_unsigned method and changes a few
> spots to use it.
>
> gdb/ChangeLog
> 2020-03-28 Tom Tromey <tom@tromey.com>
>
> * dwarf2/read.c (dw2_get_file_names_reader)
> (dwarf2_build_include_psymtabs, handle_DW_AT_stmt_list)
> (dwarf2_cu::setup_type_unit_groups, fill_in_loclist_baton)
> (dwarf2_symbol_mark_computed): Use get_unsigned.
> * dwarf2/attribute.h (struct attribute) <get_unsigned>: New
> method.
> <form_is_section_offset>: Update comment.
> ---
> gdb/ChangeLog | 10 ++++++++++
> gdb/dwarf2/attribute.h | 11 ++++++++++-
> gdb/dwarf2/read.c | 22 +++++++++++-----------
> 3 files changed, 31 insertions(+), 12 deletions(-)
>
> diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
> index 9b387e5df05..546156283b3 100644
> --- a/gdb/dwarf2/attribute.h
> +++ b/gdb/dwarf2/attribute.h
> @@ -82,9 +82,18 @@ struct attribute
> return u.unsnd;
> }
>
> + /* Return the unsigned value. Requires that the form be an unsigned
> + form, and that reprocessing not be needed. */
> + ULONGEST get_unsigned () const
> + {
> + gdb_assert (form_is_unsigned ());
> + gdb_assert (!requires_reprocessing);

I see what you're trying to do here, but I don't think it's really useful to assert !requires_reprocessing.

For string and address forms that require reprocessing, it's true that we set an unsigned value, but the
form is still some string of address form (we don't change it to some unsigned form). So it's not really
possible for form_is_unsigned() to return true, and requires_reprocessing to be true.

Instead, gdb_assert (!requires_reprocessing) should be added to ::address and ::string.

Note that I don't mind if we leave the assert in this function, it's true in any case that requires_reprocessing
should be false at this point.

Also, I noted that this method is named "get_unsigned", since you obviously can't name it "unsigned". If you
used my idea to name the others "as_string" and "as_address", this one could be "as_unsigned", and the names
would all be coherent.

Simon