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

Rui Ueyama ruiu at google.com
Sun Mar 8 20:22:29 PDT 2015


As a general comment, this patch is still too large, and I prefer smaller, incremental patches. For examples, we could drop some directives from this patch, or remove some features such as wildcard pattern matching from this, and basic features should still work. And then we could add missing features one by one. It may be too late, and I read the whole patch anyway, but please consider doing that way next time. Thanks!

Review comments are inline.


================
Comment at: include/lld/ReaderWriter/LinkerScript.h:1130
@@ +1129,3 @@
+  /// number (caching results).
+  struct SectionKeyHash {
+    int64_t operator()(const SectionKey &k) const {
----------------
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:.

================
Comment at: include/lld/ReaderWriter/LinkerScript.h:1138
@@ +1137,3 @@
+  /// our hash table mapping Sections to a Layout Order number (caching results)
+  struct SectionKeyEq {
+    bool operator()(const SectionKey &lhs, const SectionKey &rhs) const {
----------------
Ditto

================
Comment at: include/lld/ReaderWriter/LinkerScript.h:1154
@@ +1153,3 @@
+
+  std::vector<std::unique_ptr<Parser>> &getLinkerScripts() { return _scripts; }
+
----------------
Returning a const std::vector would be better?

================
Comment at: lib/ReaderWriter/ELF/DefaultLayout.h:350
@@ +349,3 @@
+/// override some of the functionalities.
+template <class ELFT> class ScriptLayout : public DefaultLayout<ELFT> {
+public:
----------------
Merge this with DefaultLayout and remove this class.

================
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());
----------------
It's good to add a brief comment here, like, section names may be renamed by linker scripts.

================
Comment at: lib/ReaderWriter/ELF/DefaultLayout.h:993
@@ +992,3 @@
+    }
+  }
+}
----------------
This code is complicated. If we just want to group sections by content type, can you just stable_sort them by content type?

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

================
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;
----------------
StringRef::find returns npos if not found.

================
Comment at: lib/ReaderWriter/LinkerScript.cpp:2210
@@ +2209,3 @@
+      return exprOrder;
+    }
+  }
----------------
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.

================
Comment at: lib/ReaderWriter/LinkerScript.cpp:2378
@@ +2377,3 @@
+
+    linearizeAST(dyn_cast<Sections>(c));
+  }
----------------
You can do

  if (Sections *sec = dyn_cast<Sections>(c))
    linearlizeAst(sec)

http://reviews.llvm.org/D8157

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






More information about the llvm-commits mailing list