`XXX_ptr` vs `XXX_t`

[albinahlback]

In some functions, for example arf_mul_si, there is use of arf_ptr instead of arf_t as well as arf_srcptr instead of const arf_t. I think arf_ptr and arf_srcptr "only should be used as a pointers". What I mean by that is that they either represent an array or a (temporary) pointer, where the pointer might change value. In the named function, it is clear that the pointers only point to one arf_struct, in which case I think arf_t should be used.

Do you agree? In the case that you do not agree, I think that arf_t should be omitted from the definitions, and that only arf_[src]ptr should be used in order to be consistent.

[fredrik-johansson]

I agree that there is an inconsistency in the fact that some arf methods use arf_ptr / arf_srcptr where arf_t / const arf_t would normally be used. IIRC I did this just for the convenience of being able to swap pointers inside the function bodies. To use arf_t / const arf_t in the signatures, one would have to assign the pointers to temporary variables; just a small inconvenience.

In some way using ptr types srcptr instead of _t types in function signatures would be more convenient precisely because pointers can be swapped, but it's not the convention we adopted for FLINT. In practice it should not matter much since the C calling convention for array-of-length-1 and pointer is exactly the same. Apart from the minor inconsistency, is there a problem that needs to be fixed?

[albinahlback]

GCC complains at some points that arf_[src]ptr is used in the header and that arf_t is used in the function definition (or if it was the other way around). This happens in arf_mul_via_mpfr for example. But nothing serious.

Edit: I now think that arf_t should be replaced by arf_ptr.

[albinahlback]

By now, I seriously doubt that XXX_t and XXX_ptr is equivalent in function definitions. I am brewing on a PR for Flint to suppress warnings, and it seems like the only way to do that is to write XXX_ptr instead of XXX_t.

Edit: This is done by looking at GCC's output. I have not found another way.

[fredrik-johansson]

Before embarking on that I would try to investigate if there any real implications of the warning (performance, wrong code generation) or if it's just a false positive that we can suppress safely with compiler flags.

[albinahlback]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102810

If I understand this correctly:

void fun(int a[]);                  // Nemas problemas
void fun(int * a);                  // Nemas problemas
void fun(int a[3]);                 // ? haven't encountered a problem with these
void fun(const int * a);            // Nemas problemas
void fun(const int a[]);            // Nemas problemas
void fun(const int a[3]);           // Muchos problemos

As for the last function, it assumes that the input is an array of 3 ints, and will therefore read 3 int (for optimization reasons I guess). That is why we see problems with const XXX_ctx_t in FLINT where it reads a large amount of bytes, when it only required 8 (think I've seen up to ~160 bytes read), stemming from the fact that some XXX_ctx_struct contains alot of objects.

With this in mind, I think the most reasonable thing is to take GMP's approach and simply introduce stuff like XXX_ptr, XXX_srcptr, and so on. I suspect this would take about one hour to do with some find and sed/awk.

[fredrik-johansson]

This is still mysterious to me. arf_t (for example) is defined as an array of arf_struct of length 1, so it should always be safe to read exactly sizeof(arf_struct) bytes. Same with the various context objects.

[albinahlback]

This is still mysterious to me. arf_t (for example) is defined as an array of arf_struct of length 1, so it should always be safe to read exactly sizeof(arf_struct) bytes. Same with the various context objects.

Don't know if I've encountered problems with arf in this aspect (the thing I mentioned has to do with different definitions of a function in header vs function file).

However, problems occur for at least mag where it reads 16 bytes instead of 8. This is expected since sizeof(mag_struct) == 16. The first time this occurs is when compiling arb functions.

[fredrik-johansson]

I'm still not seeing the problem. Yes, it reads 16 bytes. Why "instead of 8"? Why would it only be allowed to read 8 bytes?

[albinahlback]

I'm not sure, but can it be because of this?

#include <stdio.h>

int
main()
{
    long int a[3] = {0, 1, 2};

    printf("&a = %p\n", &a);
    printf("a + 0 = %p\n", a + 0);
    printf("a + 1 = %p\n", a + 1);
    printf("a + 2 = %p\n", a + 2);
}

Output:

&a = 0x7ffd458adca0       // Same as below
a + 0 = 0x7ffd458adca0
a + 1 = 0x7ffd458adca8
a + 2 = 0x7ffd458adcb0

And so the it reads the following bytes after the pointer? So if a mag_t is really only a pointer, it will still read the bytes after this pointer. But perhaps I'm out cycling as we say in Sweden.

[albinahlback]

Any thoughts into this? As mentioned, I can prepare a PR which shouldn't take too long with some sed.

[wbhart]

I still don't understand what the issue is either. GMP have had _ptr and _srcptr types for a very long time, long before this was an issue with recent GCC (and probably for other reasons).

I think I'd like to understand the issue properly before just doing a radical overhaul of everything in Flint on the assumption that it fixes the problem.

If things are declared properly, there should be no overreading. I currently don't understand why that is happening.

[wbhart]

@albinahlback I don't understand what your example is supposed to show. You are not reading anything. You are merely printing the pointer values and these seem to be printed correctly to me.

[albinahlback]

Yeah, my example is incorrect. I read somewhere that something similar is fixed in the current state of GCC, so let's wait until GCC 12 is released until we draw any conclusions. Should be released in about a month.

[albinahlback]

GCC complains at some points that arf_[src]ptr is used in the header and that arf_t is used in the function definition (or if it was the other way around). This happens in arf_mul_via_mpfr for example. But nothing serious.

Edit: I now think that arf_t should be replaced by arf_ptr.

The compiler warning of arf_mul_via_mpfr was fixed in https://github.com/fredrik-johansson/arb/commit/c3a944c54da9be88a86babf73e1948936b29655f. However, I think there is still one or two other places where this occurs.