[PATCH] D33194: [DWARF] - Cleanup relocations proccessing.

George Rimar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue May 16 02:40:34 PDT 2017


grimar added a comment.

In https://reviews.llvm.org/D33194#755181, @aprantl wrote:

> I'm assuming the checks are removed because they are already done elsewhere?


Let me explain in details my position. Lets take a look more closer what was checked.
There was 2 checks I removed:

1. if (Address + R.Width > SectionSize) { ...
2. if (R.Width > 8)

R.Width field is just a size of a relocation. For example for x86_64 target it can be 4 or 8 usually:

  RelocToApply visitELF_X86_64_64(RelocationRef R, uint64_t Value) {
    int64_t Addend = getELFAddend(R);
    return RelocToApply(Value + Addend, 8);
  }
  RelocToApply visitELF_X86_64_PC32(RelocationRef R, uint64_t Value) {
    int64_t Addend = getELFAddend(R);
    uint64_t Address = R.getOffset();
    return RelocToApply(Value + Addend - Address, 4);
  }

That means that first check is always true until implementation of relocation visitor is correct and
until we do not trying to parse broken object. Implementation issues should be checked in testcases,
and not in code. And broken inputs is unlikely can cause problem here. If we have broken input, it is ok to produce
broken dump. Does not seem that DWARF parsers really should check that relocations produced by compiler are correct in a hot loop ?

Second check tests that relocation size is <= 8. I do not know why it was checked, because Width is unused in code at all.
If something wrong with such relocations, then visitor should report an error "failed to compute relocation" that we still handle. 
So check looks never triggers while visitor implementation is correct. Currently (RelocVisitor.h) never returns width > 8.


https://reviews.llvm.org/D33194





More information about the llvm-commits mailing list