This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [Patch, tentative, AVR] Displaying per-device memory usage info
- From: Andrew Burgess <andrew dot burgess at embecosm dot com>
- To: Senthil Kumar Selvaraj <senthil_kumar dot selvaraj at atmel dot com>
- Cc: binutils at sourceware dot org, nickc at redhat dot com, chertykov at gmail dot com, pitchumani dot s at atmel dot com
- Date: Mon, 24 Nov 2014 18:04:52 +0000
- Subject: Re: [Patch, tentative, AVR] Displaying per-device memory usage info
- Authentication-results: sourceware.org; auth=none
- References: <20141117131755 dot GA9898 at atmel dot com>
I can't give you commit approval. I spotted a few minor coding
standard issues while taking a look at this patch, comments inline
below.
Thanks,
Andrew
* Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com> [2014-11-17 18:47:56 +0530]:
> diff --git binutils/od-elf32_avr.c binutils/od-elf32_avr.c
> new file mode 100644
> index 0000000..f74d602
> --- /dev/null
> +++ binutils/od-elf32_avr.c
> @@ -0,0 +1,243 @@
> +/* od-avrelf.c -- dump information about an xcoff object file.
That header line seems wrong.
> +static char * elf32_avr_get_note_section_contents (bfd *abfd, bfd_size_type *size)
Should have newline after "static char *".
> +
> +static char* elf32_avr_get_note_desc (bfd *abfd, char *contents,
> + bfd_size_type size)
Should be "static char *" then newline.
> +static void
> +elf32_avr_get_device_info (bfd *abfd, char *description,
> + deviceinfo *device)
> +{
> + if (description == NULL)
> + return;
> +
> + const bfd_size_type memory_sizes = 6;
> +
> + memcpy (device, description, memory_sizes * sizeof(uint32_t));
Whitespace after sizeof.
> + device->name = NULL;
> +
> + uint32_t *stroffset_table = ((uint32_t *)description) + memory_sizes;
Should have whitespace after cast.
> + bfd_size_type stroffset_table_size = bfd_get_32 (abfd, stroffset_table);
> + char *str_table = ((char *)stroffset_table) + stroffset_table_size;
Whitespace after cast.
> +static void
> +elf32_avr_dump (bfd *abfd)
> +{
> + char *description = NULL;
> + bfd_size_type note_section_size = 0;
> +
> + deviceinfo device = {0};
> +
> + bfd_size_type data_usage = 0;
> + bfd_size_type text_usage = 0;
> + bfd_size_type eeprom_usage = 0;
> +
> + if (!options[OPT_MEMUSAGE].selected)
> + return;
In order to allow for future code growth it might be nice to move the
core of this function out, into elf32_avr_dump_mem_usage, then write:
if (options[OPT_MEMUSAGE].selected)
elf32_avr_dump_mem_usage (abfd);
this way if/when more options are added the code changes required are
reduced. What do you think?
> +
> + device.name = "Unknown";
> +
> + char *contents = elf32_avr_get_note_section_contents (abfd,
> + ¬e_section_size);
> +
> + if (contents != NULL)
> + {
> + description = elf32_avr_get_note_desc (abfd, contents, note_section_size);
> + elf32_avr_get_device_info (abfd, description, &device);
> + }
> +
> + elf32_avr_get_memory_usage (abfd, &text_usage, &data_usage,
> + &eeprom_usage);
> +
> + printf ("AVR Memory Usage\n"
> + "----------------\n"
> + "Device: %s\n\n", device.name);
> +
> + /* Text size */
> + printf ("Program:%8ld bytes", text_usage);
> + if (device.flash_size > 0)
> + printf (" (%2.1f%% Full)", ((float)text_usage / device.flash_size) * 100);
Whitespace after cast.
> +
> + printf ("\n(.text + .data + .bootloader)\n\n");
> +
> + /* Data size */
> + printf ("Data: %8ld bytes", data_usage);
> + if (device.ram_size > 0)
> + {
> + printf (" (%2.1f%% Full)", ((float)data_usage / device.ram_size) * 100);
Whitespace after cast.
> + }
> + printf ("\n(.data + .bss + .noinit)\n\n");
> +
> + /* EEPROM size */
> + if (device.eeprom_size > 0)
> + {
> + printf ("EEPROM: %8ld bytes", eeprom_usage);
> + printf (" (%2.1f%% Full)", ((float)eeprom_usage / device.eeprom_size) * 100);
Whitespace after cast.
> + printf ("\n(.eeprom)\n\n");
> + }
> +
> + if (contents != NULL)
> + free (contents);
> +}