Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
carpajr opened this issue Jan 5, 2020 · 9 comments

Comments

@carpajr
Copy link

carpajr commented Jan 5, 2020

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
Copy link
Author

carpajr commented Jan 5, 2020

I dug a little more.

Some instructions in alert_when_imminent, 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_commands_published as a global variable.

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

@carpajr carpajr changed the title Getting an unexpected behavior when try to capture local variable Pages 295 and 296: Getting an unexpected behavior when try to capture local variable Jan 5, 2020
@JLospinoso
Copy link
Owner

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?

JLospinoso pushed a commit that referenced this issue Jan 9, 2020
@carpajr
Copy link
Author

carpajr commented Jan 21, 2020

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
Copy link
Owner

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
Copy link

hlg commented Oct 18, 2021

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.

@JLospinoso JLospinoso reopened this Oct 23, 2021
@sihyeon-kim
Copy link

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.

DomasAquinas added a commit to DomasAquinas/cpp-crash-course that referenced this issue Jun 6, 2022
@JLospinoso
Copy link
Owner

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
Copy link
Collaborator

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
Copy link

spoxus commented Apr 23, 2023

sihyeon-kim's solution worked for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants