rgbds icon indicating copy to clipboard operation
rgbds copied to clipboard

Use gcc's static analyzer with `make develop`

Open Rangi42 opened this issue 3 years ago • 8 comments

Passing -fanalyzer -fanalyzer-verbosity=0 warns about various potential issues (double free, use after free, NULL dereference, etc), without having to use ASan at runtime. https://gcc.gnu.org/onlinedocs/gcc-10.1.0/gcc/Static-Analyzer-Options.html

These are the current warnings (although the bison-generated parser.c warnings are unavoidable):

src/asm/parser.c:1199:21: warning: leak of ‘yyptr’ [CWE-401] [-Wanalyzer-malloc-leak]
src/asm/parser.c:1205:19: warning: ‘free’ of ‘yyss1’ which points to memory not on the heap [CWE-590] [-Wanalyzer-free-of-non-heap]
src/asm/symbol.c:747:66: warning: dereference of NULL ‘0’ [CWE-476] [-Wanalyzer-null-dereference]
src/asm/symbol.c:746:63: warning: dereference of NULL ‘0’ [CWE-476] [-Wanalyzer-null-dereference]
src/asm/symbol.c:746:63: warning: dereference of NULL ‘0’ [CWE-476] [-Wanalyzer-null-dereference]
src/gfx/gb.c:231:24: warning: leak of ‘<unknown>’ [CWE-401] [-Wanalyzer-malloc-leak]
src/gfx/makepng.c:618:25: warning: leak of ‘malloc(png_get_rowbytes(*img.png, *img.info))’ [CWE-401] [-Wanalyzer-malloc-leak]

Rangi42 avatar Jan 14 '22 21:01 Rangi42

Imo that's a bit of a crapshoot given that -fanalyzer is GCC-specific, clashing with #283. Possible alternatives:

  • Only run -fanalyzer in CI, as a separate job
  • Only run -fanalyzer in the CMakeLists if GCC is detected

ISSOtm avatar Feb 05 '22 09:02 ISSOtm

The are other warning flag differences between gcc and clang (e.g. only gcc supports -Wduplicated-branches -Wduplicated-cond -Wlogical-op and only clang supports -Wkeyword-macro) so I'd be fine with adding additional warning flags for specific compilers on top of the default cross-platform ones.

Rangi42 avatar Feb 05 '22 19:02 Rangi42

Clang does not warn about unknown warning flags thanks to -Wno-unknown-warning-option, but the opposite is not true of GCC (it warns unconditionally of unknown warning flags, unless it's a -Wno-).

The problem is that GCC's static analyzer is a -f flag, which I don't think would play nice with this.

As for per-compiler warning flags, the question is how to detect them in the Makefile?

ISSOtm avatar Feb 05 '22 19:02 ISSOtm

As for per-compiler warning flags, the question is how to detect them in the Makefile?

https://github.com/aaaaaa123456789/libplum/blob/develop/Makefile#L10-L15

Rangi42 avatar Feb 05 '22 19:02 Rangi42

That's not compatible with BSD Make.

ISSOtm avatar Feb 05 '22 19:02 ISSOtm

Hmm. Alright, make develop doesn't have to run -fanalyzer, but its output is worth paying attention to when manually cleaning up the code.

Rangi42 avatar Feb 05 '22 19:02 Rangi42

As it turns out, at least GCC 12's analyzer is over-eager, for example not understanding that this does not leak the contents of sectionList:

https://github.com/gbdev/rgbds/blob/d51ab3520328fe7bd82f37da276180c3d7d4059d/src/asm/section.c#L385-L387

I'm not sure we want to enable it in CI, then. At least not yet.

ISSOtm avatar Jun 12 '22 14:06 ISSOtm

Plus, building the parser with the static analyzer OOMs on my machine (and I have upwards of 3 GiB of RAM available!), so, uh...

ISSOtm avatar Jun 12 '22 14:06 ISSOtm