This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Factor out relocation checks into separate functions.
ClosedPublic

Authored by ikudrin on Nov 24 2015, 1:35 AM.

Diff Detail

Event Timeline

ikudrin updated this revision to Diff 41011.Nov 24 2015, 1:35 AM
ikudrin retitled this revision from to [ELF] Factor out relocation checks into separate functions..
ikudrin updated this object.
ikudrin added reviewers: ruiu, rafael.
ikudrin added a project: lld.
ikudrin added a subscriber: llvm-commits.
rafael accepted this revision.Nov 24 2015, 6:29 AM
rafael edited edge metadata.

LGTM for me, but please wait to see what Rui thinks.

There is a few extra functions to look at, but there are many uses of them, which makes for a good tradeoff IMHO.

This revision is now accepted and ready to land.Nov 24 2015, 6:29 AM
ruiu added inline comments.Nov 24 2015, 11:09 AM
ELF/Target.cpp
45

checkIsInt -> checkInt
Val -> V

46–48

It's a minor point, but I don't like to mix strings with getElfRelocationTypeName() function call because the function name is too long. I'd write like this.

if (isInt<N>(V))
  return;
StringRef S = getELFRelocationTypeName(Config->EMachine, Type);
error("Relocation " + S + " out of range");
51

Ditto

57

Ditto

ikudrin updated this revision to Diff 41103.Nov 24 2015, 6:13 PM
ikudrin edited edge metadata.
  • Rename check functions and rework their bodies.
ruiu accepted this revision.Nov 25 2015, 10:33 AM
ruiu edited edge metadata.

LGTM with these fixes.

ELF/Target.cpp
59

We usually represent an alignment by its value and not in the form of log2, so for consistency,

uint64_t Mask = N - 1;

or

if ((V & (N - 1)) == 0)
558

So this becomes

checkAlignment<4>
761

checkAlignment<8>

This revision was automatically updated to reflect the committed changes.