Page MenuHomePhabricator

[llvm-readobj/elf] - Generalize the code for printing dynamic relocations. NFCI.

Authored by grimar on Sep 3 2020, 6:23 AM.



Currently we have 2 large printDynamicRelocations methods that
have a very similar code for GNU/LLVM styles.

This patch removes the duplication and renames them to printDynamicReloc
for consistency.

Depends on D87141

Diff Detail

Event Timeline

grimar created this revision.Sep 3 2020, 6:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 3 2020, 6:23 AM
Herald added a subscriber: rupprecht. · View Herald Transcript
grimar requested review of this revision.Sep 3 2020, 6:23 AM
grimar updated this revision to Diff 289709.Sep 3 2020, 6:49 AM
  • Fix minor nit.
jhenderson added inline comments.Sep 4 2020, 12:47 AM

I think if you move this function to immediately after the GNUStyle<ELFT>::printDynamicRelocations function, it will make the diff much easier to follow. As it is, there doesn't seem to be a consistent ordering of DumpStyle and GNUStyle methods, so keeping the diff/blame history simpler seems beneficial.

grimar updated this revision to Diff 289888.Sep 4 2020, 1:29 AM
grimar marked an inline comment as done.
  • Addressed review comment.


jhenderson added inline comments.Sep 4 2020, 2:04 AM

I'm not a fan of having to go through the printRelaReloc, just to get to the printDynamicRelocation method which was being called before. I take it this is because printDynamicRelocation is templated and can't be virtual? I'm beginning to wonder if using the template solution to the Elf_Rel/Elf_Rela problem was a mistake - it's making the code harder to follow, due to there being unnecessary layers of functions (and in this case, parameters). Maybe we should use a class that can represent either an Elf_Rel or Elf_Rela, ideally with an implicit conversion constructor that is used instead of the two of them. That way, the template can be avoided, and the printDynamicRelocation could become a part of the interface. To avoid confusion, the class would not be an Elf_Rela (so that we aren't misrepresenting Elf_Rel instances internally). What do you think?

grimar added inline comments.Sep 4 2020, 2:21 AM

Yes, I think it makes sense. I'll am going to try to introduce such class and experiment with the code a bit.

grimar updated this revision to Diff 289937.Sep 4 2020, 6:26 AM
grimar edited the summary of this revision. (Show Details)
  • Updated in according to discussion. Now depends on D87141.
jhenderson accepted this revision.Sep 7 2020, 12:37 AM

LGTM. I think this is a much cleaner version now.

This revision is now accepted and ready to land.Sep 7 2020, 12:37 AM