Re: [pushed] Re: [PATCH 1/6] Code cleanup: C++ify .gdb_index producer.

Pedro Alves
2017-06-12T16:18:52Z

On 06/12/2017 05:08 PM, Pedro Alves wrote:

> Thanks. This looks generally good.
>
> It had a number of GDB/GCC code convention issues though.
>
> - Should use /**/ for comments.
> - CamelCase should only used in template parameters.
> - Function/ctors should be placed before data members.
> - Private data members should be prefixed with "m_".
> - Some function intro comments still referred to
> obstacks, while the functions now take a DataBuf.
>
> Also, the big TRY block would be better done with a RAII type.
> Tromey posted a patch to add a unique_ptr with a deleter that calls
> fclose, and makes that code use it, but that's not merged yet.
> Meanwhile we can add a little RAII class locally that does the
> same thing, still avoiding the churn.

(...)

> Anyway, in interest of forward progress:
>
> - I fixed up the style issues mentioned above, and pushed the
> result in, as below. I'll post the diff as a reply to this
> email.
>
> I'd much appreciate it if you'd adjust the rest of the series
> similarly.

Thanks,
Pedro Alves

diff --git a/gdb/common/common-defs.h b/gdb/common/common-defs.h
index e624c5e..439bce6 100644
--- a/gdb/common/common-defs.h
+++ b/gdb/common/common-defs.h
@@ -91,18 +91,4 @@
/* Pull in gdb::unique_xmalloc_ptr. */
#include "common/gdb_unique_ptr.h"

-// Provide C++14 std::make_unique<> for C++11 compilation mode.
-// A copy from: gcc/libstdc++-v3/include/bits/unique_ptr.h
-#if __cplusplus <= 201103L
-namespace std {
- template<typename _Tp>
- struct _MakeUniq
- { typedef unique_ptr<_Tp> __single_object; };
- template<typename _Tp, typename... _Args>
- inline typename _MakeUniq<_Tp>::__single_object
- make_unique(_Args&&... __args)
- { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); }
-}
-#endif
-
#endif /* COMMON_DEFS_H */
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 12a194a..c5c5d55 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -69,6 +69,7 @@
#include "filestuff.h"
#include "build-id.h"
#include "namespace.h"
+#include "common/gdb_unlinker.h"
#include "common/function-view.h"
#include "common/gdb_optional.h"
#include "common/underlying.h"
@@ -23194,7 +23195,8 @@ dwarf2_per_objfile_free (struct objfile *objfile, void *d)
\f
/* The "save gdb-index" command. */

-// Write to FILE bytes starting at DATA of length SIZE with error checking.
+/* Write SIZE bytes from the buffer pointed to by DATA to FILE, with
+ error checking. */

static void
file_write (FILE *file, const void *data, size_t size)
@@ -23203,66 +23205,58 @@ file_write (FILE *file, const void *data, size_t size)
error (_("couldn't data write to file"));
}

-// Write to FILE bytes of std::vector VEC with error checking.
+/* Write the contents of VEC to FILE, with error checking. */

-template<class Elem> static void
+template<class Elem>
+static void
file_write (FILE *file, const std::vector<Elem> &vec)
{
file_write (file, vec.data (), vec.size() * sizeof (vec[0]));
}

