[PATCH 1/2] gold: extend Output_file to support read/write access to existing files.

[PATCH 1/2] gold: extend Output_file to support read/write access to existing files.

[Mikolaj Zalewski]

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

Added options to open file without removing its contents and made
map_anonymous behave similarly to other *map* methods, e.g. to modify
this->base_ on success.

2009-09-01  Mikolaj Zalewski  <mikolajz@google.com>

	* output.cc (Output_file::Output_file): Initialize
map_include_original_content_ .
	(Output_file::open_for_modification): New method.
	(Output_file::try_map_anonymous): Renamed from
Output_file::map_anonymous, changed behavior to similar to other
*map*.
	(Output_file::map): Don't try anonymous mapping it
this->map_include_original_content_.
	(Output_file::try_map_no_anonymous): New method (most code from
Output_file::map).
	* output.h (Output_file::open_for_modification): New method.
	(Output_file::open): Update comment.
	(Output_file::resize): Update comment.
	(Output_file::close): Update comment.
	(Output_file::map): Update comment.
	(Output_file::try_map_anonymous): Renamed from Output_file::map_anonymous.
	(Output_file::try_map_no_anonymous): New method.
	(Output_file::map_include_original_content_): New field.

[-- Attachment #2: 0001-gold-extend-Output_file-to-support-read-write-acces.patch --]
[-- Type: text/x-patch, Size: 6865 bytes --]

From 1f7b4c9a802f851e6d3a1f78b2e2333648ae2da4 Mon Sep 17 00:00:00 2001
From: Mikolaj Zalewski <mikolajz@google.com>
Date: Tue, 1 Sep 2009 17:16:25 +0200
Subject: [PATCH] gold: extend Output_file to support read/write access to existing files.

---
 gold/output.cc |   91 +++++++++++++++++++++++++++++++++++++++++++++----------
 gold/output.h  |   24 ++++++++++++---
 2 files changed, 93 insertions(+), 22 deletions(-)

diff --git a/gold/output.cc b/gold/output.cc
index 64fcb37..4d1b144 100644
--- a/gold/output.cc
+++ b/gold/output.cc
@@ -3392,11 +3392,47 @@ Output_file::Output_file(const char* name)
     o_(-1),
     file_size_(0),
     base_(NULL),
+    map_include_original_content_(false),
     map_is_anonymous_(false),
     is_temporary_(false)
 {
 }
 
+// Try to open an existing file. Returns false if the file doesn't exist,
+// has a size of 0 or can't be mmaped.
+
+bool
+Output_file::open_for_modification()
+{
+  // The name "-" means "stdout".
+  if (strcmp(this->name_, "-") == 0)
+    return false;
+
+  // Don't bother opening files with a size of zero.
+  struct stat s;
+  if (::stat(this->name_, &s) != 0 || s.st_size == 0)
+    return false;
+
+  int mode = parameters->options().relocatable() ? 0666 : 0777;
+  int o = open_descriptor(-1, this->name_, O_RDWR | O_CREAT, mode);
+  if (o < 0)
+    gold_fatal(_("%s: open: %s"), this->name_, strerror(errno));
+  this->o_ = o;
+  this->file_size_ = s.st_size;
+
+  // If the file can't be mmapped, copying the content to an anonymous map will
+  // probably negate the performance benefits of incremental linking. This
+  // could be helped by using views and loading only the necessary parts, but
+  // this is not supported as of now.
+  if (!this->try_map_no_anonymous()) {
+    this->o_ = -1;
+    this->file_size_ = 0;
+    return false;
+  }
+  this->map_include_original_content_ = true;
+  return true;
+}
+
 // Open the output file.
 
 void
@@ -3468,12 +3504,17 @@ Output_file::resize(off_t file_size)
 // Map a block of memory which will later be written to the file.
 // Return a pointer to the memory.
 
-void*
-Output_file::map_anonymous()
+bool
+Output_file::try_map_anonymous()
 {
-  this->map_is_anonymous_ = true;
-  return ::mmap(NULL, this->file_size_, PROT_READ | PROT_WRITE,
-		MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  void* base = ::mmap(NULL, this->file_size_, PROT_READ | PROT_WRITE,
+		      MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  if (this->base_ != MAP_FAILED) {
+    this->map_is_anonymous_ = true;
+    this->base_ = static_cast<unsigned char*>(base);
+    return true;
+  }
+  return false;
 }
 
 // Map the file into memory.
@@ -3481,6 +3522,28 @@ Output_file::map_anonymous()
 void
 Output_file::map()
 {
+  if (try_map_no_anonymous())
+    return;
+
+  // The mmap call might fail because of file system issues: the
+  // file system might not support mmap at all, or it might not
+  // support mmap with PROT_WRITE.  I'm not sure which errno
+  // values we will see in all cases, so if the mmap fails for any
+  // reason and we don't care about file contents, try for an anonymous map.
+  if (!this->map_include_original_content_ && this->try_map_anonymous())
+    return;
+
+  gold_fatal(_("%s: mmap: failed to allocate %lu bytes for output file: %s"),
+             this->name_, static_cast<unsigned long>(this->file_size_),
+             strerror(errno));
+}
+
+// Map the file into memory. Unlike map(), we don't fallback into creating
+// an anonymous mapping if the file can't be mmapped.
+
+bool
+Output_file::try_map_no_anonymous()
+{
   const int o = this->o_;
 
   // If the output file is not a regular file, don't try to mmap it;
@@ -3492,7 +3555,7 @@ Output_file::map()
       || ::fstat(o, &statbuf) != 0
       || !S_ISREG(statbuf.st_mode)
       || this->is_temporary_)
-    base = this->map_anonymous();
+    return false;
   else
     {
       // Ensure that we have disk space available for the file.  If we
@@ -3510,20 +3573,14 @@ Output_file::map()
       this->map_is_anonymous_ = false;
       base = ::mmap(NULL, this->file_size_, PROT_READ | PROT_WRITE,
                     MAP_SHARED, o, 0);
-
-      // The mmap call might fail because of file system issues: the
-      // file system might not support mmap at all, or it might not
-      // support mmap with PROT_WRITE.  I'm not sure which errno
-      // values we will see in all cases, so if the mmap fails for any
-      // reason try for an anonymous map.
-      if (base == MAP_FAILED)
-	base = this->map_anonymous();
     }
+  // The mmap call might fail because of file system issues: the
+  // file system might not support mmap at all, or it might not
+  // support mmap with PROT_WRITE.
   if (base == MAP_FAILED)
-    gold_fatal(_("%s: mmap: failed to allocate %lu bytes for output file: %s"),
-	       this->name_, static_cast<unsigned long>(this->file_size_),
-	       strerror(errno));
+    return false;
   this->base_ = static_cast<unsigned char*>(base);
+  return true;
 }
 
 // Unmap the file from memory.
diff --git a/gold/output.h b/gold/output.h
index f9cbfa6..d104952 100644
--- a/gold/output.h
+++ b/gold/output.h
@@ -3093,16 +3093,23 @@ class Output_file
   set_is_temporary()
   { this->is_temporary_ = true; }
 
+  // Try to open an existing file. Returns false if the file doesn't exist,
+  // has a size of 0 or can't be mmaped. This method is thread-unsafe.
+  bool
+  open_for_modification();
+
   // Open the output file.  FILE_SIZE is the final size of the file.
+  // If the file already exists, it is deleted/truncated. This method
+  // is thread-unsafe.
   void
   open(off_t file_size);
 
-  // Resize the output file.
+  // Resize the output file. This method is thread-unsafe.
   void
   resize(off_t file_size);
 
   // Close the output file (flushing all buffered data) and make sure
-  // there are no errors.
+  // there are no errors. This method is thread-unsafe.
   void
   close();
 
@@ -3153,13 +3160,17 @@ class Output_file
   { }
 
  private:
-  // Map the file into memory.
+  // Map the file into memory or, if this fails, allocate anonymous memory.
   void
   map();
 
   // Allocate anonymous memory for the file.
-  void*
-  map_anonymous();
+  bool
+  try_map_anonymous();
+
+  // Map the file into memory. Don't fall back info allocating anonymous memory.
+  bool
+  try_map_no_anonymous();
 
   // Unmap the file from memory (and flush to disk buffers).
   void
@@ -3173,6 +3184,9 @@ class Output_file
   off_t file_size_;
   // Base of file mapped into memory.
   unsigned char* base_;
+  // Whether the mmapped memory should include original content (needed only
+  // during incremental links).
+  bool map_include_original_content_;
   // True iff base_ points to a memory buffer rather than an output file.
   bool map_is_anonymous_;
   // True if this is a temporary file which should not be output.
-- 
1.5.4.3

Re: [PATCH 1/2] gold: extend Output_file to support read/write access to existing files.

[Ian Lance Taylor]

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

Mikolaj Zalewski <mikolajz@google.com> writes:

> Added options to open file without removing its contents and made
> map_anonymous behave similarly to other *map* methods, e.g. to modify
> this->base_ on success.
>
> 2009-09-01  Mikolaj Zalewski  <mikolajz@google.com>
>
> 	* output.cc (Output_file::Output_file): Initialize
> map_include_original_content_ .
> 	(Output_file::open_for_modification): New method.
> 	(Output_file::try_map_anonymous): Renamed from
> Output_file::map_anonymous, changed behavior to similar to other
> *map*.
> 	(Output_file::map): Don't try anonymous mapping it
> this->map_include_original_content_.
> 	(Output_file::try_map_no_anonymous): New method (most code from
> Output_file::map).
> 	* output.h (Output_file::open_for_modification): New method.
> 	(Output_file::open): Update comment.
> 	(Output_file::resize): Update comment.
> 	(Output_file::close): Update comment.
> 	(Output_file::map): Update comment.
> 	(Output_file::try_map_anonymous): Renamed from Output_file::map_anonymous.
> 	(Output_file::try_map_no_anonymous): New method.
> 	(Output_file::map_include_original_content_): New field.

Thanks.  I made a bunch of minor changes to this patch.

1) I didn't like the names "try_map_anonymous" and
   "try_map_no_anonymous".  The functions don't try to do maps: they
   actually do maps.

2) I didn't see any reason to include the map_include_original_content_
   field.

