gold incremental file format on 64-bit

gold incremental file format on 64-bit

[David Miller]

One naggling issue with a native 64-bit sparc GOLD is that the
incremental linking file format leads to many unaligned accesses which
SIGBUS on sparc.

Actually, much care has already been taken in the file format to put
potentially 64-bit values into 8-byte sized and offsetted locations.

But the individual sub-sections of the incremental linking file can
end up not being 8-byte aligned.

I intend to fix this by padding up each sub-section by enough to
8-byte align each one, when necessary, to fix this problem.

What is the state of the incremental file format version?  I need
to increment it when I make this layout change, right?  Or are we
still at a point where we can just adjust things with impunity and
without a version bump?

Also, it would have been nice for the gdb-index file format to not
have the unaligned cases I ran into on 64-bit sparc the other week and
fixed by using Swap_unaligned.  How much flexibility exists to
rearrange the gdb-index file layout?

Thanks.

Re: gold incremental file format on 64-bit

[Cary Coutant]

> One naggling issue with a native 64-bit sparc GOLD is that the
> incremental linking file format leads to many unaligned accesses which
> SIGBUS on sparc.
>
> Actually, much care has already been taken in the file format to put
> potentially 64-bit values into 8-byte sized and offsetted locations.
>
> But the individual sub-sections of the incremental linking file can
> end up not being 8-byte aligned.
>
> I intend to fix this by padding up each sub-section by enough to
> 8-byte align each one, when necessary, to fix this problem.
>
> What is the state of the incremental file format version?  I need
> to increment it when I make this layout change, right?  Or are we
> still at a point where we can just adjust things with impunity and
> without a version bump?

You should be able to align the supplemental info blocks without an
incompatible format change, since we find those via offsets stored in
the input file entries. All you'd need to do to preserve 8-byte
alignment is add 4 bytes of zeroes to each INCREMENTAL_INPUT_SCRIPT,
INCREMENTAL_INPUT_OBJECT, and INCREMENTAL_INPUT_ARCHIVE_MEMBER
supplemental info block.

Is there anything else that's unaligned?

> Also, it would have been nice for the gdb-index file format to not
> have the unaligned cases I ran into on 64-bit sparc the other week and
> fixed by using Swap_unaligned.  How much flexibility exists to
> rearrange the gdb-index file layout?

The gdb index format is pretty experimental at the moment, so there is
flexibility, but I'd guess that it's probably going to get replaced
with something a bit more compact. The DWARF committee is about to
look at a similar index that Apple is using, so we'll probably end up
standardizing on something different from .gdb_index eventually. Thus,
it may not be worth bothering with it until then -- you'll have to
change binutils, gdb, and gold if you change the format. The
gdb-discuss mailing list would be the place to discuss changes to that
format.

That said, the only unaligned pieces are the 64-bit values in the
address area, right? The only way to align that would be to extend the
third field from 32 bits to 64 bits. I think those pieces should
always be 4-byte aligned as is, though, so maybe it would be better to
have a version of Swap_unaligned that can handle 8-byte reads and
writes to 4-byte-aligned addresses. That should be considerably faster
than the pure unaligned version you're using now.

-cary

Re: gold incremental file format on 64-bit

[David Miller]

From: Cary Coutant <ccoutant@google.com>
Date: Fri, 20 Apr 2012 15:52:21 -0700

> Is there anything else that's unaligned?

Yes, some of the other sections can end up with an unaligned ending if
there is an odd number of entries because the individual objects are a
multiple of 4 bytes in size, but not a multiple of 8.

> That said, the only unaligned pieces are the 64-bit values in the
> address area, right? The only way to align that would be to extend the
> third field from 32 bits to 64 bits. I think those pieces should
> always be 4-byte aligned as is, though, so maybe it would be better to
> have a version of Swap_unaligned that can handle 8-byte reads and
> writes to 4-byte-aligned addresses. That should be considerably faster
> than the pure unaligned version you're using now.

Makes sense.

Re: gold incremental file format on 64-bit

[Cary Coutant]

>> Is there anything else that's unaligned?
>
> Yes, some of the other sections can end up with an unaligned ending if
> there is an odd number of entries because the individual objects are a
> multiple of 4 bytes in size, but not a multiple of 8.

As far as I can tell, besides .gnu_incremental_inputs, the only other
section that has any address-sized items in it is
.gnu_incremental_relocs. That would probably be a bug in
Incremental_inputs::create_data_sections():

  this->relocs_section_ = new Output_data_space(4, "** incremental_relocs");

