[RFA] Change target_write_memory_blocks to use std::vector
Commit Message
On 2018-02-25 12:37 PM, Tom Tromey wrote:
> - CORE_ADDR load_offset;
> - struct load_progress_data *progress_data;
> - VEC(memory_write_request_s) *requests;
> + CORE_ADDR load_offset = 0;
> + struct load_progress_data *progress_data = nullptr;
> + std::vector<struct memory_write_request> requests;
> +
> + load_section_data ()
> + {
> + }
Is this empty constructor needed?
Actually, I think it would be nice to give constructors to the data
structures when possible, to make it less likely to have them in
invalid states.
Here's an example, you can integrate it in your patch if you like it.
Simon
From 59f9a9d763dc4c0a879f1d36696efa2c4d423c86 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Sun, 25 Feb 2018 17:10:18 -0500
Subject: [PATCH] Add constructors to structures
---
gdb/symfile.c | 99 +++++++++++++++++++++++++----------------------------
gdb/target-memory.c | 17 ++-------
gdb/target.h | 25 ++++++++------
3 files changed, 63 insertions(+), 78 deletions(-)
Comments
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
>> + load_section_data ()
>> + {
>> + }
Simon> Is this empty constructor needed?
I only added it for clarity. Is there some standard approach to this?
Or a gdb standard?
Simon> Actually, I think it would be nice to give constructors to the data
Simon> structures when possible, to make it less likely to have them in
Simon> invalid states.
Simon> Here's an example, you can integrate it in your patch if you like it.
Yeah, this seems better to me.
I will pull it in.
Tom
On 2018-02-26 14:37, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
>
>>> + load_section_data ()
>>> + {
>>> + }
>
> Simon> Is this empty constructor needed?
>
> I only added it for clarity. Is there some standard approach to this?
> Or a gdb standard?
Of all the C++ patches you did, I can't remember one where you added an
empty constructor :). I think the de-facto standard in C++ is to let
the compiler generate a default constructor instead. If we wanted to be
explicit, we should rather do
load_section_data () = default;
but I think it's just fine to not declare anything.
> Simon> Actually, I think it would be nice to give constructors to the
> data
> Simon> structures when possible, to make it less likely to have them in
> Simon> invalid states.
>
> Simon> Here's an example, you can integrate it in your patch if you
> like it.
>
> Yeah, this seems better to me.
> I will pull it in.
Ok, well at least it invalidates the point above :)
Simon
@@ -1892,46 +1892,56 @@ add_section_size_callback (bfd *abfd, asection *asec, void *data)
*sum += bfd_get_section_size (asec);
}
-/* Opaque data for load_section_callback. */
-struct load_section_data {
- CORE_ADDR load_offset = 0;
- struct load_progress_data *progress_data = nullptr;
- std::vector<struct memory_write_request> requests;
-
- load_section_data ()
- {
- }
-
- ~load_section_data ()
- {
- for (auto &&request : requests)
- {
- xfree (request.data);
- xfree (request.baton);
- }
- }
-};
-
/* Opaque data for load_progress. */
-struct load_progress_data {
+struct load_progress_data
+{
/* Cumulative data. */
- unsigned long write_count;
- unsigned long data_count;
- bfd_size_type total_size;
+ unsigned long write_count = 0;
+ unsigned long data_count = 0;
+ bfd_size_type total_size = 0;
};
/* Opaque data for load_progress for a single section. */
-struct load_progress_section_data {
+struct load_progress_section_data
+{
+ load_progress_section_data (load_progress_data *cumulative_,
+ const char *section_name_, ULONGEST section_size_,
+ CORE_ADDR lma_, gdb_byte *buffer_)
+ : cumulative (cumulative_), section_name (section_name_),
+ section_size (section_size_), lma (lma_), buffer (buffer_)
+ {}
+
struct load_progress_data *cumulative;
/* Per-section data. */
const char *section_name;
- ULONGEST section_sent;
+ ULONGEST section_sent = 0;
ULONGEST section_size;
CORE_ADDR lma;
gdb_byte *buffer;
};
+/* Opaque data for load_section_callback. */
+struct load_section_data
+{
+ load_section_data (load_progress_data *progress_data_)
+ : progress_data (progress_data_)
+ {}
+
+ ~load_section_data ()
+ {
+ for (auto &&request : requests)
+ {
+ xfree (request.data);
+ delete ((load_progress_section_data *) request.baton);
+ }
+ }
+
+ CORE_ADDR load_offset = 0;
+ struct load_progress_data *progress_data;
+ std::vector<struct memory_write_request> requests;
+};
+
/* Target write callback routine for progress reporting. */
static void
@@ -2001,11 +2011,8 @@ load_progress (ULONGEST bytes, void *untyped_arg)
static void
load_section_callback (bfd *abfd, asection *asec, void *data)
{
- struct memory_write_request new_request;
struct load_section_data *args = (struct load_section_data *) data;
- struct load_progress_section_data *section_data;
bfd_size_type size = bfd_get_section_size (asec);
- gdb_byte *buffer;
const char *sect_name = bfd_get_section_name (abfd, asec);
if ((bfd_get_section_flags (abfd, asec) & SEC_LOAD) == 0)
@@ -2014,25 +2021,16 @@ load_section_callback (bfd *abfd, asection *asec, void *data)
if (size == 0)
return;
- memset (&new_request, 0, sizeof (struct memory_write_request));
- section_data = XCNEW (struct load_progress_section_data);
- new_request.begin = bfd_section_lma (abfd, asec) + args->load_offset;
- new_request.end = new_request.begin + size; /* FIXME Should size
- be in instead? */
- new_request.data = (gdb_byte *) xmalloc (size);
- new_request.baton = section_data;
-
- buffer = new_request.data;
-
- section_data->cumulative = args->progress_data;
- section_data->section_name = sect_name;
- section_data->section_size = size;
- section_data->lma = new_request.begin;
- section_data->buffer = buffer;
+ ULONGEST begin = bfd_section_lma (abfd, asec) + args->load_offset;
+ ULONGEST end = begin + size;
+ gdb_byte *buffer = (gdb_byte *) xmalloc (size);
+ bfd_get_section_contents (abfd, asec, buffer, 0, size);
- args->requests.push_back (new_request);
+ load_progress_section_data *section_data
+ = new load_progress_section_data (args->progress_data, sect_name, size,
+ begin, buffer);
- bfd_get_section_contents (abfd, asec, buffer, 0, size);
+ args->requests.emplace_back (begin, end, buffer, section_data);
}
static void print_transfer_performance (struct ui_file *stream,
@@ -2043,15 +2041,10 @@ static void print_transfer_performance (struct ui_file *stream,
void
generic_load (const char *args, int from_tty)
{
- struct load_section_data cbdata;
struct load_progress_data total_progress;
+ struct load_section_data cbdata (&total_progress);
struct ui_out *uiout = current_uiout;
- CORE_ADDR entry;
-
- memset (&total_progress, 0, sizeof (total_progress));
- cbdata.progress_data = &total_progress;
-
if (args == NULL)
error_no_arg (_("file to load"));
@@ -2100,7 +2093,7 @@ generic_load (const char *args, int from_tty)
steady_clock::time_point end_time = steady_clock::now ();
- entry = bfd_get_start_address (loadfile_bfd.get ());
+ CORE_ADDR entry = bfd_get_start_address (loadfile_bfd.get ());
entry = gdbarch_addr_bits_remove (target_gdbarch (), entry);
uiout->text ("Start address ");
uiout->field_fmt ("address", "%s", paddress (target_gdbarch (), entry));
@@ -160,15 +160,7 @@ blocks_to_erase (const std::vector<memory_write_request> &written)
if (!result.empty () && result.back ().end >= begin)
result.back ().end = end;
else
- {
- memory_write_request n;
-
- memset (&n, 0, sizeof (struct memory_write_request));
- n.begin = begin;
- n.end = end;
-
- result.push_back (n);
- }
+ result.emplace_back (begin, end);
}
return result;
@@ -239,12 +231,7 @@ compute_garbled_blocks (const std::vector<memory_write_request> &erased_blocks,
again for the remainder. */
if (written->begin > erased.begin)
{
- struct memory_write_request n;
-
- memset (&n, 0, sizeof (struct memory_write_request));
- n.begin = erased.begin;
- n.end = written->begin;
- result.push_back (n);
+ result.emplace_back (erased.begin, written->begin);
erased.begin = written->begin;
continue;
}
@@ -1463,16 +1463,21 @@ void target_flash_done (void);
/* Describes a request for a memory write operation. */
struct memory_write_request
- {
- /* Begining address that must be written. */
- ULONGEST begin;
- /* Past-the-end address. */
- ULONGEST end;
- /* The data to write. */
- gdb_byte *data;
- /* A callback baton for progress reporting for this request. */
- void *baton;
- };
+{
+ memory_write_request (ULONGEST begin_, ULONGEST end_,
+ gdb_byte *data_ = nullptr, void *baton_ = nullptr)
+ : begin (begin_), end (end_), data (data_), baton (baton_)
+ {}
+
+ /* Begining address that must be written. */
+ ULONGEST begin;
+ /* Past-the-end address. */
+ ULONGEST end;
+ /* The data to write. */
+ gdb_byte *data;
+ /* A callback baton for progress reporting for this request. */
+ void *baton;
+};
/* Enumeration specifying different flash preservation behaviour. */
enum flash_preserve_mode