I checked Geany with valgrind with this command line: G_SLICE=always-malloc G_DEBUG=gc-friendly valgrind --tool=memcheck --leak-check=full --log-file=vgdump --suppressions=gtk.suppression --track-origins=yes --show-possibly-lost=no src/geany
gtk.suppression you may find here: https://wiki.gnome.org/Valgrind
Report: https://gist.github.com/scriptum/6691838
Possible leaks: - symbols.c:1877 - somewhere in tag manager (too many possibly leaks reported)
Other suggestions: - enable -Wall -Wextra -Werror in compiler - add cppcheck into makefile - add valgrind command line and exceptions into project - make debugging mode that toggles with defines/environment vars
-- Pavel
Le 25/09/2013 00:02, Pavel Roschin a écrit :
I checked Geany with valgrind with this command line: G_SLICE=always-malloc G_DEBUG=gc-friendly valgrind --tool=memcheck --leak-check=full --log-file=vgdump --suppressions=gtk.suppression --track-origins=yes --show-possibly-lost=no src/geany
gtk.suppression you may find here: https://wiki.gnome.org/Valgrind
Report: https://gist.github.com/scriptum/6691838
Possible leaks:
- symbols.c:1877
Fixed, thanks (https://github.com/geany/geany/commit/1ab97fe2e0c4222900bb6e4e8d54655cfefd87...)
- somewhere in tag manager (too many possibly leaks reported)
Other suggestions:
- enable -Wall -Wextra -Werror in compiler
Yes, kinda advertized in HACKING -- but -Werror is harder to use because you need to check your flags VERY carefully not to produce any false positive. E.g. -Wwrite-strings have a few false positive due to imperfect API. But yeah, it's nice to stop on possible errors. FTR, we (devs) all (I believe) use this or stricter options (but maybe not with -Werror).
- add cppcheck into makefile
We have this in geany-plugins. I'm not sure how useful it was 'til now, but at least it didn't report false positives. Out of curiosity, did cppcheck report any errors?
- add valgrind command line and exceptions into project
Maybe we should at least document it yeah.
- make debugging mode that toggles with defines/environment vars
You mean the various -DDEBUG_STUFF? Those are a bit too specific in general, e.g. if you enable this for CTags you'll get a lot of debugging prints that are of no interest if debugging anything else (and even might make it harder to see what you want).
And for environment vars, what are you referring to? G_DEBUG= and G_SLICE= ? IMO that's to be set by the caller depending on the need. E.g. I have custom GDB and Valgrind run scripts that set those as needed -- and these don't need the same actually, Valgrind needs resident-modules to have a chance to see a leak in a loaded module, but GDB don't since it's used to analyze running code.
Regards, Colomban
On Wed, Sep 25, 2013 at 12:27:14AM +0200, Colomban Wendling wrote:
[...]
Other suggestions:
- enable -Wall -Wextra -Werror in compiler
Yes, kinda advertized in HACKING -- but -Werror is harder to use because you need to check your flags VERY carefully not to produce any false positive. E.g. -Wwrite-strings have a few false positive due to imperfect API. But yeah, it's nice to stop on possible errors. FTR, we
s/imperfect/broken/
It's not acceptable to modify const strings. Ever.
(devs) all (I believe) use this or stricter options (but maybe not with -Werror). [...]
Le 25/09/2013 03:23, Chow Loong Jin a écrit :
On Wed, Sep 25, 2013 at 12:27:14AM +0200, Colomban Wendling wrote:
[...]
Other suggestions:
- enable -Wall -Wextra -Werror in compiler
Yes, kinda advertized in HACKING -- but -Werror is harder to use because you need to check your flags VERY carefully not to produce any false positive. E.g. -Wwrite-strings have a few false positive due to imperfect API. But yeah, it's nice to stop on possible errors. FTR, we
s/imperfect/broken/
It's not acceptable to modify const strings. Ever.
It's rather the other way around, API that expects string literals but also non-constant pointers. I'm watching you GtkTargetEntry.
(devs) all (I believe) use this or stricter options (but maybe not with -Werror). [...]
On Wed, Sep 25, 2013 at 03:37:11AM +0200, Colomban Wendling wrote:
Le 25/09/2013 03:23, Chow Loong Jin a écrit :
On Wed, Sep 25, 2013 at 12:27:14AM +0200, Colomban Wendling wrote:
[...]
Other suggestions:
- enable -Wall -Wextra -Werror in compiler
Yes, kinda advertized in HACKING -- but -Werror is harder to use because you need to check your flags VERY carefully not to produce any false positive. E.g. -Wwrite-strings have a few false positive due to imperfect API. But yeah, it's nice to stop on possible errors. FTR, we
s/imperfect/broken/
It's not acceptable to modify const strings. Ever.
It's rather the other way around, API that expects string literals but also non-constant pointers. I'm watching you GtkTargetEntry.
An API that takes a const char* can also take non-const char*'s. So if the function doesn't need to modify the string, it should just take a const char * instead.
On Wed, 25 Sep 2013 09:23:15 +0800 Chow Loong Jin hyperair@ubuntu.com wrote:
On Wed, Sep 25, 2013 at 12:27:14AM +0200, Colomban Wendling wrote:
[...]
Other suggestions:
- enable -Wall -Wextra -Werror in compiler
This combinarion may lead to a lot of unnecesary "fixes" purely to satisfy -Werror.
Yes, kinda advertized in HACKING -- but -Werror is harder to use because you need to check your flags VERY carefully not to produce any false positive. E.g. -Wwrite-strings have a few false positive due to imperfect API. But yeah, it's nice to stop on possible errors. FTR, we
s/imperfect/broken/
It's not acceptable to modify const strings. Ever.
Nobody really modifies them, that causes SIGSEGV.
An API that takes a const char* can also take non-const char*'s. So if the function doesn't need to modify the string, it should just take a const char * instead.
The non-Geany API-s are not under out control.
A function may modify a string (or any other *) with one set of documented arguments, and never modify it with another set of arguments. That's rare, but the separation between const and non-const is not absolute. And of course, there are memchr(), strstr() etc. which take a "maybe-const" and return it as non-const.
Yes, kinda advertized in HACKING -- but -Werror is harder to use because you need to check your flags VERY carefully not to produce any false positive. E.g. -Wwrite-strings have a few false positive due to imperfect API. But yeah, it's nice to stop on possible errors. FTR, we (devs) all (I believe) use this or stricter options (but maybe not with -Werror).
I think it's better to check compiler warning in 2 minutes than spending two days on debugging:) I prefer always enable all warnings in new projects. Clearly that Geany is big project and enabling all warnings may be quite painful. But it will certainly reduce number of errors - when you consider every line of code twice.
-Werror is a good practice for people who ignore compiler warnings:)
Some stupid warnings may be easily disabled (-Wno-unused-parameter).
-- Best regards Pavel Roschin aka RPG
Le 25/09/2013 21:48, Pavel Roschin a écrit :
Yes, kinda advertized in HACKING -- but -Werror is harder to use because you need to check your flags VERY carefully not to produce any false positive. E.g. -Wwrite-strings have a few false positive due to imperfect API. But yeah, it's nice to stop on possible errors. FTR, we (devs) all (I believe) use this or stricter options (but maybe not with -Werror).
I think it's better to check compiler warning in 2 minutes than spending two days on debugging:) I prefer always enable all warnings in new projects. Clearly that Geany is big project and enabling all warnings may be quite painful. But it will certainly reduce number of errors - when you consider every line of code twice.
AFAIK, we don't currently have any warning reported by GCC that we didn't consider and just let because it was too boring to fix for no gain -- what comes to mind is tons of integer size issue, which are not great but I checked tons of them and they were pretty harmless (yet not perfectly right otherwise they'd have an explicit cast by now). Also, compiler warnings are great, but they generally don't suffice to discover subtle bugs -- and most generally not leaks.
FTR, my well know (;) CFLAGS line is:
-Wall -W -O2 -Wunused -Wunreachable-code -Wformat=2 -Wundef -Wshadow -Wpointer-arith -Wbad-function-cast -Wwrite-strings -Wconversion -Waggregate-return -Wstrict-prototypes -Wmissing-prototypes -Wmissing-declarations -Wmissing-noreturn -Wmissing-format-attribute -Wredundant-decls -Wnested-externs -pedantic -Werror-implicit-function-declaration -fshow-column
I add -g for things I care debugging, and -Wno-unused-parameter to Geany because, huh, callbacks. I probably should strip these down to something less pedantic (;) for Geany because they cause too much warnings, but it's a good start :)
-Werror is a good practice for people who ignore compiler warnings:)
Yes, but it requires the compiler to generate absolutely no false positive. And this may require sacrificing an interesting warning flag. IMHO, developer should rather come up with something that generates a reasonably small amount of false positive and care about the warnings than use -Werror and remove a warning that creates one false positive and breaks their compilation.
Some stupid warnings may be easily disabled (-Wno-unused-parameter).
Yes, but some clever ones (-Wwrite-strings) may be wanted but generate warnings on some crazy APIs (like GtkTargetEntry).
Regards, Colomban