Re: [patch][gold] Add a tool for dumping incremental linking information

[Ian Lance Taylor <iant at google dot com>]

Rafael Espindola <espindola@google.com> writes:

> elfcpp/
> 2009-11-23  Rafael Avila de Espindola  <espindola@google.com>
>
> 	* elfcpp_file.h: Include elfcpp.h.
>
> gold/
> 2009-11-23  Rafael Avila de Espindola  <espindola@google.com>
>
> 	* Makefile.am: Build incremental-dump
> 	* Makefile.in: Regenerate.
> 	* incremental-dump.cc: New.


> +#include "elfcpp.h" //for Elf_sizes

No need for this comment, please remove it.


> +#include <stdio.h>
> +#include "gold.h"
> +#include "incremental.h"

You should include gold.h first.  Write this as

#include "gold.h"

#include <stdio.h>

#include "incremental.h"

> +// Header of the .gnu_incremental_input section.
> +struct Incremental_inputs_header_data
> +{
> +  // Incremental linker version.
> +  elfcpp::Elf_Word version;
> +
> +  // Numer of input files in the link.
> +  elfcpp::Elf_Word input_file_count;
> +
> +  // Offset of command line options in .gnu_incremental_strtab.
> +  elfcpp::Elf_Word command_line_offset;
> +
> +  // Padding.
> +  elfcpp::Elf_Word reserved;
> +};
> +
> +// Data stored in .gnu_incremental_input after the header for each of the
> +// Incremental_input_header_data::input_file_count input entries.
> +struct Incremental_inputs_entry_data
> +{
> +  // Offset of file name in .gnu_incremental_strtab section.
> +  elfcpp::Elf_Word filename_offset;
> +
> +  // Offset of data in .gnu_incremental_input.
> +  elfcpp::Elf_Word data_offset;
> +
> +  // Timestamp (in seconds).
> +  elfcpp::Elf_Xword timestamp_sec;
> +
> +  // Nano-second part of timestamp (if supported).
> +  elfcpp::Elf_Word timestamp_nsec;
> +
> +  // Type of the input entry.
> +  elfcpp::Elf_Half input_type;
> +
> +  // Padding.
> +  elfcpp::Elf_Half reserved;
> +};

Even though this tool may just be temporary, please move these
structures from incremental.cc to incremental.h, and don't redefine
them here.  Thanks.


> +int
> +main(int argc, char** argv)
> +{
> +  if (argc != 2)
> +    {
> +      fprintf(stderr, "usage: incremental-dump <file>\n");

It's easier to get these error messages right the first time than it
is to fix them up later.

fprintf(stderr, "Usage: %s <file>\n", argv[0]);

> +  bool t = file->open_for_modification();
> +  if (!t)
> +    {
> +      fprintf(stderr, "failed to open file %s\n", filename);
> +      return 1;
> +    }

fprintf(stderr, "%s: open_for_modification(%s): %s\n", argv[0],
        filename, strerror(errno));


> +  Incremental_binary* inc = open_incremental_binary(file);
> +
> +  if (inc == NULL)
> +    {
> +      fprintf(stderr, "Failed to open file %s\n", filename);
> +      return 1;
> +    }

fprintf(stderr, "%s; open_incremental_binary(%s): %s\n", argv[0],
        filename, strerror(errno));

> +  t = inc->find_incremental_inputs_section(&location, &strtab_shndx);
> +  if (!t)
> +    {
> +      fprintf(stderr, "File has no .gnu_incremental_inputs section\n");
> +      return 1;
> +    }

fprintf(stderr, "%s: %s: no .gnu_incremental_inputs section\n",
        argv[0], filename);

> +  if (incremental_header->version != 1)
> +    {
> +      fprintf(stderr, "unknown version %d\n", incremental_header->version);
> +      return 1;
> +    }

fprintf(stderr, "%s: %s: unknown incremental version %d\n",
        argv[0], filename, incremental_header->version);


> +  elfcpp::Elf_file<64, false, Incremental_binary> elf_file(inc);
> +
> +  if (elf_file.section_type(strtab_shndx) != elfcpp::SHT_STRTAB)
> +    {
> +      fprintf(stderr, "invalid string table section\n");
> +      return 1;
> +    }

fprintf(stderr, "%s: %s: invalid string table section %u (type %d != %d)\n",
        argv[0], filename, strtab_shndx, elf_file.section_type(strtab_shndx),
        elfcpp::SHT_STRTAB);


> +  if (!t)
> +    {
> +      fprintf(stderr, "Failed to get the link command line\n");
> +      return 1;
> +    }

fprintf(stderr, "%s: %s: failed to get link command line: %zu out of range\n",
        argv[0], filename,
        static_cast<size_t>(incremental_header->command_line_offset);


> +  printf("Link command line: %s\n", command_line);
> +
> +  printf("input files:\n");

I think consistent capitalization would be appropriate.


> +  for (unsigned i = 0; i < incremental_header->input_file_count; ++i)
> +    {
> +      const Incremental_inputs_entry_data* input = &incremental_inputs[i];
> +      const char *objname;
> +
> +      t = strtab.get_c_string(input->filename_offset, &objname);
> +      if (!t)
> +        {
> +          fprintf(stderr, "Failed to get the name of an object file\n");
> +          return 1;
> +        }

fprintf(stderr,
        "%s: %s: failed to get file name for object %u: %zu out of range\n",
        argv[0], filename, i, input->filename_offset);

> +      case INCREMENTAL_INPUT_INVALID:
> +      default:
> +        fprintf(stderr, "Unknown file type\n");
> +        return 1;

fprintf(stderr, "%s: invalid file type for object %u: %d\n",
        argv[0], i, input->input_type);


This patch is OK with changes along those lines.

Thanks.

Ian