-// In-memory buffer to prepare data to be written later to a file.
-class DataBuf
+/* In-memory buffer to prepare data to be written later to a file. */
+class data_buf
{
-private:
- std::vector<gdb_byte> vec;
public:
-
- // Append space of SIZE number of bytes to the end of buffer.
- // Return pointer to its start.
-
- gdb_byte *
- append_space (size_t size)
+ /* Add SIZE bytes at the end of the buffer. Returns a pointer to
+ the start of the new block. */
+ gdb_byte *append_space (size_t size)
{
- vec.resize (vec.size () + size);
- return &*vec.end () - size;
+ m_vec.resize (m_vec.size () + size);
+ return &*m_vec.end () - size;
}

- // Copy DATA to the end of buffer.
-
- template<typename T> void
- append_data (const T &data)
+ /* Copy DATA to the end of the buffer. */
+ template<typename T>
+ void append_data (const T &data)
{
std::copy (reinterpret_cast<const gdb_byte *> (&data),
reinterpret_cast<const gdb_byte *> (&data + 1),
append_space (sizeof (data)));
}

- // Copy CSTR zero-terminated string to the end of buffer including its
- // terminating zero.
-
- void
- append_cstr0 (const char *cstr)
+ /* Copy CSTR (a null-terminated string) to the end of the buffer.
+ The terminating null is appended too. */
+ void append_cstr0 (const char *cstr)
{
- const size_t size (strlen (cstr) + 1);
+ const size_t size = strlen (cstr) + 1;
std::copy (cstr, cstr + size, append_space (size));
}

- // Return size of the buffer.
-
- size_t
- size () const
+ /* Return the size of the buffer. */
+ size_t size () const
{
- return vec.size ();
+ return m_vec.size ();
}

/* Write the buffer to FILE. */
-
- void
- file_write (FILE *file) const
+ void file_write (FILE *file) const
{
- ::file_write (file, vec);
+ ::file_write (file, m_vec);
}
+
+private:
+ std::vector<gdb_byte> m_vec;
};

/* An entry in the symbol table. */
@@ -23280,13 +23274,13 @@ struct symtab_index_entry
/* The symbol table. This is a power-of-2-sized hash table. */
struct mapped_symtab
{
-public:
- offset_type n_elements = 0;
- std::vector<std::unique_ptr<symtab_index_entry>> data;
mapped_symtab ()
{
data.resize (1024);
}
+
+ offset_type n_elements = 0;
+ std::vector<std::unique_ptr<symtab_index_entry>> data;
};

