[PATCH] D22960: [ELF] LinkerScript: Implement custom output section factory

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/LinkerScript.cpp:61
@@ -60,3 +46,1 @@
-}
-
 template <class ELFT>
----------------
You moved this and some other code. Was there a reason for that ? It creates some noise in the patch.

================
Comment at: ELF/LinkerScript.cpp:123
@@ +122,3 @@
+  if (Out->getType() != InHdr->sh_type)
+    error("failed to mix sections with incompatible types within `" +
+          Out->getName());
----------------
Is it intended ` ?

================
Comment at: ELF/LinkerScript.cpp:127
@@ +126,3 @@
+  if ((Out->getFlags() & SHF_TLS) != (InHdr->sh_flags & SHF_TLS))
+    error("failed to mix TLS and non-TLS sections within `" + Out->getName());
+}
----------------
Ditto.

================
Comment at: ELF/OutputSections.h:681
@@ +680,3 @@
+public:
+  virtual ~OutputSectionFactoryBase() = default;
+
----------------
Doesn't seem you destroy factory using base class pointer. Virtual descructor is excessive.

================
Comment at: ELF/OutputSections.h:683
@@ +682,3 @@
+
+  virtual OutputSectionBase<ELFT> *lookup(StringRef Name, uint32_t Type,
+                                          uintX_t Flags) {
----------------
Why not = 0 ?

================
Comment at: ELF/OutputSections.h:705
@@ -682,3 +704,3 @@
 
   OutputSectionBase<ELFT> *lookup(StringRef Name, uint32_t Type, uintX_t Flags);
 
----------------
Missing override keyword.

================
Comment at: ELF/Writer.cpp:663
@@ -663,1 +662,3 @@
   std::vector<OutputSectionBase<ELFT> *> RegularSections = OutputSections;
+  OutputSectionFactoryBase<ELFT> *Fac =
+      ScriptConfig->HasContents ? Script<ELFT>::X->getFactory() : &Factory;
----------------
May be would be better just initialize Writer::Factory differently depending on whether script is used or not ?


Repository:
  rL LLVM

https://reviews.llvm.org/D22960