PNaCl Dynamic Linking: Using unordered_set to disallow duplicate modules.

PNaCl Dynamic Linking: Using unordered_set to disallow duplicate modules.

Also adds mechanism to add DSOs as sonames rather than full filenames.
The unordered_set uses sonames (rather than pathnames) as a unique identifier.

In the future, this will allow us to ignore duplicates dependencies. If, for
example, multiple modules depend on libc.so, then there should be a quick way
to identify if libc.so already exists in the list of instantiated modules.

TEST=pll_loader_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351

Committed: https://chromium.googlesource.com/native_client/src/native_client/+/b756424472fdd7689518c557e1e66d25f5350bdd

[Sean Klein, 2016-03-23 22:12:51]

smklein@chromium.org changed reviewers:
+ mseaborn@chromium.org

[Sean Klein, 2016-03-23 22:12:51]

I'll check that the bots pass, but I thought it would be good
to get your thoughts on this change as well (I don't know if
you think preserving PLLModule order is still important at this stage).

[Sean Klein, 2016-03-23 22:13:39]

Description was changed from

==========
PNaCl Dynamic Linking: Updated list of modules to map of modules.

This will allow quick lookup of modules by name, allowing failure
if duplicate modules are passed.

In the future, this will allow us to ignore duplicates dependencies. If, for
example, multiple modules depend on libc.so, then there should be a quick way
to identify if libc.so already exists in the list of instantiated modules.

TEST=run_pll_hello_world_test (manually updated to pass pll_libc twice)
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========

to

==========
PNaCl Dynamic Linking: Updated list of modules to map of modules.

This will allow quick lookup of modules by name, allowing failure
if duplicate modules are passed.

In the future, this will allow us to ignore duplicates dependencies. If, for
example, multiple modules depend on libc.so, then there should be a quick way
to identify if libc.so already exists in the list of instantiated modules.

TEST=run_pll_hello_world_test
(manually updated to pass pll_libc twice to observe failure)
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========

[Mark Seaborn, 2016-03-24 18:25:22]

On 23 March 2016 at 15:12, <smklein@chromium.org> wrote:

> Reviewers: Mark Seaborn
> CL: https://codereview.chromium.org/1832623002/
>
> Message:
> I'll check that the bots pass, but I thought it would be good
> to get your thoughts on this change as well (I don't know if
> you think preserving PLLModule order is still important at this stage).
>

OK, some thoughts:

* On search ordering:  std::map sorts by key.  That's better than having
non-deterministic ordering, but it will still give odd behaviour.  If the
keys are pathnames, the search ordering will change if libraries are loaded
from different directories, which seems undesirable.

If the keys are sonames, that's still a bit odd, but not so bad.

I would be inclined to keep a std::vector of the modules for searching, and
hence preserve the original order.  It makes the behaviour more
predictable, and it doesn't really cost anything.

Using a std::vector might well be more efficient to search, too, because
std::vector uses an array, while std::map uses a tree.  In many cases that
kind of difference doesn't matter, but symbol resolution is supposed to be
performance sensitive -- although this performance bottleneck would go away
if we use direct binding.


* I would be inclined to do de-duping only based on soname, not based on
pathname.

So there would be two routes for loading a module:

   - Load given a soname.  In this case, we reuse the module if it's
   already loaded.  When running as a command line tool, we search for the
   module in a directory search path using the given soname.  (We might use a
   different search strategy when running in an environment that doesn't have
   a filesystem, such as Mojo.)
   - Load given a pathname or FD.  In this case, we always create a new
   instantiation of the module.  We don't add it to a filename/soname
   mapping.  This route would be used for plugins or executables.  Plugins are
   something that you might actually want to instantiate multiple times.

Pathnames aren't always canonical, and we don't always have pathnames for
modules.  For comparison, glibc's dynamic linker de-dups modules based on
inode number, but that's problematic too because inode numbers aren't
always available either.