3) GNU formatting puts the open brace on a line by itself.

I included the other patch mostly as is.

This is the patch I committed.

Thanks.

Ian


2009-09-01  Mikolaj Zalewski  <mikolajz@google.com>

	* gold.cc: Include "incremental.h".
	(queue_initial_tasks): Call Incremental_checker methods.
	* incremental.cc: Include "output.h".
	(Incremental_checker::can_incrementally_link_output_file): New
	method.
	* incremental.h (Incremental_checker): New class.

	* output.cc (Output_file::open_for_modification): New method.
	(Output_file::map_anonymous): Changed return type to bool.  Record
	map in base_ field.
	(Output_file::map_no_anonymous): New method, broken out of map.
	(Output_file::map): Use map_no_anonymous and map_anonymous.
	* output.h (class Output_file): Update declarations.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Incremental changes --]
[-- Type: text/x-patch, Size: 11938 bytes --]

Index: gold.cc
===================================================================
RCS file: /cvs/src/src/gold/gold.cc,v
retrieving revision 1.66
diff -p -u -r1.66 gold.cc
--- gold.cc	5 Aug 2009 20:51:56 -0000	1.66
+++ gold.cc	1 Sep 2009 17:27:59 -0000
@@ -42,6 +42,7 @@
 #include "defstd.h"
 #include "plugin.h"
 #include "icf.h"
