JLospinoso / ccc

Companion Code for C++ Crash Course
https://ccc.codes
GNU General Public License v3.0
376 stars 104 forks source link

Pages 295 and 296: Getting an unexpected behavior when try to capture local variable #98

Open carpajr opened 4 years ago

carpajr commented 4 years ago

Hello Josh.

I'm testing the book examples, and I'm stuck in chapter 10 when you describe an example that uses a local variable captured by a lambda function.

Page 295:

void alert_when_imminent() {
   int brake_commands_published{}; 
   AutoBrake auto_brake{
        [&brake_commands_published](const BrakeCommand&) {
           brake_commands_published++;  // --> segfault
    } };
--snip--

I'm intrigued about the correct way to use a local variable with lambdas. I tried to use a shared_pointer, but the var increment not work. :/

Could you help me?

carpajr commented 4 years ago

I dug a little more.

Some instructions in _alert_whenimminent, as the variable definition, are executed previously, of course, that the lambda function. However, It seems that stack unwinds the brake_commands_published before lambda does the increment, resulting in a segfault.

I'm not familiar with lambdas, and I don't know if that is made by the GCC compiler when doing the instruction scheduling, or it is the natural behavior of the lambdas. But the failure does not happen If I set _brake_commandspublished as a global variable.

I'm curious about how to handle this. :)

JLospinoso commented 4 years ago

Hi, @carpajr! Thanks for your ticket. Could you please provide me a full code listing? Or even better a link to a https://wandbox.org program?

carpajr commented 4 years ago

Hi, @JLospinoso! Sorry for delay.

I tried to reproduce the issue using wandbox, but there works fine. However, I'm using GCC 7.4.0 (Ubuntu 18.04.1) and finding some trick optimization that CLION is doing. Because when I run the same code*, the code has a segmentation fault.

[+] Test initial speed is 0 successful.
[+] Test initial sensivity is 5 successful.
[+] Test sensitivity greater than 1 successful.
[+] Test speed is saved successful.
Segmentation fault
JLospinoso commented 4 years ago

Thanks for posting, @carpajr!

I think this might be some sort of CLion bug. I believe the issue is that const reference publish in AutoBrake doesn't have the proper lifetime. When you bind a temporary (like the lambda on line 137) to a const reference, it should work out as you'd expect (see https://blog.galowicz.de/2016/03/23/const_reference_to_temporary_object/). I'll keep investigating.

hlg commented 2 years ago

In the closing commit for errata, I can't find any change regarding chapter 10. Is there a follow-up on this issue? I am also observing it, not with CLion, but with plain g++ 9.3.0 on Ubuntu 20.04.3 LTS.

sihyeon-kim commented 2 years ago

A simple solution to this is below. And my environment is g++ 7.5.0 with -std=c++17 option.

void alert_when_imminent() {
  int brake_commands_published{};
  auto foo = [&brake_commands_published](const BrakeCommand &) {
    brake_commands_published++;
  };
  AutoBrake auto_brake{foo};
// -- snip --
}

IMHO, lambda expression in AutoBrake object can't capture out of object's scope.

JLospinoso commented 1 year ago

I'll leave this open - honestly I'm not seeing the lifetime issue here. If someone has an explanation I'll post an errata otherwise leaving as-is for now.

dascandy commented 1 year ago

AutoBrake has a const-ref to some object. The object is created in the line where AutoBrake is created, but does not get lifetime extension due to const-ref binding since the const-ref that would get that is the argument, not the member.

AutoBrake needs to change to hold the lambda either by value, or in a std::function, or something similar in a way that it owns it. Right now nobody owns that lifetime. @sihyeon-kim 's suggestion fixes it by moving the ownership to the caller, but it is (IMHO) somewhat unclean since the type itself should handle the lifetime of its members, not the environment.

spoxus commented 1 year ago

sihyeon-kim's solution worked for me.