* On search scoping:  I don't think this matters just yet, but I'm thinking
ahead.  I think there are three ways we can scope the searches for symbols:

   - Global namespace:  We do a linear search of all modules for every
   symbol import.
   - Full direct binding:  Each symbol import by a module M is associated
   with a particular DT_NEEDED entry from module M.  So each import is looked
   up in exactly one module.
   - Half-way direct binding:  When resolving a symbol import from module
   M, we search only the modules referenced by module M's DT_NEEDED entries.
    (This option occurred to me when thinking about this CL.)  This is simpler
   to implement than full direct binding because it doesn't require any
   metadata on the symbol imports themselves in the PLL format.

There is one case I know of that either kind of direct binding will
currently break:  Currently libc's __libc_start() refers to main() defined
by the executable, but it doesn't make sense for libc to have a DT_NEEDED
dependency on the executable.  We could invert that dependency to make that
compatible with direct  binding.  (For comparison, with ELF, the
executable's entry point calls libc's __libc_start_main(), which calls back
to main().)

Half-way direct binding is simple enough that it *might* be worth using
that (rather than a global namespace) when we first have DT_NEEDED entries,
with the exception that that libc startup issue would have to be addressed
first.

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at https://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[Sean Klein, 2016-03-24 22:08:42]

Description was changed from

==========
PNaCl Dynamic Linking: Updated list of modules to map of modules.

This will allow quick lookup of modules by name, allowing failure
if duplicate modules are passed.

In the future, this will allow us to ignore duplicates dependencies. If, for
example, multiple modules depend on libc.so, then there should be a quick way
to identify if libc.so already exists in the list of instantiated modules.

TEST=run_pll_hello_world_test
(manually updated to pass pll_libc twice to observe failure)
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========

to

==========
PNaCl Dynamic Linking: Using unordered_set to track loaded modules.

This will allow quick lookup of modules by name, allowing failure
if duplicate modules are passed. The unordered_set uses filenames
(rather than pathnames) as a unique identifier.

In the future, this will allow us to ignore duplicates dependencies. If, for
example, multiple modules depend on libc.so, then there should be a quick way
to identify if libc.so already exists in the list of instantiated modules.

TEST=run_pll_hello_world_test
(manually updated to pass pll_libc twice to observe failure)
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========

[Sean Klein, 2016-03-24 22:09:13]

Description was changed from

==========
PNaCl Dynamic Linking: Using unordered_set to track loaded modules.

This will allow quick lookup of modules by name, allowing failure
if duplicate modules are passed. The unordered_set uses filenames
(rather than pathnames) as a unique identifier.

In the future, this will allow us to ignore duplicates dependencies. If, for
example, multiple modules depend on libc.so, then there should be a quick way
to identify if libc.so already exists in the list of instantiated modules.

TEST=run_pll_hello_world_test
(manually updated to pass pll_libc twice to observe failure)
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========

to

==========
PNaCl Dynamic Linking: Using unordered_set to disallow duplicate modules.

This will allow quick lookup of modules by name, allowing failure
if duplicate modules are passed. The unordered_set uses filenames
(rather than pathnames) as a unique identifier.

In the future, this will allow us to ignore duplicates dependencies. If, for
example, multiple modules depend on libc.so, then there should be a quick way
to identify if libc.so already exists in the list of instantiated modules.

TEST=run_pll_hello_world_test
(manually updated to pass pll_libc twice to observe failure)
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========

[Sean Klein, 2016-03-24 22:10:24]

Description was changed from

==========
PNaCl Dynamic Linking: Using unordered_set to disallow duplicate modules.

This will allow quick lookup of modules by name, allowing failure
if duplicate modules are passed. The unordered_set uses filenames
(rather than pathnames) as a unique identifier.

In the future, this will allow us to ignore duplicates dependencies. If, for
example, multiple modules depend on libc.so, then there should be a quick way
to identify if libc.so already exists in the list of instantiated modules.

TEST=run_pll_hello_world_test
(manually updated to pass pll_libc twice to observe failure)
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========

to

==========
PNaCl Dynamic Linking: Using unordered_set to disallow duplicate modules.