+#include "incremental.h"
 
 namespace gold
 {
@@ -176,6 +177,20 @@ queue_initial_tasks(const General_option
     thread_count = cmdline.number_of_input_files();
   workqueue->set_thread_count(thread_count);
 
+  if (cmdline.options().incremental())
+    {
+      Incremental_checker incremental_checker(
+          parameters->options().output_file_name());
+      if (incremental_checker.can_incrementally_link_output_file())
+        {
+          // TODO: remove when incremental linking implemented.
+          printf("Incremental linking might be possible "
+              "(not implemented yet)\n");
+        }
+      // TODO: If we decide on an incremental build, fewer tasks
+      // should be scheduled.
+    }
+
   // Read the input files.  We have to add the symbols to the symbol
   // table in order.  We do this by creating a separate blocker for
   // each input file.  We associate the blocker with the following
@@ -229,8 +244,8 @@ queue_initial_tasks(const General_option
     }
 }
 
-// Queue up a set of tasks to be done before queueing the middle set 
-// of tasks.  This is only necessary when garbage collection 
+// Queue up a set of tasks to be done before queueing the middle set
+// of tasks.  This is only necessary when garbage collection
 // (--gc-sections) of unused sections is desired.  The relocs are read
 // and processed here early to determine the garbage sections before the
 // relocs can be scanned in later tasks.
Index: incremental.cc
===================================================================
RCS file: /cvs/src/src/gold/incremental.cc,v
retrieving revision 1.5
diff -p -u -r1.5 incremental.cc
--- incremental.cc	6 Jul 2009 23:11:21 -0000	1.5
+++ incremental.cc	1 Sep 2009 17:27:59 -0000
@@ -25,6 +25,7 @@
 #include "output.h"
 #include "incremental.h"
 #include "archive.h"
+#include "output.h"
 
 using elfcpp::Convert;
 
@@ -149,6 +150,18 @@ class Incremental_inputs_entry_write
   internal::Incremental_inputs_entry_data* p_;
 };
 
