[PATCH] D34781: Introduce a MCReloc class

[Rafael Avila de Espindola via llvm-commits]

Gerolf Hoflehner via Phabricator <reviews at reviews.llvm.org> writes:

> Gerolf added reviewers: ab, Gerolf.
> Gerolf added a comment.
>
> Just a first superficial review. I haven't thought about the underlying concept itself yet.
>
> -Gerolf
>
>
>
> ================
> Comment at: include/llvm/MC/MCFixup.h:156
> +
> +  const MCExpr *getValue() const { return Expr; }
> +  void setValue(const MCExpr* Val) { Expr = Val; }
> ----------------
> getExpr, setExpr?

Probably. I named it getValue/setValue to keep this patch readable as
that is the name in MCFixup. Is it OK if I change the name in a followup
patch?

> ================
> Comment at: include/llvm/MC/MCFixup.h:159
> +
> +  bool isAbsolute() const { return !RefA && !SymB; }
> +  MCSymbolRefExpr::VariantKind getAccessVariant() const {
> ----------------
> Is there a better name? Absolute what?

The computed value is absolute. It doesn't depend on any symbols. I
guess isConstant would be a better name, but it probably a good idea to
change it in a followup commit.

>
> ================
> Comment at: lib/MC/ELFObjectWriter.cpp:633
>    const MCSectionELF &FixupSection = cast<MCSectionELF>(*Fragment->getParent());
>    uint64_t C = Target.getConstant();
>    uint64_t FixupOffset = Layout.getFragmentOffset(Fragment) + Fixup.getOffset();
> ----------------
> Same as FixedValue?

No, FixupOffset is the location that is being relocated. The Value
refers to what it is being relocated to.


> ================
> Comment at: lib/MC/MCAssembler.cpp:202
> +  uint64_t &Value = Reloc.getConstant();
>    Value = 0;
>    if (!Expr->evaluateAsRelocatable(Target, &Layout, &Fixup)) {
> ----------------
> Reloc.setConstant(0)?

So, this is one of the hacks I included in this patch to reduce it. The
objective is to remove all of them in a followup, but if I change all
the code to use Reloc.foo() for various foo now the patch is almost
unreadable. As is it is still possible to notice the only places where
something interesting is changing:

* Propagation of the IsPCRel flag
* Where we compute the final value for scattered relocations.


> ================
> Comment at: lib/MC/MCAssembler.cpp:249
>  
> +  // Let the backend check whether we need a relocation.
> +  Backend.processFixupValue(*this, Fixup, Target, IsResolved);
> ----------------
> The comments are at least confusing. From the check below it can be derived that IsResolved has the information where or not a relocation is needed already. Then there is no need to check for it.

IsResolved is passed by reference. I will update the comment to be
explicit.

Cheers,
Rafael