That "4" should be "size/8", where we'd determine "size" in the switch
statement above. That'll make sure the relocs section is properly
aligned.

-cary

Re: gold incremental file format on 64-bit

[David Miller]

From: Cary Coutant <ccoutant@google.com>
Date: Fri, 20 Apr 2012 16:16:12 -0700

>>> Is there anything else that's unaligned?
>>
>> Yes, some of the other sections can end up with an unaligned ending if
>> there is an odd number of entries because the individual objects are a
>> multiple of 4 bytes in size, but not a multiple of 8.
> 
> As far as I can tell, besides .gnu_incremental_inputs, the only other
> section that has any address-sized items in it is
> .gnu_incremental_relocs. That would probably be a bug in
> Incremental_inputs::create_data_sections():
> 
>   this->relocs_section_ = new Output_data_space(4, "** incremental_relocs");
> 
> That "4" should be "size/8", where we'd determine "size" in the switch
> statement above. That'll make sure the relocs section is properly
> aligned.

Yes, it's only the incremental inputs it seems.

For input objects and archive members, the global symbol entries
consume 20 bytes each and comdat groups consume 4 bytes each.

For input shared objects, global symbol entries consume 4 bytes
each.

For input archives members and global symbols each consume 4 bytes
each.

So at the end of each of these incremental input sub-parts we may
need to emit a 4-byte alignment pad.

Re: gold incremental file format on 64-bit

[Cary Coutant]

> For input objects and archive members, the global symbol entries
> consume 20 bytes each and comdat groups consume 4 bytes each.
>
> For input shared objects, global symbol entries consume 4 bytes
> each.
>
> For input archives members and global symbols each consume 4 bytes
> each.
>
> So at the end of each of these incremental input sub-parts we may
> need to emit a 4-byte alignment pad.

Agreed. Sorry, I missed those other cases on my quick scan. It would
be reasonable to do align info_offset inside the loop after the switch
statement in Output_section_incremental_inputs::set_final_data_size.
Then in Output_section_incremental_inputs::write_info_blocks, we'd
have to fill the padding with zeroes. I can make this and the
.gnu_incremental_relocs alignment fix next week, unless you want to
submit a patch yourself.

-cary

Re: gold incremental file format on 64-bit

[David Miller]

From: Cary Coutant <ccoutant@google.com>
Date: Fri, 20 Apr 2012 17:36:40 -0700

> It would be reasonable to do align info_offset inside the loop after
> the switch statement in
> Output_section_incremental_inputs::set_final_data_size.  Then in
> Output_section_incremental_inputs::write_info_blocks, we'd have to
> fill the padding with zeroes. I can make this and the
> .gnu_incremental_relocs alignment fix next week, unless you want to
> submit a patch yourself.

I plan to take care of this tonight if not over the weekend, besides
I have the crummy cpu by which one can test this fully. :-)

Re: gold incremental file format on 64-bit

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Fri, 20 Apr 2012 20:47:33 -0400 (EDT)

> From: Cary Coutant <ccoutant@google.com>
> Date: Fri, 20 Apr 2012 17:36:40 -0700
> 
>> It would be reasonable to do align info_offset inside the loop after
>> the switch statement in
>> Output_section_incremental_inputs::set_final_data_size.  Then in
>> Output_section_incremental_inputs::write_info_blocks, we'd have to
>> fill the padding with zeroes. I can make this and the
>> .gnu_incremental_relocs alignment fix next week, unless you want to
>> submit a patch yourself.
> 
> I plan to take care of this tonight if not over the weekend, besides
> I have the crummy cpu by which one can test this fully. :-)

Ok, here's what I came up with.  Tested on sparc, sparc64, and
x86-64.

2012-04-21  David S. Miller  <davem@davemloft.net>

	* incremental.cc (Incremental_inputs::create_data_sections): Align
	incremental reloc section to 8 bytes.
	(Output_section_incremental_inputs::set_final_data_size): Pad out
	offsets to ensure necessary 8-byte alignment.
	(Output_section_incremental_inputs::write_info_block): Likewise.
	* incremental.h (Incremental_inputs_reader::get_symbol_offset):
	Adjust offset calculation.
	(Incremental_inputs_reader::get_input_section): Likewise.
	(Incremental_inputs_reader::get_global_symbol_reader): Likewise.
	(Incremental_inputs_reader::get_comdat_group_signature): Likewise.

