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.
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*.