Last Comment Bug 739800 - Format profiler address tags with "0x" prefix
: Format profiler address tags with "0x" prefix
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Gecko Profiler (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Vladan Djeric (:vladan)
:
Mentors:
Depends on: 741242
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-27 14:54 PDT by Vladan Djeric (:vladan)
Modified: 2012-04-01 14:20 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Force all l-tag addresses to have a "0x" prefix (900 bytes, patch)
2012-03-27 14:54 PDT, Vladan Djeric (:vladan)
bgirard: review+
vladan.bugzilla: checkin+
Details | Diff | Splinter Review

Description Vladan Djeric (:vladan) 2012-03-27 14:54:18 PDT
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.
Comment 1 Benoit Girard (:BenWa) 2012-03-27 14:57:39 PDT
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] 2012-03-27 16:11:26 PDT
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) 2012-03-27 16:15:34 PDT
We had tried that on windows. It works in a test app but not in firefox.
Comment 4 Benoit Girard (:BenWa) 2012-03-27 16:16:16 PDT
Err nevermind that comments, I misread. Perhaps the above would work everywhere.
Comment 5 Vladan Djeric (:vladan) 2012-03-27 16:23:56 PDT
(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] 2012-03-27 16:52:06 PDT
(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) 2012-03-27 17:24:21 PDT
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 :(
Comment 8 Markus Stange [:mstange] 2012-03-27 17:52:31 PDT
Okay, strange. Then go ahead :)
Comment 10 Matt Brubeck (:mbrubeck) 2012-03-29 08:52:38 PDT
https://hg.mozilla.org/mozilla-central/rev/520ca2f9b4b7

Note You need to log in before you can comment on or make changes to this bug.