Last Comment Bug 812063 - Support absolute library path in the symbolization script.
: Support absolute library path in the symbolization script.
Status: RESOLVED FIXED
:
Product: Firefox OS
Classification: Client Software
Component: GonkIntegration (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
:
Mentors:
Depends on:
Blocks: 807854
  Show dependency treegraph
 
Reported: 2012-11-14 20:21 PST by Nicolas B. Pierron [:nbp]
Modified: 2012-11-27 20:21 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Add support for libraries refered by an absolute path. (1.44 KB, patch)
2012-11-14 20:21 PST, Nicolas B. Pierron [:nbp]
dhylands: review-
Details | Diff | Splinter Review
Add support for libraries refered by an absolute path. (1.44 KB, patch)
2012-11-26 10:33 PST, Nicolas B. Pierron [:nbp]
dhylands: review+
Details | Diff | Splinter Review

Description Nicolas B. Pierron [:nbp] 2012-11-14 20:21:17 PST
Created attachment 681843 [details] [diff] [review]
Add support for libraries refered by an absolute path.

Currently the symbolization does not work at all if the library contained in the target_name variable contains the absolute path of the library instead of the name of it.

The attached patch, or the git branch [1] is fixing this issue.  I have no idea how to test it on B2G, but my guess is that this extra case should not cause any issue since this add a last "elif" and still check that the library is found.

[1] https://github.com/nbp/B2G/compare/symbolicate-absolute-path-libraries
Comment 1 Dave Hylands [:dhylands] 2012-11-14 21:49:51 PST
Comment on attachment 681843 [details] [diff] [review]
Add support for libraries refered by an absolute path.

Review of attachment 681843 [details] [diff] [review]:
-----------------------------------------------------------------

What environment do these absolute paths appear in? I haven't seen these on the phone, so I'm guessing that you're using the script in another environment? I'm mostly just curious.

Leaving as r? for now (I'll update once I hear back from you)

::: scripts/profile-symbolicate.py
@@ +134,5 @@
>        self.symbol_table_addresses = sorted(self.symbol_table.keys())
> +    elif self.target_name[:1] == "/": # Absolute paths.
> +      basename = os.path.basename(self.target_name)
> +      dirname = os.path.dirname(self.target_name)
> +      lib_name = self.FindLibInTree(basename, dirname)

Won't lib_name wind up being the same as self.target_name ?

If so, can we just do 

lib_name = self.target_name
if os.path.exists(lib_name):

I have no objection with the code as presented if it's actually finding a library potentially different from the specified absolute path, but if its always just going to find the library specified, then we should just check for that.
Comment 2 Nicolas B. Pierron [:nbp] 2012-11-16 12:02:10 PST
(In reply to Dave Hylands [:dhylands] from comment #1)
> Comment on attachment 681843 [details] [diff] [review]
> Add support for libraries refered by an absolute path.
> 
> Review of attachment 681843 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What environment do these absolute paths appear in? I haven't seen these on
> the phone, so I'm guessing that you're using the script in another
> environment? I'm mostly just curious.

I am running NixOS (Linux x64), and taking details profiles (100µs) of JS benchmarks.
Nix provides a GCC wrapper to set precise RPATH, such as a user can compile against isolated libraries, and so all libraries appear with absolute path in the JSON profile.

This give the ability to compile against different library / toolchain without having to change the system or start a chroot / VM.

> ::: scripts/profile-symbolicate.py
> @@ +134,5 @@
> >        self.symbol_table_addresses = sorted(self.symbol_table.keys())
> > +    elif self.target_name[:1] == "/": # Absolute paths.
> > +      basename = os.path.basename(self.target_name)
> > +      dirname = os.path.dirname(self.target_name)
> > +      lib_name = self.FindLibInTree(basename, dirname)
> 
> Won't lib_name wind up being the same as self.target_name ?
> 
> If so, can we just do 
> 
> lib_name = self.target_name
> if os.path.exists(lib_name):

Ok, If we obtain an absolute path, then this probably already resolve the symbolic links, so I guess it is unlikely to be different.

Using os.path.exists(lib_name) works on my system.
Comment 3 Dave Hylands [:dhylands] 2012-11-25 10:25:51 PST
Comment on attachment 681843 [details] [diff] [review]
Add support for libraries refered by an absolute path.

So I'll r- this patch. If you submit one which just uses the absolute path (and doesn't call FindLibInTree) then I'll r+ it.
Comment 4 Nicolas B. Pierron [:nbp] 2012-11-26 10:33:02 PST
Created attachment 685239 [details] [diff] [review]
Add support for libraries refered by an absolute path.

Modified patch as suggested, as was the github branch since 11 days.
Comment 5 Dave Hylands [:dhylands] 2012-11-26 10:53:23 PST
Comment on attachment 685239 [details] [diff] [review]
Add support for libraries refered by an absolute path.

Review of attachment 685239 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nit fixed.

And could you do a pull request?

I'll merge it into the repository.

Sorry I missed your change earlier (we don't normally check anywhere but the main repo and rely on the people doing the changes to do pull requests)

::: scripts/profile-symbolicate.py
@@ +133,5 @@
>        self.symbol_table = gSpecialLibs[self.target_name]
>        self.symbol_table_addresses = sorted(self.symbol_table.keys())
> +    elif self.target_name[:1] == "/": # Absolute paths.
> +      basename = os.path.basename(self.target_name)
> +      dirname = os.path.dirname(self.target_name)

nit: This doesn't seem to be used, so this line could be removed.
Comment 6 Nicolas B. Pierron [:nbp] 2012-11-27 20:18:34 PST
https://github.com/mozilla-b2g/B2G/pull/178

Note You need to log in before you can comment on or make changes to this bug.