diff --git a/gold/incremental.cc b/gold/incremental.cc
index 60097a8..1d2bbf1 100644
--- a/gold/incremental.cc
+++ b/gold/incremental.cc
@@ -1223,7 +1223,7 @@ Incremental_inputs::create_data_sections(Symbol_table* symtab)
       gold_unreachable();
     }
   this->symtab_section_ = new Output_data_space(4, "** incremental_symtab");
-  this->relocs_section_ = new Output_data_space(4, "** incremental_relocs");
+  this->relocs_section_ = new Output_data_space(8, "** incremental_relocs");
   this->got_plt_section_ = new Output_data_space(4, "** incremental_got_plt");
 }
 
@@ -1289,7 +1289,7 @@ Output_section_incremental_inputs<size, big_endian>::set_final_data_size()
 	    // Input section count, global symbol count, local symbol offset,
 	    // local symbol count, first dynamic reloc, dynamic reloc count,
 	    // comdat group count.
-	    info_offset += 28;
+	    info_offset += 32;
 	    // Each input section.
 	    info_offset += (entry->get_input_section_count()
 			    * (8 + 2 * sizeof_addr));
@@ -1341,6 +1341,10 @@ Output_section_incremental_inputs<size, big_endian>::set_final_data_size()
 	default:
 	  gold_unreachable();
 	}
+
+      // Padding to get 8-byte alignment
+      if (info_offset & 4)
+	info_offset += 4;
     }
 
   this->set_data_size(info_offset);
@@ -1549,7 +1553,8 @@ Output_section_incremental_inputs<size, big_endian>::write_info_blocks(
 	    Swap32::writeval(pov + 16, first_dynrel);
 	    Swap32::writeval(pov + 20, ndynrel);
 	    Swap32::writeval(pov + 24, ncomdat);
-	    pov += 28;
+	    Swap32::writeval(pov + 28, 0);
+	    pov += 32;
 
 	    // Build a temporary array to map input section indexes
 	    // from the original object file index to the index in the
@@ -1745,6 +1750,13 @@ Output_section_incremental_inputs<size, big_endian>::write_info_blocks(
 	default:
 	  gold_unreachable();
 	}
+
+      // Add padding word if necessary.
+      if (static_cast<unsigned int>(pov - oview) & 4)
+	{
+	  Swap32::writeval(pov, 0);
+	  pov += 4;
+	}
     }
   return pov;
 }
diff --git a/gold/incremental.h b/gold/incremental.h
index b631ae2..1c3a833 100644
--- a/gold/incremental.h
+++ b/gold/incremental.h
@@ -866,7 +866,7 @@ class Incremental_inputs_reader
 		  || this->type() == INCREMENTAL_INPUT_ARCHIVE_MEMBER);
 
       unsigned int section_count = this->get_input_section_count();
-      return (this->info_offset_ + 28
+      return (this->info_offset_ + 32
 	      + section_count * input_section_entry_size
 	      + symndx * 20);
     }
@@ -1001,7 +1001,7 @@ class Incremental_inputs_reader
     {
       Input_section_info info;
       const unsigned char* p = (this->inputs_->p_
-				+ this->info_offset_ + 28
+				+ this->info_offset_ + 32
 				+ n * input_section_entry_size);
       unsigned int name_offset = Swap32::readval(p);
       info.name = this->inputs_->get_string(name_offset);
@@ -1019,7 +1019,7 @@ class Incremental_inputs_reader
 		  || this->type() == INCREMENTAL_INPUT_ARCHIVE_MEMBER);
       unsigned int section_count = this->get_input_section_count();
       const unsigned char* p = (this->inputs_->p_
-				+ this->info_offset_ + 28
+				+ this->info_offset_ + 32
 				+ section_count * input_section_entry_size
 				+ n * 20);
       return Incremental_global_symbol_reader<big_endian>(p);
@@ -1032,7 +1032,7 @@ class Incremental_inputs_reader
       unsigned int section_count = this->get_input_section_count();
       unsigned int symbol_count = this->get_global_symbol_count();
       const unsigned char* p = (this->inputs_->p_
-				+ this->info_offset_ + 28
+				+ this->info_offset_ + 32
 				+ section_count * input_section_entry_size
 				+ symbol_count * 20
 				+ n * 4);

Re: gold incremental file format on 64-bit

[Cary Coutant]