The unordered_set uses filenames (rather than pathnames) as a unique identifier.

In the future, this will allow us to ignore duplicates dependencies. If, for
example, multiple modules depend on libc.so, then there should be a quick way
to identify if libc.so already exists in the list of instantiated modules.

TEST=run_pll_hello_world_test
(manually updated to pass pll_libc twice to observe failure)
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========

[Sean Klein, 2016-03-24 22:10:34]

On 2016/03/24 18:25:22, Mark Seaborn wrote:
> On 23 March 2016 at 15:12, <mailto:smklein@chromium.org> wrote:
> 
> > Reviewers: Mark Seaborn
> > CL: https://codereview.chromium.org/1832623002/
> >
> > Message:
> > I'll check that the bots pass, but I thought it would be good
> > to get your thoughts on this change as well (I don't know if
> > you think preserving PLLModule order is still important at this stage).
> >
> 
> OK, some thoughts:
> 
> * On search ordering:  std::map sorts by key.  That's better than having
> non-deterministic ordering, but it will still give odd behaviour.  If the
> keys are pathnames, the search ordering will change if libraries are loaded
> from different directories, which seems undesirable.
> 
> If the keys are sonames, that's still a bit odd, but not so bad.
> 
> I would be inclined to keep a std::vector of the modules for searching, and
> hence preserve the original order.  It makes the behaviour more
> predictable, and it doesn't really cost anything.
> 
> Using a std::vector might well be more efficient to search, too, because
> std::vector uses an array, while std::map uses a tree.  In many cases that
> kind of difference doesn't matter, but symbol resolution is supposed to be
> performance sensitive -- although this performance bottleneck would go away
> if we use direct binding.
> 
> 
> * I would be inclined to do de-duping only based on soname, not based on
> pathname.
> 
> So there would be two routes for loading a module:
> 
>    - Load given a soname.  In this case, we reuse the module if it's
>    already loaded.  When running as a command line tool, we search for the
>    module in a directory search path using the given soname.  (We might use a
>    different search strategy when running in an environment that doesn't have
>    a filesystem, such as Mojo.)
>    - Load given a pathname or FD.  In this case, we always create a new
>    instantiation of the module.  We don't add it to a filename/soname
>    mapping.  This route would be used for plugins or executables.  Plugins are
>    something that you might actually want to instantiate multiple times.
> 
> Pathnames aren't always canonical, and we don't always have pathnames for
> modules.  For comparison, glibc's dynamic linker de-dups modules based on
> inode number, but that's problematic too because inode numbers aren't
> always available either.
> 
> 
> * On search scoping:  I don't think this matters just yet, but I'm thinking
> ahead.  I think there are three ways we can scope the searches for symbols:
> 
>    - Global namespace:  We do a linear search of all modules for every
>    symbol import.
>    - Full direct binding:  Each symbol import by a module M is associated
>    with a particular DT_NEEDED entry from module M.  So each import is looked
>    up in exactly one module.
>    - Half-way direct binding:  When resolving a symbol import from module
>    M, we search only the modules referenced by module M's DT_NEEDED entries.
>     (This option occurred to me when thinking about this CL.)  This is simpler
>    to implement than full direct binding because it doesn't require any
>    metadata on the symbol imports themselves in the PLL format.
> 
> There is one case I know of that either kind of direct binding will
> currently break:  Currently libc's __libc_start() refers to main() defined
> by the executable, but it doesn't make sense for libc to have a DT_NEEDED
> dependency on the executable.  We could invert that dependency to make that
> compatible with direct  binding.  (For comparison, with ELF, the
> executable's entry point calls libc's __libc_start_main(), which calls back
> to main().)
> 
> Half-way direct binding is simple enough that it *might* be worth using
> that (rather than a global namespace) when we first have DT_NEEDED entries,
> with the exception that that libc startup issue would have to be addressed
> first.
> 
> Cheers,
> Mark
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Native-Client-Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:native-client-reviews+unsubscribe@googlegroups.com.
> To post to this group, send email to
mailto:native-client-reviews@googlegroups.com.
> Visit this group at https://groups.google.com/group/native-client-reviews.
> For more options, visit https://groups.google.com/d/optout.

