From patchwork Tue Apr 4 19:06:51 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 19852 Received: (qmail 53211 invoked by alias); 4 Apr 2017 19:06:59 -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 53174 invoked by uid 89); 4 Apr 2017 19:06:56 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_HELO_PASS 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; Tue, 04 Apr 2017 19:06:54 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 37BA831F3E8; Tue, 4 Apr 2017 19:06:54 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 37BA831F3E8 Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 37BA831F3E8 Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.phx2.redhat.com [10.5.9.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9FEE5784AB; Tue, 4 Apr 2017 19:06:52 +0000 (UTC) Subject: Re: [PATCH 3/5] dwarf2read.c: Make dir_index and file_name_index strong typedefs To: Simon Marchi References: <1490754298-9455-1-git-send-email-palves@redhat.com> <1490754298-9455-4-git-send-email-palves@redhat.com> <879a6d0d-93a5-21db-aeb6-673bf3961c61@redhat.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <71ef42ed-30c5-45a7-6779-0a8ae3caa802@redhat.com> Date: Tue, 4 Apr 2017 20:06:51 +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: <879a6d0d-93a5-21db-aeb6-673bf3961c61@redhat.com> On 04/04/2017 07:54 PM, Pedro Alves wrote: > On 03/29/2017 04:35 PM, Simon Marchi wrote: >> On 2017-03-28 22:24, Pedro Alves wrote: >>> @@ -1169,7 +1179,7 @@ file_entry::include_dir (const line_header *lh) >>> const >>> { >>> /* lh->include_dirs is 0-based, but the directory index numbers in >>> the statement program are 1-based. */ >>> - return lh->include_dir_at (dir_index - 1); >>> + return lh->include_dir_at (to_underlying (d_index) - 1); >> >> Should include_dir_at's parameter be a dir_index? At this point, >> exposing a zero based index in the line_header interface is exposing its >> internal implementation. Same for file_name_at. > > You're right. I don't know what I was thinking... > > Thanks, here's what I'm squashing before pushing. And here's what I pushed. I'll reply to your other email a bit later. Thanks, Pedro Alves From ecfb656c37b982479d8eb07f240b434772d98fd6 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Tue, 4 Apr 2017 20:03:25 +0100 Subject: [PATCH] dwarf2read.c: Make dir_index and file_name_index strong typedefs This should help catch mistakes related to mixing the 1-based DWARF indexes with 0-based std::vector indexes, since the new types do not implicitly convert to anything. The change in read_formatted_entries relates to the fact that doing the seemingly simpler: - uintp = &fe.dir_index; + uintp = (unsigned int *) &fe.dir_index; would be undefined C/C++. So to address that, I made the function extract the form before assigning to the file_entry. It felt natural to use gdb::optional for "do I have this value", and this is what motivated the previous patch that added the missing observer methods to gdb::optional. gdb/ChangeLog: 2017-04-04 Pedro Alves * common/underlying.h: New file. * dwarf2read.c: Include "common/gdb_optional.h" and "common/underlying.h". (dir_index, file_name_index): New types. (file_entry): Use them. (file_entry::include): Use to_underlying. (line_header::add_file_name): Use dir_index. (read_formatted_entries): Use gdb::optional. Read form before writting to file_entry. (dwarf_decode_line_header): Use dir_index. (lnp_state_machine::current_file): Use to_underlying. (lnp_state_machine::file): Change type to file_name_index. (dwarf_record_line): Use to_underlying. (init_lnp_state_machine): Use file_name_index. (dwarf_decode_lines_1): Use dir_index and file_name_index. --- gdb/ChangeLog | 18 +++++ gdb/common/underlying.h | 32 +++++++++ gdb/dwarf2read.c | 173 +++++++++++++++++++++++++++--------------------- 3 files changed, 149 insertions(+), 74 deletions(-) create mode 100644 gdb/common/underlying.h diff --git a/gdb/ChangeLog b/gdb/ChangeLog index f2b64b9..de8ed99 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,5 +1,23 @@ 2017-04-04 Pedro Alves + * common/underlying.h: New file. + * dwarf2read.c: Include "common/gdb_optional.h" and + "common/underlying.h". + (dir_index, file_name_index): New types. + (file_entry): Use them. + (file_entry::include): Use to_underlying. + (line_header::add_file_name): Use dir_index. + (read_formatted_entries): Use gdb::optional. Read form before + writting to file_entry. + (dwarf_decode_line_header): Use dir_index. + (lnp_state_machine::current_file): Use to_underlying. + (lnp_state_machine::file): Change type to file_name_index. + (dwarf_record_line): Use to_underlying. + (init_lnp_state_machine): Use file_name_index. + (dwarf_decode_lines_1): Use dir_index and file_name_index. + +2017-04-04 Pedro Alves + * common/gdb_optional.h (gdb::optiona): Add operator->, operator*, operator bool, has_value and get methods. diff --git a/gdb/common/underlying.h b/gdb/common/underlying.h new file mode 100644 index 0000000..41ff117 --- /dev/null +++ b/gdb/common/underlying.h @@ -0,0 +1,32 @@ +/* Copyright (C) 2017 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . */ + +#ifndef COMMON_UNDERLYING_H +#define COMMON_UNDERLYING_H + +#include + +/* Convert an enum to its underlying value. */ + +template +constexpr typename std::underlying_type::type +to_underlying (E val) noexcept +{ + return static_cast::type> (val); +} + +#endif diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index d1fd187..f18c072 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -71,6 +71,8 @@ #include "namespace.h" #include "common/gdb_unlinker.h" #include "common/function-view.h" +#include "common/gdb_optional.h" +#include "common/underlying.h" #include #include @@ -1052,20 +1054,28 @@ typedef void (die_reader_func_ftype) (const struct die_reader_specs *reader, int has_children, void *data); +/* A 1-based directory index. This is a strong typedef to prevent + accidentally using a directory index as a 0-based index into an + array/vector. */ +enum class dir_index : unsigned int {}; + +/* Likewise, a 1-based file name index. */ +enum class file_name_index : unsigned int {}; + struct file_entry { file_entry () = default; - file_entry (const char *name_, unsigned int dir_index_, + file_entry (const char *name_, dir_index d_index_, unsigned int mod_time_, unsigned int length_) : name (name_), - dir_index (dir_index_), + d_index (d_index_), mod_time (mod_time_), length (length_) {} - /* Return the include directory at DIR_INDEX stored in LH. Returns - NULL if DIR_INDEX is out of bounds. */ + /* Return the include directory at D_INDEX stored in LH. Returns + NULL if D_INDEX is out of bounds. */ const char *include_dir (const line_header *lh) const; /* The file name. Note this is an observing pointer. The memory is @@ -1073,7 +1083,7 @@ struct file_entry const char *name {}; /* The directory index (1-based). */ - unsigned int dir_index {}; + dir_index d_index {}; unsigned int mod_time {}; @@ -1099,25 +1109,33 @@ struct line_header void add_include_dir (const char *include_dir); /* Add an entry to the file name table. */ - void add_file_name (const char *name, unsigned int dir_index, + void add_file_name (const char *name, dir_index d_index, unsigned int mod_time, unsigned int length); - /* Return the include dir at INDEX (0-based). Returns NULL if INDEX + /* Return the include dir at INDEX (1-based). Returns NULL if INDEX is out of bounds. */ - const char *include_dir_at (unsigned int index) const + const char *include_dir_at (dir_index index) const { - if (index >= include_dirs.size ()) + /* Convert directory index number (1-based) to vector index + (0-based). */ + size_t vec_index = to_underlying (index) - 1; + + if (vec_index >= include_dirs.size ()) return NULL; - return include_dirs[index]; + return include_dirs[vec_index]; } - /* Return the file name at INDEX (0-based). Returns NULL if INDEX + /* Return the file name at INDEX (1-based). Returns NULL if INDEX is out of bounds. */ - file_entry *file_name_at (unsigned int index) + file_entry *file_name_at (file_name_index index) { - if (index >= file_names.size ()) + /* Convert file name index number (1-based) to vector index + (0-based). */ + size_t vec_index = to_underlying (index) - 1; + + if (vec_index >= file_names.size ()) return NULL; - return &file_names[index]; + return &file_names[vec_index]; } /* Const version of the above. */ @@ -1167,9 +1185,7 @@ typedef std::unique_ptr line_header_up; const char * file_entry::include_dir (const line_header *lh) const { - /* lh->include_dirs is 0-based, but the directory index numbers in - the statement program are 1-based. */ - return lh->include_dir_at (dir_index - 1); + return lh->include_dir_at (d_index); } /* When we construct a partial symbol table entry we only @@ -17603,7 +17619,7 @@ line_header::add_include_dir (const char *include_dir) void line_header::add_file_name (const char *name, - unsigned int dir_index, + dir_index d_index, unsigned int mod_time, unsigned int length) { @@ -17611,7 +17627,7 @@ line_header::add_file_name (const char *name, fprintf_unfiltered (gdb_stdlog, "Adding file %u: %s\n", (unsigned) file_names.size () + 1, name); - file_names.emplace_back (name, dir_index, mod_time, length); + file_names.emplace_back (name, d_index, mod_time, length); } /* A convenience function to find the proper .debug_line section for a CU. */ @@ -17648,7 +17664,7 @@ read_formatted_entries (bfd *abfd, const gdb_byte **bufp, const struct comp_unit_head *cu_header, void (*callback) (struct line_header *lh, const char *name, - unsigned int dir_index, + dir_index d_index, unsigned int mod_time, unsigned int length)) { @@ -17679,71 +17695,51 @@ read_formatted_entries (bfd *abfd, const gdb_byte **bufp, for (formati = 0; formati < format_count; formati++) { - ULONGEST content_type, form; - const char *string_trash; - const char **stringp = &string_trash; - unsigned int uint_trash, *uintp = &uint_trash; - - content_type = read_unsigned_leb128 (abfd, format, &bytes_read); + ULONGEST content_type = read_unsigned_leb128 (abfd, format, &bytes_read); format += bytes_read; - switch (content_type) - { - case DW_LNCT_path: - stringp = &fe.name; - break; - case DW_LNCT_directory_index: - uintp = &fe.dir_index; - break; - case DW_LNCT_timestamp: - uintp = &fe.mod_time; - break; - case DW_LNCT_size: - uintp = &fe.length; - break; - case DW_LNCT_MD5: - break; - default: - complaint (&symfile_complaints, - _("Unknown format content type %s"), - pulongest (content_type)); - } - form = read_unsigned_leb128 (abfd, format, &bytes_read); + ULONGEST form = read_unsigned_leb128 (abfd, format, &bytes_read); format += bytes_read; + + gdb::optional string; + gdb::optional uint; + switch (form) { case DW_FORM_string: - *stringp = read_direct_string (abfd, buf, &bytes_read); + string.emplace (read_direct_string (abfd, buf, &bytes_read)); buf += bytes_read; break; case DW_FORM_line_strp: - *stringp = read_indirect_line_string (abfd, buf, cu_header, &bytes_read); + string.emplace (read_indirect_line_string (abfd, buf, + cu_header, + &bytes_read)); buf += bytes_read; break; case DW_FORM_data1: - *uintp = read_1_byte (abfd, buf); + uint.emplace (read_1_byte (abfd, buf)); buf += 1; break; case DW_FORM_data2: - *uintp = read_2_bytes (abfd, buf); + uint.emplace (read_2_bytes (abfd, buf)); buf += 2; break; case DW_FORM_data4: - *uintp = read_4_bytes (abfd, buf); + uint.emplace (read_4_bytes (abfd, buf)); buf += 4; break; case DW_FORM_data8: - *uintp = read_8_bytes (abfd, buf); + uint.emplace (read_8_bytes (abfd, buf)); buf += 8; break; case DW_FORM_udata: - *uintp = read_unsigned_leb128 (abfd, buf, &bytes_read); + uint.emplace (read_unsigned_leb128 (abfd, buf, &bytes_read)); buf += bytes_read; break; @@ -17752,9 +17748,35 @@ read_formatted_entries (bfd *abfd, const gdb_byte **bufp, current GDB. */ break; } + + switch (content_type) + { + case DW_LNCT_path: + if (string.has_value ()) + fe.name = *string; + break; + case DW_LNCT_directory_index: + if (uint.has_value ()) + fe.d_index = (dir_index) *uint; + break; + case DW_LNCT_timestamp: + if (uint.has_value ()) + fe.mod_time = *uint; + break; + case DW_LNCT_size: + if (uint.has_value ()) + fe.length = *uint; + break; + case DW_LNCT_MD5: + break; + default: + complaint (&symfile_complaints, + _("Unknown format content type %s"), + pulongest (content_type)); + } } - callback (lh, fe.name, fe.dir_index, fe.mod_time, fe.length); + callback (lh, fe.name, fe.d_index, fe.mod_time, fe.length); } *bufp = buf; @@ -17892,7 +17914,7 @@ dwarf_decode_line_header (unsigned int offset, struct dwarf2_cu *cu) /* Read directory table. */ read_formatted_entries (abfd, &line_ptr, lh.get (), &cu->header, [] (struct line_header *lh, const char *name, - unsigned int dir_index, unsigned int mod_time, + dir_index d_index, unsigned int mod_time, unsigned int length) { lh->add_include_dir (name); @@ -17901,10 +17923,10 @@ dwarf_decode_line_header (unsigned int offset, struct dwarf2_cu *cu) /* Read file name table. */ read_formatted_entries (abfd, &line_ptr, lh.get (), &cu->header, [] (struct line_header *lh, const char *name, - unsigned int dir_index, unsigned int mod_time, + dir_index d_index, unsigned int mod_time, unsigned int length) { - lh->add_file_name (name, dir_index, mod_time, length); + lh->add_file_name (name, d_index, mod_time, length); }); } else @@ -17920,17 +17942,18 @@ dwarf_decode_line_header (unsigned int offset, struct dwarf2_cu *cu) /* Read file name table. */ while ((cur_file = read_direct_string (abfd, line_ptr, &bytes_read)) != NULL) { - unsigned int dir_index, mod_time, length; + unsigned int mod_time, length; + dir_index d_index; line_ptr += bytes_read; - dir_index = read_unsigned_leb128 (abfd, line_ptr, &bytes_read); + d_index = (dir_index) read_unsigned_leb128 (abfd, line_ptr, &bytes_read); line_ptr += bytes_read; mod_time = read_unsigned_leb128 (abfd, line_ptr, &bytes_read); line_ptr += bytes_read; length = read_unsigned_leb128 (abfd, line_ptr, &bytes_read); line_ptr += bytes_read; - lh->add_file_name (cur_file, dir_index, mod_time, length); + lh->add_file_name (cur_file, d_index, mod_time, length); } line_ptr += bytes_read; } @@ -18036,7 +18059,7 @@ struct lnp_state_machine { /* lh->file_names is 0-based, but the file name numbers in the statement program are 1-based. */ - return the_line_header->file_name_at (file - 1); + return the_line_header->file_name_at (file); } /* The line number header. */ @@ -18046,7 +18069,7 @@ struct lnp_state_machine unsigned char op_index; /* The line table index (1-based) of the current file. */ - unsigned int file; + file_name_index file; unsigned int line; CORE_ADDR address; int is_stmt; @@ -18206,7 +18229,7 @@ dwarf_record_line (lnp_reader_state *reader, lnp_state_machine *state, fprintf_unfiltered (gdb_stdlog, "Processing actual line %u: file %u," " address %s, is_stmt %u, discrim %u\n", - line, state->file, + line, to_underlying (state->file), paddress (reader->gdbarch, state->address), is_stmt, discriminator); } @@ -18265,7 +18288,7 @@ init_lnp_state_machine (lnp_state_machine *state, /* Initialize these according to the DWARF spec. */ state->op_index = 0; - state->file = 1; + state->file = (file_name_index) 1; state->line = 1; /* Call `gdbarch_adjust_dwarf2_line' on the initial 0 address as if there was a line entry for it so that the backend has a chance to adjust it @@ -18423,12 +18446,13 @@ dwarf_decode_lines_1 (struct line_header *lh, struct dwarf2_cu *cu, case DW_LNE_define_file: { const char *cur_file; - unsigned int dir_index, mod_time, length; + unsigned int mod_time, length; + dir_index dindex; cur_file = read_direct_string (abfd, line_ptr, &bytes_read); line_ptr += bytes_read; - dir_index = + dindex = (dir_index) read_unsigned_leb128 (abfd, line_ptr, &bytes_read); line_ptr += bytes_read; mod_time = @@ -18437,7 +18461,7 @@ dwarf_decode_lines_1 (struct line_header *lh, struct dwarf2_cu *cu, length = read_unsigned_leb128 (abfd, line_ptr, &bytes_read); line_ptr += bytes_read; - lh->add_file_name (cur_file, dir_index, mod_time, length); + lh->add_file_name (cur_file, dindex, mod_time, length); } break; case DW_LNE_set_discriminator: @@ -18501,8 +18525,9 @@ dwarf_decode_lines_1 (struct line_header *lh, struct dwarf2_cu *cu, break; case DW_LNS_set_file: { - state_machine.file = read_unsigned_leb128 (abfd, line_ptr, - &bytes_read); + state_machine.file + = (file_name_index) read_unsigned_leb128 (abfd, line_ptr, + &bytes_read); line_ptr += bytes_read; const file_entry *fe = state_machine.current_file (); @@ -18866,11 +18891,11 @@ new_symbol_full (struct die_info *die, struct type *type, struct dwarf2_cu *cu, cu); if (attr) { - unsigned int file_index = DW_UNSND (attr); + file_name_index file_index = (file_name_index) DW_UNSND (attr); struct file_entry *fe; - if (cu->line_header != NULL && file_index > 0) - fe = cu->line_header->file_name_at (file_index - 1); + if (cu->line_header != NULL) + fe = cu->line_header->file_name_at (file_index); else fe = NULL;