Cataclysm-dda: Unable to control vehicle lights

[petermorck]

On commit e111a0b.
Trying to toggle vehicle lights does nothing.
Lights don't come on.
Menu doesn't indicate they're on either as when you go into the menu again you still see the option to enable them.
Asked about it in forum and Coolthulu said someone mentioned it in a commit comment, but there was no issue about it, so here is one :)
Easily reproducible by creating new world, no mods, spawn vehicle, try to enable lights.

[mugling]

Marked confirmation required not because I don't believe it but because neither myself nor @Coolthulhu can reproduce. More details as to platform, mods and world details could be helpful. In particular does this affect new worlds?

[Coolthulhu]

Are messages like "activating x"/"deactivating x" printed?

I suspect it's the entry-action mapping that is wrong.
Keeping two separate vectors without explicitly linking their contents could result, for example, in size_t being optimized out (and left unassigned) somewhere. And size_t is unsigned int on unix based systems, but int on windows.

It looks like yet another uimenu issue. That class could need a refactor. Adding std::function<void()> on_selected to entries would prevent weirdness like this in the future. We'd need to make sure assigning it works, but only in one place.

[petermorck]

I'm playing tiles version on Windows 10.
Reproduced again just now by downloading http://dev.narc.ro/cataclysm/jenkins-latest/Windows/Tiles/cataclysmdda-0.C-5351.zip and unpacking to a new directory so it's a totally clean default setup.
Create new world. Create random character. Spawn Car. Control lights. Nothing happens.
There's no message printed or anything.
I can honk the horn and it prints the HOOOORNK.
I can start the engine. I can toggle cruise control.
Can toggle turret auto/manual.

[Coolthulhu]

A possible cause is get_parts. The const and non-const versions aren't equivalent.
The non-const version has !e.is_broken(), while const has !e.is_broken() > 0.
This invokes undefined behavior. !((int)false) = !0 is certainly not guaranteed to be positive, only not 0.

Unix type system tend to use unsigned values, while Windows prefers signed. If this is the case here, !0 would be a big positive number on Linux and BSD, but -1 (or -0?) on Windows.

[mugling]

@Coolthulhu do you want to try that as a one line-PR and we can push it to the buildbot and see if that resolves the issue given we can't reproduce locally?

[Coolthulhu]

Pushed one #17965
I think it should be safe to merge, even if it doesn't actually work.

[mugling]

Note also #17958 which I can't help but think may be related

[petermorck]

Tried
http://dev.narc.ro/cataclysm/jenkins-latest/Windows/Tiles/cataclysmdda-0.C-5352.zip
Still doesn't work.

[petermorck]

The vehicle lights work in build Build 5345 (9e7d371).
Don't work in Build 5346 (0267835).

[Coolthulhu]

The fact that it only affects the lights means it most likely is all in add_toggle lambda.
Which seems to be the only place where an inline lambda is used within a lambda.

I'd try those:

• Extract the inlined lambdas into named variables, pass them to add_toggle
• Extract add_toggle into a named function. Ugly, because it would take 5 or 6 arguments
• Convince someone on windows machine to debug it
• Add tons of debugmsgs everywhere, cross compile it for windows, run with wine (lastest resort)

[ProfoundDarkness]

