Open
Bug 1270832
Opened 7 years ago
Updated 27 days ago
turn on debug mode for libstdc++ headers
Categories
(Firefox Build System :: General, task)
Firefox Build System
General
Tracking
(firefox49 fixed)
REOPENED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: froydnj, Unassigned)
Details
Attachments
(1 file)
1006 bytes,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
STL usages have crept into the tree and will continue to creep in, especially as we move to a C++11 STL implementation everywhere. Let's at least enable a bit of checking for those data structures so we're not completely shooting ourselves in the foot.
![]() |
Reporter | |
Comment 1•7 years ago
|
||
This patch seems to work; the FIXME appears to be no longer applicable with newer GCC: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d9eb75e3d99
Attachment #8749640 -
Flags: review?(mh+mozilla)
Comment 2•7 years ago
|
||
Comment on attachment 8749640 [details] [diff] [review] turn on debug mode for libstdc++ headers Review of attachment 8749640 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/gcc-stl-wrapper.template.h @@ -45,5 @@ > > #if defined(DEBUG) && !defined(_GLIBCXX_DEBUG) > // Enable checked iterators and other goodies > -// > -// FIXME/bug 551254: gcc's debug STL implementation requires -frtti. Is this comment outdated?
Attachment #8749640 -
Flags: review?(mh+mozilla)
![]() |
Reporter | |
Comment 3•7 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #2) > Comment on attachment 8749640 [details] [diff] [review] > turn on debug mode for libstdc++ headers > > Review of attachment 8749640 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: config/gcc-stl-wrapper.template.h > @@ -45,5 @@ > > > > #if defined(DEBUG) && !defined(_GLIBCXX_DEBUG) > > // Enable checked iterators and other goodies > > -// > > -// FIXME/bug 551254: gcc's debug STL implementation requires -frtti. > > Is this comment outdated? This comment appears to stem from bug 551254 comment 30, but the libstdc++ debug mode documentation makes no mention of the -frtti constraint: https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_using.html#debug_mode.using.mode The only instances of |typeid| I can find in the debug headers are all guarded by preprocessor checks that RTTI is enabled. Spelunking in libstdc++ leads me to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=40160 , which would have been fixed somewhere in the GCC 4.5 timeframe. So yes, I think this comment is outdated. OK to commit, then?
Flags: needinfo?(mh+mozilla)
Updated•7 years ago
|
Attachment #8749640 -
Flags: review+
Updated•7 years ago
|
Flags: needinfo?(mh+mozilla)
![]() |
Reporter | |
Comment 7•7 years ago
|
||
(In reply to Pulsebot from comment #6) > https://hg.mozilla.org/integration/mozilla-inbound/rev/ccff1c4580ab This revision landed with a change to ignore debug headers for static analysis. We're not running the static analysis build, so I figure that not having the debug-ness turned on here is not a bad thing.
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ccff1c4580ab
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 9•7 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a640e6fa8ab9
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•5 years ago
|
Product: Core → Firefox Build System
![]() |
Reporter | |
Comment 10•5 years ago
|
||
The fuzzing team was interested in this; they may also be interested in comment 9, and the commit message for the backout.
Updated•4 years ago
|
Type: defect → task
Comment 11•10 months ago
|
||
The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.
Assignee: froydnj+bz → nobody
Updated•4 months ago
|
Severity: normal → S3
Comment 12•1 month ago
|
||
This was talked about recently in the Google-organized Memory Saftey Summit. They enabled these on Chromium and saw over 800 unique issues in 3 months so it definitely seems worth running down.
Comment 13•1 month ago
|
||
For easier reading, here is the backout reason from comment 9:
It turns out that, since we're including <new> before setting
_GLIBCXX_DEBUG, the debug parts of c++config.h are not activated, and
that has an impact of how much of the debug features of the STL are
activated.
In contrast, the upcoming changes to the STL wrappers are avoiding the
include of <new> before _GLIBCXX_DEBUG is set, which in turn breaks the
build because, as we link things that use STL wrappers with things that
don't, they end up with a different state of STL debugging, and have
mismatching symbols.
Mike, do you know if this is still an issue? Are there any other blockers for pursuing this?
Flags: needinfo?(mh+mozilla)
Comment 14•27 days ago
|
||
I think it still is, short of building some things twice (e.g. some parts of the updater). Best way to tell for sure would be to do a try push.
Flags: needinfo?(mh+mozilla)
You need to log in
before you can comment on or make changes to this bug.
Description
•