808257 - build SPS on BSDs

Status: RESOLVED DUPLICATE of bug 1634205

(Core :: Gecko Profiler, defect)

Comment 1

[Jan Beich]

Attachment: Bug 808257 - Enable SPS profiler on x86 BSDs.

Not sure how to test it as the addon no longer supports "stackwalk" on Linux since 1.6.0. Would previous versions work?

https://github.com/bgirard/Gecko-Profiler-Addon/raw/975e200/geckoprofiler.xpi # 1.5.1
https://github.com/bgirard/Gecko-Profiler-Addon/commit/b666ca6 # break

Comment 2

[Jan Beich]

Attachment: Bug 808257 - A hack to unbreak on old NetBSD versions.

Comment 3

[Jan Beich]

CC'ing more people to test.

Comment 4

[Benoit Girard (:BenWa)]

(In reply to Jan Beich from comment #1)
> Created attachment 677973 [details] [diff] [review]
> Bug 808257 - Enable SPS profiler on x86 BSDs.
> 
> Not sure how to test it as the addon no longer supports "stackwalk" on Linux
> since 1.6.0. Would previous versions work?

The stackwalking situation on Linux varies. If I remember correctly on Ubuntu some system libraries and graphics drivers don't support frame pointers so trying to follow them crashes. This can be fixed by knowing the regions the stack is and aborting if we get an address outside the stack to prevent a crash. If all libraries that firefox load have frame pointers on x86 then it should be fine to use it with BSD.

We decided to use breakpad as the unwinder for Linux/Mobile so we're focusing our effort on that instead. See bug 779291.

> 
> https://github.com/bgirard/Gecko-Profiler-Addon/raw/975e200/geckoprofiler.
> xpi # 1.5.1
> https://github.com/bgirard/Gecko-Profiler-Addon/commit/b666ca6 # break

You can certainly try old version. Better yet might be to checkout the latest version of the extension. Run bootstrap.sh, remove the check for Linux in the stackwalk feature check in lib/main.js, add the path to a firefox build with the patches above and run test.sh

Comment 5

[Benoit Girard (:BenWa)]

Comment on attachment 677973 [details] [diff] [review]
Bug 808257 - Enable SPS profiler on x86 BSDs.

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

Thanks, it makes the defines a bit more clear.

::: tools/profiler/TableTicker.cpp
@@ +49,1 @@
>   #define USE_NS_STACKWALK

Like I mentioned previously we're switching over to using breakpad unwinding which is likely what you will want to use down the road.

That said you can use NS_STACKWALK until then (or wait for breakpad unwinding to support c++ unwinds). Just make sure that everything within in and its calles are signal safe.

Comment 6

[Benoit Girard (:BenWa)]

(In reply to Jan Beich from comment #1)
> Not sure how to test it as the addon no longer supports "stackwalk" on Linux
> since 1.6.0. Would previous versions work?

Actually way easier is to hardcode 'mUseStackWalk = true;'.

Comment 7

[Benoit Girard (:BenWa)]

CC'ing Julian because this patch may cause a bit of annoying rot.

Comment 8

[Landry Breuil (:gaston)]

Fwiw m-c builds fine here on OpenBSD with att 677973. How does one checks if it does smth useful ? If the profiler goes on depending on breakpad, there's no point since breakpad has zero code for the bsds.

Comment 9

[Benoit Girard (:BenWa)]

You can test this with the extension.

We've discussed keeping the current code around but we haven't decided yet. If BSD doesn't have any need for fancy profiling and as good built-in system profiler it's probably not worth to maintain this code.

Comment 10

[Landry Breuil (:gaston)]

I was going to bump that patch again, when i realized this relied on linux emulation to parse /proc... sheesh. Really, no point :)

Comment 11

[Benoit Girard (:BenWa)]

(In reply to Landry Breuil (:gaston) from comment #10)
> I was going to bump that patch again, when i realized this relied on linux
> emulation to parse /proc... sheesh. Really, no point :)

We just need any possible way to get a list of the libraries loaded and at what addresses. We're moving away from using /proc BTW.

Comment 12

[Jan Beich]

(In reply to Landry Breuil (:gaston) from comment #10)
> relied on linux emulation to parse /proc...

Only Android uses /proc/%d/maps. On Linux and BSDs the code looks like this

  SharedLibraryInfo SharedLibraryInfo::GetInfoForSelf()
  {
    SharedLibraryInfo info;

    dl_iterate_phdr(dl_iterate_callback, &info);
    return info;
  }

Comment 13

[Jan Beich]

Obsolete but there's a new effort.