1618979 - Unused BaseProfilerLogTest declaration in baseprofiler/core/platform.h

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect, P2)

Description

[Markus Stange [:mstange]]

There's a declaration of a function named BaseProfilerLogTest in mozglue/baseprofiler/core/platform.h: https://searchfox.org/mozilla-central/source/mozglue/baseprofiler/core/platform.h#44

Clicking the function does not show a "Go to definition" item.

The definition is here: https://searchfox.org/mozilla-central/source/mozglue/baseprofiler/core/platform.cpp#151

Comment 1

[Kartikaya Gupta (email:[e-mail])]

The header is BaseProfilerLogTest but the definition is mozilla::baseprofiler::BaseProfilerLogTest, so they don't link up. That might be an actual bug in the code?

Comment 2

[Markus Stange [:mstange]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #1)

The header is BaseProfilerLogTest but the definition is mozilla::baseprofiler::BaseProfilerLogTest, so they don't link up. That might be an actual bug in the code?

Cool! It might be! Things do compile and link fine though. I'm not sure of the semantics here. So yes, we should probably fix it, but if our regular linker links them up, shouldn't searchfox also be able to link them up?

Comment 3

[Botond Ballo [:botond] [away until Oct 19]]

The calls to the functions are done via macros, so all semantic processing such as name lookup happens at the site of the macro invocations. The function is also only required to be declared at those invocations, rather than at the location of the macro definition.

So what's happening is the declaration in platform.h declares an unrelated global ::BaseProfilerLogTest function which is never used, and all the macro invocations (I'm assuming) occur inside mozilla::baseprofiler and correctly find mozilla::baseprofiler::BaseProfilerLogTest.

Comment 4

[Markus Stange [:mstange]]

Hah, nice! Moving to the Gecko Profiler component. Thanks for the explanation!

Comment 5

[Botond Ballo [:botond] [away until Oct 19]]

The fact that the macro makes an unqualified call is bug-prone, because any macro invocations that may be added outside of mozilla::baseprofiler in the future could hypothetically resolve to a different function.

My suggestion would be to modify the macros to qualify the call explicitly:

#define LOG_TEST mozilla::baseprofiler::BaseProfileLogTest(3)
// etc.

and then forward-declare the function inside that namespace:

namespace mozilla {
namespace baseprofiler {
bool BaseProfilerLogTest(int aLevelToTest);
}
}

(While it's only the invocation sites of the macros that require this forward declaration to be in scope, it's still a good idea for the same header that defines the macros to provide the forward declaration.)

Comment 6

[Markus Stange [:mstange]]

I'll take care of this.

Comment 7

[Markus Stange [:mstange]]

Attachment: Bug 1618979 - Tweak base profiler logging. r=gerald

This fixes the declaration of (BaseProfiler)LogTest. It also makes it so that the logs show up on Android.
In xpcom we have printf_stderr which does something similar and also handles Windows.

Depends on D64990

Comment 8

[Markus Stange [:mstange]]

Attachment: Bug 1618979 - Use PrintToConsole for things that should always be printed. r=gerald

Depends on D64994

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c978c229b66
Tweak base profiler logging. r=gerald
https://hg.mozilla.org/integration/autoland/rev/daf1993b5140
Use PrintToConsole for things that should always be printed. r=gerald

Comment 10

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/5c978c229b66
https://hg.mozilla.org/mozilla-central/rev/daf1993b5140