Page MenuHomePhabricator

[clang-tidy] Add a check to detect returning static locals in public headers
Needs ReviewPublic

Authored by jranieri-grammatech on Nov 7 2018, 12:55 PM.

Details

Summary

This adds a checker, llvm-problematic-statics, that detects functions in header files that return the address of a local static variable. These cause problems if the resulting pointer is expected to be unique because it may have different values across shared library boundaries.

It's has been a problem in the static analyzer that I've had to deal with in the past (https://reviews.llvm.org/D52905 and https://reviews.llvm.org/D52906).

Diff Detail

Event Timeline

C++ Core Guidelines also mentioned similar patterns, so may be this check belongs to bugprone module?

docs/clang-tidy/checks/llvm-problematic-statics.rst
7

static local variable? Probably Looks for should be replaced with Detects. Please also wrap with 80 characters.

Eugene.Zelenko added a project: Restricted Project.
Eugene.Zelenko retitled this revision from [clang-tidy/checks] Add a checker to detect returning static locals in public headers to [clang-tidy] Add a check to detect returning static locals in public headers.

I agree with @Eugene.Zelenko that this check should rather live in bugprone- as the issue its diagnosing is not LLVM specific. It is ok to add an alias into the llvm module.

clang-tidy/llvm/ProblematicStaticsCheck.cpp
22

Nit: you can inline this variable, having it does not improve readability (IMHO) nor does it reduce duplication.

26

I think the matching on include should not be done. Maybe it's ok to filter for header files, but we should not assume they live in include/... (as i think this check is generally useful and not too LLVM specific).

More filters for false positives could be checking if the function itself is static or in an anonymous namespace.

33

Please assert on VD and Return

test/clang-tidy/llvm-problematic-statics.cpp
3

I think the testing should include more cases. At least normal functions, and a false case as well.

aaron.ballman added inline comments.Nov 7 2018, 3:09 PM
clang-tidy/llvm/ProblematicStaticsCheck.cpp
33

Please assert on VD and Return

Why?

Please mention new check in Release Notes (in alphabetical order).

JonasToth added inline comments.Nov 8 2018, 2:35 PM
clang-tidy/llvm/ProblematicStaticsCheck.cpp
33

because the matcher might evolve over time. Right now this can not be triggered, that is correct. Asking for it was inappropriate but having it doesn't hurt.

aaron.ballman added inline comments.Nov 8 2018, 2:39 PM
clang-tidy/llvm/ProblematicStaticsCheck.cpp
33

All checks may evolve over time -- this isn't a pattern I'd like to see used in this sort of case, personally.

JonasToth added inline comments.Nov 8 2018, 3:20 PM
clang-tidy/llvm/ProblematicStaticsCheck.cpp
33

I don't insist.

aaron.ballman added inline comments.Nov 8 2018, 3:44 PM
docs/clang-tidy/checks/llvm-problematic-statics.rst
7

to have the address -> to have the same address

NoQ added a comment.Nov 9 2018, 5:27 PM

Thanks! I don't have much to add to the review, but i very much approve this check.