+// Analyzes the output file to check if incremental linking is possible and
+// (to be done) what files need to be relinked.
+
+bool
+Incremental_checker::can_incrementally_link_output_file()
+{
+  Output_file output(this->output_name_);
+  if (!output.open_for_modification())
+    return false;
+  return true;
+}
+
 // Add the command line to the string table, setting
 // command_line_key_.  In incremental builds, the command line is
 // stored in .gnu_incremental_inputs so that the next linker run can
Index: incremental.h
===================================================================
RCS file: /cvs/src/src/gold/incremental.h,v
retrieving revision 1.3
diff -p -u -r1.3 incremental.h
--- incremental.h	6 Jul 2009 23:11:21 -0000	1.3
+++ incremental.h	1 Sep 2009 17:27:59 -0000
@@ -50,6 +50,24 @@ enum Incremental_input_type
   INCREMENTAL_INPUT_SCRIPT = 4
 };
 
+// Code invoked early during an incremental link that checks what files need
+// to be relinked.
+class Incremental_checker
+{
+ public:
+  Incremental_checker(const char* output_name)
+    : output_name_(output_name)
+  { }
+
+  // Analyzes the output file to check if incremental linking is possible and
+  // what files needs to be relinked.
+  bool
+  can_incrementally_link_output_file();
+
+ private:
+  const char* output_name_;
+};
+
 // This class contains the information needed during an incremental
 // build about the inputs necessary to build the .gnu_incremental_inputs.
 class Incremental_inputs
@@ -127,11 +145,11 @@ class Incremental_inputs
     {
       // Present if type == INCREMENTAL_INPUT_ARCHIVE.
       Archive* archive;
-  
+
       // Present if type == INCREMENTAL_INPUT_OBJECT or
       // INCREMENTAL_INPUT_SHARED_LIBRARY.
       Object* object;
-  
+
       // Present if type == INCREMENTAL_INPUT_SCRIPT.
       Script_info* script;
     };
@@ -141,7 +159,7 @@ class Incremental_inputs
 
     // Position of the entry information in the output section.
     unsigned int index;
-    
+
     // Last modification time of the file.
     Timespec mtime;
   };
@@ -151,7 +169,7 @@ class Incremental_inputs
   // A lock guarding access to inputs_ during the first phase of linking, when
   // report_ function may be called from multiple threads.
   Lock* lock_;
-  
+
   // The list of input arguments obtained from parsing the command line.
   const Input_arguments* inputs_;
 
