[PATCH] D48247: lld: add experimental support for SHT_RELR sections.
Rui Ueyama via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Jun 24 22:37:59 PDT 2018
ruiu added inline comments.
================
Comment at: ELF/Driver.cpp:859
+ Config->RelrPackDynRelocs = true;
+ else if (S == "android-relr") {
+ Config->RelrPackDynRelocs = true;
----------------
nit: if you add {} to one of `else if`, please add it to all `if` and `else if`s.
================
Comment at: ELF/SyntheticSections.cpp:1446
RelType Type) {
- // Write the addends to the relocated address if required. We skip
- // it if the written value would be zero.
- if (Config->WriteAddends && (Expr != R_ADDEND || Addend != 0))
+ // Write the addends to the relocated address if required.
+ bool WriteAddend = Config->WriteAddends;
----------------
I don't get the meaning of this comment. What does `if required` actually mean? I think that should be answered in the comment. Actually, I'd think instead of writing comment for each expression, you probably should write a function comment to give readers a big picture as to what this function is supposed to do.
================
Comment at: ELF/SyntheticSections.cpp:1461
void RelocationBaseSection::addReloc(const DynamicReloc &Reloc) {
- if (Reloc.Type == Target->RelativeRel)
- ++NumRelativeRelocs;
- Relocs.push_back(Reloc);
+ if (InX::RelrDyn && Reloc.Type == Target->RelativeRel)
+ InX::RelrDyn->addReloc(Reloc);
----------------
Add {}
================
Comment at: ELF/SyntheticSections.cpp:1717
+
+template <class ELFT>
+bool RelrPackedRelocationSection<ELFT>::updateAllocSize() {
----------------
Please write a function comment so that the first-time reader can understand your code without hassle.
================
Comment at: ELF/SyntheticSections.cpp:1718
+template <class ELFT>
+bool RelrPackedRelocationSection<ELFT>::updateAllocSize() {
+ size_t OldSize = RelrRelocs.size();
----------------
This function seems a bit too long. Please consider splitting this into multiple small functions.
================
Comment at: ELF/SyntheticSections.h:522
+template <class ELFT>
+class RelrPackedRelocationSection final : public RelocationBaseSection {
+ typedef typename ELFT::Relr Elf_Relr;
----------------
In lld, we try to keep the class hierarchy as shallow as possible. Please avoid class inheritance when possible.
Repository:
rLLD LLVM Linker
https://reviews.llvm.org/D48247
More information about the llvm-commits
mailing list