Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1825893002:
PNaCl Dynamic Linking: Added portable dependencies to shared objects. (Closed)
|
Created:
4 years, 9 months ago by Sean Klein Modified:
4 years, 8 months ago Reviewers:
Mark Seaborn CC:
native-client-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/native_client/src/native_client.git@master Target Ref:
refs/heads/master Project:
nacl Visibility:
Public. |
DescriptionPNaCl Dynamic Linking: Added portable dependencies to shared objects.
Interface:
- This patch will pass the name of any shared libraries (passed to pnacl-clang
or pnacl-ld) to the ConvertToPso pass, which can store them in a section
similar to DT_NEEDED.
- Eventually, we will have to determine a solution for translating / caching
PLLs. In the meantime, any applications using a PLL named "libfoo.so" will
expect a native shared library named "libfoo.so.translated" to exist at
runtime. This is a temporary solution; ideally the application will
translate "libfoo.so" itself.
This patch:
- Teaches drivers how to recognize and filter PLLs, passing them to opt
instead of the native linker.
- Forces the pll_loader to only load a single ELF file at a time, and
determine dependencies by reading the PLLRoot of that file. Additionally,
teaches the pll_loader how to recursively add dependencies.
- Modifies the pll_hello_world test to depend on a pll_libc using this
mechanism for "-l<name>" dependencies.
- Adds "dependencies_test" which tests recursively loading multiple dependencies
from a single PLL.
Relies on https://codereview.chromium.org/1829793002/
TEST=pll_dependencies_test, pll_hello_world_test, run_pll_loader_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
Committed: https://chromium.googlesource.com/native_client/src/native_client/+/bdf73e78fc501ba9d914f23d2686e1add56566f9
Patch Set 1 : Added basic dependencies test. Need to modify nacl.scons more. #Patch Set 2 : Merged with duplicate module detection. Updated scons file + documentation. #Patch Set 3 : TESTING (do not submit -- view for Trybot results) #Patch Set 4 : #
Total comments: 12
Patch Set 5 : Use file basename as dependency, not absolute / relative path. #
Total comments: 23
Patch Set 6 : Responded to code review #
Total comments: 15
Patch Set 7 : #Patch Set 8 : Merging #Patch Set 9 : Bumping Feature Version #
Messages
Total messages: 36 (23 generated)
Description was changed from
==========
PNaCl Dynamic Linking: Added portable dependencies to shared objects.
Interface:
- This patch expects that shared libraries (built with "pnacl-clang -shared",
and which have been finalized) have the suffix ".so". This matches the
interface for native shared libraries.
- This patch will pass the name of any shared libraries (passed to pnacl-clang
or pnacl-ld) to the ConvertToPso pass, which can store them in a section
similar to DT_NEEDED. This section does not yet have any notion of
"sonames"; it just uses the file passed from the command line.
- Eventually, we will have to determine a solution for translating / caching
PLLs. In the meantime, any applications using a PLL named "libfoo.so" will
expect a native shared library named "libfoo.so.translated" to exist at
runtime. This is a temporary solution; ideally the application will
translate "libfoo.so" itself.
This patch:
- Teaches drivers how to recognize and filter PLLs, passing them to opt
instead of the native linker.
- Forces the pll_loader to only load a single ELF file at a time, and
determine dependencies by reading the PLLRoot of that file. Additionally,
teaches the pll_Loader how to recursively add dependencies.
- Modifies the pll_hello_world test to depend on a pll_libc using this
mechanism for "-l<name>" dependencies.
TODO:
- FEATURE_VERSION bump
- Upload corresponding LLVM patch
- Finish removing logging statements
- Add documentation to codebase somewhere?
- Avoid circular / repeaded libs in AddbByFilename
TEST=run_pll_hello_world_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========
to
==========
PNaCl Dynamic Linking: Added portable dependencies to shared objects.
Interface:
- This patch expects that shared libraries (built with "pnacl-clang -shared",
and which have been finalized) have the suffix ".so". This matches the
interface for native shared libraries.
- This patch will pass the name of any shared libraries (passed to pnacl-clang
or pnacl-ld) to the ConvertToPso pass, which can store them in a section
similar to DT_NEEDED. This section does not yet have any notion of
"sonames"; it just uses the file passed from the command line.
- Eventually, we will have to determine a solution for translating / caching
PLLs. In the meantime, any applications using a PLL named "libfoo.so" will
expect a native shared library named "libfoo.so.translated" to exist at
runtime. This is a temporary solution; ideally the application will
translate "libfoo.so" itself.
This patch:
- Teaches drivers how to recognize and filter PLLs, passing them to opt
instead of the native linker.
- Forces the pll_loader to only load a single ELF file at a time, and
determine dependencies by reading the PLLRoot of that file. Additionally,
teaches the pll_Loader how to recursively add dependencies.
- Modifies the pll_hello_world test to depend on a pll_libc using this
mechanism for "-l<name>" dependencies.
TODO:
- FEATURE_VERSION bump
- Upload corresponding LLVM patch
- Finish removing logging statements
- Add documentation to codebase somewhere?
- Avoid circular / repeaded libs in AddbByFilename
Relies on https://codereview.chromium.org/1829793002/
TEST=run_pll_hello_world_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========
Description was changed from
==========
PNaCl Dynamic Linking: Added portable dependencies to shared objects.
Interface:
- This patch expects that shared libraries (built with "pnacl-clang -shared",
and which have been finalized) have the suffix ".so". This matches the
interface for native shared libraries.
- This patch will pass the name of any shared libraries (passed to pnacl-clang
or pnacl-ld) to the ConvertToPso pass, which can store them in a section
similar to DT_NEEDED. This section does not yet have any notion of
"sonames"; it just uses the file passed from the command line.
- Eventually, we will have to determine a solution for translating / caching
PLLs. In the meantime, any applications using a PLL named "libfoo.so" will
expect a native shared library named "libfoo.so.translated" to exist at
runtime. This is a temporary solution; ideally the application will
translate "libfoo.so" itself.
This patch:
- Teaches drivers how to recognize and filter PLLs, passing them to opt
instead of the native linker.
- Forces the pll_loader to only load a single ELF file at a time, and
determine dependencies by reading the PLLRoot of that file. Additionally,
teaches the pll_Loader how to recursively add dependencies.
- Modifies the pll_hello_world test to depend on a pll_libc using this
mechanism for "-l<name>" dependencies.
TODO:
- FEATURE_VERSION bump
- Upload corresponding LLVM patch
- Finish removing logging statements
- Add documentation to codebase somewhere?
- Avoid circular / repeaded libs in AddbByFilename
Relies on https://codereview.chromium.org/1829793002/
TEST=run_pll_hello_world_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========
to
==========
PNaCl Dynamic Linking: Added portable dependencies to shared objects.
Interface:
- This patch expects that shared libraries (built with "pnacl-clang -shared",
and which have been finalized) have the suffix ".so". This matches the
interface for native shared libraries.
- This patch will pass the name of any shared libraries (passed to pnacl-clang
or pnacl-ld) to the ConvertToPso pass, which can store them in a section
similar to DT_NEEDED. This section does not yet have any notion of
"sonames"; it just uses the file passed from the command line.
- Eventually, we will have to determine a solution for translating / caching
PLLs. In the meantime, any applications using a PLL named "libfoo.so" will
expect a native shared library named "libfoo.so.translated" to exist at
runtime. This is a temporary solution; ideally the application will
translate "libfoo.so" itself.
This patch:
- Teaches drivers how to recognize and filter PLLs, passing them to opt
instead of the native linker.
- Forces the pll_loader to only load a single ELF file at a time, and
determine dependencies by reading the PLLRoot of that file. Additionally,
teaches the pll_Loader how to recursively add dependencies.
- Modifies the pll_hello_world test to depend on a pll_libc using this
mechanism for "-l<name>" dependencies.
TODO:
- Upload corresponding LLVM patch
- Finish removing logging statements
- Add documentation to codebase somewhere?
- Avoid circular / repeaded libs in AddbByFilename
Relies on https://codereview.chromium.org/1829793002/
TEST=run_pll_hello_world_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========
Description was changed from
==========
PNaCl Dynamic Linking: Added portable dependencies to shared objects.
Interface:
- This patch expects that shared libraries (built with "pnacl-clang -shared",
and which have been finalized) have the suffix ".so". This matches the
interface for native shared libraries.
- This patch will pass the name of any shared libraries (passed to pnacl-clang
or pnacl-ld) to the ConvertToPso pass, which can store them in a section
similar to DT_NEEDED. This section does not yet have any notion of
"sonames"; it just uses the file passed from the command line.
- Eventually, we will have to determine a solution for translating / caching
PLLs. In the meantime, any applications using a PLL named "libfoo.so" will
expect a native shared library named "libfoo.so.translated" to exist at
runtime. This is a temporary solution; ideally the application will
translate "libfoo.so" itself.
This patch:
- Teaches drivers how to recognize and filter PLLs, passing them to opt
instead of the native linker.
- Forces the pll_loader to only load a single ELF file at a time, and
determine dependencies by reading the PLLRoot of that file. Additionally,
teaches the pll_Loader how to recursively add dependencies.
- Modifies the pll_hello_world test to depend on a pll_libc using this
mechanism for "-l<name>" dependencies.
TODO:
- Upload corresponding LLVM patch
- Finish removing logging statements
- Add documentation to codebase somewhere?
- Avoid circular / repeaded libs in AddbByFilename
Relies on https://codereview.chromium.org/1829793002/
TEST=run_pll_hello_world_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========
to
==========
PNaCl Dynamic Linking: Added portable dependencies to shared objects.
Interface:
- This patch expects that shared libraries (built with "pnacl-clang -shared",
and which have been finalized) have the suffix ".so". This matches the
interface for native shared libraries.
- This patch will pass the name of any shared libraries (passed to pnacl-clang
or pnacl-ld) to the ConvertToPso pass, which can store them in a section
similar to DT_NEEDED. This section does not yet have any notion of
"sonames"; it just uses the file passed from the command line.
- Eventually, we will have to determine a solution for translating / caching
PLLs. In the meantime, any applications using a PLL named "libfoo.so" will
expect a native shared library named "libfoo.so.translated" to exist at
runtime. This is a temporary solution; ideally the application will
translate "libfoo.so" itself.
This patch:
- Teaches drivers how to recognize and filter PLLs, passing them to opt
instead of the native linker.
- Forces the pll_loader to only load a single ELF file at a time, and
determine dependencies by reading the PLLRoot of that file. Additionally,
teaches the pll_Loader how to recursively add dependencies.
- Modifies the pll_hello_world test to depend on a pll_libc using this
mechanism for "-l<name>" dependencies.
TODO:
- Upload corresponding LLVM patch
- Finish removing logging statements
- Add documentation to codebase somewhere?
Relies on https://codereview.chromium.org/1829793002/
TEST=run_pll_hello_world_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========
Description was changed from
==========
PNaCl Dynamic Linking: Added portable dependencies to shared objects.
Interface:
- This patch expects that shared libraries (built with "pnacl-clang -shared",
and which have been finalized) have the suffix ".so". This matches the
interface for native shared libraries.
- This patch will pass the name of any shared libraries (passed to pnacl-clang
or pnacl-ld) to the ConvertToPso pass, which can store them in a section
similar to DT_NEEDED. This section does not yet have any notion of
"sonames"; it just uses the file passed from the command line.
- Eventually, we will have to determine a solution for translating / caching
PLLs. In the meantime, any applications using a PLL named "libfoo.so" will
expect a native shared library named "libfoo.so.translated" to exist at
runtime. This is a temporary solution; ideally the application will
translate "libfoo.so" itself.
This patch:
- Teaches drivers how to recognize and filter PLLs, passing them to opt
instead of the native linker.
- Forces the pll_loader to only load a single ELF file at a time, and
determine dependencies by reading the PLLRoot of that file. Additionally,
teaches the pll_Loader how to recursively add dependencies.
- Modifies the pll_hello_world test to depend on a pll_libc using this
mechanism for "-l<name>" dependencies.
TODO:
- Upload corresponding LLVM patch
- Finish removing logging statements
- Add documentation to codebase somewhere?
Relies on https://codereview.chromium.org/1829793002/
TEST=run_pll_hello_world_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========
to
==========
PNaCl Dynamic Linking: Added portable dependencies to shared objects.
Interface:
- This patch expects that shared libraries (built with "pnacl-clang -shared",
and which have been finalized) have the suffix ".so". This matches the
interface for native shared libraries.
- This patch will pass the name of any shared libraries (passed to pnacl-clang
or pnacl-ld) to the ConvertToPso pass, which can store them in a section
similar to DT_NEEDED. This section does not yet have any notion of
"sonames"; it just uses the file passed from the command line.
- Eventually, we will have to determine a solution for translating / caching
PLLs. In the meantime, any applications using a PLL named "libfoo.so" will
expect a native shared library named "libfoo.so.translated" to exist at
runtime. This is a temporary solution; ideally the application will
translate "libfoo.so" itself.
This patch:
- Teaches drivers how to recognize and filter PLLs, passing them to opt
instead of the native linker.
- Forces the pll_loader to only load a single ELF file at a time, and
determine dependencies by reading the PLLRoot of that file. Additionally,
teaches the pll_Loader how to recursively add dependencies.
- Modifies the pll_hello_world test to depend on a pll_libc using this
mechanism for "-l<name>" dependencies.
TODO:
- Upload corresponding LLVM patch
- Finish removing logging statements
- Add documentation to codebase somewhere?
- Add a test more complex than just "hello_world"
Relies on https://codereview.chromium.org/1829793002/
TEST=run_pll_hello_world_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========
Description was changed from
==========
PNaCl Dynamic Linking: Added portable dependencies to shared objects.
Interface:
- This patch expects that shared libraries (built with "pnacl-clang -shared",
and which have been finalized) have the suffix ".so". This matches the
interface for native shared libraries.
- This patch will pass the name of any shared libraries (passed to pnacl-clang
or pnacl-ld) to the ConvertToPso pass, which can store them in a section
similar to DT_NEEDED. This section does not yet have any notion of
"sonames"; it just uses the file passed from the command line.
- Eventually, we will have to determine a solution for translating / caching
PLLs. In the meantime, any applications using a PLL named "libfoo.so" will
expect a native shared library named "libfoo.so.translated" to exist at
runtime. This is a temporary solution; ideally the application will
translate "libfoo.so" itself.
This patch:
- Teaches drivers how to recognize and filter PLLs, passing them to opt
instead of the native linker.
- Forces the pll_loader to only load a single ELF file at a time, and
determine dependencies by reading the PLLRoot of that file. Additionally,
teaches the pll_Loader how to recursively add dependencies.
- Modifies the pll_hello_world test to depend on a pll_libc using this
mechanism for "-l<name>" dependencies.
TODO:
- Upload corresponding LLVM patch
- Finish removing logging statements
- Add documentation to codebase somewhere?
- Add a test more complex than just "hello_world"
Relies on https://codereview.chromium.org/1829793002/
TEST=run_pll_hello_world_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========
to
==========
PNaCl Dynamic Linking: Added portable dependencies to shared objects.
Interface:
- This patch expects that shared libraries (built with "pnacl-clang -shared",
and which have been finalized) have the suffix ".so". This matches the
interface for native shared libraries.
- This patch will pass the name of any shared libraries (passed to pnacl-clang
or pnacl-ld) to the ConvertToPso pass, which can store them in a section
similar to DT_NEEDED. This section does not yet have any notion of
"sonames"; it just uses the file passed from the command line.
- Eventually, we will have to determine a solution for translating / caching
PLLs. In the meantime, any applications using a PLL named "libfoo.so" will
expect a native shared library named "libfoo.so.translated" to exist at
runtime. This is a temporary solution; ideally the application will
translate "libfoo.so" itself.
This patch:
- Teaches drivers how to recognize and filter PLLs, passing them to opt
instead of the native linker.
- Forces the pll_loader to only load a single ELF file at a time, and
determine dependencies by reading the PLLRoot of that file. Additionally,
teaches the pll_Loader how to recursively add dependencies.
- Modifies the pll_hello_world test to depend on a pll_libc using this
mechanism for "-l<name>" dependencies.
TODO:
- Upload corresponding LLVM patch
- Add a test more complex than just "hello_world"
Relies on https://codereview.chromium.org/1829793002/
TEST=run_pll_hello_world_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========
Description was changed from
==========
PNaCl Dynamic Linking: Added portable dependencies to shared objects.
Interface:
- This patch expects that shared libraries (built with "pnacl-clang -shared",
and which have been finalized) have the suffix ".so". This matches the
interface for native shared libraries.
- This patch will pass the name of any shared libraries (passed to pnacl-clang
or pnacl-ld) to the ConvertToPso pass, which can store them in a section
similar to DT_NEEDED. This section does not yet have any notion of
"sonames"; it just uses the file passed from the command line.
- Eventually, we will have to determine a solution for translating / caching
PLLs. In the meantime, any applications using a PLL named "libfoo.so" will
expect a native shared library named "libfoo.so.translated" to exist at
runtime. This is a temporary solution; ideally the application will
translate "libfoo.so" itself.
This patch:
- Teaches drivers how to recognize and filter PLLs, passing them to opt
instead of the native linker.
- Forces the pll_loader to only load a single ELF file at a time, and
determine dependencies by reading the PLLRoot of that file. Additionally,
teaches the pll_Loader how to recursively add dependencies.
- Modifies the pll_hello_world test to depend on a pll_libc using this
mechanism for "-l<name>" dependencies.
TODO:
- Upload corresponding LLVM patch
- Add a test more complex than just "hello_world"
Relies on https://codereview.chromium.org/1829793002/
TEST=run_pll_hello_world_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========
to
==========
PNaCl Dynamic Linking: Added portable dependencies to shared objects.
Interface:
- This patch expects that shared libraries (built with "pnacl-clang -shared",
and which have been finalized) have the suffix ".so". This matches the
interface for native shared libraries.
- This patch will pass the name of any shared libraries (passed to pnacl-clang
or pnacl-ld) to the ConvertToPso pass, which can store them in a section
similar to DT_NEEDED. This section does not yet have any notion of
"sonames"; it just uses the file passed from the command line.
- Eventually, we will have to determine a solution for translating / caching
PLLs. In the meantime, any applications using a PLL named "libfoo.so" will
expect a native shared library named "libfoo.so.translated" to exist at
runtime. This is a temporary solution; ideally the application will
translate "libfoo.so" itself.
This patch:
- Teaches drivers how to recognize and filter PLLs, passing them to opt
instead of the native linker.
- Forces the pll_loader to only load a single ELF file at a time, and
determine dependencies by reading the PLLRoot of that file. Additionally,
teaches the pll_Loader how to recursively add dependencies.
- Modifies the pll_hello_world test to depend on a pll_libc using this
mechanism for "-l<name>" dependencies.
TODO:
- Upload corresponding LLVM patch
Relies on https://codereview.chromium.org/1829793002/
TEST=pll_dependencies_test, pll_hello_world_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========
Description was changed from
==========
PNaCl Dynamic Linking: Added portable dependencies to shared objects.
Interface:
- This patch expects that shared libraries (built with "pnacl-clang -shared",
and which have been finalized) have the suffix ".so". This matches the
interface for native shared libraries.
- This patch will pass the name of any shared libraries (passed to pnacl-clang
or pnacl-ld) to the ConvertToPso pass, which can store them in a section
similar to DT_NEEDED. This section does not yet have any notion of
"sonames"; it just uses the file passed from the command line.
- Eventually, we will have to determine a solution for translating / caching
PLLs. In the meantime, any applications using a PLL named "libfoo.so" will
expect a native shared library named "libfoo.so.translated" to exist at
runtime. This is a temporary solution; ideally the application will
translate "libfoo.so" itself.
This patch:
- Teaches drivers how to recognize and filter PLLs, passing them to opt
instead of the native linker.
- Forces the pll_loader to only load a single ELF file at a time, and
determine dependencies by reading the PLLRoot of that file. Additionally,
teaches the pll_Loader how to recursively add dependencies.
- Modifies the pll_hello_world test to depend on a pll_libc using this
mechanism for "-l<name>" dependencies.
TODO:
- Upload corresponding LLVM patch
Relies on https://codereview.chromium.org/1829793002/
TEST=pll_dependencies_test, pll_hello_world_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========
to
==========
PNaCl Dynamic Linking: Added portable dependencies to shared objects.
Interface:
- This patch expects that shared libraries (built with "pnacl-clang -shared",
and which have been finalized) have the suffix ".so". This matches the
interface for native shared libraries.
- This patch will pass the name of any shared libraries (passed to pnacl-clang
or pnacl-ld) to the ConvertToPso pass, which can store them in a section
similar to DT_NEEDED. This section does not yet have any notion of
"sonames"; it just uses the file passed from the command line.
- Eventually, we will have to determine a solution for translating / caching
PLLs. In the meantime, any applications using a PLL named "libfoo.so" will
expect a native shared library named "libfoo.so.translated" to exist at
runtime. This is a temporary solution; ideally the application will
translate "libfoo.so" itself.
This patch:
- Teaches drivers how to recognize and filter PLLs, passing them to opt
instead of the native linker.
- Forces the pll_loader to only load a single ELF file at a time, and
determine dependencies by reading the PLLRoot of that file. Additionally,
teaches the pll_loader how to recursively add dependencies.
- Modifies the pll_hello_world test to depend on a pll_libc using this
mechanism for "-l<name>" dependencies.
- Adds "dependencies_test" which tests recursively loading multiple
dependencies
from a single PLL.
TODO:
- Upload corresponding LLVM patch
Relies on https://codereview.chromium.org/1829793002/
TEST=pll_dependencies_test, pll_hello_world_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========
Description was changed from
==========
PNaCl Dynamic Linking: Added portable dependencies to shared objects.
Interface:
- This patch expects that shared libraries (built with "pnacl-clang -shared",
and which have been finalized) have the suffix ".so". This matches the
interface for native shared libraries.
- This patch will pass the name of any shared libraries (passed to pnacl-clang
or pnacl-ld) to the ConvertToPso pass, which can store them in a section
similar to DT_NEEDED. This section does not yet have any notion of
"sonames"; it just uses the file passed from the command line.
- Eventually, we will have to determine a solution for translating / caching
PLLs. In the meantime, any applications using a PLL named "libfoo.so" will
expect a native shared library named "libfoo.so.translated" to exist at
runtime. This is a temporary solution; ideally the application will
translate "libfoo.so" itself.
This patch:
- Teaches drivers how to recognize and filter PLLs, passing them to opt
instead of the native linker.
- Forces the pll_loader to only load a single ELF file at a time, and
determine dependencies by reading the PLLRoot of that file. Additionally,
teaches the pll_loader how to recursively add dependencies.
- Modifies the pll_hello_world test to depend on a pll_libc using this
mechanism for "-l<name>" dependencies.
- Adds "dependencies_test" which tests recursively loading multiple
dependencies
from a single PLL.
TODO:
- Upload corresponding LLVM patch
Relies on https://codereview.chromium.org/1829793002/
TEST=pll_dependencies_test, pll_hello_world_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========
to
==========
PNaCl Dynamic Linking: Added portable dependencies to shared objects.
Interface:
- This patch expects that shared libraries (built with "pnacl-clang -shared",
and which have been finalized) have the suffix ".so". This matches the
interface for native shared libraries.
- This patch will pass the name of any shared libraries (passed to pnacl-clang
or pnacl-ld) to the ConvertToPso pass, which can store them in a section
similar to DT_NEEDED. This section does not yet have any notion of
"sonames"; it just uses the file passed from the command line.
- Eventually, we will have to determine a solution for translating / caching
PLLs. In the meantime, any applications using a PLL named "libfoo.so" will
expect a native shared library named "libfoo.so.translated" to exist at
runtime. This is a temporary solution; ideally the application will
translate "libfoo.so" itself.
This patch:
- Teaches drivers how to recognize and filter PLLs, passing them to opt
instead of the native linker.
- Forces the pll_loader to only load a single ELF file at a time, and
determine dependencies by reading the PLLRoot of that file. Additionally,
teaches the pll_loader how to recursively add dependencies.
- Modifies the pll_hello_world test to depend on a pll_libc using this
mechanism for "-l<name>" dependencies.
- Adds "dependencies_test" which tests recursively loading multiple
dependencies
from a single PLL.
Relies on https://codereview.chromium.org/1829793002/
TEST=pll_dependencies_test, pll_hello_world_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========
Patchset #6 (id:100001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #3 (id:120001) has been deleted
smklein@chromium.org changed reviewers: + mseaborn@chromium.org
Description was changed from
==========
PNaCl Dynamic Linking: Added portable dependencies to shared objects.
Interface:
- This patch expects that shared libraries (built with "pnacl-clang -shared",
and which have been finalized) have the suffix ".so". This matches the
interface for native shared libraries.
- This patch will pass the name of any shared libraries (passed to pnacl-clang
or pnacl-ld) to the ConvertToPso pass, which can store them in a section
similar to DT_NEEDED. This section does not yet have any notion of
"sonames"; it just uses the file passed from the command line.
- Eventually, we will have to determine a solution for translating / caching
PLLs. In the meantime, any applications using a PLL named "libfoo.so" will
expect a native shared library named "libfoo.so.translated" to exist at
runtime. This is a temporary solution; ideally the application will
translate "libfoo.so" itself.
This patch:
- Teaches drivers how to recognize and filter PLLs, passing them to opt
instead of the native linker.
- Forces the pll_loader to only load a single ELF file at a time, and
determine dependencies by reading the PLLRoot of that file. Additionally,
teaches the pll_loader how to recursively add dependencies.
- Modifies the pll_hello_world test to depend on a pll_libc using this
mechanism for "-l<name>" dependencies.
- Adds "dependencies_test" which tests recursively loading multiple
dependencies
from a single PLL.
Relies on https://codereview.chromium.org/1829793002/
TEST=pll_dependencies_test, pll_hello_world_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========
to
==========
PNaCl Dynamic Linking: Added portable dependencies to shared objects.
Interface:
- This patch will pass the name of any shared libraries (passed to pnacl-clang
or pnacl-ld) to the ConvertToPso pass, which can store them in a section
similar to DT_NEEDED. This section does not yet have any notion of
"sonames"; it just uses the filename passed from the command line.
- Eventually, we will have to determine a solution for translating / caching
PLLs. In the meantime, any applications using a PLL named "libfoo.so" will
expect a native shared library named "libfoo.so.translated" to exist at
runtime. This is a temporary solution; ideally the application will
translate "libfoo.so" itself.
This patch:
- Teaches drivers how to recognize and filter PLLs, passing them to opt
instead of the native linker.
- Forces the pll_loader to only load a single ELF file at a time, and
determine dependencies by reading the PLLRoot of that file. Additionally,
teaches the pll_loader how to recursively add dependencies.
- Modifies the pll_hello_world test to depend on a pll_libc using this
mechanism for "-l<name>" dependencies.
- Adds "dependencies_test" which tests recursively loading multiple
dependencies
from a single PLL.
Relies on https://codereview.chromium.org/1829793002/
TEST=pll_dependencies_test, pll_hello_world_test, run_pll_loader_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========
This latest patch has merged in the changes from: https://codereview.chromium.org/1841113002/ and also updated the format of the "dependency". Previously, the dependencies were stored as file paths. They are now stored as base file names. E.g.: "/foo/bar/baz.so" --> "baz.so". This means I had to update the pll_loader to use the "search path" and "soname" functions.
I think this bit in the commit message needs updating: "This section does not yet have any notion of "sonames"; it just uses the filename passed from the command line." https://codereview.chromium.org/1825893002/diff/160001/pnacl/driver/filetype.py File pnacl/driver/filetype.py (right): https://codereview.chromium.org/1825893002/diff/160001/pnacl/driver/filetype.... pnacl/driver/filetype.py:356: # Although this file looks like a native ".so", it actually is in a "looks like" -> "has the same extension as"? https://codereview.chromium.org/1825893002/diff/160001/pnacl/driver/pnacl-ld.py File pnacl/driver/pnacl-ld.py (right): https://codereview.chromium.org/1825893002/diff/160001/pnacl/driver/pnacl-ld.... pnacl/driver/pnacl-ld.py:348: (plls, inputs) = FilterPlls(inputs) Nit: You don't need ()s around "plls, inputs" here. https://codereview.chromium.org/1825893002/diff/160001/src/untrusted/pll_load... File src/untrusted/pll_loader/pll_loader.cc (right): https://codereview.chromium.org/1825893002/diff/160001/src/untrusted/pll_load... src/untrusted/pll_loader/pll_loader.cc:94: // TODO(smklein): Deduplicate rather than failing once dependencies are added. You can remove this since you've addressed it. https://codereview.chromium.org/1825893002/diff/160001/src/untrusted/pll_load... src/untrusted/pll_loader/pll_loader.cc:125: const PLLModule module = PLLModule((const PLLRoot *) pso_root); Just: PLLModule module((const PLLRoot *) pso_root); https://codereview.chromium.org/1825893002/diff/160001/src/untrusted/pll_load... src/untrusted/pll_loader/pll_loader.cc:129: size_t string_index = 0; Maybe "offset" rather than "index"? https://codereview.chromium.org/1825893002/diff/160001/tests/pnacl_dynamic_lo... File tests/pnacl_dynamic_loading/dependencies_test.c (right): https://codereview.chromium.org/1825893002/diff/160001/tests/pnacl_dynamic_lo... tests/pnacl_dynamic_loading/dependencies_test.c:32: int main(int argc, char* argv[]) { Use " *" spacing https://codereview.chromium.org/1825893002/diff/180001/pnacl/driver/pnacl-ld.py File pnacl/driver/pnacl-ld.py (right): https://codereview.chromium.org/1825893002/diff/180001/pnacl/driver/pnacl-ld.... pnacl/driver/pnacl-ld.py:528: pll = [] Nit: "plls" plural? https://codereview.chromium.org/1825893002/diff/180001/pnacl/driver/pnacl-ld.... pnacl/driver/pnacl-ld.py:532: if ldtools.IsFlag(f): How about simplifying to "if ldtools.IsFlag(f) and not filetype.IsPll(f)"? https://codereview.chromium.org/1825893002/diff/180001/src/untrusted/pll_load... File src/untrusted/pll_loader/pll_loader.cc (right): https://codereview.chromium.org/1825893002/diff/180001/src/untrusted/pll_load... src/untrusted/pll_loader/pll_loader.cc:96: // This module has already been added to the ModuleSet. I think your current test case doesn't actually verify that a given soname isn't loaded twice. If you wanted to test that, you could have AddBySoname() return the PLLRoot*, and change pll_loader_test.cc t check that it returns the same pointer when called a second time for the same soname. (You could do that as a separate change.) https://codereview.chromium.org/1825893002/diff/180001/src/untrusted/pll_load... File src/untrusted/pll_loader/pll_loader.h (right): https://codereview.chromium.org/1825893002/diff/180001/src/untrusted/pll_load... src/untrusted/pll_loader/pll_loader.h:32: const char *GetDependenciesList() const { Since these new getters don't add much, you might consider doing root()->dependencies_list instead. https://codereview.chromium.org/1825893002/diff/180001/src/untrusted/pll_load... File src/untrusted/pll_loader/pll_loader_main.cc (right): https://codereview.chromium.org/1825893002/diff/180001/src/untrusted/pll_load... src/untrusted/pll_loader/pll_loader_main.cc:28: modset.AddBySoname(argv[2]); Can you make this one AddByFilename() rather than AddBySoname(), since executables won't usually be expected to be on the library search path? https://codereview.chromium.org/1825893002/diff/180001/tests/pnacl_dynamic_lo... File tests/pnacl_dynamic_loading/dependencies_test.c (right): https://codereview.chromium.org/1825893002/diff/180001/tests/pnacl_dynamic_lo... tests/pnacl_dynamic_loading/dependencies_test.c:33: printf("%d\n", get_module_a_var()); I would be inclined to use ASSERT_EQ(get_module_a_var(), 2345); Then you don't need the golden file. But that does assume that you can be sure that main() gets called. :-) If you're worried about that, you could do both: use ASSERT_EQ, but also do printf("main() called\n") and have the golden file check that. https://codereview.chromium.org/1825893002/diff/180001/tests/pnacl_dynamic_lo... File tests/pnacl_dynamic_loading/nacl.scons (right): https://codereview.chromium.org/1825893002/diff/180001/tests/pnacl_dynamic_lo... tests/pnacl_dynamic_loading/nacl.scons:28: Style nit: In Python it's preferred to have 2 empty lines between top-level function decls, so can you preserve that when the code has it already? https://codereview.chromium.org/1825893002/diff/180001/tests/pnacl_dynamic_lo... tests/pnacl_dynamic_loading/nacl.scons:60: if dependencies != []: "len(dependencies) != 0" is arguably more idiomatic in Python since it works for any iterable type. Or you could make this unconditional. https://codereview.chromium.org/1825893002/diff/180001/tests/pnacl_dynamic_lo... tests/pnacl_dynamic_loading/nacl.scons:67: for d in dependencies: I think you can do Depends(nonfinal_pll, dependencies) for brevity https://codereview.chromium.org/1825893002/diff/180001/tests/pnacl_dynamic_lo... tests/pnacl_dynamic_loading/nacl.scons:106: # use the "-l" linker flags and don't explicitly declare any LLVM passes. Since you're adding "-l" in this change, you could address this TODO. https://codereview.chromium.org/1825893002/diff/180001/tests/pnacl_dynamic_lo... tests/pnacl_dynamic_loading/nacl.scons:198: pll_dependencies_test]) Nit: indent to align with "["
Description was changed from
==========
PNaCl Dynamic Linking: Added portable dependencies to shared objects.
Interface:
- This patch will pass the name of any shared libraries (passed to pnacl-clang
or pnacl-ld) to the ConvertToPso pass, which can store them in a section
similar to DT_NEEDED. This section does not yet have any notion of
"sonames"; it just uses the filename passed from the command line.
- Eventually, we will have to determine a solution for translating / caching
PLLs. In the meantime, any applications using a PLL named "libfoo.so" will
expect a native shared library named "libfoo.so.translated" to exist at
runtime. This is a temporary solution; ideally the application will
translate "libfoo.so" itself.
This patch:
- Teaches drivers how to recognize and filter PLLs, passing them to opt
instead of the native linker.
- Forces the pll_loader to only load a single ELF file at a time, and
determine dependencies by reading the PLLRoot of that file. Additionally,
teaches the pll_loader how to recursively add dependencies.
- Modifies the pll_hello_world test to depend on a pll_libc using this
mechanism for "-l<name>" dependencies.
- Adds "dependencies_test" which tests recursively loading multiple
dependencies
from a single PLL.
Relies on https://codereview.chromium.org/1829793002/
TEST=pll_dependencies_test, pll_hello_world_test, run_pll_loader_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========
to
==========
PNaCl Dynamic Linking: Added portable dependencies to shared objects.
Interface:
- This patch will pass the name of any shared libraries (passed to pnacl-clang
or pnacl-ld) to the ConvertToPso pass, which can store them in a section
similar to DT_NEEDED.
- Eventually, we will have to determine a solution for translating / caching
PLLs. In the meantime, any applications using a PLL named "libfoo.so" will
expect a native shared library named "libfoo.so.translated" to exist at
runtime. This is a temporary solution; ideally the application will
translate "libfoo.so" itself.
This patch:
- Teaches drivers how to recognize and filter PLLs, passing them to opt
instead of the native linker.
- Forces the pll_loader to only load a single ELF file at a time, and
determine dependencies by reading the PLLRoot of that file. Additionally,
teaches the pll_loader how to recursively add dependencies.
- Modifies the pll_hello_world test to depend on a pll_libc using this
mechanism for "-l<name>" dependencies.
- Adds "dependencies_test" which tests recursively loading multiple
dependencies
from a single PLL.
Relies on https://codereview.chromium.org/1829793002/
TEST=pll_dependencies_test, pll_hello_world_test, run_pll_loader_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========
https://codereview.chromium.org/1825893002/diff/180001/tests/pnacl_dynamic_lo... File tests/pnacl_dynamic_loading/dependencies_test.c (right): https://codereview.chromium.org/1825893002/diff/180001/tests/pnacl_dynamic_lo... tests/pnacl_dynamic_loading/dependencies_test.c:33: printf("%d\n", get_module_a_var()); On 2016/03/30 23:24:57, Mark Seaborn wrote: > I would be inclined to use > ASSERT_EQ(get_module_a_var(), 2345); As you discovered, that currently doesn't work, because it refers to stderr, which (oddly) generates a reference to a TLS variable in newlib. So let's leave these as printf() calls until we have cross-module TLS references implemented.
Patchset #6 (id:200001) has been deleted
https://codereview.chromium.org/1825893002/diff/160001/pnacl/driver/filetype.py File pnacl/driver/filetype.py (right): https://codereview.chromium.org/1825893002/diff/160001/pnacl/driver/filetype.... pnacl/driver/filetype.py:356: # Although this file looks like a native ".so", it actually is in a On 2016/03/30 23:24:57, Mark Seaborn wrote: > "looks like" -> "has the same extension as"? Done. https://codereview.chromium.org/1825893002/diff/160001/pnacl/driver/pnacl-ld.py File pnacl/driver/pnacl-ld.py (right): https://codereview.chromium.org/1825893002/diff/160001/pnacl/driver/pnacl-ld.... pnacl/driver/pnacl-ld.py:348: (plls, inputs) = FilterPlls(inputs) On 2016/03/30 23:24:57, Mark Seaborn wrote: > Nit: You don't need ()s around "plls, inputs" here. Done. https://codereview.chromium.org/1825893002/diff/160001/src/untrusted/pll_load... File src/untrusted/pll_loader/pll_loader.cc (right): https://codereview.chromium.org/1825893002/diff/160001/src/untrusted/pll_load... src/untrusted/pll_loader/pll_loader.cc:94: // TODO(smklein): Deduplicate rather than failing once dependencies are added. On 2016/03/30 23:24:57, Mark Seaborn wrote: > You can remove this since you've addressed it. Done. https://codereview.chromium.org/1825893002/diff/160001/src/untrusted/pll_load... src/untrusted/pll_loader/pll_loader.cc:125: const PLLModule module = PLLModule((const PLLRoot *) pso_root); On 2016/03/30 23:24:57, Mark Seaborn wrote: > Just: > PLLModule module((const PLLRoot *) pso_root); Done, but with const as well. https://codereview.chromium.org/1825893002/diff/160001/src/untrusted/pll_load... src/untrusted/pll_loader/pll_loader.cc:129: size_t string_index = 0; On 2016/03/30 23:24:57, Mark Seaborn wrote: > Maybe "offset" rather than "index"? Done. https://codereview.chromium.org/1825893002/diff/160001/tests/pnacl_dynamic_lo... File tests/pnacl_dynamic_loading/dependencies_test.c (right): https://codereview.chromium.org/1825893002/diff/160001/tests/pnacl_dynamic_lo... tests/pnacl_dynamic_loading/dependencies_test.c:32: int main(int argc, char* argv[]) { On 2016/03/30 23:24:57, Mark Seaborn wrote: > Use " *" spacing Done. https://codereview.chromium.org/1825893002/diff/180001/pnacl/driver/pnacl-ld.py File pnacl/driver/pnacl-ld.py (right): https://codereview.chromium.org/1825893002/diff/180001/pnacl/driver/pnacl-ld.... pnacl/driver/pnacl-ld.py:528: pll = [] On 2016/03/30 23:24:57, Mark Seaborn wrote: > Nit: "plls" plural? Done. https://codereview.chromium.org/1825893002/diff/180001/pnacl/driver/pnacl-ld.... pnacl/driver/pnacl-ld.py:532: if ldtools.IsFlag(f): On 2016/03/30 23:24:57, Mark Seaborn wrote: > How about simplifying to "if ldtools.IsFlag(f) and not filetype.IsPll(f)"? I'm pretty sure you mean "or", not "and". Updating to that version. https://codereview.chromium.org/1825893002/diff/180001/src/untrusted/pll_load... File src/untrusted/pll_loader/pll_loader.cc (right): https://codereview.chromium.org/1825893002/diff/180001/src/untrusted/pll_load... src/untrusted/pll_loader/pll_loader.cc:96: // This module has already been added to the ModuleSet. On 2016/03/30 23:24:57, Mark Seaborn wrote: > I think your current test case doesn't actually verify that a given soname isn't > loaded twice. > > If you wanted to test that, you could have AddBySoname() return the PLLRoot*, > and change pll_loader_test.cc t check that it returns the same pointer when > called a second time for the same soname. (You could do that as a separate > change.) This is behavior I'm adding in this CL; I'll test it in this CL. Instead of checking for the same PLLRoot, I'll check that the second invocation returns NULL. This will require returning "void *" from AddByFilename as well. https://codereview.chromium.org/1825893002/diff/180001/src/untrusted/pll_load... File src/untrusted/pll_loader/pll_loader.h (right): https://codereview.chromium.org/1825893002/diff/180001/src/untrusted/pll_load... src/untrusted/pll_loader/pll_loader.h:32: const char *GetDependenciesList() const { On 2016/03/30 23:24:57, Mark Seaborn wrote: > Since these new getters don't add much, you might consider doing > root()->dependencies_list instead. I'll update "PLLModule::root()" to be a const function, and remove these getters. https://codereview.chromium.org/1825893002/diff/180001/src/untrusted/pll_load... File src/untrusted/pll_loader/pll_loader_main.cc (right): https://codereview.chromium.org/1825893002/diff/180001/src/untrusted/pll_load... src/untrusted/pll_loader/pll_loader_main.cc:28: modset.AddBySoname(argv[2]); On 2016/03/30 23:24:57, Mark Seaborn wrote: > Can you make this one AddByFilename() rather than AddBySoname(), since > executables won't usually be expected to be on the library search path? Done. https://codereview.chromium.org/1825893002/diff/180001/tests/pnacl_dynamic_lo... File tests/pnacl_dynamic_loading/dependencies_test.c (right): https://codereview.chromium.org/1825893002/diff/180001/tests/pnacl_dynamic_lo... tests/pnacl_dynamic_loading/dependencies_test.c:33: printf("%d\n", get_module_a_var()); On 2016/04/01 19:10:42, Mark Seaborn wrote: > On 2016/03/30 23:24:57, Mark Seaborn wrote: > > I would be inclined to use > > ASSERT_EQ(get_module_a_var(), 2345); > > As you discovered, that currently doesn't work, because it refers to stderr, > which (oddly) generates a reference to a TLS variable in newlib. > > So let's leave these as printf() calls until we have cross-module TLS references > implemented. Done. https://codereview.chromium.org/1825893002/diff/180001/tests/pnacl_dynamic_lo... File tests/pnacl_dynamic_loading/nacl.scons (right): https://codereview.chromium.org/1825893002/diff/180001/tests/pnacl_dynamic_lo... tests/pnacl_dynamic_loading/nacl.scons:28: On 2016/03/30 23:24:57, Mark Seaborn wrote: > Style nit: In Python it's preferred to have 2 empty lines between top-level > function decls, so can you preserve that when the code has it already? Done. https://codereview.chromium.org/1825893002/diff/180001/tests/pnacl_dynamic_lo... tests/pnacl_dynamic_loading/nacl.scons:60: if dependencies != []: On 2016/03/30 23:24:57, Mark Seaborn wrote: > "len(dependencies) != 0" is arguably more idiomatic in Python since it works for > any iterable type. > > Or you could make this unconditional. Using "len(dependencies) != 0" https://codereview.chromium.org/1825893002/diff/180001/tests/pnacl_dynamic_lo... tests/pnacl_dynamic_loading/nacl.scons:67: for d in dependencies: On 2016/03/30 23:24:57, Mark Seaborn wrote: > I think you can do > Depends(nonfinal_pll, dependencies) > for brevity Done. https://codereview.chromium.org/1825893002/diff/180001/tests/pnacl_dynamic_lo... tests/pnacl_dynamic_loading/nacl.scons:106: # use the "-l" linker flags and don't explicitly declare any LLVM passes. On 2016/03/30 23:24:57, Mark Seaborn wrote: > Since you're adding "-l" in this change, you could address this TODO. Done. Using "test_pll_a" and "test_pll_b", since MakeAndTranslatePll likes to operate on real scons nodes. https://codereview.chromium.org/1825893002/diff/180001/tests/pnacl_dynamic_lo... tests/pnacl_dynamic_loading/nacl.scons:198: pll_dependencies_test]) On 2016/03/30 23:24:57, Mark Seaborn wrote: > Nit: indent to align with "[" Done. https://codereview.chromium.org/1825893002/diff/220001/tests/pnacl_dynamic_lo... File tests/pnacl_dynamic_loading/nacl.scons (right): https://codereview.chromium.org/1825893002/diff/220001/tests/pnacl_dynamic_lo... tests/pnacl_dynamic_loading/nacl.scons:68: dest + '.nonfinal.pll', [pll_object], This update was made when trying to add "ASSERT_EQ" to dependencies_test.c. Using ComponentObject / Command separately allows header file search paths to be set correctly. I'm keeping this behavior because it may be useful for other PLLs in the future.
LGTM https://codereview.chromium.org/1825893002/diff/220001/pnacl/driver/pnacl-ld.py File pnacl/driver/pnacl-ld.py (right): https://codereview.chromium.org/1825893002/diff/220001/pnacl/driver/pnacl-ld.... pnacl/driver/pnacl-ld.py:353: assert(plls == []) Style nit: don't need ()s here However, the assertion would trigger if we invoked "pnacl-clang foo.pll", right? Given that, it should give a more friendly error rather than an assertion failure. Can you use Log.Fatal() here? https://codereview.chromium.org/1825893002/diff/220001/pnacl/driver/pnacl-ld.... pnacl/driver/pnacl-ld.py:413: Nit: remove empty line to keep this block together? https://codereview.chromium.org/1825893002/diff/220001/src/untrusted/pll_load... File src/untrusted/pll_loader/pll_loader.cc (right): https://codereview.chromium.org/1825893002/diff/220001/src/untrusted/pll_load... src/untrusted/pll_loader/pll_loader.cc:124: const PLLModule module((const PLLRoot *) pso_root); See other comment: you don't really need the first const. 'const' on local variables is usually unnecessary -- otherwise we'd be adding it all over the place. https://codereview.chromium.org/1825893002/diff/220001/src/untrusted/pll_load... File src/untrusted/pll_loader/pll_loader.h (right): https://codereview.chromium.org/1825893002/diff/220001/src/untrusted/pll_load... src/untrusted/pll_loader/pll_loader.h:22: const PLLRoot *root() const { return root_; } Nit: The whole PLLModule object is stateless since it just wraps a pointer. So there's really no difference between a PLLModule and a "const PLLModule", so I would recommend not using the latter. Adding "const" to this method suggests that the others aren't const, which would be misleading since that's not the case. https://codereview.chromium.org/1825893002/diff/220001/src/untrusted/pll_load... src/untrusted/pll_loader/pll_loader.h:49: // Returns a pointer to the PLL's pso_root. Nit: Should return a PLLRoot* rather than void* then, surely? Same below. https://codereview.chromium.org/1825893002/diff/220001/tests/pnacl_dynamic_lo... File tests/pnacl_dynamic_loading/nacl.scons (right): https://codereview.chromium.org/1825893002/diff/220001/tests/pnacl_dynamic_lo... tests/pnacl_dynamic_loading/nacl.scons:111: [test_pll_a_finalized, test_pll_b_finalized]) Nit: stock comment about arg alignment // less good SomeFunc(arg1, arg2, arg3, arg4, ...); etc. https://codereview.chromium.org/1825893002/diff/220001/tests/pnacl_dynamic_lo... tests/pnacl_dynamic_loading/nacl.scons:127: test_pll_a = TranslatePso('test_pll_a', test_pll_a_finalized) Can you keep the definitions of "test_pll_a" and "test_pll_a_finalized" together for readability?
https://codereview.chromium.org/1825893002/diff/220001/pnacl/driver/pnacl-ld.py File pnacl/driver/pnacl-ld.py (right): https://codereview.chromium.org/1825893002/diff/220001/pnacl/driver/pnacl-ld.... pnacl/driver/pnacl-ld.py:353: assert(plls == []) On 2016/04/01 23:42:19, Mark Seaborn wrote: > Style nit: don't need ()s here > > However, the assertion would trigger if we invoked "pnacl-clang foo.pll", right? > Given that, it should give a more friendly error rather than an assertion > failure. Can you use Log.Fatal() here? Well, the linker driver requires that the extension for PLLs be ".so" (see filetype.py), and pnacl-clang also verifies the "intype" of arguments so an earlier assertion would be hit. But if a user passed "pnacl-ld foo.so", where foo was a PLL, then yes, this assertion would be triggered. I'll add a more informative "Log.Fatal()". https://codereview.chromium.org/1825893002/diff/220001/pnacl/driver/pnacl-ld.... pnacl/driver/pnacl-ld.py:413: On 2016/04/01 23:42:19, Mark Seaborn wrote: > Nit: remove empty line to keep this block together? Done. https://codereview.chromium.org/1825893002/diff/220001/src/untrusted/pll_load... File src/untrusted/pll_loader/pll_loader.cc (right): https://codereview.chromium.org/1825893002/diff/220001/src/untrusted/pll_load... src/untrusted/pll_loader/pll_loader.cc:124: const PLLModule module((const PLLRoot *) pso_root); On 2016/04/01 23:42:19, Mark Seaborn wrote: > See other comment: you don't really need the first const. 'const' on local > variables is usually unnecessary -- otherwise we'd be adding it all over the > place. Done. https://codereview.chromium.org/1825893002/diff/220001/src/untrusted/pll_load... File src/untrusted/pll_loader/pll_loader.h (right): https://codereview.chromium.org/1825893002/diff/220001/src/untrusted/pll_load... src/untrusted/pll_loader/pll_loader.h:22: const PLLRoot *root() const { return root_; } On 2016/04/01 23:42:19, Mark Seaborn wrote: > Nit: The whole PLLModule object is stateless since it just wraps a pointer. So > there's really no difference between a PLLModule and a "const PLLModule", so I > would recommend not using the latter. Adding "const" to this method suggests > that the others aren't const, which would be misleading since that's not the > case. Got it. Const removed from this function. To compile, "module" in "AddByFilename" has been made non-const. https://codereview.chromium.org/1825893002/diff/220001/src/untrusted/pll_load... src/untrusted/pll_loader/pll_loader.h:49: // Returns a pointer to the PLL's pso_root. On 2016/04/01 23:42:19, Mark Seaborn wrote: > Nit: Should return a PLLRoot* rather than void* then, surely? Same below. Done. https://codereview.chromium.org/1825893002/diff/220001/tests/pnacl_dynamic_lo... File tests/pnacl_dynamic_loading/nacl.scons (right): https://codereview.chromium.org/1825893002/diff/220001/tests/pnacl_dynamic_lo... tests/pnacl_dynamic_loading/nacl.scons:111: [test_pll_a_finalized, test_pll_b_finalized]) On 2016/04/01 23:42:19, Mark Seaborn wrote: > Nit: stock comment about arg alignment > > // less good > SomeFunc(arg1, arg2, > arg3, > arg4, ...); > > etc. Done. https://codereview.chromium.org/1825893002/diff/220001/tests/pnacl_dynamic_lo... tests/pnacl_dynamic_loading/nacl.scons:127: test_pll_a = TranslatePso('test_pll_a', test_pll_a_finalized) On 2016/04/01 23:42:19, Mark Seaborn wrote: > Can you keep the definitions of "test_pll_a" and "test_pll_a_finalized" together > for readability? Done.
The CQ bit was checked by smklein@chromium.org
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/1825893002/#ps260001 (title: "Merging")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1825893002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1825893002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: nacl-precise_64-newlib-arm_qemu-pnacl on tryserver.nacl (JOB_FAILED, http://build.chromium.org/p/tryserver.nacl/builders/nacl-precise_64-newlib-ar...)
On 2016/04/04 18:17:29, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > nacl-precise_64-newlib-arm_qemu-pnacl on tryserver.nacl (JOB_FAILED, > http://build.chromium.org/p/tryserver.nacl/builders/nacl-precise_64-newlib-ar...) Since you're changing pnacl/driver/ and depending on that change in tests, you need to bump toolchain_feature_version.
On 2016/04/04 18:19:07, Mark Seaborn wrote: > On 2016/04/04 18:17:29, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > nacl-precise_64-newlib-arm_qemu-pnacl on tryserver.nacl (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.nacl/builders/nacl-precise_64-newlib-ar...) > > Since you're changing pnacl/driver/ and depending on that change in tests, you > need to bump toolchain_feature_version. Got it. Bumping feature version and retrying.
The CQ bit was checked by smklein@chromium.org
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/1825893002/#ps280001 (title: "Bumping Feature Version")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1825893002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1825893002/280001
Message was sent while issue was closed.
Description was changed from
==========
PNaCl Dynamic Linking: Added portable dependencies to shared objects.
Interface:
- This patch will pass the name of any shared libraries (passed to pnacl-clang
or pnacl-ld) to the ConvertToPso pass, which can store them in a section
similar to DT_NEEDED.
- Eventually, we will have to determine a solution for translating / caching
PLLs. In the meantime, any applications using a PLL named "libfoo.so" will
expect a native shared library named "libfoo.so.translated" to exist at
runtime. This is a temporary solution; ideally the application will
translate "libfoo.so" itself.
This patch:
- Teaches drivers how to recognize and filter PLLs, passing them to opt
instead of the native linker.
- Forces the pll_loader to only load a single ELF file at a time, and
determine dependencies by reading the PLLRoot of that file. Additionally,
teaches the pll_loader how to recursively add dependencies.
- Modifies the pll_hello_world test to depend on a pll_libc using this
mechanism for "-l<name>" dependencies.
- Adds "dependencies_test" which tests recursively loading multiple
dependencies
from a single PLL.
Relies on https://codereview.chromium.org/1829793002/
TEST=pll_dependencies_test, pll_hello_world_test, run_pll_loader_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
==========
to
==========
PNaCl Dynamic Linking: Added portable dependencies to shared objects.
Interface:
- This patch will pass the name of any shared libraries (passed to pnacl-clang
or pnacl-ld) to the ConvertToPso pass, which can store them in a section
similar to DT_NEEDED.
- Eventually, we will have to determine a solution for translating / caching
PLLs. In the meantime, any applications using a PLL named "libfoo.so" will
expect a native shared library named "libfoo.so.translated" to exist at
runtime. This is a temporary solution; ideally the application will
translate "libfoo.so" itself.
This patch:
- Teaches drivers how to recognize and filter PLLs, passing them to opt
instead of the native linker.
- Forces the pll_loader to only load a single ELF file at a time, and
determine dependencies by reading the PLLRoot of that file. Additionally,
teaches the pll_loader how to recursively add dependencies.
- Modifies the pll_hello_world test to depend on a pll_libc using this
mechanism for "-l<name>" dependencies.
- Adds "dependencies_test" which tests recursively loading multiple
dependencies
from a single PLL.
Relies on https://codereview.chromium.org/1829793002/
TEST=pll_dependencies_test, pll_hello_world_test, run_pll_loader_test
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
Committed:
https://chromium.googlesource.com/native_client/src/native_client/+/bdf73e78f...
==========
Message was sent while issue was closed.
Committed patchset #9 (id:280001) as https://chromium.googlesource.com/native_client/src/native_client/+/bdf73e78f... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