Index: output.cc
===================================================================
RCS file: /cvs/src/src/gold/output.cc,v
retrieving revision 1.92
diff -p -u -r1.92 output.cc
--- output.cc	24 Jun 2009 19:48:51 -0000	1.92
+++ output.cc	1 Sep 2009 17:27:59 -0000
@@ -3397,6 +3397,42 @@ Output_file::Output_file(const char* nam
 {
 }
 
+// Try to open an existing file.  Returns false if the file doesn't
+// exist, has a size of 0 or can't be mmapped.
+
+bool
+Output_file::open_for_modification()
+{
+  // The name "-" means "stdout".
+  if (strcmp(this->name_, "-") == 0)
+    return false;
+
+  // Don't bother opening files with a size of zero.
+  struct stat s;
+  if (::stat(this->name_, &s) != 0 || s.st_size == 0)
+    return false;
+
+  int o = open_descriptor(-1, this->name_, O_RDWR, 0);
+  if (o < 0)
+    gold_fatal(_("%s: open: %s"), this->name_, strerror(errno));
+  this->o_ = o;
+  this->file_size_ = s.st_size;
+
+  // If the file can't be mmapped, copying the content to an anonymous
+  // map will probably negate the performance benefits of incremental
+  // linking.  This could be helped by using views and loading only
+  // the necessary parts, but this is not supported as of now.
+  if (!this->map_no_anonymous())
+    {
+      release_descriptor(o, true);
+      this->o_ = -1;
+      this->file_size_ = 0;
+      return false;
+    }
+
+  return true;
+}
+
 // Open the output file.
 
 void
@@ -3465,21 +3501,27 @@ Output_file::resize(off_t file_size)
     }
 }
 
-// Map a block of memory which will later be written to the file.
-// Return a pointer to the memory.
+// Map an anonymous block of memory which will later be written to the
+// file.  Return whether the map succeeded.
 
