* [gold patch] Incremental 15/18: Add --incremental-base option. @ 2011-04-04 23:30 Cary Coutant 2011-04-25 20:33 ` Cary Coutant 0 siblings, 1 reply; 7+ messages in thread From: Cary Coutant @ 2011-04-04 23:30 UTC (permalink / raw) To: Ian Lance Taylor, Binutils [-- Attachment #1: Type: text/plain, Size: 957 bytes --] This patch adds a --incremental-base option, so that the base file for the incremental link can be different from the output file. For simplicity, I simply copy the base file contents to a new output file, then carry on with a normal incremental link. -cary * gold.cc (queue_initial_tasks): Pass incremental base filename to Output_file::open_for_modification. * incremental-dump.cc (main): Adjust call to Output_file::open_for_modification. * incremental.cc (Incremental_inputs::report_command_line): Ignore --incremental-base option when comparing command lines. * options.h (class General_options): Add --incremental-base. * output.cc (Output_file::Output_file): (Output_file::open_for_modification): Add base_name parameter; copy base file to new file. * output.h (Output_file::open_for_modification): Add base_name parameter. * testsuite/Makefile.am (incremental_test_4): Test --incremental-base. * testsuite/Makefile.in: Regenerate. [-- Attachment #2: incr-patch-15.txt --] [-- Type: text/plain, Size: 7022 bytes --] Add --incremental-base option. 2011-04-04 Cary Coutant <ccoutant@google.com> * gold.cc (queue_initial_tasks): Pass incremental base filename to Output_file::open_for_modification. * incremental-dump.cc (main): Adjust call to Output_file::open_for_modification. * incremental.cc (Incremental_inputs::report_command_line): Ignore --incremental-base option when comparing command lines. * options.h (class General_options): Add --incremental-base. * output.cc (Output_file::Output_file): (Output_file::open_for_modification): Add base_name parameter; copy base file to new file. * output.h (Output_file::open_for_modification): Add base_name parameter. * testsuite/Makefile.am (incremental_test_4): Test --incremental-base. * testsuite/Makefile.in: Regenerate. diff --git a/gold/gold.cc b/gold/gold.cc index 84008ca..85b2907 100644 --- a/gold/gold.cc +++ b/gold/gold.cc @@ -204,7 +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()) + if (!of->open_for_modification(options.incremental_base())) gold_info(_("incremental update not possible: " "cannot open %s"), options.output_file_name()); diff --git a/gold/incremental-dump.cc b/gold/incremental-dump.cc index 1887db8..1a3cce9 100644 --- a/gold/incremental-dump.cc +++ b/gold/incremental-dump.cc @@ -449,7 +449,7 @@ main(int argc, char** argv) Output_file* file = new Output_file(filename); - bool t = file->open_for_modification(); + bool t = file->open_for_modification(NULL); if (!t) { fprintf(stderr, "%s: open_for_modification(%s): %s\n", argv[0], filename, diff --git a/gold/incremental.cc b/gold/incremental.cc index 824608b..817000e 100644 --- a/gold/incremental.cc +++ b/gold/incremental.cc @@ -861,6 +861,7 @@ 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; 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 f66d471..6af06c5 100644 --- a/gold/output.cc +++ b/gold/output.cc @@ -4433,30 +4433,36 @@ 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. bool -Output_file::open_for_modification() +Output_file::open_for_modification(const char* base_name) { // 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) + if (::stat(base_name, &s) != 0 || s.st_size == 0) return false; - int o = open_descriptor(-1, this->name_, O_RDWR, 0); + int o = open_descriptor(-1, base_name, O_RDWR, 0); if (o < 0) - gold_fatal(_("%s: open: %s"), this->name_, strerror(errno)); + gold_fatal(_("%s: open: %s"), base_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); @@ -4465,6 +4471,17 @@ Output_file::open_for_modification() return false; } + // If the base file and the output file are different, open a + // new output file and copy the contents from the base file. + if (use_base_file) + { + unsigned char* base = this->base_; + this->open(s.st_size); + memcpy(this->base_, base, s.st_size); + ::munmap(base, s.st_size); + ::close(o); + } + return true; } diff --git a/gold/output.h b/gold/output.h index 54ce4f9..dedf898 100644 --- a/gold/output.h +++ b/gold/output.h @@ -4231,9 +4231,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_for_modification(const char* base_name); // Open the output file. FILE_SIZE is the final size of the file. // If the file already exists, it is deleted/truncated. This method 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [gold patch] Incremental 15/18: Add --incremental-base option. 2011-04-04 23:30 [gold patch] Incremental 15/18: Add --incremental-base option Cary Coutant @ 2011-04-25 20:33 ` Cary Coutant 2011-05-24 14:59 ` Ian Lance Taylor 0 siblings, 1 reply; 7+ messages in thread From: Cary Coutant @ 2011-04-25 20:33 UTC (permalink / raw) To: Ian Lance Taylor, Binutils [-- Attachment #1: Type: text/plain, Size: 1451 bytes --] > This patch adds a --incremental-base option, so that the base file for > the incremental link can be different from the output file. For > simplicity, I simply copy the base file contents to a new output file, > then carry on with a normal incremental link. This patch tried to open the base file for writing, and some of the error messages would give the wrong file name. Here's a replacement patch... -cary * 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. * 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; copy base file to 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-15b.txt --] [-- Type: text/plain, Size: 12297 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. * 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; copy base file to 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 4066e54..eb5bf97 100644 --- a/gold/incremental.cc +++ b/gold/incremental.cc @@ -861,6 +861,7 @@ 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; 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 f66d471..4870011 100644 --- a/gold/output.cc +++ b/gold/output.cc @@ -4433,31 +4433,56 @@ 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; + } - int o = open_descriptor(-1, this->name_, O_RDWR, 0); + // If we're using a base file, we want to open it read-only. + if (use_base_file) + writable = false; + + 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; + } + 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; @@ -4465,6 +4490,17 @@ Output_file::open_for_modification() return false; } + // If the base file and the output file are different, open a + // new output file and copy the contents from the base file. + if (use_base_file) + { + unsigned char* base = this->base_; + this->open(s.st_size); + memcpy(this->base_, base, s.st_size); + ::munmap(base, s.st_size); + ::close(o); + } + return true; } @@ -4546,7 +4582,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)); } } @@ -4569,9 +4605,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_; @@ -4593,12 +4630,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 @@ -4616,7 +4655,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 54ce4f9..3617948 100644 --- a/gold/output.h +++ b/gold/output.h @@ -4231,9 +4231,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 @@ -4318,7 +4319,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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [gold patch] Incremental 15/18: Add --incremental-base option. 2011-04-25 20:33 ` Cary Coutant @ 2011-05-24 14:59 ` Ian Lance Taylor 2011-05-24 22:20 ` Cary Coutant 0 siblings, 1 reply; 7+ messages in thread From: Ian Lance Taylor @ 2011-05-24 14:59 UTC (permalink / raw) To: Cary Coutant; +Cc: Binutils Cary Coutant <ccoutant@google.com> writes: > * 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. > * 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; > copy base file to 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. > @@ -861,6 +861,7 @@ 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; Will this work correctly if the user does ld --incremental-base old-file ? That is, if they pass the argument as a separate argv entry, rather than using =? I guess the same question arises for --debug. > + // If the base file and the output file are different, open a > + // new output file and copy the contents from the base file. > + if (use_base_file) > + { > + unsigned char* base = this->base_; > + this->open(s.st_size); > + memcpy(this->base_, base, s.st_size); > + ::munmap(base, s.st_size); > + ::close(o); > + } 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. Ian ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [gold patch] Incremental 15/18: Add --incremental-base option. 2011-05-24 14:59 ` Ian Lance Taylor @ 2011-05-24 22:20 ` Cary Coutant 2011-05-24 22:54 ` Cary Coutant 0 siblings, 1 reply; 7+ messages in thread From: Cary Coutant @ 2011-05-24 22:20 UTC (permalink / raw) To: Ian Lance Taylor; +Cc: Binutils >> @@ -861,6 +861,7 @@ 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; > > Will this work correctly if the user does > ld --incremental-base old-file > ? That is, if they pass the argument as a separate argv entry, rather > than using =? I guess the same question arises for --debug. No, that's a known limitation. I plan to overhaul the command-line comparison, and was intending to fix that issue then. I'd like to store each argument as a separate string in the string table (where many of the filename arguments probably already exist), and have an array of pointers to the command-line options. That should reduce the storage space required and eliminate the escaping of quotes. Well, it's fairly trivial to go ahead and fix that now. How does the following look, added immediately after the above if statement? 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; } >> + // If the base file and the output file are different, open a >> + // new output file and copy the contents from the base file. >> + if (use_base_file) >> + { >> + unsigned char* base = this->base_; >> + this->open(s.st_size); >> + memcpy(this->base_, base, s.st_size); >> + ::munmap(base, s.st_size); >> + ::close(o); >> + } > > 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. -cary ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [gold patch] Incremental 15/18: Add --incremental-base option. 2011-05-24 22:20 ` Cary Coutant @ 2011-05-24 22:54 ` Cary Coutant 2011-05-24 23:08 ` Ian Lance Taylor 0 siblings, 1 reply; 7+ messages in thread From: Cary Coutant @ 2011-05-24 22:54 UTC (permalink / raw) To: Ian Lance Taylor; +Cc: Binutils [-- 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [gold patch] Incremental 15/18: Add --incremental-base option. 2011-05-24 22:54 ` Cary Coutant @ 2011-05-24 23:08 ` Ian Lance Taylor 2011-05-24 23:33 ` Cary Coutant 0 siblings, 1 reply; 7+ messages in thread From: Ian Lance Taylor @ 2011-05-24 23:08 UTC (permalink / raw) To: Cary Coutant; +Cc: Binutils Cary Coutant <ccoutant@google.com> writes: > 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. This is OK. Thanks. Ian ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [gold patch] Incremental 15/18: Add --incremental-base option. 2011-05-24 23:08 ` Ian Lance Taylor @ 2011-05-24 23:33 ` Cary Coutant 0 siblings, 0 replies; 7+ messages in thread From: Cary Coutant @ 2011-05-24 23:33 UTC (permalink / raw) To: Ian Lance Taylor; +Cc: Binutils >> 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. > > This is OK. Thanks, committed. -cary ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-05-24 23:33 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-04-04 23:30 [gold patch] Incremental 15/18: Add --incremental-base option 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 2011-05-24 23:08 ` Ian Lance Taylor 2011-05-24 23:33 ` Cary Coutant
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).