Closed Gladuin closed 4 years ago
I'm getting this error on running dfhack
in the terminal, then stonesense
:
.dwarffortress/dfhack: line 89: 281534 Segmentation fault (core dumped) setarch "$setarch_arch" -R env LD_PRELOAD="$PRELOAD_LIB" ./libs/Dwarf_Fortress "$@"
The version of dfhack I'm using is DFHack version 0.47.04-beta1 (release) on x86_64
, and my system is Linux archlinux 5.6.3-arch1-1 #1 SMP PREEMPT Wed, 08 Apr 2020 07:47:16 +0000 x86_64 GNU/Linux
.
The issue seems to be exactly the same as: https://github.com/DFHack/dfhack/issues/1521#issuecomment-600003893
Same issue on DFHack version 0.47.04-r1 (release) on x86_64
Yeah, Stonesense hasn't been meaningfully changed in almost a year. I'm not sure if this issue is due to something that changed in 0.47 that DFHack hasn't caught yet (which would be surprising at this point, but fixable), an issue that needs to be addressed on the Stonesense side (which would be unlikely to happen any time soon unless someone steps up to maintain Stonesense), or something related to newer systems (in which case I would expect at least some more detail in stderr.log).
You wouldn't happen to have a stack trace from this to narrow it down, would you? If you have GDB installed, you could run ./dfhack -g
to attach GDB and hopefully generate one when it crashes.
It's a memory corruption from something being loaded improperly as soon as stonesense tries to initialize its assets:
stonesense.plug.dll!std::operator==<ItemConfiguration,std::default_delete
>(const std::unique_ptr<ItemConfiguration,std::default_delete > & _Left, void __formal) Line 2657 C++ stonesense.plug.dll!parseItemElement(TiXmlElement elemRoot, int basefile) Line 83 C++ stonesense.plug.dll!addSingleItemConfig(TiXmlElement elemRoot) Line 32 C++ stonesense.plug.dll!ContentLoader::parseItemContent(TiXmlElement elemRoot) Line 561 C++ stonesense.plug.dll!ContentLoader::parseContentXMLFile(const char filepath) Line 497 C++ stonesense.plug.dll!ContentLoader::parseContentIndexFile(const char filepath) Line 431 C++ stonesense.plug.dll!ContentLoader::parseContentIndexFile(const char * filepath) Line 436 C++
the filepath is : stonesense\items\index.txt
EDIT: grei_items.png is the culprit. At first I thought it might be because of the space in the subfolder's name so I replaced it with underscore, and I also try to open and re-save the PNG as 8-bits colors instead of 32-bits colors. But neither helped.
I think it crashes because of this :
contentLoader->itemConfigs[main_type]
, itemConfigs has a capacity of 0. You can't access cell 31 in an array or vector that has zero cells.What I don't understand though, is why the array is not enlarged beforehand. ItemConfiguration seems to be managed in an different way than, for example, CreatureConfiguration, which has a "push" mechanism instead of just accessing the cells by index.
https://github.com/DFHack/dfhack/issues/1521#issuecomment-600003893 also reaches the same conclusion.
I'll try to get this fixed.
29b430dc broke this by removing the part where it makes sure that the item array is the right length.
Not sure you're quoting the correct commit, as the one you quoted mentions TerrainMaterialConfiguration, whereas this issue is with ItemConfiguration ? (Even though the logic does seem similar and does seem to have been broken)
Oh, he changed a bunch of things, but I realized that his specific chances weren't what was causing it. Still trying to make sure I caught everything, 5b7e774 may fix it.
Your change does two things :
1) You changed the way the resizing is done
This first change is not really necessary. EDIT: It is necessary
2) You change the size of the array to _ENUM_LAST_ITEM(itemtype) +1 instead of _ENUM_LAST_ITEM(itemtype)
I think that's the solution to our issue. Before, if we encountered an item type which enum was going up to, let's say, 31, then we would resize the array to 31 instead of 32. Which means we could never go up to the cell at index 31! That was clearly a bug that needed to be fixed.
I shall end with a sour comment about how once again when looking at C++ code, there's a fatal bug in a simple operation because someone thought they were too smart to just use off-the-shelf, one-liner array primitives... and made a mistake in their custom 10-lines replacement. ;)
I've tested it. It works. Contrary to what I said, both changes are needed. And I cant explain why this works...
if (config.size() != (ENUM_LAST_ITEM(item_type) + 1))
config.resize(ENUM_LAST_ITEM(item_type) + 1);
for (size_t i = 0; i < config.size(); i++)
{
config[i] = nullptr;
}
...while this crashes :
uint32_t currentsize = (uint32_t)config.size();
if (currentsize < ENUM_LAST_ITEM(item_type) +1) {
currentsize = ENUM_LAST_ITEM(item_type) +1;
}
config.clear();
while (currentsize < config.size()) {
config.push_back(nullptr);
This is why I hate C++.
config = vector<whatever_type>(ENUM_LAST_ITEM(item_type) + 1, nullptr)
would probably be less error-prone than either of those
Ha ha I love how the hardened C++ dev answers "this would be less error-prone" rather than "look, those 10 lines are now one, I used a built-in function".
I mean, re-implementing built-in operations usually is more error-prone regardless of the language. I'm more inclined to blame the logic here rather than the language itself.
Looking again at the two snippets you posted, the main difference I'm seeing is that the first one (which you said works) resizes config to be exactly the desired size, while the second one resizes it to be at least the desired size - if it was larger before, it will still be larger. Just by looking at it, I don't see any way that it can crash by itself, but it could probably affect other parts of Stonesense in difficult-to-predict ways. (I would have guessed that the first snippet would be more likely to result in a crash later on, but it's hard to say without more context.)
Casting currentsize from a size_t to a uint32_t (which is sometimes smaller) is inadvisable, but probably unlikely to trigger any issues for smaller vectors.
Edit: oh, I think I see the issue in the second snippet - currentsize
is the desired size of the vector (it's a misleading name IMO), so if it's set to e.g. 5, it will never be smaller than config.size()
after config.clear()
is called (5 < 0
is false, so the body of the while loop will never run).
I'm all for using the actual builtin function for this.
Also DFhack really should have a ENUM_SIZE macro.
Ah yes, indeed, it should be while(config.size()<currentSize ) , not while(currentsize < config.size()). Mystery solved!
But that can be dumped altogether if the one-liner is used. So yes, I vote for that.
@RosaryMala not to be lazy but can you check that this way of doing things is not used in other XXXConfiguration.cpp files?
Yeah, I'll take a look at the rest of the things and make sure they're all working.
DFHack/stonesense@5b7e774 Can confirm this works. I've managed to build DFHack, with stonesense checkout to this commit, against a last version of Lazy Mac Pack (v0.47.04). It still crashes on any attemp to resize the window, but if you don't change it after launching stonesense it seems to run smoothly. It is also worth mentioning, that I didn't have xcode installed, XML::LibXML installation threw a bunch of errors and XML::LibXSLT didn't install at all. But i was so desperate that I tried the build, and it was OK.
My first time playing DF =D
Also I see that the keyboard control issue is still unsolved, and won't be? P.S. would be grateful for tips on bulding twbt, as the one included in the Pack refuses to work with latest build of DFHack.
We would probably have to move Stonesense to a standalone process to make keyboard input work (that would have other benefits, like crashes not bringing down DF, but would also need someone familiar with Stonesense and willing to put in the effort).
For reference, I'm pretty sure that the automated builds from https://dfhack.org/builds/ (like this one) include Stonesense.
Until this is fixed, could someone provide a brief walkthrough/workaround on how to get this working with say, the LNP? It would be much appreciated <3
If you follow my automated builds link from above, you should get to a page like this (this page may be outdated by the time you click the link, though, so I recommend using the first link instead). Under "Build artifacts", choose the build that matches your DF installation (e.g. "Windows64" for 64-bit Windows DF). Once that file has downloaded, extract "hack/plugins/stonesense.plug.(dll, so, or dylib)" from it and replace the file in your existing hack/plugins folder with it. Generally, when doing this with Stonesense, you should also replace the "stonesense" folder with the downloaded copy, but that shouldn't be necessary in this case. When you start DF, you'll probably see a warning that the plugin was built for a different DFHack build, but as long as it starts with "Warning", it's safe to ignore.
Perfect, thank you. I can confirm this works with PeridexisErrant's Starter Pack 0.47.04-r06.
Doesn't work for me :/
[DFHack]# load stonesense
Plugin stonesense was not built for this version of DFHack.
Plugin: 0.47.04-r1, DFHack: 0.47.03-beta1
You need to upgrade to DFHack 0.47.04-r1 (which involves upgrading to DF 0.47.04) - the plugin will only work for that version.
Assuming https://github.com/DFHack/stonesense/commit/5b7e7743a1372b929acb2ccb75080e139ac11691 fixed this, this should be addressed in 0.47.04-r2. Feel free to open a new issue if problems continue.
Hello,
A crash occurs upon loading StoneSense in DFHack 0.47.03-beta1 on Windows 10. Here is my
stderr.log
:EDIT: This also seems to happen on 0.44.12-r3