Introduce a default destructor for PJ objects #583

busstoptaktik · 2017-09-28T10:25:24Z

Most PJ objects contain no additionally allocated memory beyond the PJ->opaque level. Hence, most PJs can be deallocated using a default destructor.

This PR introduces pj_default_destructor and modifies all PJ_*.c files to use it. It results in a simplified deallocation and error messaging code flow, reduces the overall weight of the library by approximately 1400 LOC, and makes it easier to introduce new projections (and other coordinate operations).

QuLogic · 2017-09-29T03:00:05Z

src/pj_init.c

@@ -415,6 +415,7 @@ pj_init_plus_ctx( projCtx ctx, const char *definition )
 }


+void *dealloc_params (projCtx ctx, paralist *start, int errlev);


I'm not sure this makes sense here. If it's used elsewhere, it should be in a header file; if it's only used in this file, then it should be static and no prototype is needed.

Thanks, @QuLogic - I actually needed dealloc_params() (now pj_dealloc_params()) in pj_init_ctx() as well as in pj_default_destructor(), but had to take a break half way through, as it got very late :-)

pj_dealloc_params()is now in projects.h

busstoptaktik · 2017-09-29T13:32:09Z

@QuLogic @hobu @rouault - and everyone else with better “memory leak fu” than myself: I think this PR is getting ready for merging, but @kbevers and I have introduced some rather intrusive and extensive refactorings to the memory management aspect of PROJ.4, so a few new memory leaks would probably not be surprising, although the new state of things should help avoiding leaks in the future.

I have checked the library by running the debug build of proj -VC through DrMemory, which found nothing suspicious. If any of you have more serious superpowers and/or power tools to throw at the analysis, I would be grateful for any results and comments.

Note: We know already that leaks, related to grid management remain, cf. comment in pj_malloc.c:

/* We used to call pj_dalloc( P->catalog ), but this will leak */
/* memory. The safe way to clear catalog and grid is to call */
/* pj_gc_unloadall(pj_get_default_ctx()); and pj_deallocate_grids(); */
/* TODO: we should probably have a public pj_cleanup() method to do all */
/* that */

rouault · 2017-09-29T14:32:26Z

Note: We know already that leaks, related to grid management remain, cf. comment in pj_malloc.c:

/* We used to call pj_dalloc( P->catalog ), but this will leak /
/ memory. The safe way to clear catalog and grid is to call /
/ pj_gc_unloadall(pj_get_default_ctx()); and pj_deallocate_grids(); /
/ TODO: we should probably have a public pj_cleanup() method to do all /
/ that */

Those are not real "leaks". They are memory still reachable through proj.4 static variables. They will appear as "still reachable" in a Valgrind output, to be opposed to "definitely lost" blocks.

What could help to track memory mis-use/leaks is to have a Travis-CI target that would do a build with -fsanitize=address (requires a gcc/clang recent enough), that way we could detect issues in code that is tested by the test suit. and/or run it through Valgrind

busstoptaktik · 2017-09-30T15:40:59Z

What could help to track memory mis-use/leaks is to have a Travis-CI target that would do a build with -fsanitize=address (requires a gcc/clang recent enough)

Good idea. But since my .travis-fu is even weaker than my memleak-fu, I will check it by doing a commit simply adding -fsanitize=address to CFLAGS in the travis setup, letting the tests run, and then removing that change

rouault · 2017-10-01T14:10:08Z

src/PJ_ob_tran.c

+    size_t argc = paralist_params_argc (params);
+    if (0==argc)
+        return args;
+    args.argv = pj_calloc (argc, sizeof (char *));


As far as I can see this args.argv is never deallocated

Oops! Thanks! Correcting immediately

rouault · 2017-10-01T14:10:51Z

src/PJ_ob_tran.c

+        if (0==strcmp (params->param, "proj=ob_tran"))
+            continue;
+        args.argv[i++] = params->param;
+        if (0==params->next)


useless test. the for() test condition will take care of that

Very well spotted... remnants from debugging instrumentation - removed now

In most cases memory deallocation is completely removed from the code since it can be handled by the default destructor. In a few special cases a local destructor overrides the default destructor and makes sure that locally allocated memored is cleaned up correctly. Move all deallocation from pj_free to pj_default_destructor Rename pj_latlong.c to fit with the conventional format PJ_latlong.c - freeup was missed here due to wrong naming Clean up pj_init to avoid double deallocation; Also resolve #576 by adding z_0 and t_0 options in pj_init, while cleaning Add a prototype for dealloc_params Added missing errno.h include in pj_ctx.c Temporarily removing ob_tran from testvarious, to be sure that is where the trouble is Make PJ_ob_tran.c use proper initialization for the chained projection proj=ob_tran: make it clear, that we disallow ellipsoidal projections, and, for improved backwards compatibility, turns off default settings, which could inject unwanted ellipsoid definitions ... then also remove the ellipsoid definition from the testvarious test case - which is probably buggy anyway Work around cs2cs spherical init bug in testvarious; Forbid defs for ob_tran in pj_init

