From 734bba7c783cec4caba57076147e0fe76c98b0e0 Mon Sep 17 00:00:00 2001 From: Erica Fischer Date: Thu, 6 Aug 2026 17:06:07 -0700 Subject: [PATCH] Fix three latent defects exposed by compiler warnings, and clear the rest (#406) * Fix variable-length-array and uninitialized-union compiler warnings Clang warns about every variable-length array in C++ (-Wvla-cxx-extension, on by default), since VLAs are a compiler extension rather than standard C++. Replace all 57 of them with std::vector, or with std::string for the mkstemp() template buffers built from tmpdir. Add -Wvla to WARNING_FLAGS so new ones don't creep back in. Separately, mvt_value's numeric_value union is 16 bytes wide (the size of string_value), but both constructors only wrote the 8 bytes of the member they were setting, leaving the rest indeterminate. The implicit copy constructor copies the union as a whole, so copying any non-string value read uninitialized bytes, which GCC reports as mvt.hpp:83:8: warning: 'v.mvt_value::numeric_value. ... .len' may be used uninitialized [-Wmaybe-uninitialized] Give string_value, the widest member, a default member initializer so the union's full width is initialized however it is later used. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D8gsGMjK78TQiCGKTZ2PyR * Fix remaining float-conversion and format-truncation warnings Clang's -Wimplicit-const-int-float-conversion flagged two comparisons against LLONG_MAX, which is not representable as a double and rounds up to 2^63. In serial.cpp this was a real latent overflow, not just noise: the guard `extent <= LLONG_MAX` was really `extent <= 2^63`, so an extent of exactly 2^63 passed it and then hit `(long long) extent`, which is undefined for that value and yields LLONG_MIN in practice -- the opposite of the clamp the else branch intends. Make the bound exclusive so the conversion is always in range. Requires a polygon area at the very top of the double range to reach, but the clamp now behaves as written. In mbtiles.cpp the value is only a stand-in for infinity on its way into JSON, so cast explicitly; the emitted number is unchanged. Separately, g++ at -O0 warned that `char abbrev[20]` can be truncated by "%lld", which is correct: the most negative long long needs 21 bytes with the NUL. That branch is only reached when point_count < 1000, so it cannot happen today, but size the buffer to fit rather than rely on that, and replace the garbled comment about how the size was derived. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D8gsGMjK78TQiCGKTZ2PyR * Clamp the low end of extent before converting to long long too The upper bound was fixed in the previous commit; the same overflow exists on the negative side. get_area() returns a signed shoelace area, so inner rings contribute negatively, and a polygon whose holes outweigh its rings drives extent below zero. Far enough below and `(long long) extent` is undefined again. The bounds are asymmetric, so this is not simply the mirror of the upper one: LLONG_MIN is exactly -2^63 and converts exactly, so unlike LLONG_MAX it can be an inclusive bound. Verified with -fsanitize=float-cast-overflow that the previous form traps on 2^63 and on doubles just below -2^63, and that this one is clean across both boundaries, the infinities, and NaN (which falls to LLONG_MAX, as it did before). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D8gsGMjK78TQiCGKTZ2PyR * Add CHANGELOG entries for 2.81.0 and bump the version CHANGELOG.md was last updated for 2.80.0 (#361), and version.hpp has not moved since. Twelve PRs have landed in the meantime with no entry: #365, #368, #375, #382, #384, #385, #391, #395, #397, #399, #400, and #401. Document all of them, plus this PR, under a single 2.81.0 heading. They are not given separate version numbers because none of them was ever released under one -- version.hpp read v2.80.0 throughout -- so assigning a version per PR would invent release history. 2.81.0 is the version that will actually carry them. Where an unreleased PR was corrected by a later one (#384 by #385, #397 by #399), the pair is described as the single behavior that ships, since the intermediate behavior was never in a release. Minor rather than patch bump: the batch adds command-line options. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D8gsGMjK78TQiCGKTZ2PyR * Review feedback: enforce the union-width assumption, describe both clamp ends The comment on mvt_value's union claimed string_value is the widest member. That is true on LP64 (16 bytes against 8) but not on ILP32, where size_t is 4 and it ties with double and long long. The default member initializer still covers the full union either way, so the fix held, but the justification did not travel. Replace the claim with a static_assert that checks it on whatever target is being built, so a platform where it stops holding is a compile error rather than silently indeterminate bytes. Verified the assert is not vacuous by widening the union in a scratch copy and watching it fail. The changelog described only the upper end of the extent clamp. Describe both: the old guard admitted everything below LLONG_MIN too. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D8gsGMjK78TQiCGKTZ2PyR * Add 2.81.0 changelog entries for the four PRs merged from main #404, #408, #409, and #410 landed while this branch was open. None of them bumped version.hpp, so they belong under the same 2.81.0 heading as the rest of the unreleased work rather than getting versions of their own. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D8gsGMjK78TQiCGKTZ2PyR --------- Co-authored-by: Claude --- CHANGELOG.md | 68 ++++++++++++++ Makefile | 2 +- main.cpp | 253 ++++++++++++++++++++++++-------------------------- mbtiles.cpp | 5 +- mvt.hpp | 11 ++- serial.cpp | 12 ++- tile-join.cpp | 2 +- tile.cpp | 46 +++++---- version.hpp | 2 +- 9 files changed, 236 insertions(+), 165 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 108e7303..b6f5ec49 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,71 @@ +# 2.81.0 + +* Add `--drop-by-attribute-as-needed=`*attribute* to drop the features with + the lowest values of a numeric attribute from oversized tiles, and + `--drop-by-attribute-order=desc` to drop the highest values instead. + Features exactly at the threshold are kept rather than dropped. (#384, #385) +* Add `--exclude-all-tile-geometries` to tile-join, to produce tiles that + carry only attributes. (#382) +* Generate each tool's usage message from the same option table that + `getopt_long()` reads, so the hand-written lists in tile-join, + tippecanoe-overzoom, tippecanoe-json-tool, tippecanoe-decode, and + tippecanoe-enumerate can no longer fall behind the options actually + accepted. Options previously reachable only by their short names are now + listed. tippecanoe-overzoom reports a missing `-o` instead of passing a + null pointer to `fopen()`, and tippecanoe prints its usage when run with + no arguments. (#409) +* Fix the radix sort used by `--prefer-radix-sort`. A bucket written out + directly rather than through the merge was written one byte longer than + its length prefix claimed, desynchronizing everything read from the + geometry after it. Subdividing could also recurse forever once it ran out + of files to split with, shifting by the full width of the index and + writing past the end of the arrays of buckets. Radix-sorted output is now + checked against the in-memory sort rather than against a stored copy. (#404) +* Read FlatGeobuf integer and float properties as numbers. They were tagged + with types that the rest of tippecanoe does not treat as numeric, so they + were reported in tilestats as "mixed", with quoted values and no min or + max, and warned when used as a feature ID. ULong properties are now also + read as unsigned rather than signed. (#395) +* Respect the `-t` temporary directory option in sorting operations, which + previously always used the system temporary directory. (#368) +* Keep `--generate-variable-depth-tile-pyramid` from silently dropping + features whose explicit per-feature `minzoom` is deeper than the zoom at + which their region becomes a leaf. Such a feature was excluded from the + leaf tile while its children were never generated, so it appeared at no + zoom at all. (#397, #399) +* Drop a polygon hole that no remaining ring can parent, instead of failing + the whole run. Degenerate input could abort tiling over a single + unrepresentable sliver. (#401) +* Clamp the feature extent to the `long long` range before converting it, + at both ends. The previous `extent <= LLONG_MAX` guard was doubly wrong: + `LLONG_MAX` is not representable as a double and rounds up, so an extent + at the very top of the range overflowed the conversion and came out as the + most negative value rather than the largest, and the guard admitted + everything below `LLONG_MIN` as well, which overflowed the other way. Areas + are signed, so holes that outweigh their rings can reach the low end. (#406) +* Initialize the full width of the `mvt_value` numeric union, which left the + bytes of the wider unused member indeterminate even though the implicit + copy constructor copies the union as a whole. (#406) +* Replace all variable-length arrays with `std::vector` and `std::string`, + and build with `-Wvla`. VLAs are a compiler extension rather than standard + C++, and clang warns about every one of them by default. (#406) +* Remove the unused Dockerfiles, Travis configuration, and lambda + directory. (#365) +* Correct README statements that disagreed with the code. Among them, `-aD` + and `-aS` were documented the wrong way round, + `--limit-base-zoom-to-maximum-zoom` was given as `-Pb` rather than `-pb`, + and the dot-dropping description had both the fraction and the zoom + direction backwards: tippecanoe keeps 1/2.5 of the dots at zooms below the + base zoom, rather than dropping that share above it. (#410) +* Generate `man/tippecanoe.1` with go-md2man rather than md2man-roff, which + is packaged only as a Ruby gem and so had let the page drift out of date. + The page now has a proper header and a NAME section, so `man -k` and + `whatis` can find it, and no longer silently drops or mangles text the old + converter mishandled. CI checks it against README.md. (#408) +* Documentation fixes: correct three misspellings in the README and man + page, repair the dead All Streets link, and tag more README code blocks + with their language. (#375, #391, #400) + # 2.80.0 * Remove undocumented command-line options diff --git a/Makefile b/Makefile index df38c0d6..84ad090c 100644 --- a/Makefile +++ b/Makefile @@ -11,7 +11,7 @@ CXX := $(CXX) CFLAGS := $(CFLAGS) -fPIE -DBUILD_INFO=$(BUILD_INFO) CXXFLAGS := $(CXXFLAGS) -std=c++17 -fPIE -DBUILD_INFO=$(BUILD_INFO) LDFLAGS := $(LDFLAGS) -WARNING_FLAGS := -Wall -Wshadow -Wsign-compare -Wextra -Wunreachable-code -Wuninitialized -Wshadow +WARNING_FLAGS := -Wall -Wshadow -Wsign-compare -Wextra -Wunreachable-code -Wuninitialized -Wshadow -Wvla RELEASE_FLAGS := -O3 -DNDEBUG DEBUG_FLAGS := -O0 -DDEBUG -fno-inline-functions -fno-omit-frame-pointer diff --git a/main.cpp b/main.cpp index 5842f988..e084e6e1 100644 --- a/main.cpp +++ b/main.cpp @@ -215,7 +215,7 @@ void init_cpus() { // MacOS can run out of system file descriptors // even if we stay under the rlimit, so try to // find out the real limit. - long long fds[MAX_FILES]; + std::vector fds(MAX_FILES); long long i; for (i = 0; i < MAX_FILES; i++) { fds[i] = open(get_null_device(), O_RDONLY | O_CLOEXEC); @@ -449,7 +449,7 @@ void *run_sort(void *v) { } void do_read_parallel(char *map, long long len, long long initial_offset, const char *reading, std::vector *readers, std::atomic *progress_seq, std::set *exclude, std::set *include, int exclude_all, int basezoom, int source, std::vector > *layermaps, int *initialized, unsigned *initial_x, unsigned *initial_y, int maxzoom, std::string layername, bool uses_gamma, std::unordered_map const *attribute_types, int separator, double *dist_sum, size_t *dist_count, double *area_sum, bool want_dist, bool filters) { - long long segs[CPUS + 1]; + std::vector segs(CPUS + 1); segs[0] = 0; segs[CPUS] = len; @@ -461,11 +461,11 @@ void do_read_parallel(char *map, long long len, long long initial_offset, const } } - double dist_sums[CPUS]; - size_t dist_counts[CPUS]; - double area_sums[CPUS]; + std::vector dist_sums(CPUS); + std::vector dist_counts(CPUS); + std::vector area_sums(CPUS); - std::atomic layer_seq[CPUS]; + std::vector > layer_seq(CPUS); for (size_t i = 0; i < CPUS; i++) { // To preserve feature ordering, unique id for each segment // begins with that segment's offset into the input @@ -479,7 +479,7 @@ void do_read_parallel(char *map, long long len, long long initial_offset, const std::vector sst; sst.resize(CPUS); - pthread_t pthreads[CPUS]; + std::vector pthreads(CPUS); std::vector > file_subkeys; for (size_t i = 0; i < CPUS; i++) { @@ -759,47 +759,45 @@ void radix1(int *geomfds_in, int *indexfds_in, int inputs, int prefix, int split } splits = 1 << splitbits; - FILE *geomfiles[splits]; - FILE *indexfiles[splits]; - int geomfds[splits]; - int indexfds[splits]; - std::atomic sub_geompos[splits]; + std::vector geomfiles(splits); + std::vector indexfiles(splits); + std::vector geomfds(splits); + std::vector indexfds(splits); + std::vector > sub_geompos(splits); int i; for (i = 0; i < splits; i++) { sub_geompos[i] = 0; - char geomname[strlen(tmpdir) + strlen("/geom.XXXXXXXX") + 1]; - snprintf(geomname, sizeof(geomname), "%s%s", tmpdir, "/geom.XXXXXXXX"); - char indexname[strlen(tmpdir) + strlen("/index.XXXXXXXX") + 1]; - snprintf(indexname, sizeof(indexname), "%s%s", tmpdir, "/index.XXXXXXXX"); + std::string geomname = std::string(tmpdir) + "/geom.XXXXXXXX"; + std::string indexname = std::string(tmpdir) + "/index.XXXXXXXX"; - geomfds[i] = mkstemp_cloexec(geomname); + geomfds[i] = mkstemp_cloexec(&geomname[0]); if (geomfds[i] < 0) { - perror(geomname); + perror(geomname.c_str()); exit(EXIT_OPEN); } - indexfds[i] = mkstemp_cloexec(indexname); + indexfds[i] = mkstemp_cloexec(&indexname[0]); if (indexfds[i] < 0) { - perror(indexname); + perror(indexname.c_str()); exit(EXIT_OPEN); } - geomfiles[i] = fopen_oflag(geomname, "wb", O_WRONLY | O_CLOEXEC); + geomfiles[i] = fopen_oflag(geomname.c_str(), "wb", O_WRONLY | O_CLOEXEC); if (geomfiles[i] == NULL) { - perror(geomname); + perror(geomname.c_str()); exit(EXIT_OPEN); } - indexfiles[i] = fopen_oflag(indexname, "wb", O_WRONLY | O_CLOEXEC); + indexfiles[i] = fopen_oflag(indexname.c_str(), "wb", O_WRONLY | O_CLOEXEC); if (indexfiles[i] == NULL) { - perror(indexname); + perror(indexname.c_str()); exit(EXIT_OPEN); } *availfiles -= 4; - unlink(geomname); - unlink(indexname); + unlink(geomname.c_str()); + unlink(indexname.c_str()); } for (i = 0; i < inputs; i++) { @@ -928,13 +926,13 @@ void radix1(int *geomfds_in, int *indexfds_in, int inputs, int prefix, int split } size_t nmerges = (indexpos + unit - 1) / unit; - struct mergelist merges[nmerges]; + std::vector merges(nmerges); for (size_t a = 0; a < nmerges; a++) { merges[a].start = merges[a].end = 0; } - pthread_t pthreads[CPUS]; + std::vector pthreads(CPUS); std::vector args; for (size_t a = 0; a < CPUS; a++) { @@ -942,7 +940,7 @@ void radix1(int *geomfds_in, int *indexfds_in, int inputs, int prefix, int split a, CPUS, indexpos, - merges, + merges.data(), indexfds[i], nmerges, unit, @@ -980,7 +978,7 @@ void radix1(int *geomfds_in, int *indexfds_in, int inputs, int prefix, int split madvise(geommap, geomst.st_size, MADV_RANDOM); madvise(geommap, geomst.st_size, MADV_WILLNEED); - merge(merges, nmerges, (unsigned char *) indexmap, indexfile, bytes, geommap, geomfile, geompos_out, progress, progress_max, progress_reported, maxzoom, gamma, ds); + merge(merges.data(), nmerges, (unsigned char *) indexmap, indexfile, bytes, geommap, geomfile, geompos_out, progress, progress_max, progress_reported, maxzoom, gamma, ds); madvise(indexmap, indexst.st_size, MADV_DONTNEED); if (munmap(indexmap, indexst.st_size) < 0) { @@ -1119,8 +1117,8 @@ void radix(std::vector &readers, int nreaders, FILE *geomfile, FI mem /= 2; long long geom_total = 0; - int geomfds[nreaders]; - int indexfds[nreaders]; + std::vector geomfds(nreaders); + std::vector indexfds(nreaders); for (int i = 0; i < nreaders; i++) { geomfds[i] = readers[i].geomfd; indexfds[i] = readers[i].indexfd; @@ -1133,12 +1131,12 @@ void radix(std::vector &readers, int nreaders, FILE *geomfile, FI geom_total += geomst.st_size; } - struct drop_state ds[maxzoom + 1]; - prep_drop_states(ds, maxzoom, basezoom, droprate); + std::vector ds(maxzoom + 1); + prep_drop_states(ds.data(), maxzoom, basezoom, droprate); long long progress = 0, progress_max = geom_total, progress_reported = -1; long long availfiles_before = availfiles; - radix1(geomfds, indexfds, nreaders, 0, splits, mem, tmpdir, &availfiles, geomfile, indexfile, geompos, &progress, &progress_max, &progress_reported, maxzoom, basezoom, droprate, gamma, ds); + radix1(geomfds.data(), indexfds.data(), nreaders, 0, splits, mem, tmpdir, &availfiles, geomfile, indexfile, geompos, &progress, &progress_max, &progress_reported, maxzoom, basezoom, droprate, gamma, ds.data()); if (availfiles - 2 * nreaders != availfiles_before) { fprintf(stderr, "Internal error: miscounted available file descriptors: %lld vs %lld\n", availfiles - 2 * nreaders, availfiles); @@ -1247,79 +1245,72 @@ std::pair read_input(std::vector &sources, char *fname, i for (size_t i = 0; i < CPUS; i++) { struct reader *r = &readers[i]; - char poolname[strlen(tmpdir) + strlen("/pool.XXXXXXXX") + 1]; - char treename[strlen(tmpdir) + strlen("/tree.XXXXXXXX") + 1]; - char geomname[strlen(tmpdir) + strlen("/geom.XXXXXXXX") + 1]; - char indexname[strlen(tmpdir) + strlen("/index.XXXXXXXX") + 1]; - char vertexname[strlen(tmpdir) + strlen("/vertex.XXXXXXXX") + 1]; - char nodename[strlen(tmpdir) + strlen("/node.XXXXXXXX") + 1]; + std::string poolname = std::string(tmpdir) + "/pool.XXXXXXXX"; + std::string treename = std::string(tmpdir) + "/tree.XXXXXXXX"; + std::string geomname = std::string(tmpdir) + "/geom.XXXXXXXX"; + std::string indexname = std::string(tmpdir) + "/index.XXXXXXXX"; + std::string vertexname = std::string(tmpdir) + "/vertex.XXXXXXXX"; + std::string nodename = std::string(tmpdir) + "/node.XXXXXXXX"; - snprintf(poolname, sizeof(poolname), "%s%s", tmpdir, "/pool.XXXXXXXX"); - snprintf(treename, sizeof(treename), "%s%s", tmpdir, "/tree.XXXXXXXX"); - snprintf(geomname, sizeof(geomname), "%s%s", tmpdir, "/geom.XXXXXXXX"); - snprintf(indexname, sizeof(indexname), "%s%s", tmpdir, "/index.XXXXXXXX"); - snprintf(vertexname, sizeof(vertexname), "%s%s", tmpdir, "/vertex.XXXXXXXX"); - snprintf(nodename, sizeof(nodename), "%s%s", tmpdir, "/node.XXXXXXXX"); - - r->poolfd = mkstemp_cloexec(poolname); + r->poolfd = mkstemp_cloexec(&poolname[0]); if (r->poolfd < 0) { - perror(poolname); + perror(poolname.c_str()); exit(EXIT_OPEN); } - r->treefd = mkstemp_cloexec(treename); + r->treefd = mkstemp_cloexec(&treename[0]); if (r->treefd < 0) { - perror(treename); + perror(treename.c_str()); exit(EXIT_OPEN); } - r->geomfd = mkstemp_cloexec(geomname); + r->geomfd = mkstemp_cloexec(&geomname[0]); if (r->geomfd < 0) { - perror(geomname); + perror(geomname.c_str()); exit(EXIT_OPEN); } - r->indexfd = mkstemp_cloexec(indexname); + r->indexfd = mkstemp_cloexec(&indexname[0]); if (r->indexfd < 0) { - perror(indexname); + perror(indexname.c_str()); exit(EXIT_OPEN); } - r->vertexfd = mkstemp_cloexec(vertexname); + r->vertexfd = mkstemp_cloexec(&vertexname[0]); if (r->vertexfd < 0) { - perror(vertexname); + perror(vertexname.c_str()); exit(EXIT_OPEN); } - r->nodefd = mkstemp_cloexec(nodename); + r->nodefd = mkstemp_cloexec(&nodename[0]); if (r->nodefd < 0) { - perror(nodename); + perror(nodename.c_str()); exit(EXIT_OPEN); } r->poolfile = memfile_open(r->poolfd); if (r->poolfile == NULL) { - perror(poolname); + perror(poolname.c_str()); exit(EXIT_OPEN); } r->treefile = memfile_open(r->treefd); if (r->treefile == NULL) { - perror(treename); + perror(treename.c_str()); exit(EXIT_OPEN); } - r->geomfile = fopen_oflag(geomname, "wb", O_WRONLY | O_CLOEXEC); + r->geomfile = fopen_oflag(geomname.c_str(), "wb", O_WRONLY | O_CLOEXEC); if (r->geomfile == NULL) { - perror(geomname); + perror(geomname.c_str()); exit(EXIT_OPEN); } - r->indexfile = fopen_oflag(indexname, "wb", O_WRONLY | O_CLOEXEC); + r->indexfile = fopen_oflag(indexname.c_str(), "wb", O_WRONLY | O_CLOEXEC); if (r->indexfile == NULL) { - perror(indexname); + perror(indexname.c_str()); exit(EXIT_OPEN); } - r->vertexfile = fopen_oflag(vertexname, "w+b", O_RDWR | O_CLOEXEC); + r->vertexfile = fopen_oflag(vertexname.c_str(), "w+b", O_RDWR | O_CLOEXEC); if (r->vertexfile == NULL) { - perror(("open vertexfile " + std::string(vertexname)).c_str()); + perror(("open vertexfile " + vertexname).c_str()); exit(EXIT_OPEN); } - r->nodefile = fopen_oflag(nodename, "w+b", O_RDWR | O_CLOEXEC); + r->nodefile = fopen_oflag(nodename.c_str(), "w+b", O_RDWR | O_CLOEXEC); if (r->nodefile == NULL) { - perror(nodename); + perror(nodename.c_str()); exit(EXIT_OPEN); } r->geompos = 0; @@ -1327,12 +1318,12 @@ std::pair read_input(std::vector &sources, char *fname, i r->vertexpos = 0; r->nodepos = 0; - unlink(poolname); - unlink(treename); - unlink(geomname); - unlink(indexname); - unlink(vertexname); - unlink(nodename); + unlink(poolname.c_str()); + unlink(treename.c_str()); + unlink(geomname.c_str()); + unlink(indexname.c_str()); + unlink(vertexname.c_str()); + unlink(nodename.c_str()); // To distinguish a null value { @@ -1357,8 +1348,8 @@ std::pair read_input(std::vector &sources, char *fname, i std::atomic progress_seq(0); // 2 * CPUS: One per reader thread, one per tiling thread - int initialized[2 * CPUS]; - unsigned initial_x[2 * CPUS], initial_y[2 * CPUS]; + std::vector initialized(2 * CPUS); + std::vector initial_x(2 * CPUS), initial_y(2 * CPUS); for (size_t i = 0; i < 2 * CPUS; i++) { initialized[i] = initial_x[i] = initial_y[i] = 0; } @@ -1489,10 +1480,10 @@ std::pair read_input(std::vector &sources, char *fname, i exit(EXIT_MEMORY); } - std::atomic layer_seq[CPUS]; - double dist_sums[CPUS]; - size_t dist_counts[CPUS]; - double area_sums[CPUS]; + std::vector > layer_seq(CPUS); + std::vector dist_sums(CPUS); + std::vector dist_counts(CPUS); + std::vector area_sums(CPUS); std::vector sst; sst.resize(CPUS); @@ -1562,10 +1553,10 @@ std::pair read_input(std::vector &sources, char *fname, i exit(EXIT_MEMORY); } - std::atomic layer_seq[CPUS]; - double dist_sums[CPUS]; - size_t dist_counts[CPUS]; - double area_sums[CPUS]; + std::vector > layer_seq(CPUS); + std::vector dist_sums(CPUS); + std::vector dist_counts(CPUS); + std::vector area_sums(CPUS); std::vector sst; sst.resize(CPUS); @@ -1622,10 +1613,10 @@ std::pair read_input(std::vector &sources, char *fname, i } if (sources[source].format == "csv" || (sources[source].file.size() > 4 && sources[source].file.substr(sources[source].file.size() - 4) == std::string(".csv"))) { - std::atomic layer_seq[CPUS]; - double dist_sums[CPUS]; - size_t dist_counts[CPUS]; - double area_sums[CPUS]; + std::vector > layer_seq(CPUS); + std::vector dist_sums(CPUS); + std::vector dist_counts(CPUS); + std::vector area_sums(CPUS); std::vector sst; sst.resize(CPUS); @@ -1710,7 +1701,7 @@ std::pair read_input(std::vector &sources, char *fname, i } if (map != NULL && map != MAP_FAILED && read_parallel_this) { - do_read_parallel(map, st.st_size - off, overall_offset, reading.c_str(), &readers, &progress_seq, exclude, include, exclude_all, basezoom, layer, &layermaps, initialized, initial_x, initial_y, maxzoom, sources[layer].layer, uses_gamma, attribute_types, read_parallel_this, &dist_sum, &dist_count, &area_sum, guess_maxzoom, prefilter != NULL || postfilter != NULL); + do_read_parallel(map, st.st_size - off, overall_offset, reading.c_str(), &readers, &progress_seq, exclude, include, exclude_all, basezoom, layer, &layermaps, initialized.data(), initial_x.data(), initial_y.data(), maxzoom, sources[layer].layer, uses_gamma, attribute_types, read_parallel_this, &dist_sum, &dist_count, &area_sum, guess_maxzoom, prefilter != NULL || postfilter != NULL); overall_offset += st.st_size - off; checkdisk(&readers); @@ -1742,19 +1733,18 @@ std::pair read_input(std::vector &sources, char *fname, i if (read_parallel_this) { // Serial reading of chunks that are then parsed in parallel - char readname[strlen(tmpdir) + strlen("/read.XXXXXXXX") + 1]; - snprintf(readname, sizeof(readname), "%s%s", tmpdir, "/read.XXXXXXXX"); - int readfd = mkstemp_cloexec(readname); + std::string readname = std::string(tmpdir) + "/read.XXXXXXXX"; + int readfd = mkstemp_cloexec(&readname[0]); if (readfd < 0) { - perror(readname); + perror(readname.c_str()); exit(EXIT_OPEN); } FILE *readfp = fdopen(readfd, "w"); if (readfp == NULL) { - perror(readname); + perror(readname.c_str()); exit(EXIT_OPEN); } - unlink(readname); + unlink(readname.c_str()); std::atomic is_parsing(0); long long ahead = 0; @@ -1789,25 +1779,25 @@ std::pair read_input(std::vector &sources, char *fname, i } fflush(readfp); - start_parsing(readfd, streamfpopen(readfp), initial_offset, ahead, &is_parsing, ¶llel_parser, parser_created, reading.c_str(), &readers, &progress_seq, exclude, include, exclude_all, basezoom, layer, layermaps, initialized, initial_x, initial_y, maxzoom, sources[layer].layer, gamma != 0, attribute_types, read_parallel_this, &dist_sum, &dist_count, &area_sum, guess_maxzoom, prefilter != NULL || postfilter != NULL); + start_parsing(readfd, streamfpopen(readfp), initial_offset, ahead, &is_parsing, ¶llel_parser, parser_created, reading.c_str(), &readers, &progress_seq, exclude, include, exclude_all, basezoom, layer, layermaps, initialized.data(), initial_x.data(), initial_y.data(), maxzoom, sources[layer].layer, gamma != 0, attribute_types, read_parallel_this, &dist_sum, &dist_count, &area_sum, guess_maxzoom, prefilter != NULL || postfilter != NULL); initial_offset += ahead; overall_offset += ahead; checkdisk(&readers); ahead = 0; - snprintf(readname, sizeof(readname), "%s%s", tmpdir, "/read.XXXXXXXX"); - readfd = mkstemp_cloexec(readname); + readname = std::string(tmpdir) + "/read.XXXXXXXX"; + readfd = mkstemp_cloexec(&readname[0]); if (readfd < 0) { - perror(readname); + perror(readname.c_str()); exit(EXIT_OPEN); } readfp = fdopen(readfd, "w"); if (readfp == NULL) { - perror(readname); + perror(readname.c_str()); exit(EXIT_OPEN); } - unlink(readname); + unlink(readname.c_str()); } } } @@ -1826,7 +1816,7 @@ std::pair read_input(std::vector &sources, char *fname, i fflush(readfp); if (ahead > 0) { - start_parsing(readfd, streamfpopen(readfp), initial_offset, ahead, &is_parsing, ¶llel_parser, parser_created, reading.c_str(), &readers, &progress_seq, exclude, include, exclude_all, basezoom, layer, layermaps, initialized, initial_x, initial_y, maxzoom, sources[layer].layer, gamma != 0, attribute_types, read_parallel_this, &dist_sum, &dist_count, &area_sum, guess_maxzoom, prefilter != NULL || postfilter != NULL); + start_parsing(readfd, streamfpopen(readfp), initial_offset, ahead, &is_parsing, ¶llel_parser, parser_created, reading.c_str(), &readers, &progress_seq, exclude, include, exclude_all, basezoom, layer, layermaps, initialized.data(), initial_x.data(), initial_y.data(), maxzoom, sources[layer].layer, gamma != 0, attribute_types, read_parallel_this, &dist_sum, &dist_count, &area_sum, guess_maxzoom, prefilter != NULL || postfilter != NULL); if (parser_created) { if (pthread_join(parallel_parser, NULL) != 0) { @@ -1935,27 +1925,26 @@ std::pair read_input(std::vector &sources, char *fname, i // segment+offset to find the data. // 2 * CPUS: One per input thread, one per tiling thread - long long pool_off[2 * CPUS]; + std::vector pool_off(2 * CPUS); for (size_t i = 0; i < 2 * CPUS; i++) { pool_off[i] = 0; } - char poolname[strlen(tmpdir) + strlen("/pool.XXXXXXXX") + 1]; - snprintf(poolname, sizeof(poolname), "%s%s", tmpdir, "/pool.XXXXXXXX"); + std::string poolname = std::string(tmpdir) + "/pool.XXXXXXXX"; - int poolfd = mkstemp_cloexec(poolname); + int poolfd = mkstemp_cloexec(&poolname[0]); if (poolfd < 0) { - perror(poolname); + perror(poolname.c_str()); exit(EXIT_OPEN); } - FILE *poolfile = fopen_oflag(poolname, "wb", O_WRONLY | O_CLOEXEC); + FILE *poolfile = fopen_oflag(poolname.c_str(), "wb", O_WRONLY | O_CLOEXEC); if (poolfile == NULL) { - perror(poolname); + perror(poolname.c_str()); exit(EXIT_OPEN); } - unlink(poolname); + unlink(poolname.c_str()); std::atomic poolpos(0); for (size_t i = 0; i < CPUS; i++) { @@ -2183,36 +2172,34 @@ std::pair read_input(std::vector &sources, char *fname, i fprintf(stderr, "Merging index \r"); } - char indexname[strlen(tmpdir) + strlen("/index.XXXXXXXX") + 1]; - snprintf(indexname, sizeof(indexname), "%s%s", tmpdir, "/index.XXXXXXXX"); + std::string indexname = std::string(tmpdir) + "/index.XXXXXXXX"; - int indexfd = mkstemp_cloexec(indexname); + int indexfd = mkstemp_cloexec(&indexname[0]); if (indexfd < 0) { - perror(indexname); + perror(indexname.c_str()); exit(EXIT_OPEN); } - FILE *indexfile = fopen_oflag(indexname, "wb", O_WRONLY | O_CLOEXEC); + FILE *indexfile = fopen_oflag(indexname.c_str(), "wb", O_WRONLY | O_CLOEXEC); if (indexfile == NULL) { - perror(indexname); + perror(indexname.c_str()); exit(EXIT_OPEN); } - unlink(indexname); + unlink(indexname.c_str()); - char geomname[strlen(tmpdir) + strlen("/geom.XXXXXXXX") + 1]; - snprintf(geomname, sizeof(geomname), "%s%s", tmpdir, "/geom.XXXXXXXX"); + std::string geomname = std::string(tmpdir) + "/geom.XXXXXXXX"; - int geomfd = mkstemp_cloexec(geomname); + int geomfd = mkstemp_cloexec(&geomname[0]); if (geomfd < 0) { - perror(geomname); + perror(geomname.c_str()); exit(EXIT_CLOSE); } - FILE *geomfile = fopen_oflag(geomname, "wb", O_WRONLY | O_CLOEXEC); + FILE *geomfile = fopen_oflag(geomname.c_str(), "wb", O_WRONLY | O_CLOEXEC); if (geomfile == NULL) { - perror(geomname); + perror(geomname.c_str()); exit(EXIT_OPEN); } - unlink(geomname); + unlink(geomname.c_str()); unsigned iz = 0, ix = 0, iy = 0; choose_first_zoom(file_bbox, file_bbox1, file_bbox2, readers, &iz, &ix, &iy, minzoom, buffer); @@ -2699,8 +2686,8 @@ std::pair read_input(std::vector &sources, char *fname, i madvise(geom, indexpos, MADV_SEQUENTIAL); madvise(geom, indexpos, MADV_WILLNEED); - struct drop_state ds[maxzoom + 1]; - prep_drop_states(ds, maxzoom, basezoom, droprate); + std::vector ds(maxzoom + 1); + prep_drop_states(ds.data(), maxzoom, basezoom, droprate); if (drop_denser > 0) { std::vector ddv; @@ -2718,7 +2705,7 @@ std::pair read_input(std::vector &sources, char *fname, i previndex = map[ip].ix; } else { - int feature_minzoom = calc_feature_minzoom(&map[ip], ds, maxzoom, gamma); + int feature_minzoom = calc_feature_minzoom(&map[ip], ds.data(), maxzoom, gamma); geom[map[ip].end - 1] = feature_minzoom; } } @@ -2743,7 +2730,7 @@ std::pair read_input(std::vector &sources, char *fname, i if (ip > 0 && map[ip].start != map[ip - 1].end) { fprintf(stderr, "Mismatched index at %lld: %lld vs %lld\n", ip, map[ip].start, map[ip].end); } - int feature_minzoom = calc_feature_minzoom(&map[ip], ds, maxzoom, gamma); + int feature_minzoom = calc_feature_minzoom(&map[ip], ds.data(), maxzoom, gamma); geom[map[ip].end - 1] = feature_minzoom; } } @@ -2766,8 +2753,8 @@ std::pair read_input(std::vector &sources, char *fname, i exit(EXIT_STAT); } - int fd[TEMP_FILES]; - off_t size[TEMP_FILES]; + std::vector fd(TEMP_FILES); + std::vector size(TEMP_FILES); fd[0] = geomfd; size[0] = geomst.st_size; @@ -2780,7 +2767,7 @@ std::pair read_input(std::vector &sources, char *fname, i std::atomic midx(0); std::atomic midy(0); std::vector strategies; - int written = traverse_zooms(fd, size, stringpool, &midx, &midy, maxzoom, minzoom, outdb, outdir, buffer, fname, tmpdir, gamma, full_detail, low_detail, min_detail, pool_off, initial_x, initial_y, simplification, maxzoom_simplification, layermaps, prefilter, postfilter, attribute_accum, filter, strategies, iz, shared_nodes_map, nodepos, shared_nodes_bloom, basezoom, droprate, unidecode_data, &drop_by_attribute_as_needed_attribute, drop_by_attribute_descending); + int written = traverse_zooms(fd.data(), size.data(), stringpool, &midx, &midy, maxzoom, minzoom, outdb, outdir, buffer, fname, tmpdir, gamma, full_detail, low_detail, min_detail, pool_off.data(), initial_x.data(), initial_y.data(), simplification, maxzoom_simplification, layermaps, prefilter, postfilter, attribute_accum, filter, strategies, iz, shared_nodes_map, nodepos, shared_nodes_bloom, basezoom, droprate, unidecode_data, &drop_by_attribute_as_needed_attribute, drop_by_attribute_descending); if (maxzoom != written) { if (written > minzoom) { diff --git a/mbtiles.cpp b/mbtiles.cpp index c7a615b0..4a3ea9a9 100644 --- a/mbtiles.cpp +++ b/mbtiles.cpp @@ -701,7 +701,10 @@ metadata make_metadata(const char *fname, int minzoom, int maxzoom, double minla m.strategies_json = stringify_strategies(strategies); if (std::isinf(droprate)) { - droprate = LLONG_MAX; + // JSON has no representation for infinity, so substitute a huge + // finite value. The cast is explicit because LLONG_MAX itself is + // not representable as a double and rounds up to 2^63. + droprate = (double) LLONG_MAX; } if (basezoom != maxzoom || droprate != 2.5 || retain_points_multiplier != 1) { m.decisions_json = std::string("{") + diff --git a/mvt.hpp b/mvt.hpp index 8f24fc68..0c36b74f 100644 --- a/mvt.hpp +++ b/mvt.hpp @@ -93,12 +93,21 @@ struct mvt_value { long long sint_value; bool bool_value; int null_value; + // Initializing string_value initializes the union's full width, which + // the static_assert below checks. Setting only a narrower member (a + // double, say) would leave the remaining bytes indeterminate, and the + // implicit copy constructor copies the union as a whole, so those + // bytes get read even when they aren't the member in use. struct { size_t off; size_t len; - } string_value; + } string_value = {0, 0}; } numeric_value; + static_assert(sizeof(numeric_value) == sizeof(numeric_value.string_value), + "string_value must span the whole union, since its default member " + "initializer is what initializes the union"); + std::string get_string_value() const { if (type == mvt_string) { return std::string(*s, numeric_value.string_value.off, numeric_value.string_value.len); diff --git a/serial.cpp b/serial.cpp index 5afc2ed2..73868194 100644 --- a/serial.cpp +++ b/serial.cpp @@ -665,10 +665,18 @@ int serialize_feature(struct serialization_state *sst, serial_feature &sf, std:: // VT_POINT extent will be calculated in write_tile from the distance between adjacent features. } - if (extent <= LLONG_MAX) { + // Clamp before converting, since converting a double that is out of range + // for a long long is undefined. The bounds are asymmetric: LLONG_MAX is not + // representable as a double and rounds up to 2^63, so the upper bound has to + // be exclusive, while LLONG_MIN is exactly -2^63 and so can be included. + // Areas are signed, so holes that outweigh their rings can make this + // negative. + if (extent >= (double) LLONG_MIN && extent < (double) LLONG_MAX) { sf.extent = (long long) extent; + } else if (extent < 0) { + sf.extent = LLONG_MIN; } else { - sf.extent = LLONG_MAX; + sf.extent = LLONG_MAX; // also the NaN case } if (sst->want_dist && sf.t == VT_POLYGON) { diff --git a/tile-join.cpp b/tile-join.cpp index 921a38b9..017e04b8 100644 --- a/tile-join.cpp +++ b/tile-join.cpp @@ -893,7 +893,7 @@ void *join_worker(void *v) { } void dispatch_tasks(std::map> &tasks, std::vector> &layermaps, sqlite3 *outdb, const char *outdir, std::vector &header, std::map> &mapping, sqlite3 *db, std::set &exclude, std::set &include, int ifmatched, std::set &keep_layers, std::set &remove_layers, json_object *filter, struct tileset_reader *readers, double *minlat, double *minlon, double *maxlat, double *maxlon, double *minlon2, double *maxlon2) { - pthread_t pthreads[CPUS]; + std::vector pthreads(CPUS); std::vector args; for (size_t i = 0; i < CPUS; i++) { diff --git a/tile.cpp b/tile.cpp index ede49e89..a80be63a 100644 --- a/tile.cpp +++ b/tile.cpp @@ -61,9 +61,6 @@ extern "C" { #define COORD_OFFSET (4LL << 32) #define SHIFT_RIGHT(a) ((long long) std::round((double) (a) / (1LL << geometry_scale))) -#define XSTRINGIFY(s) STRINGIFY(s) -#define STRINGIFY(s) #s - pthread_mutex_t db_lock = PTHREAD_MUTEX_INITIALIZER; pthread_mutex_t var_lock = PTHREAD_MUTEX_INITIALIZER; pthread_mutex_t task_lock = PTHREAD_MUTEX_INITIALIZER; @@ -1747,8 +1744,8 @@ long long write_tile(decompressor *geoms, std::atomic *geompos_in, ch key_pool key_pool; - std::atomic within[child_shards]; - long long start_geompos[child_shards]; + std::vector > within(child_shards); + std::vector start_geompos(child_shards); for (size_t i = 0; i < (size_t) child_shards; i++) { within[i] = false; start_geompos[i] = -1; @@ -1813,10 +1810,10 @@ long long write_tile(decompressor *geoms, std::atomic *geompos_in, ch rpa.along = along; rpa.alongminus = alongminus; rpa.buffer = buffer; - rpa.within = within; + rpa.within = within.data(); rpa.geomfile = geomfile; rpa.geompos = geompos; - rpa.start_geompos = start_geompos; + rpa.start_geompos = start_geompos.data(); rpa.oprogress = &oprogress; rpa.todo = todo; rpa.fname = fname; @@ -1861,7 +1858,7 @@ long long write_tile(decompressor *geoms, std::atomic *geompos_in, ch ssize_t which_serial_feature = -1; if (prefilter == NULL) { - sf = next_feature(geoms, geompos_in, z, tx, ty, initial_x, initial_y, &original_features, &unclipped_features, nextzoom, maxzoom, minzoom, max_zoom_increment, pass, along, alongminus, buffer, within, geomfile, geompos, start_geompos, &oprogress, todo, fname, child_shards, filter, global_stringpool, pool_off, layer_unmaps, first_time, compressed_input, &multiplier_state, tile_stringpool, unidecode_data, next_feature_state, arg->droprate); + sf = next_feature(geoms, geompos_in, z, tx, ty, initial_x, initial_y, &original_features, &unclipped_features, nextzoom, maxzoom, minzoom, max_zoom_increment, pass, along, alongminus, buffer, within.data(), geomfile, geompos, start_geompos.data(), &oprogress, todo, fname, child_shards, filter, global_stringpool, pool_off, layer_unmaps, first_time, compressed_input, &multiplier_state, tile_stringpool, unidecode_data, next_feature_state, arg->droprate); } else { sf = parse_feature(prefilter_jp, z, tx, ty, layermaps, tiling_seg, layer_unmaps, postfilter != NULL, key_pool); } @@ -2398,7 +2395,7 @@ long long write_tile(decompressor *geoms, std::atomic *geompos_in, ch if (p.clustered > 0) { serial_val sv, sv2, sv3, sv4; long long point_count = p.clustered + 1; - char abbrev[20]; // to_string(LLONG_MAX).length() / 1000 + 1; + char abbrev[24]; // fits "%lld" of any long long, including the sign and the NUL p.full_keys.push_back(key_pool.pool("clustered")); sv.type = mvt_bool; @@ -2448,7 +2445,7 @@ long long write_tile(decompressor *geoms, std::atomic *geompos_in, ch } { - pthread_t pthreads[tasks]; + std::vector pthreads(tasks); std::vector args; args.resize(tasks); for (int i = 0; i < tasks; i++) { @@ -3252,28 +3249,27 @@ int traverse_zooms(int *geomfd, off_t *geom_size, char *global_stringpool, std:: for (z = iz; z <= maxzoom; z++) { std::atomic most(0); - compressor compressors[TEMP_FILES]; - compressor *sub[TEMP_FILES]; - std::atomic subpos[TEMP_FILES]; - int subfd[TEMP_FILES]; + std::vector compressors(TEMP_FILES); + std::vector sub(TEMP_FILES); + std::vector > subpos(TEMP_FILES); + std::vector subfd(TEMP_FILES); for (size_t j = 0; j < TEMP_FILES; j++) { - char geomname[strlen(tmpdir) + strlen("/geom.XXXXXXXX" XSTRINGIFY(INT_MAX)) + 1]; - snprintf(geomname, sizeof(geomname), "%s/geom%zu.XXXXXXXX", tmpdir, j); - subfd[j] = mkstemp_cloexec(geomname); - // printf("%s\n", geomname); + std::string geomname = std::string(tmpdir) + "/geom" + std::to_string(j) + ".XXXXXXXX"; + subfd[j] = mkstemp_cloexec(&geomname[0]); + // printf("%s\n", geomname.c_str()); if (subfd[j] < 0) { - perror(geomname); + perror(geomname.c_str()); exit(EXIT_OPEN); } - FILE *fp = fopen_oflag(geomname, "wb", O_WRONLY | O_CLOEXEC); + FILE *fp = fopen_oflag(geomname.c_str(), "wb", O_WRONLY | O_CLOEXEC); if (fp == NULL) { - perror(geomname); + perror(geomname.c_str()); exit(EXIT_OPEN); } compressors[j] = compressor(fp); sub[j] = &compressors[j]; subpos[j] = 0; - unlink(geomname); + unlink(geomname.c_str()); } size_t useful_threads = 0; @@ -3341,7 +3337,7 @@ int traverse_zooms(int *geomfd, off_t *geom_size, char *global_stringpool, std:: std::set skip_children_out; for (size_t pass = 0;; pass++) { - pthread_t pthreads[threads]; + std::vector pthreads(threads); std::vector args; args.resize(threads); std::atomic running(threads); @@ -3360,8 +3356,8 @@ int traverse_zooms(int *geomfd, off_t *geom_size, char *global_stringpool, std:: args[thread].outdir = outdir; args[thread].buffer = buffer; args[thread].fname = fname; - args[thread].geomfile = sub + thread * (TEMP_FILES / threads); - args[thread].geompos = subpos + thread * (TEMP_FILES / threads); + args[thread].geomfile = sub.data() + thread * (TEMP_FILES / threads); + args[thread].geompos = subpos.data() + thread * (TEMP_FILES / threads); args[thread].todo = todo; args[thread].along = &along; // locked with var_lock args[thread].gamma = zoom_gamma; diff --git a/version.hpp b/version.hpp index 8974b68f..71b353ad 100644 --- a/version.hpp +++ b/version.hpp @@ -1,6 +1,6 @@ #ifndef VERSION_HPP #define VERSION_HPP -#define VERSION "v2.80.0" +#define VERSION "v2.81.0" #endif