Closed Bug 808257 Opened 10 years ago Closed 2 years ago

build SPS on BSDs

Categories

(Core :: Gecko Profiler, defect)

x86_64
FreeBSD
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1634205

People

(Reporter: jbeich, Unassigned)

Details

Attachments

(2 files)

No description provided.
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
Attachment #677973 - Flags: review?(bgirard)
CC'ing more people to test.
(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 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.
Attachment #677973 - Flags: review?(bgirard) → review+
(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;'.
CC'ing Julian because this patch may cause a bit of annoying rot.
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.
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.
I was going to bump that patch again, when i realized this relied on linux emulation to parse /proc... sheesh. Really, no point :)
(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.
(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;
  }

Obsolete but there's a new effort.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1634205
You need to log in before you can comment on or make changes to this bug.