[PATCH] [lld] [LinkerScript] Implement semantics for simple sections mappings

[Rafael Auler]

Thank you all for your patience with this patch size and for the reviews. I'll keep the next patches shorter. Comments inline.


================
Comment at: include/lld/ReaderWriter/LinkerScript.h:168
@@ -163,2 +167,3 @@
     Sections,
+    SortedGroup,
     SymbolAssignment,
----------------
shankarke wrote:
> Could you represent all the different kinds of functions that can operate on the input sections as Functions instead of InputSectionKinds ?
I'm afraid I'm not on the same page, which functions are you mentioning?

================
Comment at: include/lld/ReaderWriter/LinkerScript.h:1122
@@ +1121,3 @@
+  struct SectionKey {
+    StringRef archiveName;
+    StringRef memberName;
----------------
shankarke wrote:
> can you rename this to archiveNameOrPath
I was under the impression that, from the linker script perspective, we should always work with file names instead of paths. For example, to make my test case work correctly, I need to drop the path that comes from the File object and use only the file name. Only then I can match it with a linker script rule because they omit the full path. Can a linker script specify a full path to an archive?

================
Comment at: include/lld/ReaderWriter/LinkerScript.h:1130
@@ +1129,3 @@
+  /// number (caching results).
+  struct SectionKeyHash {
+    int64_t operator()(const SectionKey &k) const {
----------------
ruiu wrote:
> Instead of defining a class (struct) implementing an operator=(const SectionKey &), you can simply define of a function (int64_t f(const SectionKey &)) and pass it to std::unordered_map.
> 
> You want to move this below private:.
I don't understand how caching here would be error prone. I'm simply implementing the design that was suggested from the previous thread of D7915:

* Make DefaultLayout ask Sema about the order of two sections by section name (or, more precisely, section "key") and do not expose "ids".

* Since these questions are frequently asked for the same input section, we cache the result.

Which users do you refer to? You mean, the TargetLayout?

================
Comment at: include/lld/ReaderWriter/LinkerScript.h:1154
@@ +1153,3 @@
+
+  std::vector<std::unique_ptr<Parser>> &getLinkerScripts() { return _scripts; }
+
----------------
shankarke wrote:
> ruiu wrote:
> > Returning a const std::vector would be better?
> All the end users should be using Sema right ? So This function could be made private.
Rui: Yes, I should return a const std::vector.

You are right, Shankar, but this function is currently not private to enable unit testing of linker script expression evaluation.

================
Comment at: lib/ReaderWriter/ELF/DefaultLayout.h:229-235
@@ +228,9 @@
+
+  /// \brief Add extra chunks to a segment just before including the input
+  /// section given by <archiveName, memberName, sectionName>. This is used by
+  /// ScriptLayout to add linker script expressions before each section.
+  virtual void addExtraChunksToSegment(Segment<ELFT> *segment,
+                                       StringRef archiveName,
+                                       StringRef memberName,
+                                       StringRef sectionName) {}
+
----------------
shankarke wrote:
> If you can associate the expression also to an outputsection, this would not be needed. The code gets overly simplified too.
Ok, I'll work on this. Thanks.

================
Comment at: lib/ReaderWriter/ELF/DefaultLayout.h:350
@@ +349,3 @@
+/// override some of the functionalities.
+template <class ELFT> class ScriptLayout : public DefaultLayout<ELFT> {
+public:
----------------
ruiu wrote:
> Merge this with DefaultLayout and remove this class.
No problem, but first can we get a consensus on whether do we need ScriptLayout or not? I'm asking this because I only moved the code out of DefaultLayout due to Shankar's solicitation.

================
Comment at: lib/ReaderWriter/ELF/DefaultLayout.h:587
@@ +586,3 @@
+
+  StringRef archiveName = llvm::sys::path::filename(da->file().archivePath());
+  StringRef memberName = llvm::sys::path::filename(da->file().memberPath());
----------------
ruiu wrote:
> It's good to add a brief comment here, like, section names may be renamed by linker scripts.
Sure

================
Comment at: lib/ReaderWriter/ELF/DefaultLayout.h:993
@@ +992,3 @@
+    }
+  }
+}
----------------
ruiu wrote:
> This code is complicated. If we just want to group sections by content type, can you just stable_sort them by content type?
Unfortunately this turned out to be more complicated than intended. Here is the problem:

We can't sort everything again because this would undo the linker script order. We can only move sections that weren't assigned by the linker script.

If we stable_sort everything by content_type, we may also move sections that should be pinned by the linker script.

================
Comment at: lib/ReaderWriter/ELF/SegmentChunks.h:437
@@ -402,1 +436,3 @@
+        if (auto expr = dyn_cast<ExpressionChunk<ELFT>>(section))
+          fileOffset += expr->virtualAddr() - lastVirtualAddress;
       // Align fileoffset to the alignment of the section.
----------------
shankarke wrote:
> why would you do this ?
This is used to keep in sync the file image with the memory image.

================
Comment at: lib/ReaderWriter/LinkerScript.cpp:2111
@@ +2110,3 @@
+                            name.drop_front(i - name.begin() + 1)))
+        ++i;
+      break;
----------------
ruiu wrote:
> I think you need to check whether or not i is not greater than the length of name on each iteration.
Will do.

================
Comment at: lib/ReaderWriter/LinkerScript.cpp:2123
@@ +2122,3 @@
+      StringRef chars = pattern.slice(j - pattern.begin(), end);
+      if (chars.find(i) == chars.size())
+        return false;
----------------
ruiu wrote:
> StringRef::find returns npos if not found.
Thanks for pointing out, I will fix this.

================
Comment at: lib/ReaderWriter/LinkerScript.cpp:2210
@@ +2209,3 @@
+      return exprOrder;
+    }
+  }
----------------
ruiu wrote:
> This loop content is duplicate of the previous one. Do we really need to separate non-wildcard patterns and wildcard patterns? Technically, the former is a special case of the latter, and I don't think we have to optimize this routine. Simplicity is preferred, I guess.
I will refactor this.

================
Comment at: lib/ReaderWriter/LinkerScript.cpp:2378
@@ +2377,3 @@
+
+    linearizeAST(dyn_cast<Sections>(c));
+  }
----------------
ruiu wrote:
> You can do
> 
>   if (Sections *sec = dyn_cast<Sections>(c))
>     linearlizeAst(sec)
Thanks, will do.

http://reviews.llvm.org/D8157

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/