This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - unify the use of uint32_t instead of unsigned in Target class for relocations types
ClosedPublic

Authored by grimar on Mar 4 2016, 3:16 AM.

Details

Summary

I was confused about mixing of type names in Target and its callers for a long time.
Patch suggest the fix for that. It changes all relocations types to be uint32_t and also fixes some dependent inconsistency in callers code.

Diff Detail

Event Timeline

grimar updated this revision to Diff 49811.Mar 4 2016, 3:16 AM
grimar retitled this revision from to [ELF] - unify the use of uint32_t instead of unsigned in Target class for relocations types.
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar.
rafael accepted this revision.Mar 4 2016, 5:55 AM
rafael edited edge metadata.

LGTM with nits

The part about using uint32_t for relocation type LGTM. That is what getType in ELFTypes.h returns and we may as well just propagate it.

ELF/Target.cpp
885

return unsigned for now.

ELF/Target.h
72

I would leave this one as unsigned. Or at least do it in another patch.

This revision is now accepted and ready to land.Mar 4 2016, 5:55 AM
grimar added inline comments.Mar 4 2016, 6:11 AM
ELF/Target.cpp
885

it is used as:

void InputSectionBase<ELFT>::relocate(uint8_t *Buf, uint8_t *BufEnd,
                                      RelIteratorRange<isRela> Rels) {
  typedef Elf_Rel_Impl<ELFT, isRela> RelType;
  size_t Num = Rels.end() - Rels.begin();
  for (size_t I = 0; I < Num; ++I) {
...
   I += Target->relaxTls(BufLoc, BufEnd, Type, AddrLoc, SymVA, Body);

I believe size_t is expected here. Why do you want to change that ?

ELF/Target.h
72

Do you mean that I can change that to size_t but just commit separatelly ?
Then I can do that for sure.

grimar added a comment.Mar 4 2016, 6:46 AM

I meant reviewed separately, but given the use, it LGTM too. Just
commit separately.

Thanks,
Rafael

Sure, I will do soon.
Thank you for review !

George

This revision was automatically updated to reflect the committed changes.