Re: [gold] Error out when there is an access beyond the end of the merged section

[Alexander Ivchenko <aivchenk at gmail dot com>]

Thanks, Cary :)

Okey, then I'll follow your suggestions and write the comment that we
expect "false" only in case (3).

Will update the patch after your commit.

--Alexander

2014-08-05 21:13 GMT+04:00 Cary Coutant <ccoutant@google.com>:
>> I think you are right that it would be better to give an error from
>> Merged_symbol_value::value_from_output_section, but I think that just
>> replacing gold_assert is wrong:
>>
>> Here is how "found" is initialized:
>> bool found = object->merge_map()->get_output_offset(NULL, input_shndx,
>>                                                     input_offset,
>>                                                     &output_offset,
>>                                                     object);
>>
>> And Object_merge_map::get_output_offset can return false in three cases:
>>
>> 1)
>>   if (map == NULL
>>       || (merge_map != NULL && map->merge_map != merge_map))
>>     return false;
>>
>> 2)
>>   std::vector<Input_merge_entry>::const_iterator p =
>>     std::lower_bound(map->entries.begin(), map->entries.end(),
>>                      entry, Input_merge_compare());
>>   if (p == map->entries.end() || p->input_offset > input_offset)
>>     {
>>       if (p == map->entries.begin())
>>         return false;
>>       --p;
>>       gold_assert(p->input_offset <= input_offset);
>>     }
>>
>> and
>>
>> 3)
>>   if (input_offset - p->input_offset
>>       >= static_cast<section_offset_type>(p->length))
>>        return false;
>>
>> AFAIU, for cases 1) and 2) the gold_assert in
>> Merged_symbol_value::value_from_output_section should really be hit,
>> and only 3) should give us an error with "access beyond end of merged
>> section".
>>
>> So, I think, we should move gold_assert to 1), 2) in order to catch
>> that situation.
>
> I don't think that Object_merge_map::get_output_offset should assert
> on these conditions -- there are other callers that may legitimately
> expect a false return for either of these reasons. In the case of
> Merged_symbol_value::value_from_output_section(), I think that we've
> gone long enough without ever hitting that assert for either of
> reasons (1) or (2) that it's safe to assume that a false return is due
> to reason (3).
>
>
>> There is an unfortunate necessity of const_cast, if we want to print
>> out the name of the section.
>> What do you think?
>
> We really should be able to call section_name() without a const_cast.
> I'll commit a patch to fix that shortly, then you can update this
> patch.
>
> Thanks, and sorry for the delay. I was on vacation, and am catching up.
>
> -cary