[PATCH] D27040: [ELF] - Add support for access to most of synthetic sections from linkerscript.

George Rimar via llvm-commits llvm-commits at lists.llvm.org
Thu Nov 24 02:29:47 PST 2016


grimar added inline comments.


================
Comment at: ELF/SyntheticSections.h:35
   virtual void finalize() {}
+  virtual bool isPresent() const { return true; }
 
----------------
ruiu wrote:
> `isPresent` sound a bit odd to me, because as soon as you instantiate objects, they are present. How about `empty`?
Done.


================
Comment at: ELF/Writer.cpp:230
+  Symtab<ELFT>::X->Sections.push_back(Targ);
+};
+
----------------
ruiu wrote:
> grimar wrote:
> > evgeny777 wrote:
> > > This looks better
> > > 
> > > ```
> > > auto add = [](InputSection<ELFT> *IS) {
> > >   Symtab<ELFT>::X->Sections.push_back(IS);
> > >   return IS;
> > > };
> > > ```
> > > 
> > Unfortunately this will not work. You can not assign base class pointer to derived.
> > I applied the idea of your suggestion though.
> It seems unnecessarily complicated. I'd just remove this function and inline it.
I did that though I find this a bit bulky code. I think we may want to refactor it in this way separatelly.


================
Comment at: ELF/Writer.cpp:878
+template <class ELFT>
+static void removeUnusedSynthetics(std::vector<OutputSectionBase *> &V) {
+  // Synthetic sections are placed after all regular ones. We iterate over them
----------------
ruiu wrote:
> `removeUnusedSyntheticSections` because we have other synthetic things such as synthetic symbols.
Done.


================
Comment at: ELF/Writer.cpp:881-882
+  // all and exit at first non-synthetic.
+  for (auto I = Symtab<ELFT>::X->Sections.rbegin();
+       I != Symtab<ELFT>::X->Sections.rend(); ++I) {
+    SyntheticSection<ELFT> *SS = dyn_cast<SyntheticSection<ELFT>>(*I);
----------------
ruiu wrote:
> Use `llvm::reverse`.
Done.


https://reviews.llvm.org/D27040





More information about the llvm-commits mailing list