-void*
+bool
 Output_file::map_anonymous()
 {
-  this->map_is_anonymous_ = true;
-  return ::mmap(NULL, this->file_size_, PROT_READ | PROT_WRITE,
-		MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  void* base = ::mmap(NULL, this->file_size_, PROT_READ | PROT_WRITE,
+		      MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  if (base != MAP_FAILED)
+    {
+      this->map_is_anonymous_ = true;
+      this->base_ = static_cast<unsigned char*>(base);
+      return true;
+    }
+  return false;
 }
 
-// Map the file into memory.
+// Map the file into memory.  Return whether the mapping succeeded.
 
-void
-Output_file::map()
+bool
+Output_file::map_no_anonymous()
 {
   const int o = this->o_;
 
@@ -3492,38 +3534,52 @@ Output_file::map()
       || ::fstat(o, &statbuf) != 0
       || !S_ISREG(statbuf.st_mode)
       || this->is_temporary_)
-    base = this->map_anonymous();
-  else
-    {
-      // Ensure that we have disk space available for the file.  If we
-      // don't do this, it is possible that we will call munmap,
-      // close, and exit with dirty buffers still in the cache with no
-      // assigned disk blocks.  If the disk is out of space at that
-      // point, the output file will wind up incomplete, but we will
-      // have already exited.  The alternative to fallocate would be
-      // to use fdatasync, but that would be a more significant
-      // performance hit.
-      if (::posix_fallocate(o, 0, this->file_size_) < 0)
-	gold_fatal(_("%s: %s"), this->name_, strerror(errno));
-
-      // Map the file into memory.
-      this->map_is_anonymous_ = false;
-      base = ::mmap(NULL, this->file_size_, PROT_READ | PROT_WRITE,
-                    MAP_SHARED, o, 0);
-
-      // The mmap call might fail because of file system issues: the
-      // file system might not support mmap at all, or it might not
-      // support mmap with PROT_WRITE.  I'm not sure which errno
-      // values we will see in all cases, so if the mmap fails for any
-      // reason try for an anonymous map.
-      if (base == MAP_FAILED)
-	base = this->map_anonymous();
-    }
+    return false;
+
+  // Ensure that we have disk space available for the file.  If we
+  // don't do this, it is possible that we will call munmap, close,
+  // and exit with dirty buffers still in the cache with no assigned
+  // disk blocks.  If the disk is out of space at that point, the
+  // output file will wind up incomplete, but we will have already
+  // exited.  The alternative to fallocate would be to use fdatasync,
+  // but that would be a more significant performance hit.
+  if (::posix_fallocate(o, 0, this->file_size_) < 0)
+    gold_fatal(_("%s: %s"), this->name_, strerror(errno));
+
+  // Map the file into memory.
+  base = ::mmap(NULL, this->file_size_, PROT_READ | PROT_WRITE,
+		MAP_SHARED, o, 0);
+
+  // The mmap call might fail because of file system issues: the file
+  // system might not support mmap at all, or it might not support
+  // mmap with PROT_WRITE.
   if (base == MAP_FAILED)
-    gold_fatal(_("%s: mmap: failed to allocate %lu bytes for output file: %s"),
-	       this->name_, static_cast<unsigned long>(this->file_size_),
-	       strerror(errno));
+    return false;
+
+  this->map_is_anonymous_ = false;
   this->base_ = static_cast<unsigned char*>(base);
+  return true;
+}
+
+// Map the file into memory.
+
+void
+Output_file::map()
+{
+  if (this->map_no_anonymous())
+    return;
+
+  // The mmap call might fail because of file system issues: the file
+  // system might not support mmap at all, or it might not support
+  // mmap with PROT_WRITE.  I'm not sure which errno values we will
+  // see in all cases, so if the mmap fails for any reason and we
+  // don't care about file contents, try for an anonymous map.
+  if (this->map_anonymous())
+    return;
+
+  gold_fatal(_("%s: mmap: failed to allocate %lu bytes for output file: %s"),
+             this->name_, static_cast<unsigned long>(this->file_size_),
+             strerror(errno));
 }
 
 // Unmap the file from memory.
Index: output.h
===================================================================
RCS file: /cvs/src/src/gold/output.h,v
retrieving revision 1.80
diff -p -u -r1.80 output.h
--- output.h	24 Jun 2009 19:48:51 -0000	1.80
+++ output.h	1 Sep 2009 17:27:59 -0000
@@ -3093,16 +3093,24 @@ class Output_file
   set_is_temporary()
   { this->is_temporary_ = true; }
 
+  // Try to open an existing file. Returns false if the file doesn't
+  // exist, has a size of 0 or can't be mmaped.  This method is
+  // thread-unsafe.
+  bool
+  open_for_modification();
+
   // Open the output file.  FILE_SIZE is the final size of the file.
+  // If the file already exists, it is deleted/truncated.  This method
+  // is thread-unsafe.
   void
   open(off_t file_size);
 
-  // Resize the output file.
+  // Resize the output file.  This method is thread-unsafe.
   void
   resize(off_t file_size);
 
   // Close the output file (flushing all buffered data) and make sure
-  // there are no errors.
+  // there are no errors.  This method is thread-unsafe.
   void
   close();
 
@@ -3153,14 +3161,19 @@ class Output_file
   { }
 
  private:
-  // Map the file into memory.
+  // Map the file into memory or, if that fails, allocate anonymous
+  // memory.
   void
   map();
 
   // Allocate anonymous memory for the file.
-  void*
+  bool
   map_anonymous();
 
+  // Map the file into memory.
+  bool
+  map_no_anonymous();
+
   // Unmap the file from memory (and flush to disk buffers).
   void
   unmap();

Re: [PATCH 1/2] gold: extend Output_file to support read/write access to existing files.

[Mikolaj Zalewski]

> 2) I didn't see any reason to include the map_include_original_content_
>   field.
  This was added to avoid such a scenario:
  1. The program opens a file with Output_file::open_for_modification,
the mapping succeeds.
  2. The programs calls Output_file::resize, this calls unmap() and map()
  3. Without map_include_original_content_, if inside
Output_file::map() mapping this file fails but anonymous mapping
succeeds, we loose all the contents of the file.

  I don't think this would happen often but could, in theory, happen
if e.g. the first map in 3. failed because of lack of address space
for the larger mapping but the second succeeded because another
threads freed some space between the calls.
  However, it seems this could also be a problem for new files created