/* Find a slot in SYMTAB for the symbol NAME. Returns a reference to
@@ -23316,15 +23310,15 @@ find_slot (struct mapped_symtab *symtab, const char *name)
static void
hash_expand (struct mapped_symtab *symtab)
{
- auto old_entries (std::move (symtab->data));
+ auto old_entries = std::move (symtab->data);

symtab->data.clear ();
symtab->data.resize (old_entries.size () * 2);

- for (auto &it:old_entries)
- if (it)
+ for (auto &it : old_entries)
+ if (it != NULL)
{
- auto &ref (find_slot (symtab, it->name));
+ auto &ref = find_slot (symtab, it->name);
ref = std::move (it);
}
}
@@ -23344,10 +23338,10 @@ add_index_entry (struct mapped_symtab *symtab, const char *name,
if (4 * symtab->n_elements / 3 >= symtab->data.size ())
hash_expand (symtab);

- std::unique_ptr<symtab_index_entry> &slot (find_slot (symtab, name));
- if (!slot)
+ std::unique_ptr<symtab_index_entry> &slot = find_slot (symtab, name);
+ if (slot == NULL)
{
- slot = std::make_unique<symtab_index_entry> ();
+ slot.reset (new symtab_index_entry ());
slot->name = name;
/* index_offset is set later. */
}
@@ -23372,7 +23366,7 @@ add_index_entry (struct mapped_symtab *symtab, const char *name,
static void
uniquify_cu_indices (struct mapped_symtab *symtab)
{
- for (const auto &entry:symtab->data)
+ for (const auto &entry : symtab->data)
{
if (entry && !entry->cu_indices.empty ())
{
@@ -23397,87 +23391,89 @@ uniquify_cu_indices (struct mapped_symtab *symtab)
}
}

-// Provide form of const char * suitable for container keys.
-// Only the pointer is being stored.
-// Comparison is done for the strings themselves - not for the pointer.
-class CstrView {
-private:
- friend class CstrViewHasher;
- const char *const cstr;
+/* A form of 'const char *' suitable for container keys. Only the
+ pointer is stored. The strings themselves are compared, not the
+ pointers. */
+class c_str_view
+{
public:
- CstrView (const char *cstr_) : cstr (cstr_)
- {
- }
- bool
- operator == (const CstrView &other) const
+ c_str_view (const char *cstr)
+ : m_cstr (cstr)
+ {}
+
+ bool operator== (const c_str_view &other) const
{
- return !strcmp (cstr, other.cstr);
+ return strcmp (m_cstr, other.m_cstr) == 0;
}
+
+private:
+ friend class c_str_view_hasher;
+ const char *const m_cstr;
};

-// Provide std::unordered_map::hasher for CstrView.
-class CstrViewHasher
+/* A std::unordered_map::hasher for c_str_view that uses the right
+ hash function for strings in a mapped index. */
+class c_str_view_hasher
{
public:
- size_t
- operator () (const CstrView &x) const
+ size_t operator () (const c_str_view &x) const
{
- return mapped_index_string_hash (INT_MAX, x.cstr);
+ return mapped_index_string_hash (INT_MAX, x.m_cstr);
}
};

-// Provide std::unordered_map::hasher for std::vector<>.
-template<class T> class VectorHasher
+/* A std::unordered_map::hasher for std::vector<>. */
+template<class T>
+class vector_hasher
{
public:
- size_t
- operator () (const std::vector<T> &key) const
+ size_t operator () (const std::vector<T> &key) const
{
- // return boost::hash_value<std::vector<T>> (key);
return iterative_hash (key.data (),
sizeof (key.front ()) * key.size (), 0);
}
};

-/* Write the mapped hash table SYMTAB to the obstack OUTPUT, with
- constant pool entries going into the obstack CPOOL. */
+/* Write the mapped hash table SYMTAB to the data buffer OUTPUT, with
+ constant pool entries going into the data buffer CPOOL. */

static void
-write_hash_table (struct mapped_symtab *symtab, DataBuf &output, DataBuf &cpool)
+write_hash_table (mapped_symtab *symtab, data_buf &output, data_buf &cpool)
{
{
- /* Elements are sorted vectors of the indices of all the CUs that hold
- an object of this name. */
+ /* Elements are sorted vectors of the indices of all the CUs that
+ hold an object of this name. */
std::unordered_map<std::vector<offset_type>, offset_type,
- VectorHasher<offset_type>> symbol_hash_table;
+ vector_hasher<offset_type>>
+ symbol_hash_table;

/* We add all the index vectors to the constant pool first, to
ensure alignment is ok. */
- for (const std::unique_ptr<symtab_index_entry> &it:symtab->data)
+ for (const std::unique_ptr<symtab_index_entry> &it : symtab->data)
{
- if (!it)
+ if (it == NULL)
continue;
gdb_assert (it->index_offset == 0);
- const auto insertpair (symbol_hash_table.emplace (it->cu_indices,
- cpool.size ()));
+ const auto insertpair
+ = symbol_hash_table.emplace (it->cu_indices, cpool.size ());
it->index_offset = insertpair.first->second;
if (!insertpair.second)
continue;
cpool.append_data (MAYBE_SWAP (it->cu_indices.size ()));
- for (const auto iter:it->cu_indices)
+ for (const auto iter : it->cu_indices)
cpool.append_data (MAYBE_SWAP (iter));
}
}

/* Now write out the hash table. */
- std::unordered_map<CstrView, offset_type, CstrViewHasher> str_table;
- for (const auto &it:symtab->data)
+ std::unordered_map<c_str_view, offset_type, c_str_view_hasher> str_table;
+ for (const auto &it : symtab->data)
{
offset_type str_off, vec_off;

- if (it)
+ if (it != NULL)
{
- const auto insertpair (str_table.emplace (it->name, cpool.size ()));
+ const auto insertpair = str_table.emplace (it->name, cpool.size ());
if (insertpair.second)
cpool.append_cstr0 (it->name);
str_off = insertpair.first->second;
@@ -23496,12 +23492,18 @@ write_hash_table (struct mapped_symtab *symtab, DataBuf &output, DataBuf &cpool)
}
}

+typedef std::unordered_map<partial_symtab *, unsigned int> psym_index_map;
+
/* Helper struct for building the address table. */
struct addrmap_index_data
{
+ addrmap_index_data (data_buf &addr_vec_, psym_index_map &cu_index_htab_)
+ : addr_vec (addr_vec_), cu_index_htab (cu_index_htab_)
+ {}
+
struct objfile *objfile;
- DataBuf &addr_vec;
- std::unordered_map<struct partial_symtab *, unsigned int> &cu_index_htab;
+ data_buf &addr_vec;
+ psym_index_map &cu_index_htab;

/* Non-zero if the previous_* fields are valid.
We can't write an entry until we see the next entry (since it is only then
@@ -23511,17 +23513,12 @@ struct addrmap_index_data
unsigned int previous_cu_index;
/* Start address of the CU. */
CORE_ADDR previous_cu_start;
-
- addrmap_index_data (DataBuf &addr_vec_,
- std::unordered_map<struct partial_symtab *, unsigned int> &cu_index_htab_)
- : addr_vec (addr_vec_), cu_index_htab (cu_index_htab_)
- {}
};

-/* Write an address entry to OBSTACK. */
+/* Write an address entry to ADDR_VEC. */

static void
-add_address_entry (struct objfile *objfile, DataBuf &addr_vec,
+add_address_entry (struct objfile *objfile, data_buf &addr_vec,
CORE_ADDR start, CORE_ADDR end, unsigned int cu_index)
{
CORE_ADDR baseaddr;
@@ -23551,24 +23548,24 @@ add_address_entry_worker (void *datap, CORE_ADDR start_addr, void *obj)
data->previous_cu_start = start_addr;
if (pst != NULL)
{
- const auto it (data->cu_index_htab.find (pst));
+ const auto it = data->cu_index_htab.find (pst);
gdb_assert (it != data->cu_index_htab.cend ());
data->previous_cu_index = it->second;
data->previous_valid = 1;
}
else
- data->previous_valid = 0;
+ data->previous_valid = 0;

return 0;
}

-/* Write OBJFILE's address map to OBSTACK.
+/* Write OBJFILE's address map to ADDR_VEC.
CU_INDEX_HTAB is used to map addrmap entries to their CU indices
in the index file. */

static void
-write_address_map (struct objfile *objfile, DataBuf &addr_vec,
- std::unordered_map<struct partial_symtab *, unsigned int> &cu_index_htab)
+write_address_map (struct objfile *objfile, data_buf &addr_vec,
+ psym_index_map &cu_index_htab)
{
struct addrmap_index_data addrmap_index_data (addr_vec, cu_index_htab);

@@ -23662,16 +23659,16 @@ write_psymbols (struct mapped_symtab *symtab,
/* A helper struct used when iterating over debug_types. */
struct signatured_type_index_data
{
+ signatured_type_index_data (data_buf &types_list_,
+ std::unordered_set<partial_symbol *> &psyms_seen_)
+ :types_list (types_list_), psyms_seen (psyms_seen_)
+ {}
+
struct objfile *objfile;
struct mapped_symtab *symtab;
- DataBuf &types_list;
+ data_buf &types_list;
std::unordered_set<partial_symbol *> &psyms_seen;
int cu_index;
-
- signatured_type_index_data (DataBuf &types_list_,
- std::unordered_set<partial_symbol *> &psyms_seen_)
- :types_list (types_list_), psyms_seen (psyms_seen_)
- {}
};

/* A helper function that writes a single signatured_type to an
@@ -23741,6 +23738,20 @@ recursively_write_psymbols (struct objfile *objfile,
1);
}

+/* Closes FILE on scope exit. */
+struct file_closer
+{
+ explicit file_closer (FILE *file)
+ : m_file (file)
+ {}
+
+ ~file_closer ()
+ { fclose (m_file); }
+
+private:
+ FILE *m_file;
+};
+
/* Create an index file for OBJFILE in the directory DIR. */

static void
@@ -23762,121 +23773,116 @@ write_psymtabs_to_index (struct objfile *objfile, const char *dir)
std::string filename (std::string (dir) + SLASH_STRING
+ lbasename (objfile_name (objfile)) + INDEX_SUFFIX);

- FILE *out_file (gdb_fopen_cloexec (filename.c_str (), "wb"));
+ FILE *out_file = gdb_fopen_cloexec (filename.c_str (), "wb");
if (!out_file)
error (_("Can't open `%s' for writing"), filename.c_str ());

- TRY
- {
- mapped_symtab symtab;
- DataBuf cu_list;
- std::unordered_set<partial_symbol *> psyms_seen;
-
- /* While we're scanning CU's create a table that maps a psymtab pointer
- (which is what addrmap records) to its index (which is what is recorded
- in the index file). This will later be needed to write the address
- table. */
- std::unordered_map<struct partial_symtab *, unsigned int> cu_index_htab;
- cu_index_htab.reserve (dwarf2_per_objfile->n_comp_units);
-
- /* The CU list is already sorted, so we don't need to do additional
- work here. Also, the debug_types entries do not appear in
- all_comp_units, but only in their own hash table. */
- for (int i = 0; i < dwarf2_per_objfile->n_comp_units; ++i)
- {
- struct dwarf2_per_cu_data *per_cu
- = dwarf2_per_objfile->all_comp_units[i];
- struct partial_symtab *psymtab = per_cu->v.psymtab;
-
- /* CU of a shared file from 'dwz -m' may be unused by this main file.
- It may be referenced from a local scope but in such case it does
- not need to be present in .gdb_index. */
- if (psymtab == NULL)
- continue;
+ file_closer close_out_file (out_file);
+ gdb::unlinker unlink_file (filename.c_str ());
+
+ mapped_symtab symtab;
+ data_buf cu_list;
+ std::unordered_set<partial_symbol *> psyms_seen;
+
+ /* While we're scanning CU's create a table that maps a psymtab pointer
+ (which is what addrmap records) to its index (which is what is recorded
+ in the index file). This will later be needed to write the address
+ table. */
+ psym_index_map cu_index_htab;
+ cu_index_htab.reserve (dwarf2_per_objfile->n_comp_units);
+
+ /* The CU list is already sorted, so we don't need to do additional
+ work here. Also, the debug_types entries do not appear in
+ all_comp_units, but only in their own hash table. */
+ for (int i = 0; i < dwarf2_per_objfile->n_comp_units; ++i)
+ {
+ struct dwarf2_per_cu_data *per_cu
+ = dwarf2_per_objfile->all_comp_units[i];
+ struct partial_symtab *psymtab = per_cu->v.psymtab;
+
+ /* CU of a shared file from 'dwz -m' may be unused by this main file.
+ It may be referenced from a local scope but in such case it does not
+ need to be present in .gdb_index. */
+ if (psymtab == NULL)
+ continue;

- if (psymtab->user == NULL)
- recursively_write_psymbols (objfile, psymtab, &symtab, psyms_seen,
- i);
+ if (psymtab->user == NULL)
+ recursively_write_psymbols (objfile, psymtab, &symtab,
+ psyms_seen, i);

- const auto insertpair (cu_index_htab.emplace (psymtab, i));
- gdb_assert (insertpair.second);
+ const auto insertpair = cu_index_htab.emplace (psymtab, i);
+ gdb_assert (insertpair.second);

- store_unsigned_integer (cu_list.append_space (8), 8,
- BFD_ENDIAN_LITTLE,
- to_underlying (per_cu->sect_off));
- store_unsigned_integer (cu_list.append_space (8), 8,
- BFD_ENDIAN_LITTLE, per_cu->length);
- }
+ store_unsigned_integer (cu_list.append_space (8), 8,
+ BFD_ENDIAN_LITTLE,
+ to_underlying (per_cu->sect_off));
+ store_unsigned_integer (cu_list.append_space (8), 8,
+ BFD_ENDIAN_LITTLE, per_cu->length);
+ }

- /* Dump the address map. */
- DataBuf addr_vec;
- write_address_map (objfile, addr_vec, cu_index_htab);
+ /* Dump the address map. */
+ data_buf addr_vec;
+ write_address_map (objfile, addr_vec, cu_index_htab);

- /* Write out the .debug_type entries, if any. */
- DataBuf types_cu_list;
- if (dwarf2_per_objfile->signatured_types)
- {
- struct signatured_type_index_data sig_data (types_cu_list,
- psyms_seen);
-
- sig_data.objfile = objfile;
- sig_data.symtab = &symtab;
- sig_data.cu_index = dwarf2_per_objfile->n_comp_units;
- htab_traverse_noresize (dwarf2_per_objfile->signatured_types,
- write_one_signatured_type, &sig_data);
- }
+ /* Write out the .debug_type entries, if any. */
+ data_buf types_cu_list;
+ if (dwarf2_per_objfile->signatured_types)
+ {
+ signatured_type_index_data sig_data (types_cu_list,
+ psyms_seen);

- /* Now that we've processed all symbols we can shrink their cu_indices
- lists. */
- uniquify_cu_indices (&symtab);
+ sig_data.objfile = objfile;
+ sig_data.symtab = &symtab;
+ sig_data.cu_index = dwarf2_per_objfile->n_comp_units;
+ htab_traverse_noresize (dwarf2_per_objfile->signatured_types,
+ write_one_signatured_type, &sig_data);
+ }

- DataBuf symtab_vec, constant_pool;
- write_hash_table (&symtab, symtab_vec, constant_pool);
+ /* Now that we've processed all symbols we can shrink their cu_indices
+ lists. */
+ uniquify_cu_indices (&symtab);

- DataBuf contents;
- const offset_type size_of_contents (6 * sizeof (offset_type));
- offset_type total_len (size_of_contents);
+ data_buf symtab_vec, constant_pool;
+ write_hash_table (&symtab, symtab_vec, constant_pool);

- /* The version number. */
- contents.append_data (MAYBE_SWAP (8));
+ data_buf contents;
+ const offset_type size_of_contents = 6 * sizeof (offset_type);
+ offset_type total_len = size_of_contents;

- /* The offset of the CU list from the start of the file. */
- contents.append_data (MAYBE_SWAP (total_len));
- total_len += cu_list.size ();
+ /* The version number. */
+ contents.append_data (MAYBE_SWAP (8));

- /* The offset of the types CU list from the start of the file. */
- contents.append_data (MAYBE_SWAP (total_len));
- total_len += types_cu_list.size ();
+ /* The offset of the CU list from the start of the file. */
+ contents.append_data (MAYBE_SWAP (total_len));
+ total_len += cu_list.size ();

- /* The offset of the address table from the start of the file. */
- contents.append_data (MAYBE_SWAP (total_len));
- total_len += addr_vec.size ();
+ /* The offset of the types CU list from the start of the file. */
+ contents.append_data (MAYBE_SWAP (total_len));
+ total_len += types_cu_list.size ();

- /* The offset of the symbol table from the start of the file. */
- contents.append_data (MAYBE_SWAP (total_len));
- total_len += symtab_vec.size ();
+ /* The offset of the address table from the start of the file. */
+ contents.append_data (MAYBE_SWAP (total_len));
+ total_len += addr_vec.size ();

- /* The offset of the constant pool from the start of the file. */
- contents.append_data (MAYBE_SWAP (total_len));
- total_len += constant_pool.size ();
+ /* The offset of the symbol table from the start of the file. */
+ contents.append_data (MAYBE_SWAP (total_len));
+ total_len += symtab_vec.size ();

- gdb_assert (contents.size () == size_of_contents);
+ /* The offset of the constant pool from the start of the file. */
+ contents.append_data (MAYBE_SWAP (total_len));
+ total_len += constant_pool.size ();

- contents.file_write (out_file);
- cu_list.file_write (out_file);
- types_cu_list.file_write (out_file);
- addr_vec.file_write (out_file);
- symtab_vec.file_write (out_file);
- constant_pool.file_write (out_file);
- }
- CATCH (except, RETURN_MASK_ALL)
- {
- fclose (out_file);
- unlink (filename.c_str ());
- throw_exception (except);
- }
- END_CATCH
- fclose (out_file);
+ gdb_assert (contents.size () == size_of_contents);
+
+ contents.file_write (out_file);
+ cu_list.file_write (out_file);
+ types_cu_list.file_write (out_file);
+ addr_vec.file_write (out_file);
+ symtab_vec.file_write (out_file);
+ constant_pool.file_write (out_file);
+
+ /* We want to keep the file. */
+ unlink_file.keep ();
}

/* Implementation of the `save gdb-index' command.