Just curious if anyone has tried turning the stereo on/off? Or any other electrical components for that matter. I updated a while ago today (using windows tiles x64). I didn't really do much yesterday with vehicles so I don't know exactly when it started. I tried to turn my stereo off to get some sleep and it won't turn off. Tried turning on headlights, nope, also no dome lights and I can't turn off the minifridge either (not that I'd want to normally). I generally update once per day if I see something of note in the github commits. I am running with a collection of mods including blazemod (from forum).

Out of curiosity, since I saw it mentioned there could be issues between linux and windows, I ran a compile for linux (x64 ascii) with a copy of my save and mods. Same issue there. for reference my flags: RELEASE=1 LUA=1 CLANG=1 CCACHE=1

Running with that curiosity I stumbled around in debug, after removing my mods and save (so back to clean clone from git), spawned an RV on a new random character/world (no mods) and I couldn't turn any of the electronics on. Still on x64 linux ascii.

[mugling]

@Coolthulhu neither of us have tried building with RELEASE=1?

[Coolthulhu]

Unless stated otherwise, I always build with CLANG=1 RELEASE=1 LUA=1.

[mugling]

I can't reproduce with RELEASE=1 TILES=1

[mugling]

I'm starting to suspect we're dependent upon someone who can reproduce this inserting some debugmsg

[petermorck]

I'd love to try and help but I'd also love to get the VS2015 setup working as I really like all the help from the IDE. I tend to lose track of what I was doing when I have to jump around and look at definitions and other functions while following the code flow, and as I recall the debugger is really nice.
This is mine btw: http://smf.cataclysmdda.com/index.php?topic=13063.0

[mugling]

@Coolthulhu do jenkins builds include unit tests - we could push something via them to get some feedback without touching the main game binary?

[mugling]

@ProfoundDarkness are you absolutely certain that it's repeatable on 64-bit linux in a clean world as that considerably alters the approach to debugging. How about with a clean build (run ccache -C first)

[ghost]

Having the same problem on 64-bit linux in a virtualbox. Built g8eae8a3 (most recent) from scratch (cleared ccache, removed all object files just in case), made a clean modless world. The problem persists.

[mugling]

@BruceHoss what linux distro and I'll fire up a VM and see if I can step through it in gdb

[ghost]

@mugling openSuse 4.1.27-27-default

[petermorck]

Maybe I'm too lost to draw any conclusions. I finally managed to set up VS2015 and make a debug build that runs. I added a breakpoint at
for( auto e : get_parts( flag ) ) {
https://github.com/CleverRaven/Cataclysm-DDA/blob/master/src/vehicle.cpp#L954
When I select "Turn on headlights" in the menu, I end up there and I see the variable flag looks like an empty string ("").
Though I've never used or even seen lambda functions before, and I don't know how well the debugger handles them.
I'll keep looking.

[mugling]

@petermorck that's very interesting and suggests a scoping problem. Try changing const std::string &flag to std::string flag at vehicle.cpp:941

[petermorck]

Uhm.. I'm getting totally confused by the C++ magic debug stuff :)
Doing the change you suggested didn't help, however, it seems like the string is actually there, but the first character is replaced by null.
I'll paste a cropped screenshot like an idiot to show you what I'm looking at.

[mugling]

Try changing both lambda captures from by ref to by value:

• Before: actions.push_back( [&]{
• After: actions.push_back( [=]{

[petermorck]

That did it. Nice!
Was it the Myproxy being NULL that was hinting or just a guess since references were involved?

[mugling]

@petermock can you back out of the first change (const flag) and just leave the second (lambda capture) and check that's the issue.

@BevapDin you fancy being language lawyer to why that should work?

[petermorck]

Still works with const std::string &flag as argument.
Also dome lights and stereo work btw, toggling on and off.

[codemime]

I'm not a fancy language lawyer, but I believe when you pass a local argument by reference like this:
add_toggle( _( "headlights" ), keybind( "TOGGLE_HEADLIGHT" ), "CONE_LIGHT" )
it creates std::string (from char * "CONE_LIGHT") which gets destroyed right after add_toggle() finishes. Capturing the reference for later usage is definitely UB (referencing a destroyed object).

[mugling]

I'm fairly sure CONE_LIGHT should be in static storage though? It would be permissible to return it from vehicle::use_controls so why does the lambda fail?

[codemime]

I'm fairly sure CONE_LIGHT should be in static storage though?

char * "CONE_LIGHT" - certainly yes, but std::string made of it - maybe no (this may depend on compiler optimisations). I'd say It's rather dangerous to rely on them on this matter.

so why does the lambda fail?

It's referencing a destroyed object. That's what I'm pretty sure of looking on both the code and the screenshot.

[mugling]

That std::string is still in scope though?

[codemime]

No, Its scope is add_toggle() and once it finishes, its arguments are out of scope. The lambda is called after that.

[mugling]

It's a reference so it should refer to...

What is the lifetime of the std::string implicitly constructed from the static char *?

That could be either implementation specific or undefined behavior?

[petermorck]

Assuming the implicit std::string object gets deallocated as soon as add_toggle() exits, could it be that some implementations put a NULL at the string start while deallocating it (for cases like this) while some don't. So for the people that haven't seen the problem it may just have been dangling reference access that happened to still work since the string was still intact?

[mugling]

I would be surprised if the compiler spends the time to zero out the memory when it would be faster to just leave it and continue. Either way I think we have identified the cause.