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

GMT_IN or GMT_IN|GMT_IS_REFERENCE for external vectors? #3515

Closed
seisman opened this issue Jun 21, 2020 · 9 comments
Closed

GMT_IN or GMT_IN|GMT_IS_REFERENCE for external vectors? #3515

seisman opened this issue Jun 21, 2020 · 9 comments

Comments

@seisman
Copy link
Member

seisman commented Jun 21, 2020

The issue was first reported in GenericMappingTools/pygmt#406. The PyGMT script plots two vectors, and PyGMT crashes when plotting the second one.

Here is an equivalent C code, which mimics what PyGMT does.

#include "gmt.h"
int main () {
	void *API = NULL;                 /* The API control structure */
	struct GMT_VECTOR *V = NULL;	  /* Structure to hold input dataset as vectors */
	char input[GMT_VF_LEN] = {""};     			/* String to hold virtual input filename */
	char args[128] = {""};            			/* String to hold module command arguments */

	uint64_t dim[4] = {2, 2, 1, 0};
	double x[2] = {5.0, 5.0};
	double y[2] = {3.0, 8.0};

	/* Initialize the GMT session */
	API = GMT_Create_Session ("test", 2U, GMT_SESSION_EXTERNAL, NULL);
	/* Create a dataset */
	if ((V = GMT_Create_Data (API, GMT_IS_DATASET|GMT_VIA_VECTOR, GMT_IS_POINT, GMT_CONTAINER_ONLY, dim, NULL, NULL, 0, 0, NULL)) == NULL) return (EXIT_FAILURE);
	/**/
	GMT_Put_Vector(API, V, 0, GMT_DOUBLE, x);
	GMT_Put_Vector(API, V, 1, GMT_DOUBLE, y);
	/* Associate our data table with a virtual file */
	//GMT_Open_VirtualFile (API, GMT_IS_DATASET|GMT_VIA_VECTOR, GMT_IS_POINT, GMT_IN|GMT_IS_REFERENCE, V, input);
	GMT_Open_VirtualFile (API, GMT_IS_DATASET|GMT_VIA_VECTOR, GMT_IS_POINT, GMT_IN, V, input);
	/* Prepare the module arguments */
	sprintf (args, "%s -JX10c -R0/10/0/10 -Baf -W1p,black+ve0.2c -P -Vd > vectors.ps", input);
	/* Call the psxy module */
	GMT_Call_Module (API, "psxy", GMT_MODULE_CMD, args);
	GMT_Close_VirtualFile (API, input);
	/* Destroy the GMT session */
	if (GMT_Destroy_Session (API)) return EXIT_FAILURE;
};

The C code crashes at this line

gmt_M_free (GMT, x); gmt_M_free (GMT, y);

However, if I change the parameter GMT_IN to GMT_IN|GMT_IS_REFERENCE when calling GMT_Open_VirtualFile, it works fine.

Is it a bug in function gmtvector_fix_up_path_cartesian? What's the difference between GMT_IN to GMT_IN|GMT_IS_REFERENCE? Should PyGMT always uses GMT_IN|GMT_IS_REFERENCE?

@joa-quim
Copy link
Member

joa-quim commented Jun 21, 2020

As a general rule, all data that is allocated outside GMT must have in the corresponding dataset alloc_mode = GMT_ALLOC_EXTERNALLY so that GMT does not try to free it at the end.

@PaulWessel
Copy link
Member

GMT_IS_REFERENCE means that GMT must treat what you pass as read-only and cannot modify or free. But I may have a look later (Sunday am is complicated by obligatory morning swim and 2 hour zoom with son) so will get to this in ~5 hours... I am surprised that we look for REFERENCE in the direction argument.

@PaulWessel
Copy link
Member

Virtual file is created corectly, but when the file is passed to grdinfo and grdinfo calls GMT_Read_Data, the API object has had its family changed from GMT_IS_GRID to GMT_IS_DATASET and it is all downhill from there. Have not found out where family is reset to 0 (GMT_IS_DATASET) so will need to step through very carefully.

@seisman
Copy link
Member Author

seisman commented Jun 23, 2020

Ping to draw @PaulWessel's attention to this issue.

@PaulWessel
Copy link
Member

Surely this was fixed on the weekend. It had to do with not checking direction. Is this still a problem?

@seisman
Copy link
Member Author

seisman commented Jun 23, 2020

Do you mean this PR #3514? I don't think it's related to this issue, and it's still failing for me.

@PaulWessel
Copy link
Member

Well, if it crashes it does not matter what else I fixed. TO debug I need this C code to be in the repo. Is there a branch with this code? Needs to be bild with Xcode to be available to debug.

@seisman
Copy link
Member Author

seisman commented Jun 23, 2020

Please work on this PR #3528.

seisman added a commit to GenericMappingTools/pygmt that referenced this issue Jun 24, 2020
External programs like PyGMT can pass dataset/momory to GMT. By default,
GMT can read, modify and free the momery, which sometimes can cause
crashes.

Issue #406 reports an example in which PyGMT crashes. The issue was reported
to the upstream (see GenericMappingTools/gmt#3515
and GenericMappingTools/gmt#3528). It turns out
to be a API user error (i.e., a PyGMT bug).

As per the explanation of Paul, external programs like PyGMT should
always use `GMT_IN|GMT_IS_REFERENCE` to tell GMT that the data is
read-only, so that GMT won't try to change and free the memory.

This PR makes the change from `GMT_IN` to `GMT_IN|GMT_IS_REFERENCE`
in the `Session.open_virtual_file()` function, updates a few docstrings,
and also adds the script in #406 as a test.
seisman added a commit to GenericMappingTools/pygmt that referenced this issue Jun 24, 2020
External programs like PyGMT can pass dataset/momory to GMT. By default,
GMT can read, modify and free the momery, which sometimes can cause
crashes.

Issue #406 reports an example in which PyGMT crashes. The issue was reported
to the upstream (see GenericMappingTools/gmt#3515
and GenericMappingTools/gmt#3528). It turns out
to be a API user error (i.e., a PyGMT bug).

As per the explanation of Paul, external programs like PyGMT should
always use `GMT_IN|GMT_IS_REFERENCE` to tell GMT that the data is
read-only, so that GMT won't try to change and free the memory.

This PR makes the change from `GMT_IN` to `GMT_IN|GMT_IS_REFERENCE`
in the `Session.open_virtual_file()` function, updates a few docstrings,
and also adds the script in #406 as a test.
@PaulWessel
Copy link
Member

We have resolved this issue to use GMT_IN only, unless you are very sure passing the raw pointers will work (GMT_IN|GMT_IS_REFERENCE).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants