666501 - --enable-jprof broken by build system changes in bug 552864

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

Bug 552864 broke linking in libjprof.so and also broke atexit() used in jprof.

Patch in a sec (taken from bigger patch in bug 664453)

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch

Broken out (and added to) from bug 664453.  Running a build now to test this patch

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 541288 [details] [diff] [review]
Patch

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

::: tools/jprof/stub/libmalloc.cpp
@@ +541,5 @@
> +    // This instanciates the dummy class above, and will trigger the class
> +    // destructor when libxul is unloaded. This is equivalent to atexit(),
> +    // but gracefully handles dlclose().
> +    static JprofShutdown t;
> +}

You are missing the part where you replace the atexit() call. Note that you don't need the RegisterJprofShutdown function to be extern "C" (that was only required for trace-malloc because the function was defined in C++ and called from C), and you actually don't need it at all, you can just define static JprofShutdown t where atexit() is called.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch - built and tested

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 541296 [details] [diff] [review]
Patch - built and tested

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

::: tools/jprof/stub/libmalloc.cpp
@@ +102,5 @@
> +        DumpAddressMap();
> +    }
> +};
> +
> +void RegisterJprofShutdown() {

You can make that function static, or as I wrote in my previous comment, just define static JprofShutdown t instead of the function call.

r=me with either.

@@ +543,5 @@
>      } else {
>          printf("setupProfilingStuff() called multiple times\n");
>      }
>  }
> +

You may want to avoid that empty line being added.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 541296 [details] [diff] [review]
Patch - built and tested

Checked in as http://hg.mozilla.org/mozilla-central/rev/0d5c92e9760e with nits addressed.