Segmentation fault (core dumped) when adding new setting to Setup

Discussion around programming R'n'D, its source code and its tools.

Moderators: Flumminator, Zomis

Post Reply
mat
Posts: 43
Joined: Sun Sep 20, 2020 8:24 am

Segmentation fault (core dumped) when adding new setting to Setup

Post by mat »

As an example, I've added "Show level preview" option to Setup -> Graphics. I've inserted all needed things to some files (files.c, screens.c, tools.c, system.h). The compilation is ok but when I want to run the compiled game, a message appears: "Segmentation fault (core dumped)". Oddly enough, the game works at the sacrifice of removing other option (for example "Small game graphics").

I know what the message means but I can't find a source of the problem. Could anyone help me? I try to launch on Linux Mate 20.04.1 LTS (Focal Fossa) 64-bit
User avatar
Holger
Site Admin
Posts: 3489
Joined: Fri Jun 18, 2004 4:13 pm
Location: Germany
Contact:

Re: Segmentation fault (core dumped) when adding new setting to Setup

Post by Holger »

Hi mat, to check what went wrong, it would be very helpful to be able to reproduce the problem! To do this, please add a patch (or output of "git diff") and the commit hash against which the patch should be used (if you are not using the latest release version).

A segmentation fault is *very* easy to create in C, and although I assume that most of your changes are just fine, there is probably a tiny little bug with a big effect. :-)

You could also start the game inside "gdb" (the GNU debugger) and type "bt" after the crash to get the back trace, so you should get an idea what went wrong (and where).

If you provide me with the patch, I will have a look at all that.
mat
Posts: 43
Joined: Sun Sep 20, 2020 8:24 am

Re: Segmentation fault (core dumped) when adding new setting to Setup

Post by mat »

Hash: 84a28d5bf05056e1a70eeb37ff6b73aca21c5533
Patch below.

Thank you in advance :)
Attachments
show_level_preview.patch
(2.2 KiB) Downloaded 13 times
filbo
Posts: 499
Joined: Fri Jun 20, 2014 10:06 am

Re: Segmentation fault (core dumped) when adding new setting to Setup

Post by filbo »

I was able to reproduce this, and I think it is not a bug in mat's code!

After a bit of debugging, nothing was making sense, so I did:

$ make clean
$ make

and then it worked fine. It appears the bug is in Holger's makefiles, which do not declare any dependencies on src/lib/system.h, which is easily demonstrated:

$ make
$ touch src/libgame/system.h
$ make

