Revert "typified map.h and dependents" (#79106)

[moxian]

Summary

None

Purpose of change

#79106 seems to have introduced ASan failure. See failed runs

• in the PR itself https://github.com/CleverRaven/Cataclysm-DDA/actions/runs/12734142857/job/35491383881
• my, unrelated code: https://github.com/CleverRaven/Cataclysm-DDA/actions/runs/12737041610/job/35502236262
• my, different unrelated code: https://github.com/CleverRaven/Cataclysm-DDA/actions/runs/12737253216/job/35499182096
• not mine, json only change https://github.com/CleverRaven/Cataclysm-DDA/actions/runs/12739638961/job/35503637305

The timing is suspicious (the failures started happening rouhgly after #79106 got merged), and the errors have map_test.cpp in them, which is vaguely related to what #79106 was doing.

The error itself is rather cryptic and I don't think this helps, but I'll add it here for posterity:

=================================================================
(~[slow] ~[.],starting_items)=> ==7357==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7ffdd8770030 at pc 0x000002580c27 bp 0x7ffdd876fef0 sp 0x7ffdd876f6b8
(~[slow] ~[.],starting_items)=> READ of size 12 at 0x7ffdd8770030 thread T0
(~[slow] ~[.],starting_items)=>     #0 0x2580c26 in __asan_memcpy (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x2580c26)
(~[slow] ~[.],starting_items)=>     #1 0x33c6f94 in ____C_A_T_C_H____T_E_S_T____0() /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/../tests/map_test.cpp:33:9
(~[slow] ~[.],starting_items)=>     #2 0x3d0d33a in Catch::TestCase::invoke() const /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/./catch/catch.hpp:14137:15
(~[slow] ~[.],starting_items)=>     #3 0x3d0d33a in Catch::RunContext::invokeActiveTestCase() /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/./catch/catch.hpp:12997:27
(~[slow] ~[.],starting_items)=>     #4 0x3d09441 in Catch::RunContext::runCurrentTest(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/./catch/catch.hpp:12970:17
(~[slow] ~[.],starting_items)=>     #5 0x3d0791c in Catch::RunContext::runTest(Catch::TestCase const&) /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/./catch/catch.hpp:12731:13
(~[slow] ~[.],starting_items)=>     #6 0x3d14fd6 in Catch::(anonymous namespace)::TestGroup::execute() /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/./catch/catch.hpp:13324:45
(~[slow] ~[.],starting_items)=>     #7 0x3d14fd6 in Catch::Session::runInternal() /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/./catch/catch.hpp:13530:39
(~[slow] ~[.],starting_items)=>     #8 0x3d135c9 in Catch::Session::run() /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/./catch/catch.hpp:13486:24
(~[slow] ~[.],starting_items)=>     #9 0x3d4cd48 in main /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/../tests/test_main.cpp:420:26
(~[slow] ~[.],starting_items)=>     #10 0x7f1ed2829d8f  (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f)
(~[slow] ~[.],starting_items)=>     #11 0x7f1ed2829e3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f)
(~[slow] ~[.],starting_items)=>     #12 0x25069b4 in _start (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x25069b4)
(~[slow] ~[.],starting_items)=> 
(~[slow] ~[.],starting_items)=> Address 0x7ffdd8770030 is located in stack of thread T0 at offset 304 in frame
(~[slow] ~[.],starting_items)=>     #0 0x33c670f in ____C_A_T_C_H____T_E_S_T____0() /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/../tests/map_test.cpp:25
(~[slow] ~[.],starting_items)=> 
(~[slow] ~[.],starting_items)=>   This frame has 74 object(s):
(~[slow] ~[.],starting_items)=>     [32, 48) 'agg.tmp.i533'
(~[slow] ~[.],starting_items)=>     [64, 80) 'agg.tmp.i523'
(~[slow] ~[.],starting_items)=>     [96, 112) 'agg.tmp.i'
(~[slow] ~[.],starting_items)=>     [128, 132) 'ref.tmp.i.i457' (line 37)
(~[slow] ~[.],starting_items)=>     [144, 148) 'ref.tmp2.i.i' (line 37)
(~[slow] ~[.],starting_items)=>     [160, 164) 'ref.tmp3.i.i' (line 37)
(~[slow] ~[.],starting_items)=>     [176, 180) 'ref.tmp4.i.i458' (line 37)
(~[slow] ~[.],starting_items)=>     [192, 200) 'ref.tmp.i459'
(~[slow] ~[.],starting_items)=>     [224, 264) 'ref.tmp3.i460'
(~[slow] ~[.],starting_items)=>     [304, 316) 'ref.tmp.i.i' (line 33) <== Memory access at offset 304 is inside this variable
(~[slow] ~[.],starting_items)=>     [336, 348) 'ref.tmp4.i.i' (line 33)
(~[slow] ~[.],starting_items)=>     [368, 380) 'ref.tmp10.i.i' (line 33)
(~[slow] ~[.],starting_items)=>     [400, 412) 'ref.tmp16.i.i' (line 33)
(~[slow] ~[.],starting_items)=>     [432, 440) 'ref.tmp.i'
(~[slow] ~[.],starting_items)=>     [464, 504) 'ref.tmp3.i'
(~[slow] ~[.],starting_items)=>     [544, 576) 'restore_vertical_shift' (line 28)
(~[slow] ~[.],starting_items)=>     [608, 640) 'ref.tmp' (line 28)
(~[slow] ~[.],starting_items)=>     [672, 684) 'test_point' (line 32)
(~[slow] ~[.],starting_items)=>     [704, 720) 'agg.tmp'
(~[slow] ~[.],starting_items)=>     [736, 752) 'ref.tmp2' (line 33)
(~[slow] ~[.],starting_items)=>     [768, 780) 'test_bub' (line 36)
(~[slow] ~[.],starting_items)=>     [800, 804) 'z' (line 37)
(~[slow] ~[.],starting_items)=>     [816, 832) 'agg.tmp7'
(~[slow] ~[.],starting_items)=>     [848, 864) 'ref.tmp8' (line 37)
(~[slow] ~[.],starting_items)=>     [880, 920) 'capturer4' (line 48)
(~[slow] ~[.],starting_items)=>     [960, 976) 'agg.tmp38'
(~[slow] ~[.],starting_items)=>     [992, 1008) 'ref.tmp39' (line 48)
(~[slow] ~[.],starting_items)=>     [1024, 1040) 'agg.tmp40'
(~[slow] ~[.],starting_items)=>     [1056, 1068) 'ref.tmp43' (line 48)
(~[slow] ~[.],starting_items)=>     [1088, 1160) 'catchAssertionHandler' (line 50)
(~[slow] ~[.],starting_items)=>     [1200, 1216) 'ref.tmp52' (line 50)
(~[slow] ~[.],starting_items)=>     [1232, 1248) 'ref.tmp54' (line 50)
(~[slow] ~[.],starting_items)=>     [1264, 1280) 'agg.tmp55'
(~[slow] ~[.],starting_items)=>     [1296, 1344) 'ref.tmp58' (line 50)
(~[slow] ~[.],starting_items)=>     [1376, 1388) 'ref.tmp61' (line 50)
(~[slow] ~[.],starting_items)=>     [1408, 1412) 'ref.tmp74' (line 50)
(~[slow] ~[.],starting_items)=>     [1424, 1496) 'catchAssertionHandler93' (line 51)
(~[slow] ~[.],starting_items)=>     [1536, 1552) 'ref.tmp94' (line 51)
(~[slow] ~[.],starting_items)=>     [1568, 1584) 'ref.tmp96' (line 51)
(~[slow] ~[.],starting_items)=>     [1600, 1616) 'agg.tmp97'
(~[slow] ~[.],starting_items)=>     [1632, 1680) 'ref.tmp102' (line 51)
(~[slow] ~[.],starting_items)=>     [1712, 1724) 'ref.tmp105' (line 51)
(~[slow] ~[.],starting_items)=>     [1744, 1748) 'ref.tmp118' (line 51)
(~[slow] ~[.],starting_items)=>     [1760, 1832) 'catchAssertionHandler144' (line 52)
(~[slow] ~[.],starting_items)=>     [1872, 1888) 'ref.tmp145' (line 52)
(~[slow] ~[.],starting_items)=>     [1904, 1920) 'ref.tmp147' (line 52)
(~[slow] ~[.],starting_items)=>     [1936, 1952) 'agg.tmp148'
(~[slow] ~[.],starting_items)=>     [1968, 2016) 'ref.tmp153' (line 52)
(~[slow] ~[.],starting_items)=>     [2048, 2060) 'ref.tmp156' (line 52)
(~[slow] ~[.],starting_items)=>     [2080, 2092) 'test_abs' (line 56)
(~[slow] ~[.],starting_items)=>     [2112, 2152) 'capturer5' (line 62)
(~[slow] ~[.],starting_items)=>     [2192, 2208) 'agg.tmp239'
(~[slow] ~[.],starting_items)=>     [2224, 2240) 'ref.tmp240' (line 62)
(~[slow] ~[.],starting_items)=>     [2256, 2272) 'agg.tmp241'
(~[slow] ~[.],starting_items)=>     [2288, 2328) 'capturer6' (line 63)
(~[slow] ~[.],starting_items)=>     [2368, 2384) 'agg.tmp247'
(~[slow] ~[.],starting_items)=>     [2400, 2416) 'ref.tmp248' (line 63)
(~[slow] ~[.],starting_items)=>     [2432, 2448) 'agg.tmp249'
(~[slow] ~[.],starting_items)=>     [2464, 2536) 'catchAssertionHandler256' (line 66)
(~[slow] ~[.],starting_items)=>     [2576, 2592) 'ref.tmp257' (line 66)
(~[slow] ~[.],starting_items)=>     [2608, 2624) 'ref.tmp259' (line 66)
(~[slow] ~[.],starting_items)=>     [2640, 2656) 'agg.tmp260'
(~[slow] ~[.],starting_items)=>     [2672, 2720) 'ref.tmp265' (line 66)
(~[slow] ~[.],starting_items)=>     [2752, 2760) 'ref.tmp266' (line 66)
(~[slow] ~[.],starting_items)=>     [2784, 2796) 'ref.tmp268' (line 66)
(~[slow] ~[.],starting_items)=>     [2816, 2828) 'ref.tmp269' (line 66)
(~[slow] ~[.],starting_items)=>     [2848, 2920) 'catchAssertionHandler310' (line 67)
(~[slow] ~[.],starting_items)=>     [2960, 2976) 'ref.tmp311' (line 67)
(~[slow] ~[.],starting_items)=>     [2992, 3008) 'ref.tmp313' (line 67)
(~[slow] ~[.],starting_items)=>     [3024, 3040) 'agg.tmp314'
(~[slow] ~[.],starting_items)=>     [3056, 3104) 'ref.tmp319' (line 67)
(~[slow] ~[.],starting_items)=>     [3136, 3144) 'ref.tmp320' (line 67)
(~[slow] ~[.],starting_items)=>     [3168, 3180) 'ref.tmp322' (line 67)
(~[slow] ~[.],starting_items)=>     [3200, 3212) 'ref.tmp323' (line 67)
(~[slow] ~[.],starting_items)=> HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
(~[slow] ~[.],starting_items)=>       (longjmp and C++ exceptions *are* supported)
(~[slow] ~[.],starting_items)=> SUMMARY: AddressSanitizer: stack-use-after-scope (/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/cata_test+0x2580c26) in __asan_memcpy
(~[slow] ~[.],starting_items)=> Shadow bytes around the buggy address:
(~[slow] ~[.],starting_items)=>   0x10003b0e5fb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
(~[slow] ~[.],starting_items)=>   0x10003b0e5fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
(~[slow] ~[.],starting_items)=>   0x10003b0e5fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
(~[slow] ~[.],starting_items)=>   0x10003b0e5fe0: f1 f1 f1 f1 f8 f8 f2 f2 f8 f8 f2 f2 f8 f8 f2 f2
(~[slow] ~[.],starting_items)=>   0x10003b0e5ff0: f8 f2 f8 f2 f8 f2 f8 f2 f8 f2 f2 f2 f8 f8 f8 f8
(~[slow] ~[.],starting_items)=> =>0x10003b0e6000: f8 f2 f2 f2 f2 f2[f8]f8 f2 f2 f8 f8 f2 f2 f8 f8
(~[slow] ~[.],starting_items)=>   0x10003b0e6010: f2 f2 f8 f8 f2 f2 f8 f2 f2 f2 f8 f8 f8 f8 f8 f2
(~[slow] ~[.],starting_items)=>   0x10003b0e6020: f2 f2 f2 f2 00 00 00 00 f2 f2 f2 f2 f8 f8 f8 f8
(~[slow] ~[.],starting_items)=>   0x10003b0e6030: f2 f2 f2 f2 00 04 f2 f2 00 00 f2 f2 00 00 f2 f2
(~[slow] ~[.],starting_items)=>   0x10003b0e6040: f8 f8 f2 f2 f8 f2 00 00 f2 f2 f8 f8 f2 f2 f8 f8
(~[slow] ~[.],starting_items)=>   0x10003b0e6050: f8 f8 f8 f2 f2 f2 f2 f2 00 00 f2 f2 f8 f8 f2 f2
(~[slow] ~[.],starting_items)=> Shadow byte legend (one shadow byte represents 8 application bytes):
(~[slow] ~[.],starting_items)=>   Addressable:           00
(~[slow] ~[.],starting_items)=>   Partially addressable: 01 02 03 04 05 06 07 
(~[slow] ~[.],starting_items)=>   Heap left redzone:       fa
(~[slow] ~[.],starting_items)=>   Freed heap region:       fd
(~[slow] ~[.],starting_items)=>   Stack left redzone:      f1
(~[slow] ~[.],starting_items)=>   Stack mid redzone:       f2
(~[slow] ~[.],starting_items)=>   Stack right redzone:     f3
(~[slow] ~[.],starting_items)=>   Stack after return:      f5
(~[slow] ~[.],starting_items)=>   Stack use after scope:   f8
(~[slow] ~[.],starting_items)=>   Global redzone:          f9
(~[slow] ~[.],starting_items)=>   Global init order:       f6
(~[slow] ~[.],starting_items)=>   Poisoned by user:        f7
(~[slow] ~[.],starting_items)=>   Container overflow:      fc
(~[slow] ~[.],starting_items)=>   Array cookie:            ac
(~[slow] ~[.],starting_items)=>   Intra object redzone:    bb
(~[slow] ~[.],starting_items)=>   ASan internal:           fe
(~[slow] ~[.],starting_items)=>   Left alloca redzone:     ca
(~[slow] ~[.],starting_items)=>   Right alloca redzone:    cb
(~[slow] ~[.],starting_items)=>   Shadow gap:              cc
(~[slow] ~[.],starting_items)=> ==7357==ABORTING

I don't think I am able to debug this, but I do believe it's valuable to revert it first in order to get CI back to mostly-green, and then reapply later when we better understand what's going on and how to fix.

Describe the solution

Click the Revert button on github.

Describe alternatives you've considered

• Try to actively fix the error, and suffer CI wrath in the meanwhile. This might mean introducing more breakages that get masked by already-failing CI, which is not an improvement IMO.
• Try to revert partially instead of wholesale. I feel this is not any easier tbh...

Testing

None.
Hopefully CI will pass.

Additional context

[moxian]

cc @PatrikLundell (forgot to tag you in the OP and i'm not sure if github pings people upon edits)

[PatrikLundell]

I did receive two messages for whatever reason, so I guess the explicit pinging might be redundant (not that it matters).

I certainly don't understand what's going on, nor why this wasn't flagged on the PR.
However, I think your change might be overkill. The error reports points to a line in map_test which I did change, and I'd suggest it would be better to change that line back (to using untyped tripoints) and use test_bub instead of test_point where things don't compile.

I don't understand what GENERATE does, but it wouldn't be impossible that it chokes on typed coordinates for some reason.

Edit:
I'm producing a PR that just does what I indicated above, as I suspect that should be enough to deal with the issue. It can then be up to the administrators to determine which PR to merge and which one to discard.

[mqrause]

I don't understand what GENERATE does, but it wouldn't be impossible that it chokes on typed coordinates for some reason.

I would expect it chokes on the calculations instead of the typed points, though I don't actually know. Personally I'd try to use it with tripoint_rel_ms instead.

And I agree that a complete revert is overkill for "just" an asan failure.

[moxian]

I would be extremely happy if this can be fixed in a more surgical fashion! Thank you!

[PatrikLundell]

The tests are using coordinates in a rather odd way at times, setting up relative coordinates and then using them as bubble ones (and sometimes relative ones in other places in the same test). This is one of these tests.

This type of calculations work fine elsewhere, so I don't think switching to tripoint_rel_ms would help, but I guess we won't find out unless someone is willing to expend the effort to test it.

@moxian: It seems to me the limited approach should work, but I guess we won't know until the tests have been performed.
I guess the lesson from this is to not merge PRs until all tests have been run unless there are good reasons to speed up the process.
And thanks for sounding the alarm.

Edit:
@mqrause: Ah, now I understand what you meant, getting rid of the calculation by switching to rel coordinates. Might work, in which case feeding GENERATE pre calculated bub constants might work as well. Still, we won't know unless someone wants to experiment with it.