[-- Attachment #1: Type: text/plain, Size: 2476 bytes --]

> 2012-04-21  David S. Miller  <davem@davemloft.net>
>
>        * incremental.cc (Incremental_inputs::create_data_sections): Align
>        incremental reloc section to 8 bytes.
>        (Output_section_incremental_inputs::set_final_data_size): Pad out
>        offsets to ensure necessary 8-byte alignment.
>        (Output_section_incremental_inputs::write_info_block): Likewise.
>        * incremental.h (Incremental_inputs_reader::get_symbol_offset):
>        Adjust offset calculation.
>        (Incremental_inputs_reader::get_input_section): Likewise.
>        (Incremental_inputs_reader::get_global_symbol_reader): Likewise.
>        (Incremental_inputs_reader::get_comdat_group_signature): Likewise.

That patch should do fine for your purposes, but I wanted to take the
opportunity to replace some of those magic numbers with symbolic
constants, something I've been putting off for too long. The attached
patch does that, sets the relocation section alignment based on the
target size, and bumps the incremental info version number (because
this is, in fact, an incompatible change -- somehow, on Friday I
wasn't seeing the fact that this changes the layout of the info block
for object files).

Tested on x86_64. David, can you test it on Sparc?

Ian, OK to commit if it works for David?

-cary


2012-04-23  David S. Miller  <davem@davemloft.net>
	    Cary Coutant  <ccoutant@google.com>

	* incremental-dump.cc (find_input_containing_global): Replace
	magic number with symbolic constant.
	(dump_incremental_inputs): Update version number.
	* incremental.cc (Output_section_incremental_inputs): Update version
	number; import symbolic constants from Incremental_inputs_reader.
	(Incremental_inputs::create_data_sections): Align relocations
	section correctly for 64-bit targets.
	(Output_section_incremental_inputs::set_final_data_size): Use symbolic
	constants; add padding.
	(Output_section_incremental_inputs::write_header): Add assert for
	header_size.
	(Output_section_incremental_inputs::write_input_files): Add assert
	for input_entry_size.
	(Output_section_incremental_inputs::write_info_blocks): Add padding;
	add assert for object_info_size, input_section_entry_size,
	global_sym_entry_size.
	* incremental.h (Incremental_inputs_reader): Add symbolic constants
	for data structure sizes; use them.
	(Incremental_input_entry_reader): Import symbolic constants from
	Incremental_inputs_reader; use them.

[-- Attachment #2: padding-patch.txt --]
[-- Type: text/plain, Size: 15413 bytes --]

2012-04-23  David S. Miller  <davem@davemloft.net>
	    Cary Coutant  <ccoutant@google.com>

	* incremental-dump.cc (find_input_containing_global): Replace
	magic number with symbolic constant.
	(dump_incremental_inputs): Update version number.
	* incremental.cc (Output_section_incremental_inputs): Update version
	number; import symbolic constants from Incremental_inputs_reader.
	(Incremental_inputs::create_data_sections): Align relocations
	section correctly for 64-bit targets.
	(Output_section_incremental_inputs::set_final_data_size): Use symbolic
	constants; add padding.
	(Output_section_incremental_inputs::write_header): Add assert for
	header_size.
	(Output_section_incremental_inputs::write_input_files): Add assert
	for input_entry_size.
	(Output_section_incremental_inputs::write_info_blocks): Add padding;
	add assert for object_info_size, input_section_entry_size,
	global_sym_entry_size.
	* incremental.h (Incremental_inputs_reader): Add symbolic constants
	for data structure sizes; use them.
	(Incremental_input_entry_reader): Import symbolic constants from
	Incremental_inputs_reader; use them.


commit 2e47d3156deec3c3e22e97103d3bbd83b67997ff
Author: Cary Coutant <ccoutant@google.com>
Date:   Mon Apr 23 13:20:44 2012 -0700

    Add padding where necessary; use symbolic constants.

diff --git a/gold/incremental-dump.cc b/gold/incremental-dump.cc
index fb3d25f..5365265 100644
--- a/gold/incremental-dump.cc
+++ b/gold/incremental-dump.cc
@@ -52,6 +52,9 @@ find_input_containing_global(
     unsigned int* symndx)
 {
   typedef Incremental_inputs_reader<size, big_endian> Inputs_reader;
+  static const unsigned int global_sym_entry_size =
+      Incremental_inputs_reader<size, big_endian>::global_sym_entry_size;
+
   for (unsigned int i = 0; i < incremental_inputs.input_file_count(); ++i)
     {
       typename Inputs_reader::Incremental_input_entry_reader input_file =
@@ -63,7 +66,8 @@ find_input_containing_global(
       if (offset >= input_file.get_symbol_offset(0)
           && offset < input_file.get_symbol_offset(nsyms))
 	{
-	  *symndx = (offset - input_file.get_symbol_offset(0)) / 20;
+	  *symndx = ((offset - input_file.get_symbol_offset(0))
+		     / global_sym_entry_size);
 	  return input_file;
 	}
     }
@@ -92,7 +96,7 @@ dump_incremental_inputs(const char* argv0, const char* filename,
   Incremental_inputs_reader<size, big_endian>
       incremental_inputs(inc->inputs_reader());
 
-  if (incremental_inputs.version() != 1)
+  if (incremental_inputs.version() != 2)
     {
       fprintf(stderr, "%s: %s: unknown incremental version %d\n", argv0,
               filename, incremental_inputs.version());
diff --git a/gold/incremental.cc b/gold/incremental.cc
index 60097a8..6436a35 100644
--- a/gold/incremental.cc
+++ b/gold/incremental.cc
@@ -41,9 +41,10 @@
 
 namespace gold {
 
-// Version information. Will change frequently during the development, later
-// we could think about backward (and forward?) compatibility.
-const unsigned int INCREMENTAL_LINK_VERSION = 1;
+// Version number for the .gnu_incremental_inputs section.
+// Version 1 was the initial checkin.
+// Version 2 adds some padding to ensure 8-byte alignment where necessary.
+const unsigned int INCREMENTAL_LINK_VERSION = 2;
 
 // This class manages the .gnu_incremental_inputs section, which holds
 // the header information, a directory of input files, and separate
@@ -112,8 +113,18 @@ class Output_section_incremental_inputs : public Output_section_data
 
   // Sizes of various structures.
   static const int sizeof_addr = size / 8;
-  static const int header_size = 16;
-  static const int input_entry_size = 24;
+  static const int header_size =
+      Incremental_inputs_reader<size, big_endian>::header_size;
+  static const int input_entry_size =
+      Incremental_inputs_reader<size, big_endian>::input_entry_size;
+  static const unsigned int object_info_size =
+      Incremental_inputs_reader<size, big_endian>::object_info_size;
+  static const unsigned int input_section_entry_size =
+      Incremental_inputs_reader<size, big_endian>::input_section_entry_size;
+  static const unsigned int global_sym_entry_size =
+      Incremental_inputs_reader<size, big_endian>::global_sym_entry_size;
+  static const unsigned int incr_reloc_size =
+      Incremental_relocs_reader<size, big_endian>::reloc_size;
 
   // The Incremental_inputs object.
   const Incremental_inputs* inputs_;
@@ -1193,37 +1204,44 @@ Incremental_inputs::finalize()
 void
 Incremental_inputs::create_data_sections(Symbol_table* symtab)
 {
+  int reloc_align = 4;
+
   switch (parameters->size_and_endianness())
     {
 #ifdef HAVE_TARGET_32_LITTLE
     case Parameters::TARGET_32_LITTLE:
       this->inputs_section_ =
           new Output_section_incremental_inputs<32, false>(this, symtab);
+      reloc_align = 4;
       break;
 #endif
 #ifdef HAVE_TARGET_32_BIG
     case Parameters::TARGET_32_BIG:
       this->inputs_section_ =
           new Output_section_incremental_inputs<32, true>(this, symtab);
+      reloc_align = 4;
       break;
 #endif
 #ifdef HAVE_TARGET_64_LITTLE
     case Parameters::TARGET_64_LITTLE:
       this->inputs_section_ =
           new Output_section_incremental_inputs<64, false>(this, symtab);
+      reloc_align = 8;
       break;
 #endif
 #ifdef HAVE_TARGET_64_BIG
     case Parameters::TARGET_64_BIG:
       this->inputs_section_ =
           new Output_section_incremental_inputs<64, true>(this, symtab);
+      reloc_align = 8;
       break;
 #endif
     default:
       gold_unreachable();
     }
   this->symtab_section_ = new Output_data_space(4, "** incremental_symtab");
-  this->relocs_section_ = new Output_data_space(4, "** incremental_relocs");
+  this->relocs_section_ = new Output_data_space(reloc_align,
+						"** incremental_relocs");
   this->got_plt_section_ = new Output_data_space(4, "** incremental_got_plt");
 }
 
@@ -1244,8 +1262,6 @@ void
 Output_section_incremental_inputs<size, big_endian>::set_final_data_size()
 {
   const Incremental_inputs* inputs = this->inputs_;
-  const unsigned int sizeof_addr = size / 8;
-  const unsigned int rel_size = 8 + 2 * sizeof_addr;
 
   // Offset of each input entry.
   unsigned int input_offset = this->header_size;
@@ -1289,13 +1305,13 @@ Output_section_incremental_inputs<size, big_endian>::set_final_data_size()
 	    // Input section count, global symbol count, local symbol offset,
 	    // local symbol count, first dynamic reloc, dynamic reloc count,
 	    // comdat group count.
-	    info_offset += 28;
+	    info_offset += this->object_info_size;
 	    // Each input section.
 	    info_offset += (entry->get_input_section_count()
-			    * (8 + 2 * sizeof_addr));
+			    * this->input_section_entry_size);
 	    // Each global symbol.
 	    const Object::Symbols* syms = entry->object()->get_global_symbols();
-	    info_offset += syms->size() * 20;
+	    info_offset += syms->size() * this->global_sym_entry_size;
 	    // Each comdat group.
 	    info_offset += entry->get_comdat_group_count() * 4;
 	  }
@@ -1341,7 +1357,11 @@ Output_section_incremental_inputs<size, big_endian>::set_final_data_size()
 	default:
 	  gold_unreachable();
 	}
-    }
+
+     // Pad so each supplemental info block begins at an 8-byte boundary.
+     if (info_offset & 4)
+       info_offset += 4;
+   }
 
   this->set_data_size(info_offset);
 
@@ -1351,7 +1371,7 @@ Output_section_incremental_inputs<size, big_endian>::set_final_data_size()
 
   // Set the size of the .gnu_incremental_relocs section.
   inputs->relocs_section()->set_current_data_size(inputs->get_reloc_count()
-						  * rel_size);
+						  * this->incr_reloc_size);
 
   // Set the size of the .gnu_incremental_got_plt section.
   Sized_target<size, big_endian>* target =
@@ -1442,6 +1462,7 @@ Output_section_incremental_inputs<size, big_endian>::write_header(
   Swap32::writeval(pov + 4, input_file_count);
   Swap32::writeval(pov + 8, command_line_offset);
   Swap32::writeval(pov + 12, 0);
+  gold_assert(this->header_size == 16);
   return pov + this->header_size;
 }
 
@@ -1476,6 +1497,7 @@ Output_section_incremental_inputs<size, big_endian>::write_input_files(
       Swap32::writeval(pov + 16, mtime.nanoseconds);
       Swap16::writeval(pov + 20, flags);
       Swap16::writeval(pov + 22, (*p)->arg_serial());
+      gold_assert(this->input_entry_size == 24);
       pov += this->input_entry_size;
     }
   return pov;
@@ -1549,7 +1571,9 @@ Output_section_incremental_inputs<size, big_endian>::write_info_blocks(
 	    Swap32::writeval(pov + 16, first_dynrel);
 	    Swap32::writeval(pov + 20, ndynrel);
 	    Swap32::writeval(pov + 24, ncomdat);
-	    pov += 28;
+	    Swap32::writeval(pov + 28, 0);
+	    gold_assert(this->object_info_size == 32);
+	    pov += this->object_info_size;
 
 	    // Build a temporary array to map input section indexes
 	    // from the original object file index to the index in the
@@ -1581,7 +1605,9 @@ Output_section_incremental_inputs<size, big_endian>::write_info_blocks(
 		Swap32::writeval(pov + 4, out_shndx);
 		Swap::writeval(pov + 8, out_offset);
 		Swap::writeval(pov + 8 + sizeof_addr, sh_size);
-		pov += 8 + 2 * sizeof_addr;
+		gold_assert(this->input_section_entry_size
+			    == 8 + 2 * sizeof_addr);
+		pov += this->input_section_entry_size;
 	      }
 
 	    // For each global symbol, write its associated relocations,
@@ -1634,7 +1660,8 @@ Output_section_incremental_inputs<size, big_endian>::write_info_blocks(
 		Swap32::writeval(pov + 12, nrelocs);
 		Swap32::writeval(pov + 16,
 				 first_reloc * (8 + 2 * sizeof_addr));
-		pov += 20;
+		gold_assert(this->global_sym_entry_size == 20);
+		pov += this->global_sym_entry_size;
 	      }
 
 	    // For each kept COMDAT group, write the group signature.
@@ -1745,6 +1772,13 @@ Output_section_incremental_inputs<size, big_endian>::write_info_blocks(
 	default:
 	  gold_unreachable();
 	}
+
+     // Pad the info block to a multiple of 8 bytes.
+     if (static_cast<unsigned int>(pov - oview) & 4)
+      {
+	Swap32::writeval(pov, 0);
+	pov += 4;
+      }
     }
   return pov;
 }
diff --git a/gold/incremental.h b/gold/incremental.h
index b631ae2..20ae772 100644
--- a/gold/incremental.h
+++ b/gold/incremental.h
@@ -758,6 +758,23 @@ class Incremental_inputs_reader
   typedef elfcpp::Swap<64, big_endian> Swap64;
 
  public:
+  // Size of the .gnu_incremental_inputs header.
+  // (3 x 4-byte fields, plus 4 bytes padding.)
+  static const unsigned int header_size = 16;
+  // Size of an input file entry.
+  // (2 x 4-byte fields, 1 x 12-byte field, 2 x 2-byte fields.)
+  static const unsigned int input_entry_size = 24;
+  // Size of the first part of the supplemental info block for
+  // relocatable objects and archive members.
+  // (7 x 4-byte fields, plus 4 bytes padding.)
+  static const unsigned int object_info_size = 32;
+  // Size of an input section entry.
+  // (2 x 4-byte fields, 2 x address-sized fields.)
+  static const unsigned int input_section_entry_size = 8 + 2 * size / 8;
+  // Size of a global symbol entry in the supplemental info block.
+  // (5 x 4-byte fields.)
+  static const unsigned int global_sym_entry_size = 20;
+
   Incremental_inputs_reader()
     : p_(NULL), strtab_(NULL, 0), input_file_count_(0)
   { }
@@ -788,6 +805,14 @@ class Incremental_inputs_reader
   // Reader class for an input file entry and its supplemental info.
   class Incremental_input_entry_reader
   {
+   private:
+    static const unsigned int object_info_size =
+	Incremental_inputs_reader<size, big_endian>::object_info_size;
+    static const unsigned int input_section_entry_size =
+	Incremental_inputs_reader<size, big_endian>::input_section_entry_size;
+    static const unsigned int global_sym_entry_size =
+	Incremental_inputs_reader<size, big_endian>::global_sym_entry_size;
+
    public:
     Incremental_input_entry_reader(const Incremental_inputs_reader* inputs,
 				   unsigned int offset)
@@ -866,9 +891,10 @@ class Incremental_inputs_reader
 		  || this->type() == INCREMENTAL_INPUT_ARCHIVE_MEMBER);
 
       unsigned int section_count = this->get_input_section_count();
-      return (this->info_offset_ + 28
-	      + section_count * input_section_entry_size
-	      + symndx * 20);
+      return (this->info_offset_
+	      + this->object_info_size
+	      + section_count * this->input_section_entry_size
+	      + symndx * this->global_sym_entry_size);
     }
 
     // Return the global symbol count -- for objects & shared libraries only.
