PNaCl Dynamic Linking: Using unordered_set to disallow duplicate modules.

src/untrusted/pll_loader/pll_loader.h

Index: src/untrusted/pll_loader/pll_loader.h
diff --git a/src/untrusted/pll_loader/pll_loader.h b/src/untrusted/pll_loader/pll_loader.h
index 65577f51f7388f2abc3cc83f42b233139f181836..706dc7e5fbfbea10fdd39e299c6e11ad0e40f0b6 100644
--- a/src/untrusted/pll_loader/pll_loader.h
+++ b/src/untrusted/pll_loader/pll_loader.h
@@ -5,6 +5,8 @@
 #ifndef NATIVE_CLIENT_SRC_UNTRUSTED_PLL_LOADER_PLL_LOADER_H_
 #define NATIVE_CLIENT_SRC_UNTRUSTED_PLL_LOADER_PLL_LOADER_H_ 1
 
+#include <string>
+#include <unordered_set>
 #include <vector>
 
 #include "native_client/src/untrusted/pll_loader/pll_root.h"
@@ -42,9 +44,19 @@ class PLLModule {
 // ModuleSet represents a set of loaded PLLs.
 class ModuleSet {
  public:
-  // Load a PLL and add it to the set.
+  // Load a PLL by filename. Does not add the filename to a known set of loaded
+  // modules.
+  // TODO(smklein): At the moment, "AddByFilename" has no mechanism to

[Mark Seaborn, 2016/03/26 00:30:10]

As I have been saying, I think this current behaviour of AddByFilename() *is*
desirable, so I don't think there should be a TODO on this interface.  (Having a
TODO on AddBySoname() is OK.)

I want to distinguish between:

 1) A primitive mechanism for loading a module, which always instantiates the
module.  This is appropriate for executables and plugins.

 2) An abstraction for loading libraries, built on top of (1), where a library
is only loaded once if >1 things depend on it.  This maintains a mapping of
libraries keyed by soname.

Executables and plugins just aren't like libraries.  We don't want to load them
via a library search path.  Nothing will depend on them via DT_NEEDED
references.  They don't need to have sonames.

It seems you are disagreeing and don't like the distinction I'm making, but I'm
not sure.

[Sean Klein, 2016/03/26 00:47:25]

Okay, I understand now! I was under the impression that you were leaving the
interface simply as an alternative for shared libraries (with sonames) to be
loaded. In reality, there shouldn't be any shared libraries (being used as
supplementary libraries) loaded with AddByFilename -- I understand that you
aren't promoting that use case.

I see that executables and plugins have a different use-case, and should be
considered differently. That is a valid case to leave a different API exposed.

TODO removed.

+  // de-duplicate libraries. If "libfoo.so" is passed multiple times, it will be
+  // loaded multiple times. Proper de-duplication should be added.
   void AddByFilename(const char *filename);
 
+  // Change the search paths used by the linker when looking for PLLs by soname.

[Mark Seaborn, 2016/03/26 00:30:11]

Terminology nit: A "search path" normally refers to a list of directories to
search.  "Path" is singular and refers to a list, rather than referring to the
pathnames in the list.

e.g. https://en.wikipedia.org/wiki/PATH_(variable)

So I'd recommend using the singular "search path" here.

[Sean Klein, 2016/03/26 00:47:25]

Done.

+  void SetSonameSearchPaths(const std::vector<std::string> &dir_list);
+
+  // Load a PLL by soname and add the soname to the set of loaded modules.
+  void AddBySoname(const char *soname);
+
   // Looks up a symbol in the set of modules.  This does a linear search of
   // the modules, in the order that they were added using AddByFilename().
   void *GetSym(const char *name);
@@ -53,6 +65,11 @@ class ModuleSet {
   void ResolveRefs();
 
  private:
+  // The search paths used to look for "sonames".
+  std::vector<std::string> search_paths_;
+  // An unordered set of "sonames" (to see if a module has been loaded) and the

[Mark Seaborn, 2016/03/26 00:30:10]

Nit: remove "and the modules themselves"

[Sean Klein, 2016/03/26 00:47:25]

Done.

+  // modules themselves.
+  std::unordered_set<std::string> sonames_;
   std::vector<PLLModule> modules_;
 };