[llvm-commits] [PATCH 7/10] Initial TOC support for PowerPC64 object creation

[Adhemerval Zanella]

Hi Rafael,


On 10/05/2012 10:58 AM, Rafael Espíndola wrote:
> +void MCStreamer::EmitTCEntry(const MCExpr *Expr) {
> +  errs() << "Not implemented yet\n";
> +  abort();
> +}
>
> The error message is not correct, this will never be implemented. If
> this cannot be reached by a user running llvm-mc, we can probably use
> a llvm_unreachable.

Ok, changing.

>
> +    virtual unsigned GetRelocTypeInner(const MCValue &Target,
> +                                       const MCFixup &Fixup,
> +                                       bool IsPCRel) const;
>
> Naming convention: please use lowercase function names for new code.
>
> +  bool EmitThisSym = false;
> +
>
> Instead of the following switch you can just say
>
> bool EmitThisSym = RelocType != ELF::R_PPC64_TOC.
>
> no?
>
> +  if (EmitThisSym && !Symbol.isTemporary())
>
> Why the Symbol.isTemporary()?

Indeed the 'RelocType != ELF::R_PPC64_TOC' is simpler, I used the switch mainly
to prepare in case of extra logic in possible future patches. I'll use the
your suggestion though.

And without the 'Symbol.isTemporary()' both the TOC relocations for symbols and 
the TOC field in the ODP will be against temporary symbols, instead of TOC and
.text segments. For instance, on the test 'ppc64-relocs-01.ll' the generated
relocation without the test would be:

Relocation section '.rela.text' at offset 0x600 contains 2 entries:
  Offset          Info           Type           Sym. Value    Sym. Name + Addend
000000000002  00040000003f R_PPC64_TOC16_DS  0000000000000000 .LC1 + 0

Relocation section '.rela.opd' at offset 0x630 contains 4 entries:
  Offset          Info           Type           Sym. Value    Sym. Name + Addend
000000000000  000200000026 R_PPC64_ADDR64    0000000000000000 .L.access_int64 + 0
000000000018  000300000026 R_PPC64_ADDR64    0000000000000028 .L.test_branch24 + 0


Where the correct way should be against the segments:


Relocation section '.rela.text' at offset 0x598 contains 2 entries:
  Offset          Info           Type           Sym. Value    Sym. Name + Addend
000000000002  00060000003f R_PPC64_TOC16_DS  0000000000000000 .toc + 0

Relocation section '.rela.opd' at offset 0x5c8 contains 4 entries:
  Offset          Info           Type           Sym. Value    Sym. Name + Addend
000000000000  000200000026 R_PPC64_ADDR64    0000000000000000 .text + 0
000000000018  000200000026 R_PPC64_ADDR64    0000000000000000 .text + 28

>
>
> I think the main question is why are you using ExplicitRelSym?
> Wouldn't it be better to add a new hook with a default implementation
> that returns &ASymbol? That way you can avoid all the
> checkRelocationUndefined plumbing.

I was in doubt which would be the best approach. I decided to use this one
to in fact avoid another hook, but if you think it is better I can modify
the patch to use it.

Thanks for the review.