From: Cary Coutant <ccoutant@google.com>
To: Ian Lance Taylor <iant@google.com>
Cc: Binutils <binutils@sourceware.org>
Subject: Re: [gold patch] Incremental 15/18: Add --incremental-base option.
Date: Tue, 24 May 2011 22:54:00 -0000 [thread overview]
Message-ID: <BANLkTinv8Dr2rcPGXbv-1hXLwV2khGdPnA@mail.gmail.com> (raw)
In-Reply-To: <BANLkTi=uPhqYhsbo0P6KHYwsaK90Qhcj=w@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1767 bytes --]
>> You're going to some mild contortions to map in the base file, and then
>> you are throwing away the mapping. If you are going to stick with this
>> copying approach, then I think it would be simpler to just map the
>> output file as for a non-incremental link, and then open() the base file
>> and read() the contents into the mapping you just created. That will be
>> just as efficient--it's a nice sequential read--and will save you from
>> running out of memory when you in effect map the output file twice.
>
> Argh, good point. I'll rework this.
Revised patch attached.
-cary
2011-05-24 Cary Coutant <ccoutant@google.com>
* gold.cc (queue_initial_tasks): Pass incremental base filename
to Output_file::open_base_file; don't print error message.
* incremental-dump.cc (main): Adjust call to
Output_file::open_for_modification.
* incremental-dump.cc (main): Likewise.
* incremental.cc (Incremental_inputs::report_command_line):
Ignore --incremental-base option when comparing command lines.
Ignore parameter when given as separate argument.
* options.h (class General_options): Add --incremental-base.
* output.cc (Output_file::Output_file):
(Output_file::open_base_file): Add base_name and writable parameters;
read base file into new file; print error message here.
(Output_file::map_no_anonymous): Add writable parameter; adjust all
callers.
* output.h (Output_file::open_for_modification): Rename to...
(Output_file::open_base_file): ...this; add base_name and
writable parameters; adjust all callers.
(Output_file::map_no_anonymous): Add writable parameter; adjust all
callers.
* testsuite/Makefile.am (incremental_test_4): Test
--incremental-base.
* testsuite/Makefile.in: Regenerate.
[-- Attachment #2: incr-patch-15c.txt --]
[-- Type: text/plain, Size: 12791 bytes --]
Add --incremental-base option.
2011-04-14 Cary Coutant <ccoutant@google.com>
* gold.cc (queue_initial_tasks): Pass incremental base filename
to Output_file::open_base_file; don't print error message.
* incremental-dump.cc (main): Adjust call to
Output_file::open_for_modification.
* incremental-dump.cc (main): Likewise.
* incremental.cc (Incremental_inputs::report_command_line):
Ignore --incremental-base option when comparing command lines.
Ignore parameter when given as separate argument.
* options.h (class General_options): Add --incremental-base.
* output.cc (Output_file::Output_file):
(Output_file::open_base_file): Add base_name and writable parameters;
read base file into new file; print error message here.
(Output_file::map_no_anonymous): Add writable parameter; adjust all
callers.
* output.h (Output_file::open_for_modification): Rename to...
(Output_file::open_base_file): ...this; add base_name and
writable parameters; adjust all callers.
(Output_file::map_no_anonymous): Add writable parameter; adjust all
callers.
* testsuite/Makefile.am (incremental_test_4): Test
--incremental-base.
* testsuite/Makefile.in: Regenerate.
diff --git a/gold/gold.cc b/gold/gold.cc
index bf5ac89..4f1f871 100644
--- a/gold/gold.cc
+++ b/gold/gold.cc
@@ -204,11 +204,7 @@ queue_initial_tasks(const General_options& options,
if (parameters->incremental_update())
{
Output_file* of = new Output_file(options.output_file_name());
- if (!of->open_for_modification())
- gold_info(_("incremental update not possible: "
- "cannot open %s"),
- options.output_file_name());
- else
+ if (of->open_base_file(options.incremental_base(), true))
{
ibase = open_incremental_binary(of);
if (ibase != NULL
diff --git a/gold/incremental-dump.cc b/gold/incremental-dump.cc
index 1887db8..a727eb5 100644
--- a/gold/incremental-dump.cc
+++ b/gold/incremental-dump.cc
@@ -449,10 +449,10 @@ main(int argc, char** argv)
Output_file* file = new Output_file(filename);
- bool t = file->open_for_modification();
+ bool t = file->open_base_file(NULL, false);
if (!t)
{
- fprintf(stderr, "%s: open_for_modification(%s): %s\n", argv[0], filename,
+ fprintf(stderr, "%s: open_base_file(%s): %s\n", argv[0], filename,
strerror(errno));
return 1;
}
diff --git a/gold/incremental.cc b/gold/incremental.cc
index 0a89940..a27afee 100644
--- a/gold/incremental.cc
+++ b/gold/incremental.cc
@@ -861,8 +861,17 @@ Incremental_inputs::report_command_line(int argc, const char* const* argv)
|| strcmp(argv[i], "--incremental-changed") == 0
|| strcmp(argv[i], "--incremental-unchanged") == 0
|| strcmp(argv[i], "--incremental-unknown") == 0
+ || is_prefix_of("--incremental-base=", argv[i])
|| is_prefix_of("--debug=", argv[i]))
continue;
+ if (strcmp(argv[i], "--incremental-base") == 0
+ || strcmp(argv[i], "--debug") == 0)
+ {
+ // When these options are used without the '=', skip the
+ // following parameter as well.
+ ++i;
+ continue;
+ }
args.append(" '");
// Now append argv[i], but with all single-quotes escaped
diff --git a/gold/options.h b/gold/options.h
index 3949690..e272ae7 100644
--- a/gold/options.h
+++ b/gold/options.h
@@ -790,6 +790,11 @@ class General_options
DEFINE_special(incremental_update, options::TWO_DASHES, '\0',
N_("Do an incremental link; exit if not possible"), NULL);
+ DEFINE_string(incremental_base, options::TWO_DASHES, '\0', NULL,
+ N_("Set base file for incremental linking"
+ " (default is output file)"),
+ N_("FILE"));
+
DEFINE_special(incremental_changed, options::TWO_DASHES, '\0',
N_("Assume files changed"), NULL);
diff --git a/gold/output.cc b/gold/output.cc
index 8354da9..f26421d 100644
--- a/gold/output.cc
+++ b/gold/output.cc
@@ -4492,31 +4492,77 @@ Output_file::Output_file(const char* name)
}
// Try to open an existing file. Returns false if the file doesn't
-// exist, has a size of 0 or can't be mmapped.
+// exist, has a size of 0 or can't be mmapped. If BASE_NAME is not
+// NULL, open that file as the base for incremental linking, and
+// copy its contents to the new output file. This routine can
+// be called for incremental updates, in which case WRITABLE should
+// be true, or by the incremental-dump utility, in which case
+// WRITABLE should be false.
bool
-Output_file::open_for_modification()
+Output_file::open_base_file(const char* base_name, bool writable)
{
// The name "-" means "stdout".
if (strcmp(this->name_, "-") == 0)
return false;
+ bool use_base_file = base_name != NULL;
+ if (!use_base_file)
+ base_name = this->name_;
+ else if (strcmp(base_name, this->name_) == 0)
+ gold_fatal(_("%s: incremental base and output file name are the same"),
+ base_name);
+
// 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;
+ if (::stat(base_name, &s) != 0)
+ {
+ gold_info(_("%s: stat: %s"), base_name, strerror(errno));
+ return false;
+ }
+ if (s.st_size == 0)
+ {
+ gold_info(_("%s: incremental base file is empty"), base_name);
+ return false;
+ }
+
+ // If we're using a base file, we want to open it read-only.
+ if (use_base_file)
+ writable = false;
- int o = open_descriptor(-1, this->name_, O_RDWR, 0);
+ int oflags = writable ? O_RDWR : O_RDONLY;
+ int o = open_descriptor(-1, base_name, oflags, 0);
if (o < 0)
- gold_fatal(_("%s: open: %s"), this->name_, strerror(errno));
+ {
+ gold_info(_("%s: open: %s"), base_name, strerror(errno));
+ return false;
+ }
+
+ // If the base file and the output file are different, open a
+ // new output file and read the contents from the base file into
+ // the newly-mapped region.
+ if (use_base_file)
+ {
+ this->open(s.st_size);
+ ssize_t len = ::read(o, this->base_, s.st_size);
+ if (len < 0)
+ {
+ gold_info(_("%s: read failed: %s"), base_name, strerror(errno));
+ return false;
+ }
+ if (len < s.st_size)
+ {
+ gold_info(_("%s: file too short"), base_name);
+ return false;
+ }
+ ::close(o);
+ return true;
+ }
+
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())
+ if (!this->map_no_anonymous(writable))
{
release_descriptor(o, true);
this->o_ = -1;
@@ -4605,7 +4651,7 @@ Output_file::resize(off_t file_size)
{
this->unmap();
this->file_size_ = file_size;
- if (!this->map_no_anonymous())
+ if (!this->map_no_anonymous(true))
gold_fatal(_("%s: mmap: %s"), this->name_, strerror(errno));
}
}
@@ -4628,9 +4674,10 @@ Output_file::map_anonymous()
}
// Map the file into memory. Return whether the mapping succeeded.
+// If WRITABLE is true, map with write access.
bool
-Output_file::map_no_anonymous()
+Output_file::map_no_anonymous(bool writable)
{
const int o = this->o_;
@@ -4652,12 +4699,14 @@ Output_file::map_no_anonymous()
// 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)
+ if (writable && ::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);
+ int prot = PROT_READ;
+ if (writable)
+ prot |= PROT_WRITE;
+ base = ::mmap(NULL, this->file_size_, prot, 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
@@ -4675,7 +4724,7 @@ Output_file::map_no_anonymous()
void
Output_file::map()
{
- if (this->map_no_anonymous())
+ if (this->map_no_anonymous(true))
return;
// The mmap call might fail because of file system issues: the file
diff --git a/gold/output.h b/gold/output.h
index 6b9b589..f8693c5 100644
--- a/gold/output.h
+++ b/gold/output.h
@@ -4188,9 +4188,10 @@ class Output_file
// 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.
+ // thread-unsafe. If BASE_NAME is not NULL, use the contents of
+ // that file as the base for incremental linking.
bool
- open_for_modification();
+ open_base_file(const char* base_name, bool writable);
// Open the output file. FILE_SIZE is the final size of the file.
// If the file already exists, it is deleted/truncated. This method
@@ -4275,7 +4276,7 @@ class Output_file
// Map the file into memory.
bool
- map_no_anonymous();
+ map_no_anonymous(bool);
// Unmap the file from memory (and flush to disk buffers).
void
diff --git a/gold/testsuite/Makefile.am b/gold/testsuite/Makefile.am
index 90860f1..821a9bb 100644
--- a/gold/testsuite/Makefile.am
+++ b/gold/testsuite/Makefile.am
@@ -1840,13 +1840,15 @@ incremental_test_3: two_file_test_1.o two_file_test_1b_v1.o two_file_test_1b.o \
$(CXXLINK) -Wl,--incremental-update -Bgcctestdir/ two_file_test_1.o two_file_test_tmp.o two_file_test_2.o two_file_test_main.o
check_PROGRAMS += incremental_test_4
+MOSTLYCLEANFILES += incremental_test_4.base
incremental_test_4: two_file_test_1.o two_file_test_1b.o two_file_test_2_v1.o \
two_file_test_2.o two_file_test_main.o gcctestdir/ld
cp -f two_file_test_2_v1.o two_file_test_tmp.o
$(CXXLINK) -Wl,--incremental-full -Bgcctestdir/ two_file_test_1.o two_file_test_1b.o two_file_test_tmp.o two_file_test_main.o
+ mv -f incremental_test_4 incremental_test_4.base
@sleep 1
cp -f two_file_test_2.o two_file_test_tmp.o
- $(CXXLINK) -Wl,--incremental-update -Bgcctestdir/ two_file_test_1.o two_file_test_1b.o two_file_test_tmp.o two_file_test_main.o
+ $(CXXLINK) -Wl,--incremental-update,--incremental-base=incremental_test_4.base -Bgcctestdir/ two_file_test_1.o two_file_test_1b.o two_file_test_tmp.o two_file_test_main.o
endif DEFAULT_TARGET_X86_64
diff --git a/gold/testsuite/Makefile.in b/gold/testsuite/Makefile.in
index 537d952..fdf5e8f 100644
--- a/gold/testsuite/Makefile.in
+++ b/gold/testsuite/Makefile.in
@@ -436,7 +436,8 @@ check_PROGRAMS = $(am__EXEEXT_1) $(am__EXEEXT_2) $(am__EXEEXT_3) \
@DEFAULT_TARGET_X86_64_TRUE@@GCC_TRUE@@NATIVE_LINKER_TRUE@am__append_35 = incremental_test_2 \
@DEFAULT_TARGET_X86_64_TRUE@@GCC_TRUE@@NATIVE_LINKER_TRUE@ incremental_test_3 \
@DEFAULT_TARGET_X86_64_TRUE@@GCC_TRUE@@NATIVE_LINKER_TRUE@ incremental_test_4
-@DEFAULT_TARGET_X86_64_TRUE@@GCC_TRUE@@NATIVE_LINKER_TRUE@am__append_36 = two_file_test_tmp.o
+@DEFAULT_TARGET_X86_64_TRUE@@GCC_TRUE@@NATIVE_LINKER_TRUE@am__append_36 = two_file_test_tmp.o \
+@DEFAULT_TARGET_X86_64_TRUE@@GCC_TRUE@@NATIVE_LINKER_TRUE@ incremental_test_4.base
# These tests work with native and cross linkers.
@@ -4600,9 +4601,10 @@ uninstall-am:
@DEFAULT_TARGET_X86_64_TRUE@@GCC_TRUE@@NATIVE_LINKER_TRUE@ two_file_test_2.o two_file_test_main.o gcctestdir/ld
@DEFAULT_TARGET_X86_64_TRUE@@GCC_TRUE@@NATIVE_LINKER_TRUE@ cp -f two_file_test_2_v1.o two_file_test_tmp.o
@DEFAULT_TARGET_X86_64_TRUE@@GCC_TRUE@@NATIVE_LINKER_TRUE@ $(CXXLINK) -Wl,--incremental-full -Bgcctestdir/ two_file_test_1.o two_file_test_1b.o two_file_test_tmp.o two_file_test_main.o
+@DEFAULT_TARGET_X86_64_TRUE@@GCC_TRUE@@NATIVE_LINKER_TRUE@ mv -f incremental_test_4 incremental_test_4.base
@DEFAULT_TARGET_X86_64_TRUE@@GCC_TRUE@@NATIVE_LINKER_TRUE@ @sleep 1
@DEFAULT_TARGET_X86_64_TRUE@@GCC_TRUE@@NATIVE_LINKER_TRUE@ cp -f two_file_test_2.o two_file_test_tmp.o
-@DEFAULT_TARGET_X86_64_TRUE@@GCC_TRUE@@NATIVE_LINKER_TRUE@ $(CXXLINK) -Wl,--incremental-update -Bgcctestdir/ two_file_test_1.o two_file_test_1b.o two_file_test_tmp.o two_file_test_main.o
+@DEFAULT_TARGET_X86_64_TRUE@@GCC_TRUE@@NATIVE_LINKER_TRUE@ $(CXXLINK) -Wl,--incremental-update,--incremental-base=incremental_test_4.base -Bgcctestdir/ two_file_test_1.o two_file_test_1b.o two_file_test_tmp.o two_file_test_main.o
@NATIVE_OR_CROSS_LINKER_TRUE@script_test_10.o: script_test_10.s
@NATIVE_OR_CROSS_LINKER_TRUE@ $(TEST_AS) -o $@ $<
@NATIVE_OR_CROSS_LINKER_TRUE@script_test_10: $(srcdir)/script_test_10.t script_test_10.o gcctestdir/ld
next prev parent reply other threads:[~2011-05-24 22:54 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-04-04 23:30 Cary Coutant
2011-04-25 20:33 ` Cary Coutant
2011-05-24 14:59 ` Ian Lance Taylor
2011-05-24 22:20 ` Cary Coutant
2011-05-24 22:54 ` Cary Coutant [this message]
2011-05-24 23:08 ` Ian Lance Taylor
2011-05-24 23:33 ` Cary Coutant
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BANLkTinv8Dr2rcPGXbv-1hXLwV2khGdPnA@mail.gmail.com \
--to=ccoutant@google.com \
--cc=binutils@sourceware.org \
--cc=iant@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).