[GRASS-dev] About the vector changes

[Glynn_Clements1]

Markus Metz wrote:

>>> Ah, ok, you use the grass57 dataset. I have now changed the version
>>> number and compatibility info, you should now get reasonable
>>> warnings/errors with the spearfish57 dataset.
>>>
>> Okay, I'll check it.
>>
>
> Nope. As of r38644, the error message is unchanged.

I got the order of the checks wrong. First check must be version check,
only after that comes the check for file size. That LFS check is there
to check if sizeof(off_t) = 8 is needed to read the sidx file. Fixed in
r36845.

Yep; it now says:

  ERROR: Spatial index format version 5.0 is not supported by this release.
         Please rebuild topology.

However, as a result of the LFS changes, the code has some type
mismatches on 32-bit systems with LFS enabled.

Most of these are due to passing off_t's to G_debug() with %ld as the
format (it needs to be %lld for a 64-bit off_t on a 32-bit platform;
we really need a macro for this). There's also one G_fatal_error()
call which has "%d".

These shouldn't actually cause any problems beyond reporting bogus
offsets in debug/error messages. On little-endian platforms, the
offset will be truncated to 32 bits (so it has no effect for offsets
<2GiB). It's more of an issue on big-endian platforms, as you get the
high word, so offsets will be divided by 4GiB (i.e.they'll normally be
zero).

In lib/vector/diglib/port_init.c,

  #define OFF_T_TEST 0x0102030405060708

