Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Support absolute library path in the symbolization script.

RESOLVED FIXED

Status

Firefox OS
GonkIntegration
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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
Attachment #681843 - Flags: review?(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.
(Assignee)

Comment 2

5 years ago
(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.
(Assignee)

Updated

5 years ago
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
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.
Attachment #681843 - Flags: review?(dhylands) → review-
(Assignee)

Comment 4

5 years ago
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.
Attachment #681843 - Attachment is obsolete: true
Attachment #685239 - Flags: review?(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.
Attachment #685239 - Flags: review?(dhylands) → review+
(Assignee)

Comment 6

5 years ago
https://github.com/mozilla-b2g/B2G/pull/178
Merged
https://github.com/mozilla-b2g/B2G/commit/2ab30e7fbd40708525a287b3d58c545af73d0aa9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Component: General → Builds
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.