From patchwork Mon Jun 12 16:18:49 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 20960 Received: (qmail 120785 invoked by alias); 12 Jun 2017 16:18:52 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 120773 invoked by uid 89); 12 Jun 2017 16:18:51 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS, T_RP_MATCHES_RCVD, UNSUBSCRIBE_BODY autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 12 Jun 2017 16:18:48 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D143DC057FA5; Mon, 12 Jun 2017 16:18:51 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com D143DC057FA5 Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com D143DC057FA5 Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id B400B4FA21; Mon, 12 Jun 2017 16:18:50 +0000 (UTC) Subject: Re: [pushed] Re: [PATCH 1/6] Code cleanup: C++ify .gdb_index producer. To: Jan Kratochvil , gdb-patches@sourceware.org References: <149582312757.15869.18345460438195439402.stgit@host1.jankratochvil.net> <149582313928.15869.12647134810146005233.stgit@host1.jankratochvil.net> <8efc0742-1014-4fe0-6948-f40a9c5c4975@redhat.com> Cc: Victor Leschuk From: Pedro Alves Message-ID: <192eac60-b233-cbe6-a613-8e01b20e6630@redhat.com> Date: Mon, 12 Jun 2017 17:18:49 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <8efc0742-1014-4fe0-6948-f40a9c5c4975@redhat.com> 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 - struct _MakeUniq - { typedef unique_ptr<_Tp> __single_object; }; - template - 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) /* 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 static void +template +static void file_write (FILE *file, const std::vector &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 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 void - append_data (const T &data) + /* Copy DATA to the end of the buffer. */ + template + void append_data (const T &data) { std::copy (reinterpret_cast (&data), reinterpret_cast (&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 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> data; mapped_symtab () { data.resize (1024); } + + offset_type n_elements = 0; + std::vector> 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 &slot (find_slot (symtab, name)); - if (!slot) + std::unique_ptr &slot = find_slot (symtab, name); + if (slot == NULL) { - slot = std::make_unique (); + 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 VectorHasher +/* A std::unordered_map::hasher for std::vector<>. */ +template +class vector_hasher { public: - size_t - operator () (const std::vector &key) const + size_t operator () (const std::vector &key) const { - // return boost::hash_value> (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, offset_type, - VectorHasher> symbol_hash_table; + vector_hasher> + symbol_hash_table; /* We add all the index vectors to the constant pool first, to ensure alignment is ok. */ - for (const std::unique_ptr &it:symtab->data) + for (const std::unique_ptr &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 str_table; - for (const auto &it:symtab->data) + std::unordered_map 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 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 &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 &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 &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 &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 &psyms_seen; int cu_index; - - signatured_type_index_data (DataBuf &types_list_, - std::unordered_set &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 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 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 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.