Failed tests with -flto=auto

[Vedingrot]

Hi, when compile with -O2 and -flto=auto fails 5 tests.

The following tests FAILED:
31 - s2cell_index_test (Subprocess aborted)
33 - s2cell_iterator_testing_test (Failed)
35 - s2cell_union_test (Failed)
75 - s2point_index_test (Subprocess aborted)
88 - s2region_coverer_test (Subprocess aborted)

LastTest.log

Compiler: gcc 13.2.1

[smcallis]

Do these pass if you don't pass -flto? Very strange if so.

…

On Thu, Dec 21, 2023 at 9:15 AM Vedingrot ***@***.***> wrote: Hi, when compile with -O2 and -flto=auto fails 5 tests. The following tests FAILED: 31 - s2cell_index_test (Subprocess aborted) 33 - s2cell_iterator_testing_test (Failed) 35 - s2cell_union_test (Failed) 75 - s2point_index_test (Subprocess aborted) 88 - s2region_coverer_test (Subprocess aborted) LastTest.log <https://github.com/google/s2geometry/files/13743937/LastTest.log> — Reply to this email directly, view it on GitHub <#337>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGEMKTKI2W4YH3VZW5RIIDYKROCRAVCNFSM6AAAAABA6UGS3SVHI2DSMVQWIX3LMV43ASLTON2WKOZSGA2TENZQHE3DANA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[Vedingrot]

They're pass without -flto.

[Vedingrot]

I also tried to add -ffat-lto-objects and it helps, but there is warning that looks worrying.

ffat_lto.log

[smcallis]

OK can you provide a full set of commands for the first case that'll let me reproduce it?

…

On Thu, Dec 21, 2023 at 9:47 AM Vedingrot ***@***.***> wrote: I also tried to add -ffat-lto-objects and it helps, but there is warning that looks worrying. ffat_lto.log <https://github.com/google/s2geometry/files/13744279/ffat_lto.log> — Reply to this email directly, view it on GitHub <#337 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGEMKXGERBMJAM3C3O5QMTYKRR2LAVCNFSM6AAAAABA6UGS3SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRWGYZTEMBTG4> . You are receiving this because you commented.Message ID: ***@***.***>

[jmr]

What compiler / version?

[Vedingrot]

mkdir build1 && cd build1
cmake -DCMAKE_PREFIX_PATH=/usr/src/abseil17/ -DCMAKE_CXX_STANDARD=17 -DBUILD_TESTS=on -DCMAKE_CXX_FLAGS='-O2 -g -flto=auto -Wall' ..
make VERBOSE=1 -j4
make test -j4

I was using googletest installed on the system, but the problem with not googletest.
Here is a patch that makes this possible or just add -DGOOGLETEST_ROOT=/path/to/googletests/
libs2geometry-use-external-gtest.txt

gcc -v
Using built-in specs.
COLLECT_GCC=x86_64-alt-linux-gcc
COLLECT_LTO_WRAPPER=/usr/lib64/gcc/x86_64-alt-linux/13/lto-wrapper
Target: x86_64-alt-linux
Configured with: ../configure --host=x86_64-alt-linux --build=x86_64-alt-linux --target=x86_64-alt-linux --prefix=/usr --exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin --sysconfdir=/etc --datadir=/usr/share --includedir=/usr/include --libdir=/usr/lib64 --libexecdir=/usr/libexec --localstatedir=/var/lib --sharedstatedir=/var/lib --mandir=/usr/share/man --infodir=/usr/share/info --disable-dependency-tracking --disable-silent-rules --without-included-gettext --enable-shared --program-suffix=-13 --with-slibdir=/lib64 --libexecdir=/usr/lib64 --with-bugurl=http://bugzilla.altlinux.org --enable-__cxa_atexit --enable-threads=posix --enable-checking=release --with-system-zlib --with-zstd --without-included-gettext --enable-multilib --enable-default-pie --enable-gnu-unique-object --enable-linker-build-id --with-linker-hash-style=gnu --with-arch_32=i586 --with-tune_32=generic --with-multilib-list=m64,m32,mx32 --with-gcc-major-version-only --enable-vtable-verify --enable-bootstrap --with-build-config=bootstrap-lto --enable-link-serialization=1 --enable-languages=c,c++,fortran,objc,obj-c++,ada,go,d,lto --enable-plugin
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 13.2.1 20230817 (ALT Sisyphus 13.2.1-alt2) (GCC) 

build_log.txt

[jmr]

I can reproduce the same thing with g++ (Debian 13.2.0-5) 13.2.0. Debian clang version 16.0.6 (16) works fine.

This one looks like it is maybe the simplest failure:

[ RUN      ] MockIterator.BasicFunctionality
/tmp/s2/s2-geometry/src/s2/s2cell_iterator_testing_test.cc:49: Failure
Value of: iter.id()
Expected: is equal to 4/1032010230
  Actual: 4/103201023 (of type S2CellId)

/tmp/s2/s2-geometry/src/s2/s2cell_iterator_testing_test.cc:50: Failure
Value of: iter.value()
Expected: is equal to 0
  Actual: 5 (of type int)