(2nd `make` doesn't build anything)

=====

After a little further investigation, it turns out you MUST run `make depend` before any of this stuff will work!

Holger: please have the Makefile either automatically run `make depend` if src/.depend doesn't exist, or at least error out and tell the user to do so.

I know it's labeled a 'development target', but if someone is compiling at all, they're basically doing development! And I think if they were merely downloading your source tarball and running `make`, this could still screw them up. Suppose tarball 1 and 2 differ only in some header file. They extract tarball 1, `make`, extract 2, `make`, and nothing is compiled. Unless the tarball is created with all timestamps set to tarball-creation-time, which they shouldn't be.

This seems to do it. (It's incomplete as it doesn't check for src/*/.depend, but that doesn't seem important):

Code: Select all

$ git diff -U1 Makefile
diff --git a/Makefile b/Makefile
index 3e5877f..b1da5fa 100644
--- a/Makefile
+++ b/Makefile
@@ -62,3 +62,3 @@ MAKE_CMD_ANDROID = $(MAKE) -C $(ANDROID_DIR)
 
-all:
+all: $(SRC_DIR)/.depend
        @$(MAKE_CMD)
@@ -119,3 +119,3 @@ tags:
 
-depend dep:
+depend dep $(SRC_DIR)/.depend:
        $(MAKE_CMD) depend
filbo
Posts: 499
Joined: Fri Jun 20, 2014 10:06 am

Re: Segmentation fault (core dumped) when adding new setting to Setup

Post by filbo »

(mat: should be sufficient for you to run `make dep; make`)
mat
Posts: 43
Joined: Sun Sep 20, 2020 8:24 am

Re: Segmentation fault (core dumped) when adding new setting to Setup

Post by mat »

filbo wrote: Wed Nov 11, 2020 7:43 am (mat: should be sufficient for you to run `make dep; make`)
It works! :D
Thank you, filbo :)
User avatar
Holger
Site Admin
Posts: 3489
Joined: Fri Jun 18, 2004 4:13 pm
Location: Germany
Contact:

Re: Segmentation fault (core dumped) when adding new setting to Setup

Post by Holger »

Hi filbo, thank you for sorting things out for mat! :D

Yes, that's right: When modifying the code, you should use "make clean ; make" to rebuild, as there are only rules to rebuild if source files are newer than the compiled (object file) counterparts, but not for source and header files. To prevent having to use "make clean" every time (and always rebuild everything), there's "make depend" (or "make dep") to create the header file dependencies (which should be called every time you change the "include logic" of the header files, that is, change "#include" statements in source files or add or rename header files).

Sorry, mat, that I forgot about this and was thinking about a potential bug in your code! :shock:
I know it's labeled a 'development target', but if someone is compiling at all, they're basically doing development!
When just building the binary from source (without modifying the code itself), you do not need the "make dep".
And I think if they were merely downloading your source tarball and running `make`, this could still screw them up. Suppose tarball 1 and 2 differ only in some header file. They extract tarball 1, `make`, extract 2, `make`, and nothing is compiled. Unless the tarball is created with all timestamps set to tarball-creation-time, which they shouldn't be.
Not sure about the last sentence, but when creating a tarball (like using "tar cvzf foo.tar.gz foo"), the source files always have timestamps that are older than your currently built object or binary files, so "make" will never rebuild the code then (after having extracted a different tarball). So, when extracting different versions over another (which I would rather not do, to prevent a mixture of unused files that do not belong to the current code base), you should always do a "make clean" to ensure everything will be correctly built.
This seems to do it. (It's incomplete as it doesn't check for src/*/.depend, but that doesn't seem important):
This looks like a good solution, thanks! (That way, a manual "make dep" would only be required in the above cases.)
filbo
Posts: 499
Joined: Fri Jun 20, 2014 10:06 am

Re: Segmentation fault (core dumped) when adding new setting to Setup

Post by filbo »

With the sequential tarball situation I described:

If the user downloaded tarball 1 right around when it was issued, and immediately built from it, their .o files would be dated then. If tarball 2 was issued later than that, presumably at least one source file would be newer, so a new `make` after extracting it on top of the old built directory would rebuild things.

You are right that this whole setup is rickety. If they did the same steps, but were downloading from a historical archive, all of the files in tarball 2 would be older than their .o files, and nothing would be rebuilt.

But for normal simple user operation -- downloading each tarball as you announce it, building, playing, repeating when you announce a new one -- the simple procedure should work. Except won't, if they haven't done `make dep` and you changed any structure shapes in a header.

Noted: this Makefile change is not in 4.3.0.4; nor changes to make 'autowhat' work on levelsets with spaces in their names. I feel like there are probably a couple of other patches or ideas I've sent which I expect to come out in the code eventually. Of course there is no obligation for you to do any of those things! I 'expect' in the sense that past occurrences lead me to believe that they will eventually arrive...
User avatar
Holger
Site Admin
Posts: 3489
Joined: Fri Jun 18, 2004 4:13 pm
Location: Germany
Contact:

Re: Segmentation fault (core dumped) when adding new setting to Setup

Post by Holger »

If the user downloaded tarball 1 right around when it was issued, and immediately built from it, their .o files would be dated then. If tarball 2 was issued later than that, presumably at least one source file would be newer, so a new `make` after extracting it on top of the old built directory would rebuild things.
Yes, that scenario is somewhat plausible, so something like that might indeed happen.
But for normal simple user operation -- downloading each tarball as you announce it, building, playing, repeating when you announce a new one -- the simple procedure should work. Except won't, if they haven't done `make dep` and you changed any structure shapes in a header.
That's right, but this should not happen, as each package has its version number in the extracted directory name, so extracting into the previous version's directory should never happen (unless you rsync files to the old directory, which sounds crazy ;-) ).

So, if you download, extract and build by typing "make", everything should be fine.

Developing is another thing, and I should point out the need for executing "make dep" whenever header files have changed.
Noted: this Makefile change is not in 4.3.0.4;
No, it was too late for it, as 4.2.0.4 was already on its way to be released (and I try to not add any "last minute" changes anymore, as this went wrong too often). I also think about moving it from "Makefile" to "src/Makefile"...
nor changes to make 'autowhat' work on levelsets with spaces in their names
Sorry, haven't answered that PM yet... I'm afraid that supporting level set names with spaces for the "autoplay", "autotest" etc. commands is not possible in a clean way, as I think that it would currently be impossible to find a generic solution instead of an ugly workaround that works for only some level set names. Just think of commands like "autotest Mines 4 Freaks 001 002 003" or "autotest Fahrenheit 451 001 002 003"... It just does not work in a reliably detectable way, unfortunately. Call me oldschool, but I generally still avoid special characters in file and directory names if there is a probability that I will frequently use them on the command line... that's also the reason for my level set directory name convention like "rnd_cool_caves_01" instead of "R'n'D Cool Caves 01" (which works generally fine with R'n'D, but would need some sort of escaping for the current command line logic).
filbo
Posts: 499
Joined: Fri Jun 20, 2014 10:06 am

Re: Segmentation fault (core dumped) when adding new setting to Setup

Post by filbo »

For whatever reason, my system's Zelda levels are named 'Legend Of Zelda' (2), so autowhat fails on them. This may be a misconfiguration retained from some years-ago download of the LoZ levels as separate data, possibly not from artsoft.org. Thus my efforts to 'fix' RnD to handle it.
User avatar
Holger
Site Admin
Posts: 3489
Joined: Fri Jun 18, 2004 4:13 pm
Location: Germany
Contact:

Re: Segmentation fault (core dumped) when adding new setting to Setup

Post by Holger »

As far as I remember, the "original" Zelda level set directory names (and also those from the packages downloadable from artsoft.org) are "zelda" and "zelda2", but I know that there are quite some sets (some downloadable from bd-fans.com, for example) that do contain directory names like "My Cool Levels From 2020" etc., and while this should generally be no problem for "normal users" (which just play levels), it is a problem for the "autotest" (and related) command line tools for which I do not have a proper solution for now.

A solution could be to extend the command line syntax to something like the following:

Before (does not work):

$ ./rocksndiamonds -e "autotest My Cool Levels From 2020 001 002 003"

After (would work):

$ ./rocksndiamonds --command "autotest" --level-set "My Cool Levels From 2020" --levels "001 002 003"

Until then, just renaming the level set in question (to "My_Cool_Levels_From_2020", for example) seems to be the quickest and easiest solution...
filbo
Posts: 499
Joined: Fri Jun 20, 2014 10:06 am

Re: Segmentation fault (core dumped) when adding new setting to Setup

Post by filbo »

Renaming levelsets means also renaming ~/.rocksndiamonds/tapes/... directories and I'm not sure what else. Partly I wasn't sure if there were any *.conf files inside the levelset which might break if changed. (Of course I could/should have looked into that...!)

The change I submitted would allow levelsets with spaces in their names, as long as they didn't have space-digit; so the two I'm concerned about, 'Legend Of Zelda' and 'Legend Of Zelda II' would be fine (since luckily the '2' is in Roman Numerals!)

Anyway, since my levelsets with spaces in their names are basically a configuration bug, forget about it, I'll just rename things.
Post Reply