Re: [patch] Ignore non relobj files in gc

[Sriraman Tallam <tmsriram at google dot com>]

On Mon, Apr 27, 2015 at 1:17 PM, Cary Coutant <ccoutant@gmail.com> wrote:
>> If a relocation points to a dynamic object we can ignore it, since we
>> cannot gc an already existing .so file.
>>
>> The attached patch produces an identical chromium binary, but does so
>> a bit faster (see the attached perf logs).
>
> I'd think the one-line patch below accomplishes the same result with
> much less disruption:
>
> @@ -340,7 +341,7 @@ gc_process_relocs(
>                                                           src_obj));
>             }
>
> -          if (gsym->source() != Symbol::FROM_OBJECT)
> +          if (dst_obj == NULL || dst_obj->is_dynamic())
>              continue;
>            if (!is_ordinary)
>              continue;
>
>> Using Relobj also helps me in my patch for gcing parts of SHF_MERGE sections.
>
> If that's the case, let's move those changes to a separate patch.
> Nevertheless, I think it would be cleaner to change Section_id to use
> a Relobj* directly. That has some cascading effects, but doesn't
> affect quite as much target-independent code, and we really never need
> a Section_id that refers to a non-relocatable object. The attached
> patch does that.
>
> Sri, would you mind taking a look to make sure I haven't done anything
> in the attached patch that might break --icf? Here's the only place
> where we might have created a Section_id with a pointer to a Dynobj,
> but I don't think that makes sense:
>
> @@ -324,8 +326,11 @@ gc_process_relocs(
>            if (is_icf_tracked)
>              {
>               Address symvalue = dst_off - addend;
> -              if (is_ordinary && gsym->source() == Symbol::FROM_OBJECT)
> -               (*secvec).push_back(Section_id(dst_obj, dst_indx));
> +              if (is_ordinary
> +                 && gsym->source() == Symbol::FROM_OBJECT
> +                 && !dst_obj->is_dynamic())
> +               (*secvec).push_back(Section_id(static_cast<Relobj*>(dst_obj),
> +                                              dst_indx));
>               else
>                  (*secvec).push_back(Section_id(NULL, 0));
>                (*symvec).push_back(gsym);

You have two sections that are mostly identical except a call
instruction that is calling two different functions in two different
shared objects, you are not tracking that anymore. We are tracking the
symbol name in the following line by pushing gsym onto symvec, so this
seems fine.  If there is ever a case where a symbol corresponding to a
relocation to a dynamic object has a NULL name, we could be in trouble
but I cannot think of anything like that.

Thanks
Sri

>
> Perhaps it does make sense here to track a relocation pointing into a
> dynamic object, but in this case, the static_cast might still be OK,
> since I think at this point, we're only using the object pointer as an
> opaque ID for the object.
>
> -cary