Page MenuHomePhabricator

Introduce a MCReloc class
AcceptedPublic

Authored by espindola on Jun 28 2017, 2:29 PM.

Details

Summary

Trying to fix a bug in this area I realized just how confusing the code is. The are subtly different information in different variables. For example,

  • What relocation should we map FK_Data_1 to? What if IsPCRel was set to true?
  • What is the addend? FixedValue or Target.getConstant()?
  • What is the meaning of "foo - bar@got"?
  • There is *a lot* of duplicated code is the various obj writers.

This leads to situation where each target ( ARMELFObjectWriter::GetRelocTypeInner for example) has code that looks like

if (IsPCRel) {

big switch

} else {
very similar big switch
}

This patches introduces a MCReloc to represent the result of evaluating a MCFixup. It is a mutable structure, unlike MCFixup. This allows us to update it when we convert a relocation to be PCRel. In this patch that is done for ELF, but I have a follow up to share the code with COFF (and hopefully MachO and WebAsm).

With that we don't need the IsPCRel argument anymore. This should allow big cleanups in a followup patch.

The MCReloc also avoids any confusion as to the value to be created. There is only one constant. It is also impossible to represent broken things like "foo - bar@got".

The one downside is with scattered relocations on MachO that now need to do a bit more work. I think it is well worth it.

In this patch there are a few hacks to reduce its size. I intend to clean that up in a followup patch.

Diff Detail

Event Timeline

rafael created this revision.Jun 28 2017, 2:29 PM
Gerolf added a subscriber: Gerolf.

Just a first superficial review. I haven't thought about the underlying concept itself yet.

-Gerolf

include/llvm/MC/MCFixup.h
156

getExpr, setExpr?

159

Is there a better name? Absolute what?

lib/MC/ELFObjectWriter.cpp
633

Same as FixedValue?

lib/MC/MCAssembler.cpp
202

Reloc.setConstant(0)?

249

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.

Rebased.
Renamed a field.
Clarified a comment.

Rebased to include the shouldForceRelocation cleanup.

Trying to get phab to send email.

emaste added a subscriber: emaste.Jul 4 2017, 1:17 PM

Rebased.
Fixed a bug in Mips's adjustFixupValue

rnk edited edge metadata.Jul 12 2017, 3:06 PM

I read through this, but it's hard to see exactly the simplifications that this will yield. I like the follow-up patch to merge PCRel handling between ELF, COFF, and wasm, though.

include/llvm/MC/MCFixup.h
120

Can you bring along the doxygen comment on MCFixup::Offset? This is typically a small value, it's an offset into the last instruction, not an offset into a fragment or a section.

In general, documenting all these fields would help explain what an MCReloc *is*.

lib/MC/ELFObjectWriter.cpp
630

I take it you plan to rename these things soon, this is just a hack to avoid tons of renames in the diff.

I don't think the deferring of work helps the reviewing, though, because the names don't really make sense right now. An MCReloc is basically a combination of a relocation target and a relocation "location".

Add comments. Rebase.

rnk accepted this revision.Jul 19 2017, 3:04 PM

Looks good

This revision is now accepted and ready to land.Jul 19 2017, 3:04 PM
Gerolf edited edge metadata.Jul 19 2017, 5:03 PM

It is not clear to me if and how the original questions have been answered by this patch yet. Could you elaborate and add comments, please? Much appreciate!

What relocation should we map FK_Data_1 to? What if IsPCRel was set to true?
What is the addend? FixedValue or Target.getConstant()?
What is the meaning of "foo - bar@got"?
There is *a lot* of duplicated code is the various obj writers.

Specifically it doesn't look like the code size changed much.

Ping, Gerolf, are you OK with this?

See https://reviews.llvm.org/D34988 for a cleanup that depends on this. See ARMELFObjectWriter::GetRelocTypeInner for something else that can be cleanup once IsPCRel becomes redundant.

Again, this change is written so as to be as small as possible since it already has to change a lot of places.

Rebased. Ping.

rafael updated this revision to Diff 109453.Aug 2 2017, 4:19 PM

Rebased.

Ping

asb added a subscriber: asb.Aug 9 2017, 6:45 AM
rnk added a comment.Mar 7 2018, 2:38 PM

I take it this is still at the bottom of the dependency tree. Are there any other comments on this? Otherwise it would be good to get this simplification.

espindola commandeered this revision.Mar 14 2018, 3:51 PM
espindola added a reviewer: rafael.
nhaehnle removed a subscriber: nhaehnle.Mar 17 2018, 5:21 AM