Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use gcc's static analyzer with make develop #970

Open
Rangi42 opened this issue Jan 14, 2022 · 8 comments
Open

Use gcc's static analyzer with make develop #970

Rangi42 opened this issue Jan 14, 2022 · 8 comments
Labels
builds This affects the build process or release artifacts meta This isn't related to the tools directly: repo organization, maintainership...

Comments

@Rangi42
Copy link
Contributor

Rangi42 commented Jan 14, 2022

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 Rangi42 added the meta This isn't related to the tools directly: repo organization, maintainership... label Jan 14, 2022
@ISSOtm
Copy link
Member

ISSOtm commented Feb 5, 2022

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

@Rangi42
Copy link
Contributor Author

Rangi42 commented Feb 5, 2022

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.

@ISSOtm
Copy link
Member

ISSOtm commented Feb 5, 2022

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?

@Rangi42
Copy link
Contributor Author

Rangi42 commented Feb 5, 2022

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

@ISSOtm
Copy link
Member

ISSOtm commented Feb 5, 2022

That's not compatible with BSD Make.

@Rangi42
Copy link
Contributor Author

Rangi42 commented Feb 5, 2022

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

@ISSOtm
Copy link
Member

ISSOtm commented Jun 12, 2022

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:

rgbds/src/asm/section.c

Lines 385 to 387 in d51ab35

// Add the new section to the list (order doesn't matter)
sect->next = sectionList;
sectionList = sect;

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

@ISSOtm
Copy link
Member

ISSOtm commented Jun 12, 2022

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 added a commit to ISSOtm/rgbds that referenced this issue Jun 12, 2022
ISSOtm added a commit to ISSOtm/rgbds that referenced this issue Nov 9, 2023
@Rangi42 Rangi42 added the builds This affects the build process or release artifacts label Dec 8, 2023
ISSOtm added a commit to ISSOtm/rgbds that referenced this issue Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builds This affects the build process or release artifacts meta This isn't related to the tools directly: repo organization, maintainership...
Projects
None yet
Development

No branches or pull requests

2 participants