@@ -1001,8 +1027,9 @@ class Incremental_inputs_reader
     {
       Input_section_info info;
       const unsigned char* p = (this->inputs_->p_
-				+ this->info_offset_ + 28
-				+ n * input_section_entry_size);
+				+ this->info_offset_
+				+ this->object_info_size
+				+ n * this->input_section_entry_size);
       unsigned int name_offset = Swap32::readval(p);
       info.name = this->inputs_->get_string(name_offset);
       info.output_shndx = Swap32::readval(p + 4);
@@ -1019,9 +1046,10 @@ class Incremental_inputs_reader
 		  || this->type() == INCREMENTAL_INPUT_ARCHIVE_MEMBER);
       unsigned int section_count = this->get_input_section_count();
       const unsigned char* p = (this->inputs_->p_
-				+ this->info_offset_ + 28
-				+ section_count * input_section_entry_size
-				+ n * 20);
+				+ this->info_offset_
+				+ this->object_info_size
+				+ section_count * this->input_section_entry_size
+				+ n * this->global_sym_entry_size);
       return Incremental_global_symbol_reader<big_endian>(p);
     }
 
@@ -1032,9 +1060,10 @@ class Incremental_inputs_reader
       unsigned int section_count = this->get_input_section_count();
       unsigned int symbol_count = this->get_global_symbol_count();
       const unsigned char* p = (this->inputs_->p_
-				+ this->info_offset_ + 28
-				+ section_count * input_section_entry_size
-				+ symbol_count * 20
+				+ this->info_offset_
+				+ this->object_info_size
+				+ section_count * this->input_section_entry_size
+				+ symbol_count * this->global_sym_entry_size
 				+ n * 4);
       unsigned int name_offset = Swap32::readval(p);
       return this->inputs_->get_string(name_offset);
