turn on debug mode for libstdc++ headers
Categories
(Firefox Build System :: General, task)
Tracking
(firefox49 fixed)
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: froydnj, Unassigned)
References
Details
Attachments
(2 files)
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•8 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
Comment 2•8 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?
Reporter | ||
Comment 3•8 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?
Updated•8 years ago
|
Updated•8 years ago
|
Reporter | ||
Comment 7•8 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ccff1c4580ab
Comment 9•8 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a640e6fa8ab9
Updated•6 years ago
|
Reporter | ||
Comment 10•6 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•5 years ago
|
Comment 11•2 years ago
|
||
The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.
Updated•2 years ago
|
Comment 12•1 year 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 year 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?
Comment 14•1 year 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.
Comment 15•1 year ago
|
||
Is this something we could enable per header file (directory, translation unit, ...) in order to enforce it step by step?
Comment 16•11 months ago
|
||
June, you said you tried turning this from error to warning recently and came up with a list of issues. Can you share that list?
Comment 17•10 months ago
|
||
As an update we're somewhat shifting this bug to be solely about clang's libc++ debug mode checks since we primarily build with clang now and gcc's libstdc++ counterpart already has Bug 556708 filed.
I'm having some trouble reproducing that list both locally and in try, but I'm attaching the patch I wrote to generate it here. It attempts to set LIBCXX_ENABLE_DEBUG_MODE
before any part of Firefox is compiled and forces the copy of clang that we build in automation to be built with LIBCXX_ENABLE_DEBUG_MODE
set and a custom version of _LIBCPP_VERBOSE_ABORT
to log the error, but not abort from the build.
To be able to utilize the stl debug mode we'll need to build llvm/clang with LIBCXX_ENABLE_DEBUG_MODE
set to true and then also use that build of clang to build debug Firefox with LIBCXX_ENABLE_DEBUG_MODE
set to true. I'm not sure if we would be able to use the same copy of clang to make other builds, however. As far as I can tell the ABI changes persist even if we don't set the debug mode flag when building Firefox.
As for whether or not we can do this by header file/translation unit/component/etc we'll have to enforce it all at once due to how the build flag works and we'll also need to make sure that any third party libraries we're building also have the flag enabled. We could however do it in two stages of
- enabling the flag and surfacing the errors as warnings
- fixing everything found in tree and restoring the default functionality of calling
std::abort
on error
Comment 18•10 months ago
|
||
Comment 19•9 months ago
|
||
since we primarily build with clang now
That's not actually related to whether or not we're using libc++, which we're actually not, on most platforms. We're only using libc++ on mac, and we're using the system one, at that.
(Also clang 17 removed LIBCXX_ENABLE_DEBUG_MODE, it now has different knobs)
Comment 20•6 months ago
|
||
Changing title back to libstdc++, since it's the relevant STL for our builds
This current version of the patch builds but fails to link with the following error, but I'm unsure what the cause is. The libstdc++.so
being linked in from the sysroot in the .mozbuild
directory is exporting the symbol which the linker ends up suggesting
[task 2023-10-17T03:20:53.903Z] 03:20:53 ERROR - ld.lld: error: undefined hidden symbol: __gnu_debug::_Error_formatter::_M_message(__gnu_debug::_Debug_msg_id) const
[task 2023-10-17T03:20:53.903Z] 03:20:53 INFO - >>> referenced by stl_algobase.h:452 (/builds/worker/fetches/sysroot-i686-linux-gnu/usr/lib/gcc/i586-linux-gnu/8/../../../../include/c++/8/bits/stl_algobase.h:452)
[task 2023-10-17T03:20:53.904Z] 03:20:53 INFO - >>> /builds/worker/workspace/obj-build/toolkit/library/gtest/../../../mfbt/tests/gtest/TestAlgorithm.o:(void std::vector<long long, std::allocator<long long>>::_M_realloc_insert<long long>(__gnu_cxx::__normal_iterator<long long*, std::vector<long long, std::allocator<long long>>>, long long&&))
[task 2023-10-17T03:20:53.904Z] 03:20:53 INFO - >>> referenced by stl_algobase.h:452 (/builds/worker/fetches/sysroot-i686-linux-gnu/usr/lib/gcc/i586-linux-gnu/8/../../../../include/c++/8/bits/stl_algobase.h:452)
[task 2023-10-17T03:20:53.904Z] 03:20:53 INFO - >>> /builds/worker/workspace/obj-build/toolkit/library/gtest/../../../mfbt/tests/gtest/TestAlgorithm.o:(void std::vector<long long, std::allocator<long long>>::_M_range_initialize<long long const*>(long long const*, long long const*, std::forward_iterator_tag))
[task 2023-10-17T03:20:53.904Z] 03:20:53 INFO - >>> referenced by stl_algobase.h:1047 (/builds/worker/fetches/sysroot-i686-linux-gnu/usr/lib/gcc/i586-linux-gnu/8/../../../../include/c++/8/bits/stl_algobase.h:1047)
[task 2023-10-17T03:20:53.905Z] 03:20:53 INFO - >>> /builds/worker/workspace/obj-build/toolkit/library/gtest/../../../mfbt/tests/gtest/TestAlgorithm.o:(testing::AssertionResult testing::internal::CmpHelperEQ<std::vector<long long, std::allocator<long long>>, std::vector<long long, std::allocator<long long>>>(char const*, char const*, std::vector<long long, std::allocator<long long>> const&, std::vector<long long, std::allocator<long long>> const&))
[task 2023-10-17T03:20:53.905Z] 03:20:53 INFO - >>> referenced 14409 more times
[task 2023-10-17T03:20:53.905Z] 03:20:53 INFO - >>> did you mean: __gnu_debug::_Error_formatter::_M_message(__gnu_debug::_Debug_msg_id) const@GLIBCXX_3.4
[task 2023-10-17T03:20:53.905Z] 03:20:53 INFO - >>> defined in: /builds/worker/fetches/sysroot-i686-linux-gnu/usr/lib/gcc/i586-linux-gnu/8/libstdc++.so
Updated•6 months ago
|
Updated•6 months ago
|
Comment 21•5 months ago
|
||
We will need a new owner if we want to continue pursuing this.
Description
•