Elim some leaks by initializing PJ.destructor in PJ_ob_tran.c properly Avoid tests bombing when built with address sanitizer: Repair memory leak in test228.c Avoid tests bombing when built with address sanitizer: Repair memory leak in multistresstest.c

… wrong, it gives consistent results, and is more safe than the original hack of setting es=0

busstoptaktik · 2017-10-06T11:52:29Z

This PR seems to build and test OK now, and I intend to merge it tonight.

Following advice from @rouault, I added -fsanitize=address to the linux-clang Travis build, during the memleak hunting. It helped tracking down not only the leak I was searching for, but also additional leaks in the multistresstest.c code. This is evidently useful, so I have kept it in, for improved future memleak detection.

While chasing the leak in the ob_tran code, I also enabled the use of ellipsoidal projections with ob_tran. Originally ob_tran shared parts of the PJ object with its target projection, which was somewhat hacky.

Now, the target projection sits in a separate PJ, and while using spherical formulas to rotate an ellipsoidal projection is formally wrong, it really just amounts to defining a slightly different compound projection. Output data are consistent, so while we could return an error message, I have, for now, decided to let it claim success, and return a PJ, so you can get what you ask for if you know what you do.

Thanks go to @rouault, @QuLogic for some useful reviews and comments, and to @kbevers, who did much of the initial hard work of touching up every single PJ_xxx.c file to work with the centralised default destructor.

busstoptaktik and others added 2 commits September 28, 2017 10:52

Support a default destructor for PJ objects

fa318ed

Add PJD_ERR_* aliases for error numbers without one

0495fdc

busstoptaktik added this to the 4.9.4 milestone Sep 28, 2017

busstoptaktik added C: Core enhancement labels Sep 28, 2017

busstoptaktik self-assigned this Sep 28, 2017

QuLogic reviewed Sep 29, 2017

View reviewed changes

rouault reviewed Oct 1, 2017

View reviewed changes

busstoptaktik mentioned this pull request Oct 3, 2017

Pro forma PR to force a check of ob_tran memleak. DO NOT MERGE!!! #586

Closed

busstoptaktik added 3 commits October 6, 2017 11:39

Enable ob_tran for ellipsoidal projection definitions. While formally…

ee0e2a0

… wrong, it gives consistent results, and is more safe than the original hack of setting es=0

busstoptaktik merged commit 2024554 into OSGeo:master Oct 6, 2017

busstoptaktik deleted the destructor branch October 6, 2017 16:56

kbevers modified the milestones: 5.0.0-b, 5.0.0 Feb 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a default destructor for PJ objects #583

Introduce a default destructor for PJ objects #583

busstoptaktik commented Sep 28, 2017

QuLogic Sep 29, 2017

busstoptaktik Sep 29, 2017 •

edited

busstoptaktik commented Sep 29, 2017 •

edited

rouault commented Sep 29, 2017

busstoptaktik commented Sep 30, 2017

rouault Oct 1, 2017

busstoptaktik Oct 1, 2017

rouault Oct 1, 2017

busstoptaktik Oct 1, 2017

busstoptaktik commented Oct 6, 2017

		@@ -415,6 +415,7 @@ pj_init_plus_ctx( projCtx ctx, const char *definition )
		}


		void dealloc_params (projCtx ctx, paralist start, int errlev);

Introduce a default destructor for PJ objects #583

Introduce a default destructor for PJ objects #583

Conversation

busstoptaktik commented Sep 28, 2017

QuLogic Sep 29, 2017

Choose a reason for hiding this comment

busstoptaktik Sep 29, 2017 • edited

Choose a reason for hiding this comment

busstoptaktik commented Sep 29, 2017 • edited

rouault commented Sep 29, 2017

busstoptaktik commented Sep 30, 2017

rouault Oct 1, 2017

Choose a reason for hiding this comment

busstoptaktik Oct 1, 2017

Choose a reason for hiding this comment

rouault Oct 1, 2017

Choose a reason for hiding this comment

busstoptaktik Oct 1, 2017

Choose a reason for hiding this comment

busstoptaktik commented Oct 6, 2017

busstoptaktik Sep 29, 2017 •

edited

busstoptaktik commented Sep 29, 2017 •

edited