/tmp/s2/s2-geometry/src/s2/s2cell_iterator_testing_test.cc:62: Failure
Value of: iter.id()
Expected: is equal to 4/1032010230301
  Actual: Invalid: 89c259d200000000 (of type S2CellId)

/tmp/s2/s2-geometry/src/s2/s2cell_iterator_testing_test.cc:63: Failure
Value of: iter.value()
Expected: is equal to 1
  Actual: 4 (of type int)

/tmp/s2/s2-geometry/src/s2/s2cell_iterator_testing_test.cc:68: Failure
Value of: iter.value()
Expected: is equal to 2
  Actual: 3 (of type int)

/tmp/s2/s2-geometry/src/s2/s2cell_iterator_testing_test.cc:72: Failure
Value of: iter.id()
Expected: is equal to 4/1032010230301
  Actual: Invalid: 89c259d200000000 (of type S2CellId)

/tmp/s2/s2-geometry/src/s2/s2cell_iterator_testing_test.cc:73: Failure
Value of: iter.value()
Expected: is equal to 1
  Actual: 4 (of type int)

/tmp/s2/s2-geometry/src/s2/s2cell_iterator_testing_test.cc:77: Failure
Value of: iter.id()
Expected: is equal to 4/10320102303
  Actual: 4/103201023 (of type S2CellId)

/tmp/s2/s2-geometry/src/s2/s2cell_iterator_testing_test.cc:82: Failure
Value of: iter.done()
Expected: is true
  Actual: false (of type bool)

[  FAILED  ] MockIterator.BasicFunctionality (0 ms)

It's just doing basic stuff with absl::btree_map and iterators.

Maybe run the abseil-cpp tests with -flto.

[Vedingrot]

@jmr compiling abseil-cpp with -flto is meaningless because it's separate link unit and in addition absl::btree_map is template therefore no need to be precompiled.

[jmr]

absl::btree_map isn't totally self-contained, but it probably is unlikely abseil-cpp has a test that tickles this bug in the right way.

I think the only parts of S2 that MockIterator.BasicFunctionality depends on are S2CellId::id(), operator<(), and operator==(). Can you try to cut this down to a minimal test case so we can see whether it's an S2, abseil-cpp or g++ problem? Does it still happen without the absl_btree_prefer_linear_node_search typedef?

[Vedingrot]

Hi, I just build with latest abseil library and
MockIterator.BasicFunctionality is passed. What about other do have any
idea?

The following tests FAILED:
	 31 - s2cell_index_test (Subprocess aborted)
	 35 - s2cell_union_test (Failed)
	 75 - s2point_index_test (Subprocess aborted)
	 88 - s2region_coverer_test (Subprocess aborted)

only_failed_test_log.txt
full_tests_log.txt

[jmr]

I would start but looking at s2cell_union_test. That is probably the simplest failure.

[jmr]

Here's something to debug:

TEST(S2CellUnion, IntersectsGithubIssue337) {
  S2CellUnion xcells = s2textformat::MakeCellUnionOrDie("2/1021322000001121003");
  S2CellId yid = s2textformat::MakeCellIdOrDie("2/10213223");
  S2CellUnion ucells = xcells.Intersection(yid);
  EXPECT_THAT(ucells.cell_ids(), testing::IsEmpty());
}

[ RUN      ] S2CellUnion.IntersectsGithubIssue337
s2-geometry/src/s2/s2cell_union_test.cc:377: Failure
Value of: ucells.cell_ids()
Expected: is empty
  Actual: { 2/1021322000001121003 }, whose size is 1

[  FAILED  ] S2CellUnion.IntersectsGithubIssue337 (0 ms)

See what's going on with the lower_bound call in S2CellUnion::Intersection:

s2geometry/src/s2/s2cell_union.cc

Line 351 in 7940c9f

vector<S2CellId>::const_iterator i = std::lower_bound(

[jmr]

This looks like either a gcc LTO bug or LTO taking advantage of some UB.

    vector<S2CellId>::const_iterator i = std::lower_bound(
        cell_ids_.begin(), cell_ids_.end(), id.range_min());
    if (i != cell_ids_.end()) {
      ABSL_DCHECK_GE(*i, id.range_min())
          << i->id() << " vs. " << id.range_min().id();
    }

F0000 00:00:1709541598.504342  378352 s2cell_union.cc:353] Check failed: *i >= id.range_min() (1/10 vs. 1/112210020000000000000000000000) 2954361355555045376 vs. 3118813110698246145

All tests pass with -D_GLIBCXX_DEBUG and no LTO. s2cell_union passes with -D_GLIBCXX_DEBUG -flto=auto, but some other tests still fail.

The next step is probably to make a minimal test case.

[jmr]

I made a test case from the apparently relevant parts of the code (S2CellUnion::Intersection(S2Cellid) and everything it transitively uses -- this is just a few functions from S2CellUnion and S2CellId.) The test passed. I'll have to start with the full S2 and rip things out incrementally.