00:13:06 Unstable branch on cbro.berotato.org updated to: 0.35-a0-24-g264ef1ee83 (34) 00:56:14 Monster database of master branch on crawl.develz.org updated to: 0.35-a0-24-g264ef1ee83 01:41:36 turntechCatfish (L15 DjHW) ASSERT(spell < NUM_SPELLS && index < 2) in 'tilepick.cc' at line 5217 failed. (Lair:4) 01:43:39 turntechCatfish (L15 DjHW) ASSERT(spell < NUM_SPELLS && index < 2) in 'tilepick.cc' at line 5217 failed. (Lair:4) 01:44:21 turntechCatfish (L15 DjHW) ASSERT(spell < NUM_SPELLS && index < 2) in 'tilepick.cc' at line 5217 failed. (Lair:4) 01:45:40 turntechCatfish (L15 DjHW) ASSERT(spell < NUM_SPELLS && index < 2) in 'tilepick.cc' at line 5217 failed. (Lair:4) 04:36:16 Experimental (bcrawl) branch on underhound.eu updated to: 0.23-a0-5261-gd9800d219b 05:09:29 Unstable branch on crawl.akrasiac.org updated to: 0.35-a0-24-g264ef1e (34) 07:27:23 <04dracoomega> Would be nice if I could get my hands on that save file. Since looking at the crash log is suggesting there's a parchment with a spell ID of 638 (the enum ends at 511) 08:06:39 <09gammafunk> can ping ZureaL to ask if he could grab it, although he's in the middle of an outage with his ISP, so I'm not sure if cbro can be reached 08:20:54 <08nicolae> the forbidden parchments... 08:21:05 <08nicolae> fr new unrand consumables. the forbidden parchment. if you read it you die. 08:34:23 ??felid reasons 08:34:24 felid reasons[1/6]: Extra life! You're a member of team cat. 10:44:41 <08o____0> 638 is the dream sheep enum and that seed starts with a dream sheep right next to tile 51,50... maybe coincidence? 10:45:26 <08o____0> oh what if it's a dreamsheep corpse haha 10:45:36 <08o____0> That might make sense? 10:45:46 <04dracoomega> I mean, that would imply more deeply weird things about what is happening here C++ const item_def* item = next_pc.map_knowledge.item(); if (item) { spell_type spell = static_cast(item->plus); 10:45:50 <04dracoomega> Oh, corpse, huh 10:46:19 <08o____0> yeah reproducible 10:46:23 <04dracoomega> I have no idea where the monster type is stored in a corpse, but it could be 10:46:55 <08o____0> Kill a dream sheep on top of that escape hatch (out of los of the stairs), wizard teleport to the stairs, go down stairs, go back up, crash 10:47:36 <04dracoomega> Is there also a parchment there? 10:47:39 <08o____0> (tested in webtiles) 10:47:40 <08o____0> no 10:47:43 <04dracoomega> ???? 10:48:03 <04dracoomega> Anyway, if it's actually reproducible from that seed, I can take a look later 10:48:21 <08o____0> I will keep prodding it 11:05:21 <04dracoomega> Hmm... I am trying to do what you suggest, but am not actually reproducing it 11:06:42 I was curious if there was ever any thought to giving unarmed the same level of detail in the "equip" screen as weapons? It is tough to tell the damage + attack delay when doing an unarmed run. Apologies if this is something which has been brought up before. 11:07:16 <04dracoomega> Press @ to see your damage rating and attack delay 11:07:46 ah i see! I am new at the game, thank you for the information. 11:08:07 <08o____0> Here's a save with it set up. The corpses are southwest on top of the escape hatch. To trigger the crash go downstairs then upstairs 11:08:08 <08o____0> https://cdn.discordapp.com/attachments/747522859361894521/1466495138477113466/zZ8ZZq6.zip?ex=697cf387&is=697ba207&hm=6f9a0c6c44ecfd42ffff1c8a77a1e5b798717f5513f4bc3cf35fadf8fe348501& 11:08:44 <08o____0> (it works on local tiles too) 11:11:06 <04dracoomega> Wait, it does? 11:11:13 <04dracoomega> That part is actually more surprising 11:11:47 <08o____0> Yeah, isn't that fun? haha 11:11:48 <08o____0> https://cdn.discordapp.com/attachments/747522859361894521/1466496060162834452/Y7uRVfF.txt?ex=697cf462&is=697ba2e2&hm=8dad641e20150f8db92b055e972a02eccaf51c6f8a073069fe413121659aa856& 11:14:17 <04dracoomega> Okay, so this does crash for me too (but that further raises the question of what I was doing slightly differently before that made it not do that 11:15:45 <08o____0> Yeah, loading the level seems to be an important part of it. Does it need to be a remembered cell, out of LoS? Does it need to be on a feature like escape hatch(doubtful)? 11:16:24 <04dracoomega> I mean, I was doing exactly "Go to Lair:4, kill a dream sheep top of that hatch, wizmode teleport to the stairs, go down and up again" on 3 different clean saves now and it has never crashed 11:16:36 <04dracoomega> And your save file confirms I was using the same stairs and same hatch with the corpse on it 11:17:41 <04dracoomega> But I must be doing something else different to get consistently different results than you 11:19:05 <04dracoomega> Can you be even more specific about exactly how you did all this? 11:24:51 <04dracoomega> I keep trying minor variations on this and still nothing 11:34:03 <08o____0> I could make a recording I guess 11:36:01 <08o____0> It looks like it does depend on if the cell is rendered in the window which is unsurprising 11:47:30 <04dracoomega> Oh, silly me 11:48:20 <04dracoomega> Anyway, the issue is that there's a parchment on the same position on the lower floor and I didn't actually have that tile revealed (and you did) 11:48:51 <04dracoomega> So it's clearly looking at the tile from the previous draw and not the current draw when querying this - even if it was on a different floor 11:50:08 <08o____0> oooooh 11:50:10 <11Odds> I was messing around because this bug is fun, and it doesn't go away on further draws (if you just skip the crash somehow) 11:53:24 <04dracoomega> I am curious why it has the old tile here, though. Like, this is specifically querying the 'next frame's' tiles, but clearly gathered them from the previous floor 11:54:03 <04dracoomega> (Like, all along the crash could have been band-aided by checking that the item is actually a parchment first, but it felt like there were other issues here.) 11:54:34 <04dracoomega> It's possible this otherwise doesn't 'matter' (it might be one redraw that is immediately overwritten) and it seems likely any non-trivial change would be too scary to do after feature freeze here 11:54:50 <04dracoomega> So I might still apply a band-aid for the time-being 12:00:41 <04dracoomega> I'm reminded of this conversation: https://ptb.discord.com/channels/735056636644687913/747522859361894521/1391161841882894456 (though apparently doing this wasn't enough to ensure proper sync either) 12:05:16 <04dracoomega> However, the fact that this happens in local tiles as well might represent a different issue 12:18:58 <11Odds> I think it's a bug in _tile_place_item_marker (which happens here because of the stairs) static void _tile_place_item_marker(const coord_def &gc, const item_def &item) { tile_env.bk_fg(gc) = ((tileidx_t)tile_env.bk_fg(gc)) | TILE_FLAG_S_UNDER; if (item_needs_autopickup(item)) tile_env.bk_bg(gc) |= TILE_FLAG_CURSOR3; } This should use tileidx_item(item) in the first line - it doesn't update the item 12:23:54 <11Odds> So I think that tile_draw_map_cell is making a mistake when we have an item on stairs, and not clearing the item from the previous level - instead it just places an "item under" marker on the tile without clearing the old tile 12:24:32 <04dracoomega> Oh, you think it's specifically stair-based? (I was just about to ponder out loud if tile_env.bk_fg and such wasn't being cleared/updated properly on the first draw of a new floor) 12:24:56 <11Odds> I think this may be the right fix to tile_draw_map_cell: tile_env.bk_fg(gc) = 0; if (cell.invisible_monster()) _tile_place_invisible_monster(gc); else if (cell.monsterinfo()) _tile_place_monster(gc, *cell.monsterinfo()); else if (cell.item()) { if (feat_is_stair(cell.feat())) _tile_place_item_marker(gc, *cell.item()); else _tile_place_item(gc, *cell.item(), 12:24:56 (cell.flags & MAP_MORE_ITEMS) != 0); } 12:25:00 <04dracoomega> Since the map_knowledge for the new floor is already copied into it properly 12:25:44 <11Odds> This is the current version: if (cell.invisible_monster()) _tile_place_invisible_monster(gc); else if (cell.monsterinfo()) _tile_place_monster(gc, *cell.monsterinfo()); else if (cell.item()) { if(gc.x == 51 && gc.y == 50) { mprf("Placing item (marker) at gc (%d,%d)", gc.x, gc.y); } if (feat_is_stair(cell.feat())) _tile_place_item_marker(gc, *cell.item()); 12:25:45 else _tile_place_item(gc, *cell.item(), (cell.flags & MAP_MORE_ITEMS) != 0); } else tile_env.bk_fg(gc) = 0; So when we have an item on a stair, we don't clear the bg (just set a little * marker on what is already there) 12:26:02 <04dracoomega> That... does look very plausible when you mention it 12:26:04 <11Odds> (My understanding of all this is terribly partial) 12:26:27 <04dracoomega> I mean, so is mine. (imo, the many layers stuff has to go through to render in Crawl is incredibly tangled and needless opaque) 12:26:49 <04dracoomega> Sins of 16 years ago we're still bearing 12:27:32 <04dracoomega> One of those "It would make more sense to throw it all out and rewrite from scratch than try to disentangle some of this, but also that project is too large to expect anyone to ever actually do it." kind of things ^^; 12:29:03 <04dracoomega> That change you posted does fix the current crash and the logic for it also seems sound to me 12:30:10 <11Odds> All the other branches of that if change bk_fg, so it seems reasonably safe. Like, this function isn't supposed to care about the old value of that AFAIC. 12:30:27 <04dracoomega> It doesn't seem like it should 12:31:35 <04dracoomega> (I don't feel confident stating anything with 100% certainty on this end of the codebase, but it seems likely that if this somehow breaks something important, it will be obvious enough in short order. So I'm going to go and commit it.) 12:31:37 <04dracoomega> Thanks a bunch! 12:32:55 <11Odds> You're very welcome, I do enjoy debugging a lot 🙂 12:41:16 03DracoOmega02 07* 0.35-a0-25-gf2d9c54bfe: Fix an obscure parchment rendering crash (Odds, Rypofalem) 10(2 minutes ago, 1 file, 1+ 2-) 13https://github.com/crawl/crawl/commit/f2d9c54bfe28 12:41:17 03DracoOmega02 07[stone_soup-0.34] * 0.34-b1-26-g8cad3c52ff: Fix an obscure parchment rendering crash (Odds, Rypofalem) 10(2 minutes ago, 1 file, 1+ 2-) 13https://github.com/crawl/crawl/commit/8cad3c52ffb2 13:43:13 caliz (L13 DsAl) ASSERT(in_bounds(mg.pos)) in 'mon-place.cc' at line 3165 failed. (Lair:5) 14:49:05 <08o____0> !crash 14:49:09 <04Cerebot> 22583. caliz, XL13 DsAl, T:21688 (milestone): https://crawl.akrasiac.org/rawdata/caliz/crash-caliz-20260129-204312.txt 14:49:59 <08o____0> I think what's happening there is that the mountainshell uses landbreaker, which damages the player, triggering eel shock, killing the mountainshell, which screws with the logic for placing rubble 14:52:24 <08o____0> (killing the mountainshell while the landbreaker spell is still being cast) 14:52:26 <04dracoomega> Yeah, I'm already fixing it 14:52:31 <08o____0> ah cool! 15:00:14 <04dracoomega> Once again, I can't help but think that a lot of stuff we've needed special handling for, or fineff conversion, would automatically be fixed if we did monster reset/cleanup only at fineff times instead of 'immediately' 15:00:20 03DracoOmega02 07* 0.35-a0-26-g128f4cd87f: Fix a possible Landbreaker crash 10(4 minutes ago, 1 file, 4+ 2-) 13https://github.com/crawl/crawl/commit/128f4cd87f8a 15:00:20 03DracoOmega02 07* 0.35-a0-27-g71f451d4b8: Make reactive eeljolt a fineff 10(4 minutes ago, 3 files, 24+ 1-) 13https://github.com/crawl/crawl/commit/71f451d4b8f6 15:00:21 03DracoOmega02 07[stone_soup-0.34] * 0.34-b1-27-g59cb284ae2: Fix a possible Landbreaker crash 10(4 minutes ago, 1 file, 4+ 2-) 13https://github.com/crawl/crawl/commit/59cb284ae2ec 15:00:21 03DracoOmega02 07[stone_soup-0.34] * 0.34-b1-28-g3bee177017: Make reactive eeljolt a fineff 10(4 minutes ago, 3 files, 24+ 1-) 13https://github.com/crawl/crawl/commit/3bee177017d7 15:00:50 <04dracoomega> So that you could expect any state you're querying about the agent of a thing to remain valid until it's done whatever it's doing 15:29:51 …except I keep thinking that runs a risk of other things happening at the wrong times, such that we'd need fineff prioritization or dependency checking 15:35:22 <08o____0> 😅 16:24:48 <09gammafunk> just remove monsters imo 16:42:08 does that mean the player too? }:> 16:42:11 <04dracoomega> It may, of course, be possible that this would cause other problems I haven't yet thought of, but it's already safe to do many operations to dead (but not yet reset) monsters since we already do this in countless places, all the time. And they're already marked as dead, so they're not on the monster grid and monster iterators won't see them. This would just mean preserving the memory of them a little bit longer. 16:42:31 Unstable branch on underhound.eu updated to: 0.35-a0-27-g71f451d4b8 (34) 16:42:42 <04dracoomega> (A bunch of places already have to do manual caching of dying monsters so that they can access the memory of them after they're already dead, too. Death curses, exploding monsters, star jellies...) 18:16:08 <04dracoomega> @gammafunk So, I've been looking into the longstanding Disco Pan memory leak issue and believe I have a fix, but I wanted to run a few things about it past you, since you have more lua knowledge than I do. The immediate (and apparently way more obvious now than previously) issue is found in lua_element_colour_calc::get. Specifically, the stack doesn't get fully popped from what is pushed onto it here and I think an entire copy of 18:16:09 the colour function is left on the stack every time the colour for any tile is queried. Debug info showed the stack growing continuously, and changing the lua_pop(ls, 1); to lua_pop(ls, 2); fixes the issue without any adverse effect (that is immediately obvious, anyway). On a little investigation, luaL_checkstring() only peeks at the string rather than popping it (so presumably lua_pop pops just the string and not all of the function pushed before it). 18:16:09 But this made me a bit concerned at just how much we use luaL_checkstring elsewhere (and in clua). Is there something different about how functions are called there that implicitly does cleanup in a way that this can't? Or is that also causing stack leaks that are just much smaller since they're not leaving entire functions? 19:11:16 <09gammafunk> well luaL_checkstring() is definitely only supposed to inspect the lua C stack and not modify it. It's part of lua 5.4's "auxilliary library", and see the lua 5.4 manual reference here and note that that little stack heading there of [-0, +0, v] means that it removes 0 items from the stack and adds 0 items (and the v is about how it handles errors). So definitely use of luaL_checkstring() is always fine and will never cause any kind 19:11:16 of memory leak. It's just the best/most basic lua way to examine a string on the lua C stack and is very benign. For lua_pop(ls, n), it's just popping n elements from the top of the stack. Regarding the lua_element_colour function being pushed, I'm seeing thatlua_element_colour::get() is doing a pcall (effectively, via clua::callfn()), and note here how lua_pcall(...) removes (nargs + 1) items from the stack; as the doc says it removes the function 19:11:16 and its args from the stack. So we definitely wouldn't want to try to do this ourselves. The single call lua_pop(ls, 1) does make sense in that it's removing the return result from the lua datum function it pushed onto the stack 19:12:59 <09gammafunk> it's possible for a lua function to return multiple elements but the callfn() correctly indicates that there's only one return value 19:13:40 <04dracoomega> Well, you have more expertise here than I do, but something is definitely accumulating indefinitely on the stack here 19:13:42 <09gammafunk> now I'm wondering though about our error handler... 19:13:57 <09gammafunk> cpp // For the dlua state, we use an error handler that saves a stack trace. if (!managed_vm) { lua_pushcfunction(_state, _clua_trace_handler); lua_insert(_state, 1); msgh = 1; } return lua_pcall(_state, argc, retc, msgh); 19:14:40 <02Darby> yeah, recently (I'm guessing the lua update), reports of disco pan slowness have increased and our own testing has shown it (and other vaults that use the effect) slowing the game down to extreme levels (1 second per turn, etc) 19:14:49 <09gammafunk> this is error handling I added which is very helpful, but lua_pcall() doc of this is slightly sparse and didn't tell me, for instance, that msgh must be before the function 19:15:06 <09gammafunk> so now that you've brought this up I'm wondering if maybe msgh is what's not getting removed 19:15:26 <09gammafunk> this could even be related to the strange objstat behaviour 19:15:56 <04dracoomega> Well, even if the behavior has worsened recently, a memory leak of some sort existed with disco Pan for several versions at a minimum 19:15:58 <09gammafunk> that lua_insert() is making sure it resides just before the function being called 19:16:02 <09gammafunk> ah 19:16:22 <09gammafunk> well msgh definitely didn't exist before 0.34-a 19:16:27 <04dracoomega> (I tried looking into it a long time ago without success, but took another crack at it today) 19:17:14 <09gammafunk> now that you've given me that lead, I can take a look at this tomorrow 19:17:19 <02Darby> afaik the only reason it doesn't come up with other "disco" vaults (e.g. uskayaw_murder_on_the_dancefloor) is because they have fewer tiles than the rest 19:17:28 <02Darby> if made larger, they also slow down 19:17:39 <09gammafunk> you can examine the stack, for example, to see if the function on the stack is the same as the datum function pushed, etc 19:18:31 <09gammafunk> so I'll work some on that tomorrow and should be able to at least get some useful info about what's going on 19:18:39 <04dracoomega> Thanks 19:18:54 <02Darby> ditto 19:19:07 <04dracoomega> (I am really working with a very rudimentary understanding of lua's inner workings here) 19:19:08 <09gammafunk> ditto thanks or ditto you'll work on it some tomorrow 19:19:18 <02Darby> thanks, I don't know lua at all 19:19:40 <09gammafunk> yeah, I'm not super far ahead in terms of lua but I have learned a fair amount by all of these lua shenanigens and due to refactoring our clua class etc 19:19:45 <04dracoomega> Yeah 19:19:53 <09gammafunk> it's pretty interesting actually, but it's not most people's cup of tea 19:29:18 way better than Tcl though 21:30:57 <09gammafunk> well....that was certainly embarrassing 21:31:51 New branch created: lua-stack-fix (1 commit) 13https://github.com/crawl/crawl/tree/lua-stack-fix 21:31:52 03gammafunk02 07[lua-stack-fix] * 0.35-a0-28-g03f7a86f93: Don't pollute the lua C stack (DracoOmega) 10(28 minutes ago, 1 file, 9+ 1-) 13https://github.com/crawl/crawl/commit/03f7a86f9360 21:31:53 <09gammafunk> The bad news is that my handler was polluting the lua C stack with every lua pcall (so most instances where we call lua). The good news is that it's pretty easy to fix. Just going to let CI take a look before I merge 21:32:07 <09gammafunk> @dracoomega meant to ping you here 21:32:47 <04dracoomega> Oh boy 21:32:48 <09gammafunk> I think this was also the cause of the previous objstat stack overflow errors 21:32:57 <09gammafunk> but I'd have to test that (will do later) 21:33:14 <09gammafunk> possibly it's caused other issues in terms of performance 21:37:43 <04dracoomega> Of course, if this was only added less than two months ago, it can't be the cause of the old issues that had. ...and my memory was suddenly jogged about something that I went to dig up again https://github.com/crawl/crawl/pull/4650 21:38:32 <04dracoomega> (I couldn't remember what PR I recalled having something to do with this topic, but apparently it was this and never merged) 21:41:12 <04dracoomega> This seems completely reasonable to me, but I can't vet the lua specifics so well 21:52:26 <09gammafunk> yeah that's one I should take a look at 21:52:32 <09gammafunk> should be easier for me to understand now 21:54:28 Branch master updated to be equal with lua-stack-fix: 13https://github.com/crawl/crawl/tree/master 23:19:19 Rosstin (L16 PoEn) Crash caused by signal #11: Segmentation fault (Spider:2) 23:35:59 Unstable branch on crawl.develz.org updated to: 0.35-a0-28-g03f7a86f93 (34)