Load lldb symbols at runtime - #224
Conversation
|
Hey @springmeyer, thank you for this contribution! From my perspective, this change looks good. The test is failing because we have an experimental Node.js API for llnode, which needs the old configuration. We can keep the new configuration for the lldb plugin to mirror Linux behavior, and add a conditional to fallback to the old configuration in the API build: diff --git binding.gyp binding.gyp
index e47386b..82c4ed5 100644
--- binding.gyp
+++ binding.gyp
@@ -106,7 +106,33 @@
"libraries": ["-l<(lldb_lib)"]
}]
]
- }]
+ }],
+ [ "OS=='mac'", {
+ "conditions": [
+ [ "lldb_lib_dir == ''", {
+ "variables": {
+ "mac_shared_frameworks": "/Applications/Xcode.app/Contents/SharedFrameworks",
+ },
+ "xcode_settings": {
+ "OTHER_LDFLAGS": [
+ "-F<(mac_shared_frameworks)",
+ "-Wl,-rpath,<(mac_shared_frameworks)",
+ "-framework LLDB",
+ ],
+ },
+ },
+ # lldb_lib_dir != ""
+ {
+ "xcode_settings": {
+ "OTHER_LDFLAGS": [
+ "-Wl,-rpath,<(lldb_lib_dir)",
+ "-L<(lldb_lib_dir)",
+ "-l<(lldb_lib)",
+ ],
+ },
+ }],
+ ],
+ }],
]
}]
}]@joyeecheung WDYT? You're more familiar with llnode's building system than I am. |
|
@mmarchini Yeah moving the old config to the |
|
What exactly needs to happen to get this merged?
Is this a change I need to make? |
|
@springmeyer I think applying the diff in #224 (comment) should make the test pass? The addon target needs the old config to resolve the symbols in the shared library at build time. |
We can lazily load lldb symbols at runtime. This avoids needing to link lldb directly and therefore avoids portability problems resulting from rpath being specific to one system.
|
Thanks @joyeecheung - I'm understanding now. Just was not clicking before. I'll attempt this. In the meantime I noticed that it is only a single test failing. So, locally for me if I comment out just these lines then all the tests pass: diff --git a/test/addon/jsapi-test.js b/test/addon/jsapi-test.js
index d067d37..95fe482 100644
--- a/test/addon/jsapi-test.js
+++ b/test/addon/jsapi-test.js
@@ -22,23 +22,23 @@ tape('llnode API', (t) => {
t.error(err);
t.ok(true, 'Saved core');
- test(process.execPath, common.core, t);
+ //test(process.execPath, common.core, t);
t.end();
});
}
});
-function test(executable, core, t) {
- debug('============= Loading ==============');
- // Equivalent to lldb executable -c core
- debug(`Loading core dump: ${core}, executable: ${executable}`);
- const llnode = fromCoredump(core, executable);
-
- verifySBProcess(llnode, t);
- const typeMap = verifyBasicTypes(llnode, t);
- const processType = verifyProcessType(typeMap, llnode, t);
- verifyProcessInstances(processType, llnode, t);
-}
+// function test(executable, core, t) {
+// debug('============= Loading ==============');
+// // Equivalent to lldb executable -c core
+// debug(`Loading core dump: ${core}, executable: ${executable}`);
+// const llnode = fromCoredump(core, executable);
+//
+// verifySBProcess(llnode, t);
+// const typeMap = verifyBasicTypes(llnode, t);
+// const processType = verifyProcessType(typeMap, llnode, t);
+// verifyProcessInstances(processType, llnode, t);
+// }
function verifySBProcess(llnode, t) {
const processInfo = llnode.getProcessInfo(); |
|
Okay, c08b3ac implements the fix to:
|
We can lazily load lldb symbols at runtime. This avoids needing to link lldb directly and therefore avoids portability problems resulting from rpath being specific to one system. PR-URL: #224 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>
|
Landed in dbd95d7 🎉🎉🎉 Thank you for your contribution! |
By using
-Wl,-undefined,dynamic_lookupon OS X we can mirror the default behavior ofldon linux whereby undefined symbols in LLDB are found dynamically at runtime.Using this flag allows the plugin to be created without a linking error and without linking explicitly to the lldb shared library. The symbols are found when
plugin load <path to plugin>is run withinlldb.The benefit of this to users is that then
llnodebinaries on OS X can be distributed and used with a customlldbpackage, even one not provided by apple. (this is done in mason via https://github.com/mapbox/mason/blob/65f4b8244c7aae7909421d5cee246f3d3ee9aced/scripts/llnode/1.7.1/script.sh)