From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6738 invoked by alias); 31 Jul 2006 22:23:34 -0000 Received: (qmail 6714 invoked by uid 22791); 31 Jul 2006 22:23:30 -0000 X-Spam-Check-By: sourceware.org Received: from sibelius.xs4all.nl (HELO sibelius.xs4all.nl) (82.92.89.47) by sourceware.org (qpsmtpd/0.31) with ESMTP; Mon, 31 Jul 2006 22:23:23 +0000 Received: from elgar.sibelius.xs4all.nl (root@elgar.sibelius.xs4all.nl [192.168.0.2]) by sibelius.xs4all.nl (8.13.4/8.13.4) with ESMTP id k6VMLra2011093; Tue, 1 Aug 2006 00:21:53 +0200 (CEST) Received: from elgar.sibelius.xs4all.nl (kettenis@localhost.sibelius.xs4all.nl [127.0.0.1]) by elgar.sibelius.xs4all.nl (8.13.6/8.13.6) with ESMTP id k6VMLrr4024954; Tue, 1 Aug 2006 00:21:53 +0200 (CEST) Received: (from kettenis@localhost) by elgar.sibelius.xs4all.nl (8.13.6/8.13.6/Submit) id k6VMLrDf019709; Tue, 1 Aug 2006 00:21:53 +0200 (CEST) Date: Mon, 31 Jul 2006 22:23:00 -0000 Message-Id: <200607312221.k6VMLrDf019709@elgar.sibelius.xs4all.nl> From: Mark Kettenis To: vladimir@codesourcery.com CC: eliz@gnu.org, gdb-patches@sources.redhat.com In-reply-to: <200607311730.57313.vladimir@codesourcery.com> (message from Vladimir Prus on Mon, 31 Jul 2006 17:30:56 +0400) Subject: Re: Flash support part 2: flash programming References: <200607201342.30712.vladimir@codesourcery.com> <200607311730.57313.vladimir@codesourcery.com> Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2006-07/txt/msg00442.txt.bz2 > From: Vladimir Prus > 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 > +#include > + > +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.