Format profiler address tags with "0x" prefix

RESOLVED FIXED in mozilla14

Status

()

Core
Gecko Profiler
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: vladan, Assigned: vladan)

Tracking

Trunk
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 609901 [details] [diff] [review]
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.
(Assignee)

Updated

5 years ago
OS: Windows 7 → All
Hardware: x86_64 → All
(Assignee)

Updated

5 years ago
Attachment #609901 - Flags: review?(bgirard)
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.
Attachment #609901 - Flags: review?(bgirard) → review+
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";
We had tried that on windows. It works in a test app but not in firefox.
Err nevermind that comments, I misread. Perhaps the above would work everywhere.
(Assignee)

Comment 5

5 years ago
(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.
(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.
(Assignee)

Comment 7

5 years ago
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://vdjeric@github.com/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 :(
Okay, strange. Then go ahead :)
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/520ca2f9b4b7
(Assignee)

Updated

5 years ago
Attachment #609901 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/520ca2f9b4b7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 741242
You need to log in before you can comment on or make changes to this bug.