812063 - Support absolute library path in the symbolization script.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: GonkIntegration, defect)

Description

[Nicolas B. Pierron [:nbp — off until 29-09]]

Attachment: 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]]

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 — off until 29-09]]

(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]]

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 — off until 29-09]]

Attachment: 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]]

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 — off until 29-09]]

https://github.com/mozilla-b2g/B2G/pull/178

Comment 7

[Dave Hylands [:dhylands]]

Merged
https://github.com/mozilla-b2g/B2G/commit/2ab30e7fbd40708525a287b3d58c545af73d0aa9