[Sean Klein, 2016-03-24 22:15:43]

(sorry for the empty message. Double clicking reply is a bad idea.)

> OK, some thoughts:
> 
> * On search ordering:  std::map sorts by key.  That's better than having
> non-deterministic ordering, but it will still give odd behaviour.  If the
> keys are pathnames, the search ordering will change if libraries are loaded
> from different directories, which seems undesirable.
> 
> If the keys are sonames, that's still a bit odd, but not so bad.

Updated to use "sonames" as keys. For now, the PLL loader only knows of
shared libraries by their paths, so an "soname" is just the basename of
a path. This may change in the future to be a field within the PSO root.

> I would be inclined to keep a std::vector of the modules for searching, and
> hence preserve the original order.  It makes the behaviour more
> predictable, and it doesn't really cost anything.

Done.

> 
> * I would be inclined to do de-duping only based on soname, not based on
> pathname.

Done, although as mentioned, the idea of "what is an soname" will probably
change
eventually.

> * On search scoping:  I don't think this matters just yet, but I'm thinking
> ahead.  I think there are three ways we can scope the searches for symbols:
> 
>    - Global namespace:  We do a linear search of all modules for every
>    symbol import.
>    - Full direct binding:  Each symbol import by a module M is associated
>    with a particular DT_NEEDED entry from module M.  So each import is looked
>    up in exactly one module.
>    - Half-way direct binding:  When resolving a symbol import from module
>    M, we search only the modules referenced by module M's DT_NEEDED entries.
>     (This option occurred to me when thinking about this CL.)  This is simpler
>    to implement than full direct binding because it doesn't require any
>    metadata on the symbol imports themselves in the PLL format.
> 
> There is one case I know of that either kind of direct binding will
> currently break:  Currently libc's __libc_start() refers to main() defined
> by the executable, but it doesn't make sense for libc to have a DT_NEEDED
> dependency on the executable.  We could invert that dependency to make that
> compatible with direct  binding.  (For comparison, with ELF, the
> executable's entry point calls libc's __libc_start_main(), which calls back
> to main().)
> 
> Half-way direct binding is simple enough that it *might* be worth using
> that (rather than a global namespace) when we first have DT_NEEDED entries,
> with the exception that that libc startup issue would have to be addressed
> first.

100% agree that "half-way direct binding" seems like an easy optimization to
make
above a simple global namespace. I'll plan on implementing something like that
once the dependencies are plugged in properly.

[Mark Seaborn, 2016-03-25 17:57:28]

https://codereview.chromium.org/1832623002/diff/40001/src/untrusted/pll_loade...
File src/untrusted/pll_loader/pll_loader.cc (right):

