[PATCH] D30690: [ELF] - Simplify logic of creating "COMMON" section.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Overall looking good. Nice improvement. A minor comments.



================
Comment at: ELF/Relocations.cpp:482
   bool IsReadOnly = isReadOnly<ELFT>(SS);
-  BssRelSection<ELFT> *RelSec = IsReadOnly ? In<ELFT>::BssRelRo : In<ELFT>::Bss;
-  uintX_t Off = RelSec->addCopyRelocation(SS->getAlignment<ELFT>(), SymSize);
+  BssSection<ELFT> *RelSec = IsReadOnly ? In<ELFT>::BssRelRo : In<ELFT>::Bss;
+  uintX_t Off = RelSec->reserveSpace(SS->getAlignment<ELFT>(), SymSize);
----------------
RelSec -> Bss, as In<ELFT>::Bss is not a .rel.


================
Comment at: ELF/SyntheticSections.cpp:375-376
+  this->Alignment = std::max<size_t>(this->Alignment, AddrAlign);
+  if (OutSec)
+    OutSec->updateAlignment(AddrAlign);
   return this->Size - Size;
----------------
Do this at the beginning of this function because it doesn't depend on this->Size or this->Alignment you already computed.


================
Comment at: ELF/SyntheticSections.h:151-152
+// Section used for common symbols and storing copy relocations. We create
+// three sections now: ".bss.rel.ro", ".bss" for relro and regular copy
+// relocations and "COMMON" section for holding common symbols.
+template <class ELFT> class BssSection final : public SyntheticSection {
----------------
.bss for read-write copy relocations, .bss.rel.ro for read-only copy relocations, and COMMON for common symbols.


================
Comment at: ELF/SyntheticSections.h:162
   size_t getSize() const override { return Size; }
   size_t Size;
 };
----------------
Size = 0


https://reviews.llvm.org/D30690