Zillion lines of code or how not to choke on a big old project

In a typical code review, the development team examines the code. Code review is a tried-and-true way of spotting bugs and sharing experience with other developers. However, it has a drawback — the high cost.

A developer can use a static analysis tool to check their code before passing it to teammates for review. If you check it in advance, you can fix many bugs and dangerous spots. This will free up time for your team to focus on finding higher-level bugs and debating architectural solutions.

See how static analysis can help you with code reviews: https://pvs-studio.com/en/blog/posts/1048/

Transcript

1. Zillion code lines? How not to choke on a big old project Yuri Minaev pvs-studio.com
2. 2 Yuri Minaev C++ developer at PVS-Studio. I'm working on the core and diagnostic rules of the static analyzer. minaev@viva64.com About me
3. 3 Intro (sort of)
4. 4 • Any long-lasting project grows lots of hair • Different people write code • It layers over itself like geological strata • Epochs come undetected So, what happened?
5. 5 • Any long-lasting project grows lots of hair • Different people write code • It layers over itself like geological strata • Epochs come undetected So, what happened?
6. 6 Statistics
7. 7 Statistics Linux 1.0.0 March 14, 1994 176250 LOC Linux 4.11.7 June 24, 2017 18373471 LOC Photoshop 1.0 February 19, 1990 128000 LOC Photoshop CS 6 May 7, 2012 10000000 LOC Windows Calculator 35000 LOC
8. 8 • We see "geology" in action • More code means more errors • Old code means outdated code... • Known as... So what?
9. 9 Legacy Legacy Code - source code inherited from someone else and source code inherited from an older version of the software.
10. 10 Legacy
11. 11 Legacy Glorious modern code Core module leaking abstractions Third-party library nobody remembers about Crutches to keep the bloody thing from falling Some ancient code from the Legendary Guru* *unmaintainable Wheel invention lab* *because reasons Tactical dirty fix* *todo: make a proper one
12. 12 Hunting of the Bug
13. 13 What shall we do? Well, we debug Let's peek at the code, aaaand... Mmm, yummy
14. 14 Hours pass (probably)
15. 15 Hunting of the Bug
16. 16 • Debugging gets longer • It's hard due to crutches small architectural solutions • You add dirty fixes (todo: do it right later) • Well, at least, you feel like ----> In a nutshell
17. 17 Unnecessary digression Every time you write new code, you should do so reluctantly, under duress, because you completely exhausted all your other options. Code is only our enemy because there are so many of us programmers writing so damn much of it. Jeff Atwood. The Best Code is No Code At All https://blog.codinghorror.com/the-best-code-is-no-code-at-all/
18. 18 True story Once upon a time, there was a Glorious International Complany ©™ Ltd Product Feature A better product &
19. 19 True story A better product
20. 20 True story
21. 21 • Don't let it happen (in a perfect world) • Otherwise, refactor it (slowly) • Make a machine work (if possible) • We need a good automated toolset to begin with It's scary. What to do?
22. 22 • Cover your codebase with tests ASAP • If unsure, add more tests • Configure your CI to run them on every change (at least) Are you using tests?
23. 23 DevOps cycle CI, I has it
24. 24 Monitor?
25. 25 void CBaseCamera::GetInput(....) { .... if( m_GamePad[iUserIndex].wButtons || m_GamePad[iUserIndex].sThumbLX || m_GamePad[iUserIndex].sThumbLX || m_GamePad[iUserIndex].sThumbRX || m_GamePad[iUserIndex].sThumbRY || m_GamePad[iUserIndex].bLeftTrigger || m_GamePad[iUserIndex].bRightTrigger ) { .... } Let's review some code
26. void CAdvancedSettings::SetExtraArtwork( const TiXmlElement* arttypes, std::vector<std::string>& artworkMap) { if (!arttypes) return artworkMap.clear(); const TiXmlNode* arttype = arttypes->FirstChild("arttype"); .... } 26 Let's review more Where's the ';'? return void?
27. 27 void f() {} void f(bool something) { if (!something) return f(); } Let's have a look
28. 28 Is it even legal? A return statement with an expression of type "cv void" can be used only in functions with a return type of cv void; the expression is evaluated just before the function returns to its caller. http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2005/n1905.pdf#subsection.6.6.3
29. 29 • Compiler warnings • Dynamic analysis (aka sanitizers) • Static analysis • Code quality platforms • Quality gates in CI So, monitoring
30. 30 Trust your compiler -Wall
31. 31 Dynamic analysis
32. 32 #include <stdlib.h> int main() { char* x = (char*)malloc(10 * sizeof(char)); free(x); return x[5]; } Example
33. 33 ==9901==ERROR: AddressSanitizer: heap-use-after-free on address 0x60700000dfb5 at pc 0x45917b bp 0x7fff4490c700 sp 0x7fff4490c6f8 READ of size 1 at 0x60700000dfb5 thread T0 #0 0x45917a in main use-after-free.c:5 #1 0x7fce9f25e76c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 #2 0x459074 in _start (a.out+0x459074) 0x60700000dfb5 is located 5 bytes inside of 80-byte region [0x60700000dfb0,0x60700000e000) freed by thread T0 here: #0 0x4441ee in __interceptor_free projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64 #1 0x45914a in main use-after-free.c:4 #2 0x7fce9f25e76c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 previously allocated by thread T0 here: #0 0x44436e in __interceptor_malloc projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74 #1 0x45913f in main use-after-free.c:3 #2 0x7fce9f25e76c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 SUMMARY: AddressSanitizer: heap-use-after-free use-after-free.c:5 main Example
34. 34 ==9901==ERROR: AddressSanitizer: heap-use-after-free on address 0x60700000dfb5 at pc 0x45917b bp 0x7fff4490c700 sp 0x7fff4490c6f8 READ of size 1 at 0x60700000dfb5 thread T0 #0 0x45917a in main use-after-free.c:5 #1 0x7fce9f25e76c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 #2 0x459074 in _start (a.out+0x459074) 0x60700000dfb5 is located 5 bytes inside of 80-byte region [0x60700000dfb0,0x60700000e000) freed by thread T0 here: #0 0x4441ee in __interceptor_free projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64 #1 0x45914a in main use-after-free.c:4 #2 0x7fce9f25e76c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 previously allocated by thread T0 here: #0 0x44436e in __interceptor_malloc projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74 #1 0x45913f in main use-after-free.c:3 #2 0x7fce9f25e76c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 SUMMARY: AddressSanitizer: heap-use-after-free use-after-free.c:5 main Example heap-use-after-free on address 0x60700000dfb5 [...] in main use-after-free.c:5
35. 35 • Time for code review • But: • I'm lazy • It's boring • I've no time • I'm tired • Insert your favorite excuse • Could a robot do it? Indeed, it could Static analysis
36. 36 void CBaseCamera::GetInput(....) { .... if( m_GamePad[iUserIndex].wButtons || m_GamePad[iUserIndex].sThumbLX || m_GamePad[iUserIndex].sThumbLX || m_GamePad[iUserIndex].sThumbRX || m_GamePad[iUserIndex].sThumbRY || m_GamePad[iUserIndex].bLeftTrigger || m_GamePad[iUserIndex].bRightTrigger ) { .... } Remember this? V501 There are identical sub-expressions .... to the left and to the right of the '||' operator.
37. void CAdvancedSettings::SetExtraArtwork( const TiXmlElement* arttypes, std::vector<std::string>& artworkMap) { if (!arttypes) return artworkMap.clear(); const TiXmlNode* arttype = arttypes->FirstChild("arttype"); .... } 37 What about this? V504 It is highly probable that the semicolon ';' is missing after 'return' keyword.
38. 38 Creative use Proprietary project Cool library You
39. 39 /* * This file is part of the MySuperLibrary distribution * Copyright (c) * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by * the Free Software Foundation, version 3. * * This program is distributed in the hope that it will be useful, but * WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU * General Public License for more details. * * You should have received a copy of the GNU General Public License * along with this program. If not, see <http://www.gnu.org/licenses/>. */ V1042 This file is marked with copyleft license, which requires you to open the derived source code.
40. 40 The scary thing (kind of) On the first run
41. 41 • Old code == lots of noise • Yep, even on well-tested code • Nope, that's not an analyzer problem • So, what to do? • Suppress. And mark as technical debt • Refactoring is due, right? Wait a moment. What is this?
42. 42 OK, so which is better? Both are better Dynamic analysis Precise but slow Static analysis Fast but prone to friendly fire
43. 43 Put it all together SonarQube is an open-source platform developed by SonarSource for continuous inspection of code quality to perform automatic reviews with static analysis of code to detect bugs, code smells, and security vulnerabilities
44. 44 Put it all together SonarQube is an open-source platform developed by SonarSource for continuous inspection of code quality to perform automatic reviews with static analysis of code to detect bugs, code smells, and security vulnerabilities
45. 45 • Open source • What it can't do, you can add • Set it up once and use it in your CI • ??? • CHARTS!!11 SonarQube
46. 46 SonarQube
47. 47 SonarQube
48. 48 • Fix the most annoying issues right away • Monitor and clean up technical debt in an organised way • Watch that new code you add as to kill bugs early • Have metrics which allow you to evaluate how well you're doing • Overall have more control over the codebase OK, but what is it all for?
49. 49 • Dealing with an old project is doable • Mind the legacy • Analyzers can ease your pain • Automate it • Constantly evaluate code quality Let's summarize
50. QUESTIONS 50 How not to choke on a big old project Yuri Minaev – minaev@viva64.com www.pvs-studio.com