Repository navigation
Conversation
The types dglInt32_t and dglInt64_t were originally intended to be fixed-width integers, as indicated by the original commit from 2002 [1]. At the time, C99 (and <stdint.h>) was new and not yet widely supported, hence the use of long and long long. Today, long is 64 bits on platforms using the LP64 data model, including 64-bit Linux and macOS, while it remains 32 bits on Windows. This update restores the intended 32-bit width of dglInt32_t and uses an explicit 64-bit integer type for dglInt64_t. [1] OSGeo/grass-legacy@0f9fd9f#diff-311056b41101fbbecd4477b59c9dee3ddf50f64f38926d242790c78489e306ac
wenzeslaus
left a comment
There was a problem hiding this comment.
This may need a deeper analysis since long is 64bit for quite some time now on Linux. So there was no size saving until now on Linux (sizeof(dglInt32_t) = 8 on my machine), larger sizes and limits were allowed, but all of that was inconsistent with Windows. This brings the Windows limits to Linux and macOS. Do we want that?
AI-generated review
On `main`, `dglInt32_t` is `long`, which is 32-bit on Windows (LLP64) and 64-bit on Linux and macOS (LP64). With `int32_t`, the Windows limits apply everywhere:- Shortest-path distances (
nDistance, the accumulators insp-template.c) are 32-bit. Withcost_multip = 1000(projected location, no cost column), totals above about 2147 km overflow atsp-template.c:302(fromDist + nEdgeCost) andsp-template.c:194. Testing dglib standalone on x86-64 Linux (gcc 13,-O0and-O2) with this PR: a 2200 km route was reported as -2095 km. Given a choice between a 4500 km route (3 edges of 1500 km) and a 2100 km route (2 edges of 1050 km), it returned the 4500 km route with a distance of 205 km. The base commit returned 2200 km and the 2100 km route. UBSan reports signed integer overflow at both locations. dglcosts = (dglInt32_t)costs * 1000inVect_graph_add_edge(Vlib/graph.c:131, same inVect_graph_set_node_costs) overflows for costs above about 2.1 million. Independent of this PR, the cast also drops the fractional part before scaling: 1.7 becomes 1000, not 1700.iNodeBuffer/iEdgeBufferand the offset casts ingraph_v1.hcap flattened graphs at 2 GiB per buffer.- The shortest-path counts
cntinNetA_betweenness_closeness(neta/centrality.c:139) can overflow on grid-like networks with uniform costs.
An option for a follow-up is to use dglInt64_t for costs and distances while keeping 32-bit for IDs and offsets, or to stop with an error in net_build.c/graph.c when a scaled cost exceeds INT32_MAX.
dglWrite/dglRead(graph_v1.c,graph_v2.c) writesizeof(dglInt32_t)per field, and the header records endianness but not field width. Onmain, files are therefore not portable between Windows and Linux/macOS, anddgl_swapInt32Bytes(helpers.c) swaps only 4 of the 8 bytes on LP64, so reading a file with the opposite endianness is broken there. The PR fixes both. The trade-off is that files written by older Linux and macOS builds can no longer be read. No tool in this repo callsdglWrite, so a note in the PR description may be enough.
The dglib examples still use %ld with dglInt32_t variables: cr_from_a.c:103, parse.c:282-312, shortest_path.c, view.c. With sscanf, this writes 8 bytes into 4-byte variables on 64-bit Linux. view.c:188 uses %lld for dglInt64_t, which does not match int64_t on Linux (long). Using PRId32 and PRId64 from <inttypes.h> would make all these call sites portable. Using the same macros in the new G_debug calls in Vlib/graph.c and Vlib/net_analyze.c would keep the 32-bit and 64-bit formats consistent.
The types
dglInt32_tanddglInt64_twere originally intended to be fixed-width integers, as indicated by the original commit from 2002 [1]. At the time, C99 (and <stdint.h>) was new and not yet widely supported, hence the use oflongandlong long. Today, long is 64 bits on platforms using the LP64 data model, including 64-bit Linux and macOS, while it remains 32 bits on Windows.This update restores the intended 32-bit width of dglInt32_t and uses an explicit 64-bit integer type for dglInt64_t.
[1] OSGeo/grass-legacy@0f9fd9f#diff-311056b41101fbbecd4477b59c9dee3ddf50f64f38926d242790c78489e306ac