739800 - Format profiler address tags with "0x" prefix

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Vladan Djeric (:vladan)]

Attachment: Force all l-tag addresses to have a "0x" prefix

The profiler uses different formats for l-tag addresses on different platforms. I suspect this is probably because of some global localization setting.

This patch forces all addresses to have a "0x" prefix. Surprisingly, specifying stringstream flags was not effective.

Comment 1

[Benoit Girard (:BenWa)]

Comment on attachment 609901 [details] [diff] [review]
Force all l-tag addresses to have a "0x" prefix

This change should include a comment explaining the problem we saw on windows.

Comment 2

[Markus Stange [:mstange]]

Comment on attachment 609901 [details] [diff] [review]
Force all l-tag addresses to have a "0x" prefix

Does this work?

    stream << entry.mTagName << "-0x" << std::hex << reinterpret_cast<uintptr_t>(entry.mTagData) << "\n";

Comment 3

[Benoit Girard (:BenWa)]

We had tried that on windows. It works in a test app but not in firefox.

Comment 4

[Benoit Girard (:BenWa)]

Err nevermind that comments, I misread. Perhaps the above would work everywhere.

Comment 5

[Vladan Djeric (:vladan)]

(In reply to Markus Stange from comment #2)
> Comment on attachment 609901 [details] [diff] [review]
> Force all l-tag addresses to have a "0x" prefix
> 
> Does this work?
> 
>     stream << entry.mTagName << "-0x" << std::hex <<
> reinterpret_cast<uintptr_t>(entry.mTagData) << "\n";

Yes, this would probably work although I think there might still be a small chance for inconsistency across environments since this format does not specify whether the hex alphabetic characters should be uppercase or lowercase.

Comment 6

[Markus Stange [:mstange]]

(In reply to Vladan Djeric (:vladan) from comment #5)
> there might still be a small
> chance for inconsistency across environments since this format does not
> specify whether the hex alphabetic characters should be uppercase or
> lowercase.

The C++ spec draft at http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2010/n3092.pdf says in table 84 on page 685 to use lowercase characters (%x) as long as the "uppercase" flag isn't set (as you'd do with cout << hex << uppercase << 10, for example). Looks safe to me.

Comment 7

[Vladan Djeric (:vladan)]

I a(In reply to Markus Stange from comment #6)
> (In reply to Vladan Djeric (:vladan) from comment #5)
> > there might still be a small
> > chance for inconsistency across environments since this format does not
> > specify whether the hex alphabetic characters should be uppercase or
> > lowercase.
> 
> The C++ spec draft at
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2010/n3092.pdf says in
> table 84 on page 685 to use lowercase characters (%x) as long as the
> "uppercase" flag isn't set (as you'd do with cout << hex << https://[e-mail]/vdjeric/Gecko-Profiler-Addon.git<< 10,
> for example). Looks safe to me.

I agree that makes sense and it's what we first tried but for some reason we are seeing stringstream ignore our format specifiers on Windows Firefox but not in test apps.

It ignored setting "profile.flags(profile.flags() | std::ios::showbase)" as well as "profile << std::hex << std::showbase << ...", which is why i'm a little paranoid about trusting it to honor any string stream formatters.

Also, I just tried your suggestion and it seems to always format the hex addresses with "uppercase" and completely ignores any "nouppercase" format specifier :(

Comment 8

[Markus Stange [:mstange]]

Okay, strange. Then go ahead :)

Comment 9

[Vladan Djeric (:vladan)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/520ca2f9b4b7

Comment 10

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/520ca2f9b4b7