This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: Flash support part 2: flash programming
> From: Vladimir Prus <vladimir@codesourcery.com>
> Date: Mon, 31 Jul 2006 17:30:56 +0400
>
> === gdb/target.c
> ==================================================================
> --- gdb/target.c (/mirrors/gdb) (revision 332)
> +++ gdb/target.c (/patches/flash_memory/gdb) (revision 332)
> @@ -1350,7 +1387,7 @@
> return target_xfer_partial (ops, object, annex, buf, NULL, offset, len);
> }
>
> -static LONGEST
> +LONGEST
> target_write_partial (struct target_ops *ops,
> enum target_object object,
> const char *annex, const gdb_byte *buf,
Uh, we just made that one static. Merge botch?
> === gdb/symfile.c
> ==================================================================
> --- gdb/symfile.c (/mirrors/gdb) (revision 332)
> +++ gdb/symfile.c (/patches/flash_memory/gdb) (revision 332)
> @@ -1512,15 +1512,6 @@
> overlay_cache_invalid = 1;
> }
>
> -/* This version of "load" should be usable for any target. Currently
> - it is just used for remote targets, not inftarg.c or core files,
> - on the theory that only in that case is it useful.
> -
> - Avoiding xmodem and the like seems like a win (a) because we don't have
> - to worry about finding it, and (b) On VMS, fork() is very slow and so
> - we don't want to run a subprocess. On the other hand, I'm not sure how
> - performance compares. */
> -
> static int download_write_size = 512;
> static void
> show_download_write_size (struct ui_file *file, int from_tty,
> @@ -1532,22 +1523,11 @@
> }
> static int validate_download = 0;
>
> -/* Callback service function for generic_load (bfd_map_over_sections). */
>
> -static void
> -add_section_size_callback (bfd *abfd, asection *asec, void *data)
> -{
> - bfd_size_type *sum = data;
> -
> - *sum += bfd_get_section_size (asec);
> -}
> -
> /* Opaque data for load_section_callback. */
> struct load_section_data {
> unsigned long load_offset;
> - unsigned long write_count;
> - unsigned long data_count;
> - bfd_size_type total_size;
> + VEC(memory_write_request) *memory_write_requests;
> };
>
> /* Callback service function for generic_load (bfd_map_over_sections). */
> @@ -1563,12 +1543,9 @@
> if (size > 0)
> {
> gdb_byte *buffer;
> - struct cleanup *old_chain;
> +
> CORE_ADDR lma = bfd_section_lma (abfd, asec) + args->load_offset;
> - bfd_size_type block_size;
> - int err;
> const char *sect_name = bfd_get_section_name (abfd, asec);
> - bfd_size_type sent;
>
> if (download_write_size > 0 && size > download_write_size)
> block_size = download_write_size;
> @@ -1576,8 +1553,6 @@
> block_size = size;
>
> buffer = xmalloc (size);
> - old_chain = make_cleanup (xfree, buffer);
> -
> /* Is this really necessary? I guess it gives the user something
> to look at during a long download. */
> ui_out_message (uiout, 0, "Loading section %s, size 0x%s lma 0x%s\n",
> @@ -1585,81 +1560,88 @@
>
> bfd_get_section_contents (abfd, asec, buffer, 0, size);
>
> - sent = 0;
> - do
> - {
> - int len;
> - bfd_size_type this_transfer = size - sent;
> + {
> + memory_write_request *n =
> + VEC_safe_push (memory_write_request,
> + args->memory_write_requests, 0);
> + n->begin = lma;
> + n->end = lma + size;
> + n->data = buffer;
> + n->name = strdup (sect_name);
> + }
> + }
> + }
> +}
>
> - if (this_transfer >= block_size)
> - this_transfer = block_size;
> - len = target_write_memory_partial (lma, buffer,
> - this_transfer, &err);
> - if (err)
> - break;
> - if (validate_download)
> - {
> - /* Broken memories and broken monitors manifest
> - themselves here when bring new computers to
> - life. This doubles already slow downloads. */
> - /* NOTE: cagney/1999-10-18: A more efficient
> - implementation might add a verify_memory()
> - method to the target vector and then use
> - that. remote.c could implement that method
> - using the ``qCRC'' packet. */
> - gdb_byte *check = xmalloc (len);
> - struct cleanup *verify_cleanups =
> - make_cleanup (xfree, check);
> +static int
> +allow_flash_write (void)
> +{
> + return 1;
> +}
>
> - if (target_read_memory (lma, check, len) != 0)
> - error (_("Download verify read failed at 0x%s"),
> - paddr (lma));
> - if (memcmp (buffer, check, len) != 0)
> - error (_("Download verify compare failed at 0x%s"),
> - paddr (lma));
> - do_cleanups (verify_cleanups);
> - }
> - args->data_count += len;
> - lma += len;
> - buffer += len;
> - args->write_count += 1;
> - sent += len;
> - if (quit_flag
> - || (deprecated_ui_load_progress_hook != NULL
> - && deprecated_ui_load_progress_hook (sect_name, sent)))
> - error (_("Canceled the download"));
>
> - if (deprecated_show_load_progress != NULL)
> - deprecated_show_load_progress (sect_name, sent, size,
> - args->data_count,
> - args->total_size);
> - }
> - while (sent < size);
> +static enum flash_preserve_mode
> +dont_preserve_flash (void)
> +{
> + return flash_dont_preserve;
> +}
>
> - if (err != 0)
> - error (_("Memory access error while loading section %s."), sect_name);
> +/* Casts 'p' to vector of memory_write_request object and
> + frees that DATA field in all such objects. */
> +static void
> +clear_memory_write_data (void *p)
> +{
> + VEC(memory_write_request) *vec = p;
> + int i;
> + memory_write_request *mr;
> + for (i = 0; VEC_iterate (memory_write_request, vec, i, mr); ++i)
> + {
> + xfree (mr->data);
> + xfree (mr->name);
> + }
> +}
>
> - do_cleanups (old_chain);
> - }
> +static int
> +download_progress_cb (const char* name,
> + ULONGEST sent, ULONGEST size,
> + ULONGEST total_sent, ULONGEST total_size)
> +{
> + int ret = 0;
> + if (deprecated_ui_load_progress_hook != NULL
> + && deprecated_ui_load_progress_hook (name, sent))
> + {
> + /* Stop download. */
> + return 1;
> }
> +
> + if (deprecated_show_load_progress != NULL)
> + deprecated_show_load_progress (name, sent, size,
> + total_sent, total_size);
> +
> + return 0;
> }
>
> -void
> -generic_load (char *args, int from_tty)
> +
> +/* This version of "load" should be usable for any target. Currently
> + it is just used for remote targets, not inftarg.c or core files,
> + on the theory that only in that case is it useful. */
> +void generic_load (char *args, int from_tty)
> {
> asection *s;
> bfd *loadfile_bfd;
> - struct timeval start_time, end_time;
> char *filename;
> struct cleanup *old_cleanups = make_cleanup (null_cleanup, 0);
> struct load_section_data cbdata;
> CORE_ADDR entry;
> char **argv;
> + unsigned long total_count = 0;
> + memory_write_request *mr;
> + int i;
> + struct timeval start_time, end_time;
> +
>
> cbdata.load_offset = 0; /* Offset to add to vma for each section. */
> - cbdata.write_count = 0; /* Number of writes needed. */
> - cbdata.data_count = 0; /* Number of bytes written to target memory. */
> - cbdata.total_size = 0; /* Total size of all bfd sectors. */
> + cbdata.memory_write_requests = VEC_alloc (memory_write_request, 1);
>
> argv = buildargv (args);
>
> @@ -1705,34 +1687,52 @@
> bfd_errmsg (bfd_get_error ()));
> }
>
> - bfd_map_over_sections (loadfile_bfd, add_section_size_callback,
> - (void *) &cbdata.total_size);
> -
> gettimeofday (&start_time, NULL);
>
> bfd_map_over_sections (loadfile_bfd, load_section_callback, &cbdata);
>
> + for (i = 0;
> + VEC_iterate (memory_write_request, cbdata.memory_write_requests, i, mr);
> + ++i)
> + {
> + total_count += (mr->end - mr->begin);
> + }
> +
> + /* Note: cleanups are run in reverse order, so first register code
> + for cleaning the array, and then for cleaning the pointers contained
> + in array. */
> + make_cleanup ((void (*)(void*))VEC_OP(memory_write_request, free),
> + &cbdata.memory_write_requests);
> + make_cleanup (clear_memory_write_data, cbdata.memory_write_requests);
> +
> +
> + if (target_write_memory_blocks
> + (cbdata.memory_write_requests,
> + &allow_flash_write, &dont_preserve_flash, &download_progress_cb) != 0)
> + {
> + error (_("Failed to write memory"));
> + }
> +
> gettimeofday (&end_time, NULL);
>
> + print_transfer_performance (gdb_stdout, total_count,
> + 0 /* Write count not known. */,
> + &start_time, &end_time);
> +
> +
> entry = bfd_get_start_address (loadfile_bfd);
> +
> +
> ui_out_text (uiout, "Start address ");
> ui_out_field_fmt (uiout, "address", "0x%s", paddr_nz (entry));
> ui_out_text (uiout, ", load size ");
> - ui_out_field_fmt (uiout, "load-size", "%lu", cbdata.data_count);
> + ui_out_field_fmt (uiout, "load-size", "%lu", total_count);
> ui_out_text (uiout, "\n");
> +
> /* We were doing this in remote-mips.c, I suspect it is right
> for other targets too. */
> write_pc (entry);
>
> - /* FIXME: are we supposed to call symbol_file_add or not? According
> - to a comment from remote-mips.c (where a call to symbol_file_add
> - was commented out), making the call confuses GDB if more than one
> - file is loaded in. Some targets do (e.g., remote-vx.c) but
> - others don't (or didn't - perhaps they have all been deleted). */
> -
> - print_transfer_performance (gdb_stdout, cbdata.data_count,
> - cbdata.write_count, &start_time, &end_time);
> -
> do_cleanups (old_cleanups);
> }
This really confuses me. It looks like this completely rewrites the
"load" functionality, putting bfd completely out of the picture. Why?
How does this change a load into non-flash memory?
> === gdb/target-memory.c
> ==================================================================
> --- gdb/target-memory.c (/mirrors/gdb) (revision 332)
> +++ gdb/target-memory.c (/patches/flash_memory/gdb) (revision 332)
> @@ -0,0 +1,558 @@
> +/* Parts of target interface that deal with accessing memory and memory-like
> + objects.
> +
> + Copyright (C) 2006
> + 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 2 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, write to the Free Software
> + Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + Boston, MA 02110-1301, USA. */
> +
> +#include "defs.h"
> +#include "config.h"
> +#include "vec.h"
> +#include "target.h"
> +#include "gdb_assert.h"
> +#include "memory-map.h"
> +
> +#include <stdio.h>
> +#include <sys/time.h>
> +
> +static int
> +compare_block_starting_address (const void* a, const void *b)
> +{
> + ULONGEST a_begin = ((memory_write_request*)a)->begin;
> + ULONGEST b_begin = ((memory_write_request*)b)->begin;
> + return a_begin - b_begin;
> +}
> +
> +/* Adds to RESULT all memory write requests from BLOCK that are
> + in (BEGIN, END) range.
> +
> + If any memory request is only partially in the specified range,
> + a part of memory request will be added. */
> +static void
> +claim_memory (VEC(memory_write_request) *blocks,
> + VEC(memory_write_request) **result,
> + ULONGEST begin,
> + ULONGEST end)
> +{
> + int i;
> + ULONGEST claimed_begin;
> + ULONGEST claimed_end;
> + memory_write_request *r;
> +
> + for (i = 0; VEC_iterate (memory_write_request, blocks, i, r); ++i)
> + {
> + if (begin >= r->end || end <= r->begin)
> + continue;
> +
> + claimed_begin = max (begin, r->begin);
> + claimed_end = min (end, r->end);
> +
> + if (claimed_begin == r->begin && claimed_end == r->end)
> + VEC_safe_push (memory_write_request, *result, r);
> + else
> + {
> + memory_write_request *n =
> + VEC_safe_push (memory_write_request, *result, 0);
> + n->begin = claimed_begin;
> + n->end = claimed_end;
> + n->data = r->data + (claimed_begin - r->begin);
> + }
> + }
> +}
> +
> +/* Given an array of memory_write_request objects in BLOCKS,
> + add memory requests for flash memory into FLASH_BLOCKS, and for
> + regular memory to REGULAR_BLOCKS. */
> +static void
> +split_regular_and_flash_blocks (VEC(memory_write_request) *blocks,
> + VEC(memory_write_request) **regular_blocks,
> + VEC(memory_write_request) **flash_blocks)
> +{
> + VEC(memory_region) *regions = target_memory_map ();
> + int i;
> + memory_region *cur;
> +
> + /* This implementation runs in O(length(regions)*length(blocks)) time.
> + However, in most cases the number of blocks will be small, so this does
> + not matter.
> +
> + Note also that it's extremely unlikely that a memory write request
> + will span more than one memory region, however for safety we handle
> + such situations. */
> + if (VEC_empty (memory_region, regions))
> + {
> + int i;
> + memory_write_request* ptr;
> + for (i = 0; i < VEC_iterate (memory_write_request, blocks, i, ptr); ++i)
> + VEC_safe_push (memory_write_request, *regular_blocks, ptr);
> + return;
> + }
> +
> + if (VEC_index (memory_region, regions, 0)->begin > 0)
> + {
> + /* Nothing is said about (0, regions.begin) region. Assume regular. */
> + claim_memory (blocks, regular_blocks, 0,
> + VEC_index (memory_region, regions, 0)->begin);
> + }
> +
> + for (i = 0; VEC_iterate (memory_region, regions, i, cur); ++i)
> + {
> + VEC(memory_write_request) **r =
> + (cur->memory_type == TARGET_MEMORY_FLASH) ?
> + flash_blocks : regular_blocks;
> +
> + claim_memory (blocks, r, cur->begin, cur->end);
> +
> + if (i < VEC_length (memory_region, regions) - 1)
> + {
> + memory_region *next = VEC_index (memory_region, regions, i+1);
> + if (cur->end < next->begin)
> + {
> + claim_memory (blocks, regular_blocks,
> + cur->end, next->begin);
> + }
> + }
> + else
> + {
> + claim_memory (blocks, regular_blocks, cur->end, ULONGEST_MAX);
> + }
> + }
> +}
> +
> +/* Given 'address', return begin and end addresses for the flash segments
> + that address is in. */
> +static void
> +segment_boundaries (unsigned address, unsigned *begin, unsigned *end)
> +{
> + VEC(memory_region) *regions = target_memory_map ();
> + unsigned i;
> + memory_region *r;
> +
> + for(i = 0; VEC_iterate (memory_region, regions, i, r); ++i)
> + {
> + if (r->begin <= address && address < r->end)
> + {
> + unsigned block_size = r->flash_block_size;
> +
> + if (begin)
> + *begin = address/block_size*block_size;
> + if (end)
> + *end = (address + block_size - 1)/block_size*block_size;
> + return;
> + }
> + }
> + if (begin)
> + *begin = (unsigned)-1;
> + if (end)
> + *end = (unsigned)-1;
> +}
> +
> +/* Returns the list of flash blocks that must be erased. */
> +static VEC(memory_write_request) *
> +blocks_to_erase (VEC(memory_write_request)* written)
> +{
> + unsigned page_size = 1024;
> + unsigned i;
> + memory_write_request* ptr;
> +
> + VEC(memory_write_request) *result = NULL;
> +
> + for(i = 0; VEC_iterate (memory_write_request, written, i, ptr); ++i)
> + {
> + unsigned begin;
> + segment_boundaries (ptr->begin, &begin, 0);
> + unsigned end;
> + segment_boundaries (ptr->end, 0, &end);
> +
> + if (!VEC_empty(memory_write_request, result)
> + && VEC_last (memory_write_request, result)->end >= begin)
> + {
> + VEC_last (memory_write_request, result)->end = end;
> + }
> + else
> + {
> + memory_write_request* n =
> + VEC_safe_push (memory_write_request, result, 0);
> + n->begin = begin;
> + n->end = end;
> + }
> + }
> +
> + return result;
> +}
> +
> +
> +/* Given a list of blocks that will be erased with flash erase commands,
> + and the list of blocks that will be written, computed the set
> + of regions that will have its content erased and not written. */
> +static VEC(memory_write_request) *
> +compute_garbled_blocks(VEC(memory_write_request)* erased_blocks,
> + VEC(memory_write_request)* written_blocks)
Missing sace before the first '('.
> +{
> + VEC(memory_write_request) *result = NULL;
> +
> + unsigned i, j;
> + unsigned je = VEC_length (memory_write_request, written_blocks);
> + memory_write_request *erased_p;
> +
> + /* Look at each erased memory_write_request in turn, and see if what part of it is
> + subsequently written to. */
That line is defenitely too long.
> +
> + out:
> + do_cleanups (back_to);
> +
> + return err;
> +}
> +
> +
Please don't add extra whitespace at the end of a file.