should have an LL suffix (it's too large for an "int"), and this
should ideally be conditionalised (although I don't know if we have a
macro for "off_t is 64-bits"; _FILE_OFFSET_BITS may not be defined on
64-bit platforms, and you can't use sizeof in #if tests). I believe
that the compiler is free to truncate the above to an int, regardless
of what it's assigned to. If that happens, the vector code will fail
entirely.

Finally:

spindex_rw.c:657: warning: cast from pointer to integer of different size

spindex_rw.c:715: warning: cast to pointer from integer of different size
spindex_rw.c:746: warning: cast to pointer from integer of different size

The first one is less of an issue; a platform with 64-bit pointers
will probably have a 64-bit off_t. The converse isn't true; on a
32-bit system with LFS, storing a 64-bit off_t in a 32-bit pointer
isn't going to work.

The lines in question are:

656: if (s[top].sn->level == 0) /* leaf node */
657: s[top].pos[j] = (off_t) s[top].sn->branch[j].child;

714: s[top].sn.branch[j].child =
715: (struct Node *)s[top].childpos[j];

745: s[top].sn.branch[j].child =
746: (struct Node *)s[top].childpos[j];

ISTR that it should only cast childpos[j] to a pointer if that is what
was stored there in the first place. But I would strongly suggest
using a union rather than casting; we shouldn't expect people to
simply ignore compiler warnings, and it's unreasonable to expect
anyone to analyse the code to the extent that they can determine that
the warnings are harmless.

--
Glynn Clements <glynn@gclements.plus.com>

[Hamish]

Helena wrote:

(perhaps time to finish GRASS64 and release it -
RC1 has been released in December 2008),

remaining RC issues:

https://trac.osgeo.org/grass/query?status=new&status=assigned&status=reopened&priority=blocker&priority=critical&milestone=6.4.0&order=priority

580 WinGRASS: $GISBASE/etc/gui/scripts/ require something like $(EXE) to run
595 WinGRASS g.version -c fails
608 GIS.m: Lat/Lon support broken in Map Display zooms
660 v.delaunay producing incorrect results
677 write 6.4.0 release announcement
707 wxGUI: v.digit broken for new maps: "No vector map selected for editing." although selected
72 PNG driver: boundary rendering is off by one pixel
198 v.in.ascii: column scanning is borked
335 export floats and doubles with correct precision
384 wxGUI: vdigit crashes on a big map
461 v.to.db crashes on a shapefile connected with v.external
593 WinGRASS GIS.m: cannot select maps from mapset GUI
637 Problems with paths in the TCL/TK Windows GUI

Hamish

[Markus_GRASS]

Glynn Clements wrote:

Markus Metz wrote:

d.vect, etc use the libraries and VECT_CFLAGS, so it isn't just v.*
modules which need to be checked. It might just be v.* modules which
need to be fixed; I don't know.
      

OK. I will check all modules including <grass/vector.h> or any of the headers in include/vect. All vector modules should be fixed by now. Of the raster modules, only r.drain needed fixing, all other were ok. But I will check again.
    
I suggest checking for VECT_CFLAGS in the Makefiles.

I've done that already. I also checked for <grass/vector.h> in g.* and r.* modules, it looks all fine to me. The import/export modules are a different issue, they may need extra attention to become LFS-safe. Also some libraries, Helena said rst needs fixing, not sure about others like the segment library.

  

ideally, we should use G_{fseek,ftell} everywhere. Ultimately, it
would be better to enable -D_FILE_OFFSET_BITS=64 globally (to ensure
that there aren't any cases where a FILE* is obtained via fopen64()
then passed to code which can't handle it), which means making
everything LFS-aware.
      

That's beyond my scope because aclocal.m4 and configure.in need adjustment, and I won't touch these. I guess you are suitably qualified for the task
    
The build system changes are straigtforward; just set
_FILE_OFFSET_BITS in config.h (this is already done, but it's
conditional upon USE_LARGEFILES, which doesn't get set).
  

I thought USE_LARGEFILES is set in configure and passed to Platform.make.in line 248

The effort is in ensuring that code doesn't fail in inconvenient ways
(i.e. corrupting data) when dealing with files >2GiB, i.e. converting
everything to use off_t.
  

Is it possible that configure sets LFS_FLAGS as a result of the LFS tests which are stolen from cdrtools and in aclocal.m4?
It would be easier if configure could check if all we need is _FILE_OFFSET_BITS and not also conditionally _LARGE_FILES, _LARGEFILE_SOURCE, -n32 for LFS to work on 32bit systems.

Markus M

[Markus_GRASS]

Glynn:

[...]

However, as a result of the LFS changes, the code has some type
mismatches on 32-bit systems with LFS enabled.

Most of these are due to passing off_t's to G_debug() with %ld as the
format (it needs to be %lld for a 64-bit off_t on a 32-bit platform;
we really need a macro for this). There's also one G_fatal_error()
call which has "%d".
  

There was something about long long int, it's part of the C99 standard but not supported by all compilers if I remember correctly. That's the reason for HAVE_LONG_LONG_INT?

These shouldn't actually cause any problems beyond reporting bogus
offsets in debug/error messages. On little-endian platforms, the
offset will be truncated to 32 bits (so it has no effect for offsets
<2GiB). It's more of an issue on big-endian platforms, as you get the
high word, so offsets will be divided by 4GiB (i.e.they'll normally be
zero).

In lib/vector/diglib/port_init.c,

  #define OFF_T_TEST 0x0102030405060708

should have an LL suffix (it's too large for an "int"), and this
should ideally be conditionalised

Its usage is conditionalised: it's only used if sizeof(off_t) == 8, otherwise OFF_T_TEST is ignored and LONG_TEST is used instead. Applies to port_test.c as well. The current portability test is passed on 32bit with LFS.

(although I don't know if we have a
macro for "off_t is 64-bits"; _FILE_OFFSET_BITS may not be defined on
64-bit platforms, and you can't use sizeof in #if tests). I believe
that the compiler is free to truncate the above to an int, regardless
of what it's assigned to. If that happens, the vector code will fail
entirely.

Finally:

spindex_rw.c:657: warning: cast from pointer to integer of different size

spindex_rw.c:715: warning: cast to pointer from integer of different size
spindex_rw.c:746: warning: cast to pointer from integer of different size

The first one is less of an issue; a platform with 64-bit pointers
will probably have a 64-bit off_t. The converse isn't true; on a
32-bit system with LFS, storing a 64-bit off_t in a 32-bit pointer
isn't going to work.

The lines in question are:

656: if (s[top].sn->level == 0) /* leaf node */
657: s[top].pos[j] = (off_t) s[top].sn->branch[j].child;

714: s[top].sn.branch[j].child =
715: (struct Node *)s[top].childpos[j];

745: s[top].sn.branch[j].child =
746: (struct Node *)s[top].childpos[j];

ISTR that it should only cast childpos[j] to a pointer if that is what
was stored there in the first place.

In line 746, I was stuffing an off_t value that is smaller than INT_MAX into a pointer. There was an integer stored initially.

But I would strongly suggest
using a union rather than casting;

You suggested that earlier already. First time I work with unions, done in trunk r38654. Could please have a look at lib/vector/rtree/index.h? The compiler warnings above about pointer <-> integer casts are gone, but I'm not sure if I used that union thing correctly.

Picking your brain, unrelated: can I assume that DBL_MAX is always available on all platforms? I would like to use it in lib/vector/rtree.

we shouldn't expect people to
simply ignore compiler warnings, and it's unreasonable to expect
anyone to analyse the code to the extent that they can determine that
the warnings are harmless.
  

Agreed. Any volunteers out there who want to analyse the rtree code? That would actually be very welcome...

Markus M

[Glynn_Clements1]

Markus Metz wrote:

> However, as a result of the LFS changes, the code has some type
> mismatches on 32-bit systems with LFS enabled.
>
> Most of these are due to passing off_t's to G_debug() with %ld as the
> format (it needs to be %lld for a 64-bit off_t on a 32-bit platform;
> we really need a macro for this). There's also one G_fatal_error()
> call which has "%d".

There was something about long long int, it's part of the C99 standard
but not supported by all compilers if I remember correctly. That's the
reason for HAVE_LONG_LONG_INT?

Yep. We don't require a C99 compiler, so we can't use "long long int"
or %lld unconditionally. However, we can assume that they exist if LFS
is enabled.

> In lib/vector/diglib/port_init.c,
>
> #define OFF_T_TEST 0x0102030405060708
>
> should have an LL suffix (it's too large for an "int"), and this
> should ideally be conditionalised

Its usage is conditionalised: it's only used if sizeof(off_t) == 8,
otherwise OFF_T_TEST is ignored and LONG_TEST is used instead. Applies
to port_test.c as well. The current portability test is passed on 32bit
with LFS.

I mean compile-time conditonalisation. An integer literal without a
trailing suffix is an "int". Using a larger type to hold the value
where necessary is a gcc extension. Other compilers may simply
truncate the value to an int, even if off_t is 64 bits.

The latest change:

  #define OFF_T_TEST 0x0102030405060708LL

will fail to compile with a compiler which doesn't have long long int.
Both the definition and use of OFF_T_TEST need to be conditionalised
with "#ifdef HAVE_LONG_LONG_INT".

> But I would strongly suggest
> using a union rather than casting;
You suggested that earlier already. First time I work with unions, done
in trunk r38654. Could please have a look at lib/vector/rtree/index.h?
The compiler warnings above about pointer <-> integer casts are gone,
but I'm not sure if I used that union thing correctly.

I still see:

719: s[top].sn.branch[j].child.id =
720: (int)s[top].childpos[j];

754: s[top].sn.branch[j].child.id =
755: (int)s[top].childpos[j];

Also:

1011: if (!shcb((int)s[top].sn.branch[i].child, cbarg)) {

A narrowing cast always brings up the question of "what if the value
won't fit in the destination type"?

Am I missing something, or is childpos[j] *always* an "int"? If so,
why is it stored in the file as an off_t?

Picking your brain, unrelated: can I assume that DBL_MAX is always
available on all platforms? I would like to use it in lib/vector/rtree.

DBL_MAX is always available in float.h.

--
Glynn Clements <glynn@gclements.plus.com>

[Glynn_Clements1]

Markus Metz wrote:

> The build system changes are straigtforward; just set
> _FILE_OFFSET_BITS in config.h (this is already done, but it's
> conditional upon USE_LARGEFILES, which doesn't get set).

I thought USE_LARGEFILES is set in configure and passed to
Platform.make.in line 248

It's defined as a Makefile variable, not as a preprocessor macro.

> The effort is in ensuring that code doesn't fail in inconvenient ways
> (i.e. corrupting data) when dealing with files >2GiB, i.e. converting
> everything to use off_t.

Is it possible that configure sets LFS_FLAGS as a result of the LFS
tests which are stolen from cdrtools and in aclocal.m4?
It would be easier if configure could check if all we need is
_FILE_OFFSET_BITS and not also conditionally _LARGE_FILES,
_LARGEFILE_SOURCE, -n32 for LFS to work on 32bit systems.

Until everything is made LFS-aware, we need the separate macros so
that modules can use specific features without having to worry about
libraries which may not be LFS-aware.

OTOH, we're already in trouble due to the whole of lib/gis using
_FILE_OFFSET_BITS=64. This means that G_fopen_old() etc will open
files >2GiB, even though the caller may not be LFS-aware.

Unfortunately, there isn't an fopen32(). Once you set
_FILE_OFFSET_BITS=64, there's no way to get at the original 32-bit
fopen() function for specific cases.

--
Glynn Clements <glynn@gclements.plus.com>

[Markus_GRASS]

Glynn:

[...] An integer literal without a
trailing suffix is an "int". Using a larger type to hold the value
where necessary is a gcc extension. Other compilers may simply
truncate the value to an int, even if off_t is 64 bits.

The latest change:

  #define OFF_T_TEST 0x0102030405060708LL

will fail to compile with a compiler which doesn't have long long int.
Both the definition and use of OFF_T_TEST need to be conditionalised
with "#ifdef HAVE_LONG_LONG_INT".
  

OK. But on Linux 32 without LFS I get

port_init.c:217: warning: overflow in implicit constant conversion

at compile time because long long int is available but sizeof(off_t) = 4. Portability and its test is ok because OFF_T_TEST is not used in this case.
To avoid this warning I could use in port_init()

#ifdef HAVE_LONG_LONG_INT
#ifdef HAVE_LARGEFILES
u_o = OFF_T_TEST;
#else
u_o = LONG_TEST;
#endif
#else
u_o = LONG_TEST;
#endif

but I am not sure if HAVE_LARGEFILES is always set on 64bit systems also without --enable-largefile.

I still see:

719: s[top].sn.branch[j].child.id =
720: (int)s[top].childpos[j];

754: s[top].sn.branch[j].child.id =
755: (int)s[top].childpos[j];

Also:

1011: if (!shcb((int)s[top].sn.branch[i].child, cbarg)) {

A narrowing cast always brings up the question of "what if the value
won't fit in the destination type"?

Am I missing something, or is childpos[j] *always* an "int"?

On level 0, it is always an int. It's the ID of the rectangle passed to
int RTreeInsertRect(struct Rect *r, int tid, struct RTree *t)
as tid. That applies to all three cases above.

I am writing out the rectangle ID as off_t although it is int and have to read it as off_t then cast it to int again when loading the sidx file to memory for updating. Of course I could also write the ID as int and read it as int, then there would be no type casting, at the cost of one more if ... else ... .

If so,
why is it stored in the file as an off_t?
  

On higher levels (RTree internal nodes), it is the position in file of the child nodes of a node. For large sidx files, off_t is needed. The corresponding line in spindex_rw.c is

668 s[top].pos[s[top].branch_id - 1] = nextfreepos;

where I set the node positions in file as off_t.

This is related to the use of union Child: on level 0, int id is used, on higher levels, struct Node *ptr is used.

Markus M

[Markus_GRASS]

Glynn:

Markus:
  

Is it possible that configure sets LFS_FLAGS as a result of the LFS tests which are stolen from cdrtools and in aclocal.m4?
It would be easier if configure could check if all we need is _FILE_OFFSET_BITS and not also conditionally _LARGE_FILES, _LARGEFILE_SOURCE, -n32 for LFS to work on 32bit systems.
    
Until everything is made LFS-aware, we need the separate macros so
that modules can use specific features without having to worry about
libraries which may not be LFS-aware.

OTOH, we're already in trouble due to the whole of lib/gis using
_FILE_OFFSET_BITS=64. This means that G_fopen_old() etc will open
files >2GiB, even though the caller may not be LFS-aware.

Unfortunately, there isn't an fopen32(). Once you set
_FILE_OFFSET_BITS=64, there's no way to get at the original 32-bit
fopen() function for specific cases.
  

Maybe it is less effort and less complicated to make everything LFS-safe instead of creating workarounds for modules that are not LFS-safe when LFS is enabled for lib/gis? One way or the other, I guess these modules need to be identified and possibly modified.

Markus M

[Glynn_Clements1]

Markus Metz wrote:

> [...] An integer literal without a
> trailing suffix is an "int". Using a larger type to hold the value
> where necessary is a gcc extension. Other compilers may simply
> truncate the value to an int, even if off_t is 64 bits.
>
> The latest change:
>
> #define OFF_T_TEST 0x0102030405060708LL
>
> will fail to compile with a compiler which doesn't have long long int.
> Both the definition and use of OFF_T_TEST need to be conditionalised
> with "#ifdef HAVE_LONG_LONG_INT".

OK. But on Linux 32 without LFS I get

port_init.c:217: warning: overflow in implicit constant conversion

at compile time because long long int is available but sizeof(off_t) =
4. Portability and its test is ok because OFF_T_TEST is not used in this
case.

Making the conversion explicit would eliminate the warning:

  u_o = (off_t) OFF_T_TEST;

To avoid this warning I could use in port_init()

#ifdef HAVE_LONG_LONG_INT
#ifdef HAVE_LARGEFILES
u_o = OFF_T_TEST;
#else
u_o = LONG_TEST;
#endif
#else
u_o = LONG_TEST;
#endif

I suggest:

1. Remove OFF_T_TEST.

2. Add:

  #ifdef HAVE_LONG_LONG_INT
  #define LONG_LONG_TEST 0x0102030405060708LL
  #endif

3. Change the initialisation of u_o to e.g.:

      if (nat_off_t == 8)
  #ifdef HAVE_LONG_LONG_INT
    u_o = (off_t) LONG_LONG_TEST;
  #else
    G_fatal_error("Internal error: can't construct an off_t literal");
  #endif
      else
    u_o = (off_t) LONG_TEST;

Actually, that's not ideal. A 64-bit system might have a 64-bit off_t
but no "long long" type. We really need to have autoconf use define
SIZEOF_LONG etc in config.h (AC_CHECK_SIZEOF macro), so that we can
use e.g.:

  #if SIZEOF_LONG == 8
  #define LONG_LONG_TEST 0x0102030405060708L
  #elif defined(HAVE_LONG_LONG_INT)
  #define LONG_LONG_TEST 0x0102030405060708LL
  #endif

but I am not sure if HAVE_LARGEFILES is always set on 64bit systems also
without --enable-largefile.

It won't be set if --enable-largefile isn't used.

> I still see:
>
> 719: s[top].sn.branch[j].child.id =
> 720: (int)s[top].childpos[j];
>
> 754: s[top].sn.branch[j].child.id =
> 755: (int)s[top].childpos[j];
>
> Also:
>
> 1011: if (!shcb((int)s[top].sn.branch[i].child, cbarg)) {
>
> A narrowing cast always brings up the question of "what if the value
> won't fit in the destination type"?
>
> Am I missing something, or is childpos[j] *always* an "int"?

On level 0, it is always an int. It's the ID of the rectangle passed to
int RTreeInsertRect(struct Rect *r, int tid, struct RTree *t)
as tid. That applies to all three cases above.

I am writing out the rectangle ID as off_t although it is int and have
to read it as off_t then cast it to int again when loading the sidx file
to memory for updating. Of course I could also write the ID as int and
read it as int, then there would be no type casting, at the cost of one
more if ... else ... .

> If so,
> why is it stored in the file as an off_t?

On higher levels (RTree internal nodes), it is the position in file of
the child nodes of a node. For large sidx files, off_t is needed. The
corresponding line in spindex_rw.c is

668 s[top].pos[s[top].branch_id - 1] = nextfreepos;

where I set the node positions in file as off_t.

This is related to the use of union Child: on level 0, int id is used,
on higher levels, struct Node *ptr is used.

So it's a case of simplifying the sidx file I/O?

That's reasonable enough. But, personally, I would:

1. Make the pos and childpos fields unions (int/off_t).

2. Add an explicit range check before casting the off_t read from the
file to an int. An assert() would suffice; the main purpose is to
answer the "what if it doesn't fit in an int?" question for the
benefit of anyone trying to read that code.

--
Glynn Clements <glynn@gclements.plus.com>

[Glynn_Clements1]

Markus Metz wrote:

Maybe it is less effort and less complicated to make everything LFS-safe
instead of creating workarounds for modules that are not LFS-safe when
LFS is enabled for lib/gis? One way or the other, I guess these modules
need to be identified and possibly modified.

It certainly looks that way.

It's basically a case of examining anything which uses lseek(),
fseek() or ftell(), converting fseek/ftell to G_fseek/G_ftell
respectively, and ensuring that offsets are calculated and stored
using off_t.

I *think* that I fixed all of the lseek() cases some time ago, but I
didn't touch the fseek/ftell cases (we didn't have G_fseek/G_ftell at
that time).

--
Glynn Clements <glynn@gclements.plus.com>

[Markus_GRASS]

Glynn Clements wrote:

Markus Metz wrote:

[...] An integer literal without a
trailing suffix is an "int". Using a larger type to hold the value
where necessary is a gcc extension. Other compilers may simply
truncate the value to an int, even if off_t is 64 bits.

The latest change:

  #define OFF_T_TEST 0x0102030405060708LL

will fail to compile with a compiler which doesn't have long long int.
Both the definition and use of OFF_T_TEST need to be conditionalised
with "#ifdef HAVE_LONG_LONG_INT".
      

OK. But on Linux 32 without LFS I get

port_init.c:217: warning: overflow in implicit constant conversion

at compile time because long long int is available but sizeof(off_t) = 4. Portability and its test is ok because OFF_T_TEST is not used in this case.
    
Making the conversion explicit would eliminate the warning:

  u_o = (off_t) OFF_T_TEST;
  

OK.

[...]
A 64-bit system might have a 64-bit off_t
but no "long long" type. We really need to have autoconf use define
SIZEOF_LONG etc in config.h (AC_CHECK_SIZEOF macro), so that we can
use e.g.:

  #if SIZEOF_LONG == 8
  #define LONG_LONG_TEST 0x0102030405060708L
  #elif defined(HAVE_LONG_LONG_INT)
  #define LONG_LONG_TEST 0x0102030405060708LL
  #endif
  

You can take it from gdal.

I still see:

719: s[top].sn.branch[j].child.id =
720: (int)s[top].childpos[j];

754: s[top].sn.branch[j].child.id =
755: (int)s[top].childpos[j];

Also:

1011: if (!shcb((int)s[top].sn.branch[i].child, cbarg)) {

A narrowing cast always brings up the question of "what if the value
won't fit in the destination type"?

Am I missing something, or is childpos[j] *always* an "int"?
      

On level 0, it is always an int. It's the ID of the rectangle passed to
int RTreeInsertRect(struct Rect *r, int tid, struct RTree *t)
as tid. That applies to all three cases above.

I am writing out the rectangle ID as off_t although it is int and have to read it as off_t then cast it to int again when loading the sidx file to memory for updating. Of course I could also write the ID as int and read it as int, then there would be no type casting, at the cost of one more if ... else ... .

If so,
why is it stored in the file as an off_t?
      

On higher levels (RTree internal nodes), it is the position in file of the child nodes of a node. For large sidx files, off_t is needed. The corresponding line in spindex_rw.c is

668 s[top].pos[s[top].branch_id - 1] = nextfreepos;

where I set the node positions in file as off_t.

This is related to the use of union Child: on level 0, int id is used, on higher levels, struct Node *ptr is used.
    
So it's a case of simplifying the sidx file I/O?
  

Yes, partly my laziness. Took me ages to get that particular post-order traversal right.

That's reasonable enough. But, personally, I would:

1. Make the pos and childpos fields unions (int/off_t).
  

That's now easy, similar to the union in rtree.

2. Add an explicit range check before casting the off_t read from the
file to an int.

That should only be necessary if there is reason to suspect that the sidx file is not read properly. Hmm, actually that would be a good check for exactly that.

An assert() would suffice; the main purpose is to
answer the "what if it doesn't fit in an int?" question for the
benefit of anyone trying to read that code.
  

Only works if off_t_size = 8 is both available and used, otherwise it will obviously always fit into an int.
The answer would be that the programmer wrote bad code with bugs because it should never happen that it doesn't fit into an int.

Markus M

[Glynn_Clements1]

Markus Metz wrote:

> 2. Add an explicit range check before casting the off_t read from the
> file to an int.

That should only be necessary if there is reason to suspect that the
sidx file is not read properly. Hmm, actually that would be a good check
for exactly that.

More important than validating the file is that, unless you have
thoroughly analysed the code which generates the sidx file, it won't
be obvious to someone reading the code that the off_t value read from
the file will fit into an int. An explicit check would make it clear.

Source code is meant to be read by humans as well as by compilers.

--
Glynn Clements <glynn@gclements.plus.com>

[Glynn_Clements1]

Markus Neteler wrote:

>> I got carried away (replacing all fseek/ftell occurrences I could find
>> with G_fseek/G_ftell, adjusting off_t as you showed above) and also made
>> r3.in|out.v5d LFS-safe, but did not submit. Should I?
>
> Yes; ideally, we should use G_{fseek,ftell} everywhere.

Patch attached.

Note that it now fails to compile here:

No clue how what could be wrong in the patch. Same problem in
raster/r.in.xyz

Feel free to recycle the patch.

I've committed this (with fixes) as r38703.

lib/rst/inter_float still needs to be fixed to use off_t for offsets.
Currently, it's using "int", so it won't handle files >2GiB even on
64-bit platforms.

--
Glynn Clements <glynn@gclements.plus.com>

[Markus_GRASS]

Glynn Clements wrote:

Markus Neteler wrote:

I got carried away (replacing all fseek/ftell occurrences I could find
with G_fseek/G_ftell, adjusting off_t as you showed above) and also made
r3.in|out.v5d LFS-safe, but did not submit. Should I?
        

Yes; ideally, we should use G_{fseek,ftell} everywhere.
      

Patch attached.

[...]

I've committed this (with fixes) as r38703.
  

Do the Makefiles for the modules also need fixing with

ifneq ($(USE_LARGEFILES),)
EXTRA_CFLAGS = -D_FILE_OFFSET_BITS=64
endif

or is -D_FILE_OFFSET_BITS=64 now set globally in COMPILE_FLAGS and COMPILE_FLAGS_XX or similar?

Markus M

lib/rst/inter_float still needs to be fixed to use off_t for offsets.
Currently, it's using "int", so it won't handle files >2GiB even on
64-bit platforms.

[Glynn_Clements1]

Markus Metz wrote:

>>>> I got carried away (replacing all fseek/ftell occurrences I could find
>>>> with G_fseek/G_ftell, adjusting off_t as you showed above) and also made
>>>> r3.in|out.v5d LFS-safe, but did not submit. Should I?
>>>>
>>> Yes; ideally, we should use G_{fseek,ftell} everywhere.
>>>
>> Patch attached.
>>
>>
>
>
[...]
> I've committed this (with fixes) as r38703.

Do the Makefiles for the modules also need fixing with

ifneq ($(USE_LARGEFILES),)
EXTRA_CFLAGS = -D_FILE_OFFSET_BITS=64
endif

or is -D_FILE_OFFSET_BITS=64 now set globally in COMPILE_FLAGS and
COMPILE_FLAGS_XX or similar?

Ultimately, it will be set globally, but we need to ensure that
everything can handle it before turning it on.

I originally thought that it should be set in config.h, but that would
require every file to include <grass/config.h> before including
<stdio.h>. Setting it in the compiler switches will be more robust.

--
Glynn Clements <glynn@gclements.plus.com>

[Glynn_Clements1]

Glynn Clements wrote:

I suggest:

1. Remove OFF_T_TEST.

2. Add:

  #ifdef HAVE_LONG_LONG_INT
  #define LONG_LONG_TEST 0x0102030405060708LL
  #endif

3. Change the initialisation of u_o to e.g.:

      if (nat_off_t == 8)
  #ifdef HAVE_LONG_LONG_INT
    u_o = (off_t) LONG_LONG_TEST;
  #else
    G_fatal_error("Internal error: can't construct an off_t literal");
  #endif
      else
    u_o = (off_t) LONG_TEST;

Done in r38916.

--
Glynn Clements <glynn@gclements.plus.com>