@@ -1072,8 +1101,6 @@ class Incremental_inputs_reader
     }
 
    private:
-    // Size of an input section entry.
-    static const unsigned int input_section_entry_size = 8 + 2 * size / 8;
     // The reader instance for the containing section.
     const Incremental_inputs_reader* inputs_;
     // The flags, including the type of input file.
@@ -1089,14 +1116,14 @@ class Incremental_inputs_reader
   input_file_offset(unsigned int n) const
   {
     gold_assert(n < this->input_file_count_);
-    return 16 + n * 24;
+    return this->header_size + n * this->input_entry_size;
   }
 
   // Return the index of an input file entry given its OFFSET.
   unsigned int
   input_file_index(unsigned int offset) const
   {
-    int n = (offset - 16) / 24;
+    int n = ((offset - this->header_size) / this->input_entry_size);
     gold_assert(input_file_offset(n) == offset);
     return n;
   }
@@ -1110,7 +1137,8 @@ class Incremental_inputs_reader
   Incremental_input_entry_reader
   input_file_at_offset(unsigned int offset) const
   {
-    gold_assert(offset < 16 + this->input_file_count_ * 24);
+    gold_assert(offset < (this->header_size
+			  + this->input_file_count_ * this->input_entry_size));
     return Incremental_input_entry_reader(this, offset);
   }
 

