[PATCH] D19976: [ELF] - Prototype of possible linkerscript redesign.

[Rui Ueyama via llvm-commits]

ruiu added a comment.

This is a fairly large patch -- you moved code between files and add new features. I'd appreciate if you do this incrementally in future.


================
Comment at: ELF/LinkerScript.cpp:336
@@ +335,3 @@
+  OutputSectionBase<ELFT> *OutSection = nullptr;
+  LayoutParser().run([&](const Command &Cmd) {
+    if (Cmd.Kind == OutSectionKind) {
----------------
This seems tricky. Why do you have to parse linker scripts while processing expressions? I don't see a reason to use a callback function here. It seems to me that we can instead parse all scripts to save commands to a vector and evaluate them in this function.

================
Comment at: ELF/MarkLive.cpp:185
@@ -184,3 +185,3 @@
           scanEhFrameSection<ELFT>(*EH);
-        if (isReserved(Sec) || Script<ELFT>::X->shouldKeep(Sec))
           Enqueue({Sec, 0});
----------------
What's wrong with this code? Why did you have to change the way we handle KEEP commands? The new code seems more complicated than before.

================
Comment at: ELF/OutputSections.h:643
@@ +642,3 @@
+                                  uintX_t Flags) {
+    return Map.lookup({Name, Type, Flags, 0});
+  }
----------------
Move the definition to the .cpp file.

================
Comment at: ELF/OutputSections.h:685-698
@@ +684,16 @@
+
+  static Key getEmptyKey() {
+    return Key{DenseMapInfo<StringRef>::getEmptyKey(), 0, 0, 0};
+  }
+  static Key getTombstoneKey() {
+    return Key{DenseMapInfo<StringRef>::getTombstoneKey(), 0, 0, 0};
+  }
+  static unsigned getHashValue(const Key &Val) {
+    return hash_combine(Val.Name, Val.Type, Val.Flags, Val.Alignment);
+  }
+  static bool isEqual(const Key &LHS, const Key &RHS) {
+    return DenseMapInfo<StringRef>::isEqual(LHS.Name, RHS.Name) &&
+           LHS.Type == RHS.Type && LHS.Flags == RHS.Flags &&
+           LHS.Alignment == RHS.Alignment;
+  }
+};
----------------
Can you move them to the .cpp?

================
Comment at: ELF/Writer.cpp:143
@@ +142,3 @@
+                          const elf::ObjectFile<ELFT> &File) {
+  if (!Config->PrintGcSections || !IS || IS->Live)
+    return;
----------------
This function is called only when a given section is discarded, so checking `!IS || IS->Live` seems redundant.

================
Comment at: ELF/Writer.cpp:145
@@ +144,3 @@
+    return;
+  llvm::errs() << "removing unused section from '" << IS->getSectionName()
+               << "' in file '" << File.getName() << "'\n";
----------------
Remove `llvm::`.

================
Comment at: ELF/Writer.cpp:1162-1163
@@ -1290,1 +1161,4 @@
+Writer<ELFT>::createRegularSections(OutputSectionFactory<ELFT> &Factory) {
+  if (ScriptConfig->DoLayout)
+    return Script<ELFT>::X->createSections(Factory, Symtab);
 
----------------
Can you move this to createSections?

================
Comment at: ELF/Writer.cpp:1166
@@ -1291,4 +1165,3 @@
   // Create output sections for input object file sections.
   std::vector<OutputSectionBase<ELFT> *> RegularSections;
   for (const std::unique_ptr<elf::ObjectFile<ELFT>> &F :
----------------
Now the function gets much smaller, we don't give a long name to this variable.

================
Comment at: ELF/Writer.cpp:1171
@@ -1297,3 +1170,3 @@
       if (isDiscarded(C)) {
-        reportDiscarded(C, F);
+        reportDiscarded(C, *F.get());
         continue;
----------------
`*F.get()` -> `*F`

================
Comment at: ELF/Writer.cpp:1191
@@ +1190,3 @@
+
+  for (OutputSectionBase<ELFT> *Sec : RegularSections) {
+    OwningSections.emplace_back(Sec);
----------------
  for (OutputSectionBase<ELFT> *Sec : createRegularSections()) {

================
Comment at: ELF/Writer.cpp:1349-1353
@@ -1464,1 +1348,7 @@
 
+  // Add .interp at first because some loaders want to see that section
+  // on the first page of the executable file when loaded into memory.
+  auto I = OutputSections.begin();
+  if (needsInterpSection())
+    I = OutputSections.insert(I, Out<ELFT>::Interp);
+
----------------
Move this after BuildId so that you can write

  if (needsInterpSection())
    OutputSections.insert(OutputSections.begin(), Out<ELFT>::Interp);

================
Comment at: ELF/Writer.h:31
@@ +30,3 @@
+
+template <class ELFT> bool isDiscarded(InputSectionBase<ELFT> *IS);
+
----------------
This function should belong to InputSections.h instead of this header.


http://reviews.llvm.org/D19976