[PATCH] D34781: Introduce a MCReloc class

Rafael Avila de Espindola via llvm-commits llvm-commits at lists.llvm.org
Thu Jul 20 08:26:54 PDT 2017


Gerolf Hoflehner <ghoflehner at apple.com> writes:
>>> What relocation should we map FK_Data_1 to? What if IsPCRel was set to true?
>>> What is the addend? FixedValue or Target.getConstant()?
> There is only one less use of Target.getConstant() (an assigned to
> Value = Target.getConstant() is only in the old code). I had expected
> more differences and also explaining more comments about FixedValue vs
> Target.getConstant(). Or maybe it wasn’t a big deal - then why mention
> it for justifying the effort?

The patch intentionally changes as little as possible. It introduces
things like "Target = Fixup" to keep the patch small. The idea is to
remove that in a followup patch.

>>> What is the meaning of "foo - bar at got"?
>>> There is *a lot* of duplicated code is the various obj writers.
> The code size looks very similar before and after the patch. Maybe the
> follow up patch is supposed to show the merits? When I read “a lot” I
> thought a number of large almost equivalent code sniped in
> then-else-cases would disappear.

Yes. See the previous emails in the thread. The idea of this patch is to
add infrastructure with the smallest possible change.

>> I am not sure I understand your comment. This is what this revision
>> starts to fix.
>
> I simply sense a gap between the goals you set out and the actual code. Perhaps I just need to see more of your patches to better appreciate what you are after.

Take a look at the one combining the pcrel handling of coff, elf and
webasm.

>> For example, there is now only one addend: Reloc.getConstant(). It is
>> also impossible now to represent "foo - bar at got" since we store a symbol
>> instead of a symbolref for b.
> Right, I see. I thought that ( foo -bar at got) had caused a bug, but I only found two places in the old code that had explicit checks to prevent it. So your patch rendered the checks redundant thankfully.

The problem are the places that *don't* check for it :-)

Cheers,
Rafael


More information about the llvm-commits mailing list