[PATCH 3/8] Change fde table to a vector
Luis Machado
luis.machado@linaro.org
Tue Feb 11 10:27:00 GMT 2020
Great cleanup. Just a few comments...
On 2/8/20 12:27 PM, Tom Tromey wrote:
> This removes struct dwarf2_fde_table, replacing it with a typedef of
> std::vector. This simplifies the code somewhat.
>
> gdb/ChangeLog
> 2020-02-08 Tom Tromey <tom@tromey.com>
>
> * dwarf2/frame.c (struct dwarf2_fde_table): Remove.
> (dwarf2_fde_table): Typedef for std::vector.
> (dwarf2_frame_objfile_data): Remove the deleter.
> (dwarf2_frame_find_fde, add_fde, decode_frame_entry_1)
> (decode_frame_entry): Update.
> (dwarf2_build_frame_info): Use "new".
>
> Change-Id: Ie31c7413dce2865b76f5f70374ba7a559112ce5e
> ---
> gdb/ChangeLog | 9 ++++
> gdb/dwarf2/frame.c | 102 ++++++++++++---------------------------------
> 2 files changed, 35 insertions(+), 76 deletions(-)
>
> diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c
> index db8e5cd25f3..6d87e598345 100644
> --- a/gdb/dwarf2/frame.c
> +++ b/gdb/dwarf2/frame.c
> @@ -129,11 +129,7 @@ struct dwarf2_fde
> unsigned char eh_frame_p;
> };
>
> -struct dwarf2_fde_table
> -{
> - int num_entries;
> - struct dwarf2_fde **entries;
> -};
> +typedef std::vector<dwarf2_fde *> dwarf2_fde_table;
>
> /* A minimal decoding of DWARF2 compilation units. We only decode
> what's needed to get to the call frame information. */
> @@ -1471,9 +1467,7 @@ dwarf2_frame_cfa (struct frame_info *this_frame)
> return get_frame_base (this_frame);
> }
>
> -const struct objfile_key<dwarf2_fde_table,
> - gdb::noop_deleter<dwarf2_fde_table>>
> - dwarf2_frame_objfile_data;
> +const struct objfile_key<dwarf2_fde_table> dwarf2_frame_objfile_data;
>
>
>
> @@ -1636,7 +1630,7 @@ dwarf2_frame_find_fde (CORE_ADDR *pc, CORE_ADDR *out_offset)
> {
> for (objfile *objfile : current_program_space->objfiles ())
> {
> - struct dwarf2_fde_table *fde_table;
> + dwarf2_fde_table *fde_table;
> CORE_ADDR offset;
> CORE_ADDR seek_pc;
>
> @@ -1648,20 +1642,20 @@ dwarf2_frame_find_fde (CORE_ADDR *pc, CORE_ADDR *out_offset)
> }
> gdb_assert (fde_table != NULL);
>
> - if (fde_table->num_entries == 0)
> + if (fde_table->empty ())
> continue;
>
> gdb_assert (!objfile->section_offsets.empty ());
> offset = objfile->text_section_offset ();
>
> - gdb_assert (fde_table->num_entries > 0);
> - if (*pc < offset + fde_table->entries[0]->initial_location)
> + gdb_assert (!fde_table->empty ());
> + if (*pc < offset + (*fde_table)[0]->initial_location)
> continue;
>
> seek_pc = *pc - offset;
> - auto end = fde_table->entries + fde_table->num_entries;
> - auto it = gdb::binary_search (fde_table->entries, end, seek_pc, bsearch_fde_cmp);
> - if (it != end)
> + auto it = gdb::binary_search (fde_table->begin (), fde_table->end (),
> + seek_pc, bsearch_fde_cmp);
> + if (it != fde_table->end ())
> {
> *pc = (*it)->initial_location + offset;
> if (out_offset)
> @@ -1674,16 +1668,13 @@ dwarf2_frame_find_fde (CORE_ADDR *pc, CORE_ADDR *out_offset)
>
> /* Add a pointer to new FDE to the FDE_TABLE, allocating space for it. */
> static void
We don't quite allocate space for it anymore, but we do behind the
scenes. The comment seems slightly stale. Plus the sentence reads a bit
funny. I'd update this, but i'm fine keeping it too.
> -add_fde (struct dwarf2_fde_table *fde_table, struct dwarf2_fde *fde)
> +add_fde (dwarf2_fde_table *fde_table, struct dwarf2_fde *fde)
> {
> if (fde->address_range == 0)
> /* Discard useless FDEs. */
> return;
>
> - fde_table->num_entries += 1;
> - fde_table->entries = XRESIZEVEC (struct dwarf2_fde *, fde_table->entries,
> - fde_table->num_entries);
> - fde_table->entries[fde_table->num_entries - 1] = fde;
> + fde_table->push_back (fde);
> }
>
> #define DW64_CIE_ID 0xffffffffffffffffULL
> @@ -1702,7 +1693,7 @@ static const gdb_byte *decode_frame_entry (struct comp_unit *unit,
> const gdb_byte *start,
> int eh_frame_p,
> dwarf2_cie_table &cie_table,
> - struct dwarf2_fde_table *fde_table,
> + dwarf2_fde_table *fde_table,
> enum eh_frame_type entry_type);
>
> /* Decode the next CIE or FDE, entry_type specifies the expected type.
> @@ -1712,7 +1703,7 @@ static const gdb_byte *
> decode_frame_entry_1 (struct comp_unit *unit, const gdb_byte *start,
> int eh_frame_p,
> dwarf2_cie_table &cie_table,
> - struct dwarf2_fde_table *fde_table,
> + dwarf2_fde_table *fde_table,
> enum eh_frame_type entry_type)
> {
> struct gdbarch *gdbarch = get_objfile_arch (unit->objfile);
> @@ -2014,7 +2005,7 @@ static const gdb_byte *
> decode_frame_entry (struct comp_unit *unit, const gdb_byte *start,
> int eh_frame_p,
> dwarf2_cie_table &cie_table,
> - struct dwarf2_fde_table *fde_table,
> + dwarf2_fde_table *fde_table,
> enum eh_frame_type entry_type)
> {
> enum { NONE, ALIGN4, ALIGN8, FAIL } workaround = NONE;
> @@ -2128,11 +2119,8 @@ dwarf2_build_frame_info (struct objfile *objfile)
> struct comp_unit *unit;
> const gdb_byte *frame_ptr;
> dwarf2_cie_table cie_table;
> - struct dwarf2_fde_table fde_table;
> - struct dwarf2_fde_table *fde_table2;
> -
> - fde_table.num_entries = 0;
> - fde_table.entries = NULL;
> + dwarf2_fde_table fde_table;
> + dwarf2_fde_table *fde_table2;
>
> /* Build a minimal decoding of the DWARF2 compilation unit. */
> unit = XOBNEW (&objfile->objfile_obstack, comp_unit);
> @@ -2181,12 +2169,7 @@ dwarf2_build_frame_info (struct objfile *objfile)
> warning (_("skipping .eh_frame info of %s: %s"),
> objfile_name (objfile), e.what ());
>
> - if (fde_table.num_entries != 0)
> - {
> - xfree (fde_table.entries);
> - fde_table.entries = NULL;
> - fde_table.num_entries = 0;
> - }
> + fde_table.clear ();
> /* The cie_table is discarded below. */
> }
>
> @@ -2200,7 +2183,7 @@ dwarf2_build_frame_info (struct objfile *objfile)
> &unit->dwarf_frame_size);
> if (unit->dwarf_frame_size)
> {
> - int num_old_fde_entries = fde_table.num_entries;
> + size_t num_old_fde_entries = fde_table.size ();
>
> try
> {
> @@ -2215,42 +2198,20 @@ dwarf2_build_frame_info (struct objfile *objfile)
> warning (_("skipping .debug_frame info of %s: %s"),
> objfile_name (objfile), e.what ());
>
> - if (fde_table.num_entries != 0)
> - {
> - fde_table.num_entries = num_old_fde_entries;
> - if (num_old_fde_entries == 0)
> - {
> - xfree (fde_table.entries);
> - fde_table.entries = NULL;
> - }
> - else
> - {
> - fde_table.entries
> - = XRESIZEVEC (struct dwarf2_fde *, fde_table.entries,
> - fde_table.num_entries);
> - }
> - }
> - fde_table.num_entries = num_old_fde_entries;
> + fde_table.resize (num_old_fde_entries);
Took me a bit to figure out the code above. Quite convoluted in its use
of the size variables. But the new code looks sane as far as i can tell.
> }
> }
>
> /* Copy fde_table to obstack: it is needed at runtime. */
> - fde_table2 = XOBNEW (&objfile->objfile_obstack, struct dwarf2_fde_table);
> + fde_table2 = new dwarf2_fde_table; >
The comment mentioning obstack is now obsolete and can be removed.
> - if (fde_table.num_entries == 0)
> - {
> - fde_table2->entries = NULL;
> - fde_table2->num_entries = 0;
> - }
> - else
> + if (!fde_table.empty ())
> {
> struct dwarf2_fde *fde_prev = NULL;
> struct dwarf2_fde *first_non_zero_fde = NULL;
> - int i;
>
> /* Prepare FDE table for lookups. */
> - std::sort (fde_table.entries, fde_table.entries + fde_table.num_entries,
> - fde_is_less_than);
> + std::sort (fde_table.begin (), fde_table.end (), fde_is_less_than);
>
> /* Check for leftovers from --gc-sections. The GNU linker sets
> the relevant symbols to zero, but doesn't zero the FDE *end*
> @@ -2264,10 +2225,8 @@ dwarf2_build_frame_info (struct objfile *objfile)
> Start by finding the first FDE with non-zero start. Below
> we'll discard all FDEs that start at zero and overlap this
> one. */
> - for (i = 0; i < fde_table.num_entries; i++)
> + for (struct dwarf2_fde *fde : fde_table)
> {
> - struct dwarf2_fde *fde = fde_table.entries[i];
> -
> if (fde->initial_location != 0)
> {
> first_non_zero_fde = fde;
> @@ -2278,11 +2237,8 @@ dwarf2_build_frame_info (struct objfile *objfile)
> /* Since we'll be doing bsearch, squeeze out identical (except
> for eh_frame_p) fde entries so bsearch result is predictable.
> Also discard leftovers from --gc-sections. */
> - fde_table2->num_entries = 0;
> - for (i = 0; i < fde_table.num_entries; i++)
> + for (struct dwarf2_fde *fde : fde_table)
> {
> - struct dwarf2_fde *fde = fde_table.entries[i];
> -
> if (fde->initial_location == 0
> && first_non_zero_fde != NULL
> && (first_non_zero_fde->initial_location
> @@ -2293,16 +2249,10 @@ dwarf2_build_frame_info (struct objfile *objfile)
> && fde_prev->initial_location == fde->initial_location)
> continue;
>
> - obstack_grow (&objfile->objfile_obstack, &fde_table.entries[i],
> - sizeof (fde_table.entries[0]));
> - ++fde_table2->num_entries;
> + fde_table2->push_back (fde);
> fde_prev = fde;
> }
> - fde_table2->entries
> - = (struct dwarf2_fde **) obstack_finish (&objfile->objfile_obstack);
> -
> - /* Discard the original fde_table. */
> - xfree (fde_table.entries);
> + fde_table2->shrink_to_fit ();
> }
>
> dwarf2_frame_objfile_data.set (objfile, fde_table2);
>
Otherwise LGTM.
More information about the Gdb-patches
mailing list