Re: gold incremental file format on 64-bit

[David Miller]

From: Cary Coutant <ccoutant@google.com>
Date: Mon, 23 Apr 2012 13:41:28 -0700

> Tested on x86_64. David, can you test it on Sparc?

Works.

> +
> +     // Pad the info block to a multiple of 8 bytes.
> +     if (static_cast<unsigned int>(pov - oview) & 4)
> +      {
> +	Swap32::writeval(pov, 0);
> +	pov += 4;
> +      }

Maybe this could be indented better :-)

Re: gold incremental file format on 64-bit

[Cary Coutant]

>> Tested on x86_64. David, can you test it on Sparc?
>
> Works.

Thanks!

>> +     // Pad the info block to a multiple of 8 bytes.
>> +     if (static_cast<unsigned int>(pov - oview) & 4)
>> +      {
>> +     Swap32::writeval(pov, 0);
>> +     pov += 4;
>> +      }
>
> Maybe this could be indented better :-)

Those two lines are indented by tabs. Looks fine in my editor.

-cary

Re: gold incremental file format on 64-bit

[David Miller]

From: Cary Coutant <ccoutant@google.com>
Date: Mon, 23 Apr 2012 14:11:28 -0700

>> Maybe this could be indented better :-)
> 
> Those two lines are indented by tabs. Looks fine in my editor.

Ok, I'm continually fooled by this :-)

Re: gold incremental file format on 64-bit

[Ian Lance Taylor]

Cary Coutant <ccoutant@google.com> writes:

> 2012-04-23  David S. Miller  <davem@davemloft.net>
> 	    Cary Coutant  <ccoutant@google.com>
>
> 	* incremental-dump.cc (find_input_containing_global): Replace
> 	magic number with symbolic constant.
> 	(dump_incremental_inputs): Update version number.
> 	* incremental.cc (Output_section_incremental_inputs): Update version
> 	number; import symbolic constants from Incremental_inputs_reader.
> 	(Incremental_inputs::create_data_sections): Align relocations
> 	section correctly for 64-bit targets.
> 	(Output_section_incremental_inputs::set_final_data_size): Use symbolic
> 	constants; add padding.
> 	(Output_section_incremental_inputs::write_header): Add assert for
> 	header_size.
> 	(Output_section_incremental_inputs::write_input_files): Add assert
> 	for input_entry_size.
> 	(Output_section_incremental_inputs::write_info_blocks): Add padding;
> 	add assert for object_info_size, input_section_entry_size,
> 	global_sym_entry_size.
> 	* incremental.h (Incremental_inputs_reader): Add symbolic constants
> 	for data structure sizes; use them.
> 	(Incremental_input_entry_reader): Import symbolic constants from
> 	Incremental_inputs_reader; use them.

This is OK.

Thanks.

Ian