https://codereview.chromium.org/1832623002/diff/40001/src/untrusted/pll_loade...
src/untrusted/pll_loader/pll_loader.cc:26: const char *GetSoname(const char
*path) {
I'm not sure it makes sense to add this because we don't really want the PLL
loader to be parsing filenames, only constructing them (i.e. building "directory
+ soname" when searching).

How about instead:

* Leave the behaviour of AddByFilename() as it is.

* Add a couple of new methods:

const PLLRoot *AddBySoname(const char *soname);
void SetSonameSearchPath(const std::vector<std::string> &dir_list);

* Change pll_loader_test to exercise those new methods (in addition to
AddByFilename()) and to test that AddBySoname() de-dups.

[Sean Klein, 2016-03-25 18:43:57]

https://codereview.chromium.org/1832623002/diff/40001/src/untrusted/pll_loade...
File src/untrusted/pll_loader/pll_loader.cc (right):

https://codereview.chromium.org/1832623002/diff/40001/src/untrusted/pll_loade...
src/untrusted/pll_loader/pll_loader.cc:26: const char *GetSoname(const char
*path) {
On 2016/03/25 17:57:28, Mark Seaborn wrote:
> I'm not sure it makes sense to add this because we don't really want the PLL
> loader to be parsing filenames, only constructing them (i.e. building
"directory
> + soname" when searching).
> 
> How about instead:
> 
> * Leave the behaviour of AddByFilename() as it is.
> 
> * Add a couple of new methods:
> 
> const PLLRoot *AddBySoname(const char *soname);
> void SetSonameSearchPath(const std::vector<std::string> &dir_list);
> 
> * Change pll_loader_test to exercise those new methods (in addition to
> AddByFilename()) and to test that AddBySoname() de-dups.

Yeah, it felt like a temporary solution anyway (I was never under the impression
"GetSoname" would stick around once we got directory searching working).

I'll update to the new version. If possible, I might try to hide "AddByFilename"
from the API, and instead use directory/soname setting functions instead.

[Mark Seaborn, 2016-03-25 18:52:46]

On 25 March 2016 at 11:43, <smklein@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1832623002/diff/40001/src/untrusted/pll_loade...
> File src/untrusted/pll_loader/pll_loader.cc (right):
>
>
>
https://codereview.chromium.org/1832623002/diff/40001/src/untrusted/pll_loade...
> src/untrusted/pll_loader/pll_loader.cc:26: const char *GetSoname(const
> char *path) {
> On 2016/03/25 17:57:28, Mark Seaborn wrote:
> > I'm not sure it makes sense to add this because we don't really want
> the PLL
> > loader to be parsing filenames, only constructing them (i.e. building
> "directory
> > + soname" when searching).
> >
> > How about instead:
> >
> > * Leave the behaviour of AddByFilename() as it is.
> >
> > * Add a couple of new methods:
> >
> > const PLLRoot *AddBySoname(const char *soname);
> > void SetSonameSearchPath(const std::vector<std::string> &dir_list);
> >
> > * Change pll_loader_test to exercise those new methods (in addition to
> > AddByFilename()) and to test that AddBySoname() de-dups.
>
> Yeah, it felt like a temporary solution anyway (I was never under the
> impression "GetSoname" would stick around once we got directory
> searching working).
>
> I'll update to the new version. If possible, I might try to hide
> "AddByFilename" from the API, and instead use directory/soname setting
> functions instead.
>

I think we will still want an AddByFilename().  For comparison, dlopen()
can take a soname or a full pathname.  (I think dlopen() treats its
argument as a soname if it doesn't contain a slash, similar to how shells
choose whether or not to search PATH.)  Executables and plugins are things
we would want to load by filename and not add into the soname mapping.

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at https://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[Sean Klein, 2016-03-25 18:55:09]

> I think we will still want an AddByFilename().  For comparison, dlopen()
> can take a soname or a full pathname.  (I think dlopen() treats its
> argument as a soname if it doesn't contain a slash, similar to how shells
> choose whether or not to search PATH.)  Executables and plugins are things
> we would want to load by filename and not add into the soname mapping.

Then I will need to add an "soname" field to the PLL root, otherwise I'd
have no option but filename parsing (which we decided is bad) to distinguish
between duplicate shared libraries.

[Sean Klein, 2016-03-25 23:31:06]

Description was changed from

==========
PNaCl Dynamic Linking: Using unordered_set to disallow duplicate modules.

The unordered_set uses filenames (rather than pathnames) as a unique identifier.

In the future, this will allow us to ignore duplicates dependencies. If, for
example, multiple modules depend on libc.so, then there should be a quick way
to identify if libc.so already exists in the list of instantiated modules.

TEST=run_pll_hello_world_test
(manually updated to pass pll_libc twice to observe failure)
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========

to

==========
PNaCl Dynamic Linking: Using unordered_set to disallow duplicate modules.

Also adds mechanism to add DSOs as sonames rather than full filenames.
The unordered_set uses sonames (rather than pathnames) as a unique identifier.

In the future, this will allow us to ignore duplicates dependencies. If, for
example, multiple modules depend on libc.so, then there should be a quick way
to identify if libc.so already exists in the list of instantiated modules.

TEST=pll_loader_test, pll_hello_world_test
(manually updated to pass pll_libc twice to observe failure)
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========

[Sean Klein, 2016-03-25 23:31:24]

Description was changed from

==========
PNaCl Dynamic Linking: Using unordered_set to disallow duplicate modules.

Also adds mechanism to add DSOs as sonames rather than full filenames.
The unordered_set uses sonames (rather than pathnames) as a unique identifier.

In the future, this will allow us to ignore duplicates dependencies. If, for
example, multiple modules depend on libc.so, then there should be a quick way
to identify if libc.so already exists in the list of instantiated modules.

TEST=pll_loader_test, pll_hello_world_test
(manually updated to pass pll_libc twice to observe failure)
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========

to

==========
PNaCl Dynamic Linking: Using unordered_set to disallow duplicate modules.

Also adds mechanism to add DSOs as sonames rather than full filenames.
The unordered_set uses sonames (rather than pathnames) as a unique identifier.

In the future, this will allow us to ignore duplicates dependencies. If, for
example, multiple modules depend on libc.so, then there should be a quick way
to identify if libc.so already exists in the list of instantiated modules.

TEST=pll_loader_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========

[Sean Klein, 2016-03-25 23:35:58]

Patchset #4 (id:60001) has been deleted

[Sean Klein, 2016-03-25 23:45:14]

Patchset #4 (id:80001) has been deleted

[Sean Klein, 2016-03-25 23:57:03]

On 2016/03/25 18:55:09, Sean Klein wrote:
> > I think we will still want an AddByFilename().  For comparison, dlopen()
> > can take a soname or a full pathname.  (I think dlopen() treats its
> > argument as a soname if it doesn't contain a slash, similar to how shells
> > choose whether or not to search PATH.)  Executables and plugins are things
> > we would want to load by filename and not add into the soname mapping.
> 
> Then I will need to add an "soname" field to the PLL root, otherwise I'd
> have no option but filename parsing (which we decided is bad) to distinguish
> between duplicate shared libraries.

As we discussed offline, I'm going to avoid adding an "soname" field to
"PLLRoot" for now. Instead, I added those functions, updated pll_loader_test,
and added a warning that "AddByFilename" will not detect duplicate loaded
libraries.

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

LGTM

https://codereview.chromium.org/1832623002/diff/100001/src/untrusted/pll_load...
File src/untrusted/pll_loader/pll_loader.cc (right):

https://codereview.chromium.org/1832623002/diff/100001/src/untrusted/pll_load...
src/untrusted/pll_loader/pll_loader.cc:100: struct stat buf;
Nit: putting this immediately before the stat() call is more idiomatic (i.e. put
var definitions as closer as possible to their uses), and just as efficient.

https://codereview.chromium.org/1832623002/diff/100001/src/untrusted/pll_load...
File src/untrusted/pll_loader/pll_loader.h (right):

https://codereview.chromium.org/1832623002/diff/100001/src/untrusted/pll_load...
src/untrusted/pll_loader/pll_loader.h:49: // TODO(smklein): At the moment,
"AddByFilename" has no mechanism to
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.

https://codereview.chromium.org/1832623002/diff/100001/src/untrusted/pll_load...
src/untrusted/pll_loader/pll_loader.h:54: // Change the search paths used by the
linker when looking for PLLs by soname.
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.

https://codereview.chromium.org/1832623002/diff/100001/src/untrusted/pll_load...
src/untrusted/pll_loader/pll_loader.h:70: // An unordered set of "sonames" (to
see if a module has been loaded) and the
Nit: remove "and the modules themselves"

https://codereview.chromium.org/1832623002/diff/100001/tests/pnacl_dynamic_lo...
File tests/pnacl_dynamic_loading/nacl.scons (right):

https://codereview.chromium.org/1832623002/diff/100001/tests/pnacl_dynamic_lo...
tests/pnacl_dynamic_loading/nacl.scons:120: Depends(node, test_pll_a)
An alternative is:

CommandSelLdrTestNacl(..., extra_deps=[test_pll_a, ...])

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

https://codereview.chromium.org/1832623002/diff/100001/src/untrusted/pll_load...
File src/untrusted/pll_loader/pll_loader.cc (right):

https://codereview.chromium.org/1832623002/diff/100001/src/untrusted/pll_load...
src/untrusted/pll_loader/pll_loader.cc:100: struct stat buf;
On 2016/03/26 00:30:10, Mark Seaborn wrote:
> Nit: putting this immediately before the stat() call is more idiomatic (i.e.
put
> var definitions as closer as possible to their uses), and just as efficient.

Done.

https://codereview.chromium.org/1832623002/diff/100001/src/untrusted/pll_load...
File src/untrusted/pll_loader/pll_loader.h (right):

https://codereview.chromium.org/1832623002/diff/100001/src/untrusted/pll_load...
src/untrusted/pll_loader/pll_loader.h:49: // TODO(smklein): At the moment,
"AddByFilename" has no mechanism to
On 2016/03/26 00:30:10, Mark Seaborn wrote:
> 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.

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.

https://codereview.chromium.org/1832623002/diff/100001/src/untrusted/pll_load...
src/untrusted/pll_loader/pll_loader.h:54: // Change the search paths used by the
linker when looking for PLLs by soname.
On 2016/03/26 00:30:11, Mark Seaborn wrote:
> 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.

Done.

https://codereview.chromium.org/1832623002/diff/100001/src/untrusted/pll_load...
src/untrusted/pll_loader/pll_loader.h:70: // An unordered set of "sonames" (to
see if a module has been loaded) and the
On 2016/03/26 00:30:10, Mark Seaborn wrote:
> Nit: remove "and the modules themselves"

Done.

https://codereview.chromium.org/1832623002/diff/100001/tests/pnacl_dynamic_lo...
File tests/pnacl_dynamic_loading/nacl.scons (right):

https://codereview.chromium.org/1832623002/diff/100001/tests/pnacl_dynamic_lo...
tests/pnacl_dynamic_loading/nacl.scons:120: Depends(node, test_pll_a)
On 2016/03/26 00:30:11, Mark Seaborn wrote:
> An alternative is:
> 
> CommandSelLdrTestNacl(..., extra_deps=[test_pll_a, ...])

Done.

[Sean Klein, 2016-03-28 15:57:31]

The CQ bit was checked by smklein@chromium.org

[Sean Klein, 2016-03-28 15:57:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
mseaborn@chromium.org
Link to the patchset: https://codereview.chromium.org/1832623002/#ps120001
(title: "Code review")

[commit-bot: I haz the power, 2016-03-28 15:57:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1832623002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1832623002/120001

[commit-bot: I haz the power, 2016-03-28 16:55:59]

Description was changed from

==========
PNaCl Dynamic Linking: Using unordered_set to disallow duplicate modules.

Also adds mechanism to add DSOs as sonames rather than full filenames.
The unordered_set uses sonames (rather than pathnames) as a unique identifier.

In the future, this will allow us to ignore duplicates dependencies. If, for
example, multiple modules depend on libc.so, then there should be a quick way
to identify if libc.so already exists in the list of instantiated modules.

TEST=pll_loader_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========

to

==========
PNaCl Dynamic Linking: Using unordered_set to disallow duplicate modules.

Also adds mechanism to add DSOs as sonames rather than full filenames.
The unordered_set uses sonames (rather than pathnames) as a unique identifier.

In the future, this will allow us to ignore duplicates dependencies. If, for
example, multiple modules depend on libc.so, then there should be a quick way
to identify if libc.so already exists in the list of instantiated modules.

TEST=pll_loader_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351

Committed:
https://chromium.googlesource.com/native_client/src/native_client/+/b75642447...
==========

[commit-bot: I haz the power, 2016-03-28 16:56:00]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/native_client/src/native_client/+/b75642447...