SPEX: Store pointers to current gmp objects in archive.

[mmuetzel]

I've been staring at that part of the code for a while. And I think this is a correct change.
IIUC, x in those preprocessor macros is of type mpz_t (or other types). Casting it to mpz_t * (or the other pointer types) is a bit confusing.
Instead, store the addresses to those values in the *_archive variables.

Because mpz_t, mpq_t and mpfr_t "happen" to be array types, this probably doesn't affect the actual code execution.

[DrTimothyAldenDavis]

Yes, that's the intent. The change makes sense, and I see it passes the CI, but I'll double check it first.

[DrTimothyAldenDavis]

The change doesn't work. I tried adding two statements: one is the original one and the other is the revised one with &x. The pointers are different.

This doesn't trigger a failure in the basic tests ("make demos") because the 'archive' is only used when an out-of-memory failure occurs. That only happens in the SPEX/*/Tcov tests, run by "make cov".

The pointer itself is not dereferenced. It's only used to test if the parameter "x" is about to be freed, and the test is "are these 2 pointers the same".

So I think I still need the same value of the pointer, as x, not &x.

But the type of the pointer might be wrong; I suppose spex_gmpz_archive should be an mpz_t type, not mpz_t *.

The pointer type could be a void * since it's not deferenced, but just checked for equality. I think I just need to change the type of spec_gmpz_archive, from mpz_t * to just mpz_t. Then there would be no typecast at all, but just this:

spex_gmpz_archive = x ; // option 1

Or maybe declaring these archive pointers as void * is the best thing to do. That would denote the fact that we do not do anything with these pointers (except check for equality with them); the spex_gmpz_archive pointer is never dereferenced. In that case, I should just declare

void * spec_gmpz_archive ;

and then assign it with this:

spec_gmpz_archive = (void *) x ; // option 2

I need to think over which option is best (option 1 or 2).

I do recognize that this "archive" thing is very strange. The only purpose of this "archive" pointer is the test in SPEX_GMP_SAFE_FREE, so that spex_gmp_free does not accidentally free its input argument when a memory failure occurs. If GMP itself (and MPFR too) handled memory-failure conditions properly, then none of this would be needed. GMP just aborts the entire application if any memory allocation fails. We can't do that in SPEX, so that's why we never return a NULL pointer to GMP, but catch it ourselves with setjmp / longjmp.

[DrTimothyAldenDavis]

Your point with this PR is well-taken, though. This statement:

spex_gmpz_archive = (mpz_t *) x ;
is odd. It works, because I get the right pointer. It's confusing because the pointer is the wrong type, but its type doesn't actually matter since spex_gmpz_archive is never dereferenced.

But the confusion can be removed with option (1) or (2). I just need to think through which is best.

[mmuetzel]

The change doesn't work. I tried adding two statements: one is the original one and the other is the revised one with &x. The pointers are different.

I might have missed your point. But of course they will be different pointers. One is the pointer to the mpz_t object (an array type). The other is the pointer to the first (and only) __mpz_struct in that array.

The pointer itself is not dereferenced.

Afaict, it is derefenced. E.g., here:

SuiteSparse/SPEX/SPEX_Util/Source/SPEX_gmp.h

Line 72 in f9eab45

if (p == SPEX_MPZ_PTR(*spex_gmpz_archive)) \

And then that is dereferenced again here:

SuiteSparse/SPEX/SPEX_Util/Source/spex_util_internal.h

Line 266 in f9eab45

#define SPEX_MPZ_PTR(x) ((x)->_mp_d)

So, your option 2 won't work.

And I'm not sure if your option 1 would work either...

[DrTimothyAldenDavis]

Oops ... your right, I do dereference those variables. It's because a call to a GMP function can revise the content of its input parameters (like x->_mp_d) and I have to ensure those aren't freed.

[mmuetzel]

Looking at this in more detail, you really need to make this change (or an equivalent one). Or you might end up overriding random parts of the memory.

Could you please re-open?

[DrTimothyAldenDavis]

GMP defines its objects as arrays of size 1; see https://gmplib.org/gmp-man-6.3.0.pdf on page 18:

I should probably use mpz_ptr for the type of spec_gmpz_archive.

Perhaps the safest thing is to revise our manipulation of the internals of an mpz_t data type, to mimic how GMP does it: using "pointer decay" in a small function that accesses the contents of the variable (the _mp_d component).

[mmuetzel]

I should probably use mpz_ptr for the type of spec_gmpz_archive.

Good idea. I did that in the last commit.

[DrTimothyAldenDavis]

This looks right to me. I'll check it out.

[DrTimothyAldenDavis]

OK, this works great. I tested it in the SPEX/SPEX_Left_LU/Tcov. As an extra check, I added printf's to both the old and new versions, and it's finding the same special cases in both versions.

So the functionality is the same but this update is cleaner than the original.