with Output_file::open if someone wrote to them before calling
resize(). Maybe the solution is to change map to map_no_anonymous in
Output_file::resize?

Mikołaj

Re: [PATCH 1/2] gold: extend Output_file to support read/write access to existing files.

[Ian Lance Taylor]

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

Mikolaj Zalewski <mikolajz@google.com> writes:

>> 2) I didn't see any reason to include the map_include_original_content_
>>   field.
>   This was added to avoid such a scenario:
>   1. The program opens a file with Output_file::open_for_modification,
> the mapping succeeds.
>   2. The programs calls Output_file::resize, this calls unmap() and map()
>   3. Without map_include_original_content_, if inside
> Output_file::map() mapping this file fails but anonymous mapping
> succeeds, we loose all the contents of the file.
>
>   I don't think this would happen often but could, in theory, happen
> if e.g. the first map in 3. failed because of lack of address space
> for the larger mapping but the second succeeded because another
> threads freed some space between the calls.
>   However, it seems this could also be a problem for new files created
> with Output_file::open if someone wrote to them before calling
> resize(). Maybe the solution is to change map to map_no_anonymous in
> Output_file::resize?

Good point.  Thanks for spotting that.  I committed this patch.

Ian


2009-09-02  Ian Lance Taylor  <iant@google.com>

	* output.cc (Output_file::resize): Call map_no_anonymous rather
	than map.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Call map_no_anonymous --]
[-- Type: text/x-patch, Size: 521 bytes --]

Index: output.cc
===================================================================
RCS file: /cvs/src/src/gold/output.cc,v
retrieving revision 1.93
diff -p -u -r1.93 output.cc
--- output.cc	1 Sep 2009 17:32:20 -0000	1.93
+++ output.cc	2 Sep 2009 16:38:33 -0000
@@ -3497,7 +3497,8 @@ Output_file::resize(off_t file_size)
     {
       this->unmap();
       this->file_size_ = file_size;
-      this->map();
+      if (!this->map_no_anonymous())
+	gold_fatal(_("%s: mmap: %s"), this->name_, strerror(errno));
     }
 }
 

Re: [PATCH 1/2] gold: extend Output_file to support read/write access to existing files.

[Ian Lance Taylor]

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

Mikolaj Zalewski <mikolajz@google.com> writes:

>> 2) I didn't see any reason to include the map_include_original_content_
>>   field.
>   This was added to avoid such a scenario:
>   1. The program opens a file with Output_file::open_for_modification,
> the mapping succeeds.
>   2. The programs calls Output_file::resize, this calls unmap() and map()
>   3. Without map_include_original_content_, if inside
> Output_file::map() mapping this file fails but anonymous mapping
> succeeds, we loose all the contents of the file.
>
>   I don't think this would happen often but could, in theory, happen
> if e.g. the first map in 3. failed because of lack of address space
> for the larger mapping but the second succeeded because another
> threads freed some space between the calls.
>   However, it seems this could also be a problem for new files created
> with Output_file::open if someone wrote to them before calling
> resize(). Maybe the solution is to change map to map_no_anonymous in
> Output_file::resize?

Good point.  Thanks for spotting that.  I committed this patch.

Ian


2009-09-02  Ian Lance Taylor  <iant@google.com>

	* output.cc (Output_file::resize): Call map_no_anonymous rather
	than map.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Call map_no_anonymous --]
[-- Type: text/x-patch, Size: 521 bytes --]

Index: output.cc
===================================================================
RCS file: /cvs/src/src/gold/output.cc,v
retrieving revision 1.93
diff -p -u -r1.93 output.cc
--- output.cc	1 Sep 2009 17:32:20 -0000	1.93
+++ output.cc	2 Sep 2009 16:38:33 -0000
@@ -3497,7 +3497,8 @@ Output_file::resize(off_t file_size)
     {
       this->unmap();
       this->file_size_ = file_size;
-      this->map();
+      if (!this->map_no_anonymous())
+	gold_fatal(_("%s: mmap: %s"), this->name_, strerror(errno));
     }
 }