Open Bug 1332682 Opened 7 years ago Updated 1 year ago

Automate final keyword warnings (gcc -Wsuggest-final-types and -Wsuggest-final-methods)

Categories

(Firefox Build System :: General, defect)

defect

Tracking

(Not tracked)

People

(Reporter: tjr, Unassigned)

References

(Depends on 1 open bug)

Details

Marking classes and methods 'final' where possible will eliminate vtable lookups and a layer of indirection and also reduce the number of targets for UAF exploitation. (Performance and security gains in one!)

gcc has -Wsuggest-final-warnings and -Wsuggest-final-methods that will highlight these - I'm unaware of another compiler that does. When we get mingw builds working, this will become something we can expose and automate somehow.
Component: Other → Build Config
Product: Release Engineering → Core
QA Contact: mshal
Depends on: 620058
Depends on: 1360299
Depends on: 1360300
Depends on: 1360301
No longer depends on: 1360301
No longer depends on: 1360300
No longer depends on: 1360299
-Wsuggest-final-types and -Wsuggest-final-methods are supported in gcc 5+, but the builders are still using gcc 4.9.4.

https://gcc.gnu.org/gcc-5/changes.html

-Wsuggest-final-types
    Warn about types with virtual methods where code quality would be improved if the type were declared with the C++11 final specifier, or, if possible, declared in an anonymous namespace. This allows GCC to more aggressively devirtualize the polymorphic calls. This warning is more effective with link time optimization, where the information about the class hierarchy graph is more complete.

-Wsuggest-final-methods
    Warn about virtual methods where code quality would be improved if the method were declared with the C++11 final specifier, or, if possible, its type were declared in an anonymous namespace or with the final specifier. This warning is more effective with link time optimization, where the information about the class hierarchy graph is more complete. It is recommended to first consider suggestions of -Wsuggest-final-types and then rebuild with new annotations.
Summary: Automate final keyword warnings → Automate final keyword warnings (gcc -Wsuggest-final-types and -Wsuggest-final-methods)
I did a try run adding the warnings and bumping to gcc 6 (expecting it to not really build.) https://treeherder.mozilla.org/#/jobs?repo=try&revision=03c4c9eb9560b6a9742b5e7648cdf796c82f9dbc&selectedJob=100189541

It only output 8 instances (search for "final would enable devirtualization") - so something else is causing an issue. (Maybe it's just that the build couldn't complete successfully with gcc 6.)
(In reply to Tom Ritter [:tjr] from comment #2)
> It only output 8 instances (search for "final would enable
> devirtualization") - so something else is causing an issue. (Maybe it's just
> that the build couldn't complete successfully with gcc 6.)

Try adding -Wno-error for these warnings so the build doesn't treat these warnings as errors. You will still need to enable the warnings before adding -Wno-error.

check_and_add_gcc_warning('-Wsuggest-final-types')
check_and_add_gcc_warning('-Wno-error=suggest-final-types')

check_and_add_gcc_warning('-Wsuggest-final-methods')
check_and_add_gcc_warning('-Wno-error=suggest-final-methods')
Thanks! The build is still broken (https://treeherder.mozilla.org/#/jobs?repo=try&revision=4412b2ec386d27f5ae689161f1fe2909bb94ab82&selectedJob=100526924 - probably a gcc 6 thing), but I got 12,231 suggestions. I'm putting the analysis over in Bug 1332680
Product: Core → Firefox Build System
See Also: → 1517816
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.