views:

84

answers:

2

I've just been bitten by a nasty undefined behavior due the returning a reference to a local variable.

We know it's evil, and generally the compiler prints a nice warning to tell us so... well gcc (3.4.2) does not seem to push the checks too far though.

std::string get_env_value(std::string const& key);

std::string const& get_phase()
{
  std::string const& phase = get_env_value("PHASE"); // [1]
  std::cout << "get_phase - " << phase << '\n';
  return phase;                                      // [2]
}

This compiles without a glitch, and yet we fall in the nasty realm of undefined behavior.

Line [1] is okay because the standard specifies that the lifetime of a variable bound to a const reference should be extended to match the lifetime of the const reference.

Line [2] seems okay too...

  • Do the C++ specifications cover this case ?
  • Does anyone know if this is usually diagnosed ? (I may miss a flag or something...)

It seems to me that static analysis should be able to tell that having use a "lifetime extension" for [1], [2] is not safe, but it could get ugly rapidly I guess...

+5  A: 

The standard does not cover [2]. It allows an rvalue to be bound to a const reference, but that doesn't allow you to return a const reference and have the lifetime of the rvalue it is bound to extended as well.

And true, static analysis could catch this, but as always it's a trade-off. C++ compilation is slow enough as it is, so compiler writers have to weigh the benefits of further static analysis which might allow them to produce better diagnostics, against the increased compilation time.

jalf
Can lint be set up to trap this?
Steve Townsend
@jalf: I agree that compilation is slow enough, compiler do warn about missing returns and returning reference to local variables though, I somehow expected that `phase` would raise the warning :/
Matthieu M.
+2  A: 
  1. No, I don't think Standard mentions / covers this specific case.

  2. VS 2010 gives compilation warnings (/Za, /W4).

So, definitely it looks to be a diagnosable condition.

So, I tweaked the function slightly as follows, just to create multiple return paths:

std::string const& get_phase() 
{ 
    std::string const& phase = get_env_value("PHASE"); // [1] 
    std::cout << "get_phase - " << phase << '\n'; 

    if(1){
        while(1){
            return phase;
        }
    }
    return phase;                                      // [2] 
} 

Now, VS does not report the warning as earlier.

As an example, at a first glance, it looks that it should be easy for the compiler to detect and catch that not all paths return a value. But compilers (e.g. VS) do not.

int get_phase() 
{
    char ch;
    if(ch){
        return 0;
    }
     // nothing returned from here.
} 

So, I guess the code in OP has probably the same complexities to diagnose the condition as the example shown just above, though I am not sure. The only good thing is that the Standard is clear on this case.

$6.6.3/2 - "Flowing off the end of a function is equivalent to a return with no value; this results in undefined behavior in a value-returning function."

Coming back to the code in OP, I guess the standard does not mandate this condition to be a diagnosable condition, and hence compilers are free to do as they please. It is basically understood that the returned reference points to an object which is already destructed. So accessing such an object will lead to an undefined behavior

Chubsdad
@Chusbad: most compilers do warn about functions without return (sometimes abusively, but only in corner cases). I didn't really expected an error... but a warning would be very nice :)
Matthieu M.