3

I hate to add to the myriad of questions about undefined behavior on stack overflow, but this problem has mystified me.

When using a range-based for loop on an unordered_map that is created inline, I get an unexpected result. When using the same loop on an unordered_map that was first assigned to a variable, I get the expected result.

I would expect both loops to print 1, but that is not what I observe.

Any help understanding what is happening would be appreciated. Thanks!

I'm running on Debian 10 with g++ 8.3.0

#include <algorithm>
#include <unordered_map>
#include <iostream>
#include <vector>

int main() {

        for (const int i : std::unordered_map<int, std::vector<int>> {{0, std::vector<int> {1}}}.at(0)) {
                std::cout << i << std::endl; //prints 0
        }

        std::unordered_map<int, std::vector<int>> map {
                {0, std::vector<int> {1}}
        };

        for (const int i : map.at(0)) {
                std::cout << i << std::endl; //prints 1
        }

}
rafix07
  • 20,001
  • 3
  • 20
  • 33
Dominic Abbott
  • 83
  • 6
  • 1
    It would seem that calling `at(0)` gives you a reference to the inner `std::vector`, which would extend the temporary lifetime, however the lifetime of the created `std::unordered_map` is not extended, which results in the destructor call (before you enter the first loop body), which eventually calls the destructor of the vector you obtained a reference to. I am not even sure whether the `at()` technically extends any lifetime in such case. My guess is that, by not obtaining any reference to the map, you end up with a dangling reference to its element. I, however, cannot tell for sure. – Fureeish Oct 18 '19 at 21:14
  • @Fureeish Interesting. I was under the impression that the map would be preserved during the loop because of this post: https://stackoverflow.com/questions/30448182/is-it-safe-to-use-a-c11-range-based-for-loop-with-an-rvalue-range-init – Dominic Abbott Oct 18 '19 at 21:20
  • 2
    Notice the important difference in the expansion of the *range-based for* loop. Pay attention to the `auto && __range = range-init;` part. It's a reference that will extend the lifetime of what was returned by `range-init`, if necessary. Here, your `range-init` is, ultimately, the result of `.at(0)` call. It (I believe) extends the lifetime of the retrieved vector (effectively preventing the execution from calling its destructor **due to reaching end of the full expression**), but that does not prevent the code from calling the same destructor **due to it being called from map's destructor**. – Fureeish Oct 18 '19 at 21:27
  • I'll try to verify my assumptions and come with an answer, if my thought process ends up being correct. – Fureeish Oct 18 '19 at 21:31
  • 1
    What Fureeish said. Also note that `at` [doesn't have](https://en.cppreference.com/w/cpp/container/unordered_map/at) a special overload for rvalue references (e.g. `T&& at(const Key& key) &&`) which would have allowed you to move the vector to `__range` before `unordered_map` containing it gets destroyed at the end of expression. – ofo Oct 18 '19 at 21:35
  • @Fureeish I think you might be right. There isn't a reference to the map, so it gets destructed, leaving a dangling reference to the vector. If you put your comments as an answer, I'll accept it. – Dominic Abbott Oct 18 '19 at 21:45
  • I'm finishing an answer below – Fureeish Oct 18 '19 at 21:45

1 Answers1

4

The problem lies in the fact that you create a temporary std::unordered_map and return a reference to one of its contents. Let's inspect two behaviours that occur here:

1. Range-based for expansion:

From the question you linked in the comments we can see that the following syntax:

for ( for-range-declaration : expression ) statement

translates directly to the following:

{
  auto && __range = range-init;
  for ( auto __begin = begin-expr,
             __end = end-expr;
        __begin != __end;
        ++__begin ) {
    for-range-declaration = *__begin;
    statement
  }
}

The important part is to understand that when you either create a temporary or provide an lvalue, a reference to it will be created (auto&& __range). If we're dealing with lvalues, we encouter the results we expect. However, when range-init returns a temporary object, things get a little more interesting. We encounter lifetime extension.

2. Lifetime extension:

This is way simpler than it may seem. If you return a temporary object to initialize (bind) a reference to it, the lifetime of said object is extended to match the lifetime of said reference. That means that if range-init returns a temporary, saving a reference to it (__range) extends the lifetime of that temporary up to the last curly bracket of the code I copy-pasted above. That's the reason for those most-outer brackets.

Your case:

In your case, we have quite a tricky situation. Inspecting your loop:

for (const int i : std::unordered_map<int, std::vector<int>> {{0, std::vector<int> {1}}}.at(0)) {
    std::cout << i << std::endl;
}

We must acknowledge two things:

  1. You create a temporary std::unordered_map.
  2. range-init is not referring to your std::unordered_map. It refers to what .at(0) retuns - a value from that map.

This results in the following consequence - the lifetime of your map is not extended. That means that its destructor will be called at the end of the full expression (at the ; from the auto && __range = range-init;). When std::unordered_map::~unordered_map is called, it calls destructors of everything it manages - e.g. its keys and values. In other words, that destructor call will call the destructor of the vector you obtained a reference to via the at(0) call. Your __range is now a dangling reference.

Fureeish
  • 12,533
  • 4
  • 32
  • 62