Re: [gold patch] Incremental 8/18: Initial support for incremental update

[Cary Coutant <ccoutant at google dot com>]

>> + ? ? ?if (options.has_plugins())
>> + ? ? gold_error(_("incremental linking is incompatible with --plugin"));
>
> Will this be possible at some future date?

It could be, but I haven't thought through all the implications. With
plugins, it's hard to guarantee that all the restrictions we impose
are met. What should we do with an IR file marked
--incremental-unchanged? The LTO plugin would probably still prefer to
claim it, and would have to if it's a pure IR file. The use cases for
LTO and for incremental linking seem opposed to each other -- LTO
wants to see and recompile as much as possible, while incremental
linking wants to see minimal changes.

>> + ? ? ?// Incremental update link. ?Process the list of input files
>> + ? ? ?// stored in the base file, and queue a task for each file:
>> + ? ? ?// a Read_symbols task for a changed file, and an Add_symbols task
>> + ? ? ?// for an unchanged file. ?We need to mark all the space used by
>> + ? ? ?// unchanged files before we can start any tasks running.
>> + ? ? ?std::list<Task*> tasks;
>
> Add tasks.reserve(input_file_count);

OK.

>> +// Process an incremental input file: if it is unchanged from the previous
>> +// link, return a task to add its symbols from the base file's incremental
>> +// info; if it has changed, return a normal Read_symbols task. ?We create a
>> +// task for every input file, if only to report the file for rebuilding the
>> +// incremental info.
>> +
>> +Task*
>> +process_incremental_input(Incremental_binary* ibase,
>> + ? ? ? ? ? ? ? ? ? ? ? unsigned int input_file_index,
>> + ? ? ? ? ? ? ? ? ? ? ? Input_objects* input_objects,
>> + ? ? ? ? ? ? ? ? ? ? ? Symbol_table* symtab,
>> + ? ? ? ? ? ? ? ? ? ? ? Layout* layout,
>> + ? ? ? ? ? ? ? ? ? ? ? Dirsearch* search_path,
>> + ? ? ? ? ? ? ? ? ? ? ? Mapfile* mapfile,
>> + ? ? ? ? ? ? ? ? ? ? ? Task_token* this_blocker,
>> + ? ? ? ? ? ? ? ? ? ? ? Task_token* next_blocker)
>> +{
>
> This function should be static.

OK.

>> + ?gold_debug(DEBUG_INCREMENTAL, ? "Incremental object: %s, type %d",
>> + ? ? ? ? ?input_reader->filename(), input_type);
>
> s/ ? / /

OK.

>> + ?if (input_type == INCREMENTAL_INPUT_ARCHIVE)
>> + ? ?{
>> + ? ? ?Incremental_library* lib = ibase->get_library(input_file_index);
>> + ? ? ?gold_assert(lib != NULL);
>> + ? ? ?if (lib->filename() == "<group>"
>> + ? ? ? || !ibase->file_has_changed(input_file_index))
>
> Hmmm. ?For extreme cleanliness, suppose we change "<group>" to "/group/"
> or some other name containing a '/', so that we know for sure that there
> is no input file with that name.

Good idea. I was bothered by that myself, and intended to come back to
this with a cleaner solution. I'll use "/group/" for now.

>> @@ -396,7 +586,8 @@ queue_middle_tasks(const General_options& options,
>> ? ?// generate an empty file. ?Existing builds depend on being able to
>> ? ?// pass an empty archive to the linker and get an empty object file
>> ? ?// out. ?In order to do this we need to use a default target.
>> - ?if (input_objects->number_of_input_objects() == 0)
>> + ?if (input_objects->number_of_input_objects() == 0
>> + ? ? ?&& layout->incremental_base() == NULL)
>> ? ? ?parameters_force_valid_target();
>
> Why this change?

If there are no changed files at all, we want to base the target on
the base file rather than the arbitrary target that
parameters_force_valid_target() would choose.

Hmmm, if there are no changed files, we have no work to do, and we
should have just exited the link by this point. I'll take a closer
look at this.

>> +// Allocate an incremental object of the appropriate size and endianness.
>> +Object*
>> +make_sized_incremental_object(
>> + ? ?Incremental_binary* base,
>> + ? ?unsigned int input_file_index,
>> + ? ?Incremental_input_type input_type,
>> + ? ?const Incremental_binary::Input_reader* input_reader);
>
> Add "extern".

OK.

>> @@ -869,7 +1086,9 @@ Layout::layout_eh_frame(Sized_relobj<size, big_endian>* object,
>> ? ? ? ?this->eh_frame_section_ = os;
>> ? ? ? ?this->eh_frame_data_ = new Eh_frame();
>>
>> - ? ? ?if (parameters->options().eh_frame_hdr())
>> + ? ? ?// For incremental linking, we do not optimize .eh_frame sections
>> + ? ? ?// or create a .eh_frame_hdr section.
>> + ? ? ?if (parameters->options().eh_frame_hdr() && !parameters->incremental())
>> ? ? ? {
>> ? ? ? ? Output_section* hdr_os =
>> ? ? ? ? ? this->choose_output_section(NULL, ".eh_frame_hdr",
>> @@ -901,14 +1120,15 @@ Layout::layout_eh_frame(Sized_relobj<size, big_endian>* object,
>>
>> ? ?gold_assert(this->eh_frame_section_ == os);
>>
>> - ?if (this->eh_frame_data_->add_ehframe_input_section(object,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? symbols,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? symbols_size,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? symbol_names,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? symbol_names_size,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? shndx,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? reloc_shndx,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? reloc_type))
>> + ?if (!parameters->incremental()
>> + ? ? ?&& this->eh_frame_data_->add_ehframe_input_section(object,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?symbols,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?symbols_size,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?symbol_names,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?symbol_names_size,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?shndx,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?reloc_shndx,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?reloc_type))
>> ? ? ?{
>> ? ? ? ?os->update_flags_for_input_section(shdr.get_sh_flags());
>>
>
> Where do these things happen when doing an incremental link?

I haven't yet implemented the .eh_frame_hdr section for incremental
linking -- my plan is to rebuild it from scratch rather than try to
update it in place, but that means it'll need to rescan all the
.eh_frame sections from the unchanged files as well as changed files.

> This is OK with those changes.

Thanks. I'll post an updated patch when I resolve the
parameters_force_valid_target() issue. Or, if it's OK with you, I can
commit this patch with the changes you've noted and follow up later
with another patch to improve the no-changed-files case.

-cary