6

The following code works in clang++, but crashes spectacularly in g++

#include<vector>
#include<iostream>

template<class Iterator>
double abs_sum(double current_sum, Iterator it, Iterator it_end){
    if (it == it_end)
        return current_sum;
    return abs_sum(current_sum+std::abs(*it),++it,it_end);
}


int main(int argc, char** argv){
    std::vector<double> values {1.0, 2.0,-5};

    std::cout << abs_sum(0.0,values.begin(),values.end()) << std::endl;;
}

The culprit turns out to be this line: 

return abs_sum(current_sum+std::abs(*it),++it,it_end);

in clang, *it is evaluated before ++it, in g++ it's the reverse, causing the iterator to be increased before being dereferenced. It turns out that the order in which function arguments are evaluated is implementation defined.

My question is: How do I catch this type of error? Ideally I want to have an error or at least a warning when I'm accidentally depending on implementation specific details.

Neither clang nor gcc produces any warnings, even with -Wall.

P.W
  • 24,743
  • 6
  • 32
  • 69
LKlevin
  • 331
  • 1
  • 7
  • 1
    Perform the operations in the proper order before calling the function. – François Andrieux Dec 14 '18 at 10:46
  • 2
    Define _"crashes spectacularly"_ please :D Does it includes sounds and lights? – YSC Dec 14 '18 at 10:59
  • 1
    Add another unit test. – Bernhard Dec 14 '18 at 11:24
  • 1
    @Bernhard A unit test will not necessarily help you here. If you have UB then anything can happen and you cannot depend on a test to catch that. This is the hardest category of bug to find, and ultimately it is really just down to code review and _pure chance_. I _do_ agree that such a test should exist, because if you run your unit tests frequently then you increase the chances of this bug popping up and therefore being discovered; however, it's more than possible that the bug only shows symptoms when part of a more complex program. – Lightness Races in Orbit Dec 14 '18 at 11:29
  • 1
    These are hard to catch, we can find all sorts of fun case such as [this one](https://stackoverflow.com/q/27158812/1708801) and [this one](https://stackoverflow.com/q/33598938/1708801) that are hard to catch. That was a motivation for [changing the evaluation order rules in C++17](https://stackoverflow.com/q/38501587/1708801) but we did not get this specific one nailed down though. – Shafik Yaghmour Dec 14 '18 at 14:13
  • Well it crashed in a number of different ways, depedning on the optimization level and the choice of sanitizer. Everything from segfault to just randomly exiting. Messing around in memory always give such fun results. – LKlevin Dec 14 '18 at 15:29
  • @FrançoisAndrieux Fixing the bug was easy. Finding it was hard. – LKlevin Dec 14 '18 at 15:31

3 Answers3

4

My question is: How do I catch this type of error?

You don't. Undefined behaviour is undefined. You cannot catch it...

... but some tools could help you:

  • your compiler: all warnings enabled (g++/clang++ -Wall -Wextra -pedantic is a good start);
  • cppcheck;
  • clang-analizer;

They provide no guarantee though. This is why C++ is hard. You have (you, the coder) to know best and don't write UB. Good luck.

YSC
  • 34,418
  • 7
  • 80
  • 129
3

Unfortunately, even with -Wextra (remember, -Wall is more like -Wsome and thus insufficient), there is no warning for this, which is a bit disappointing.

In a more trivial case, with a primitive, where the race* is more obvious to the compiler:

void foo(int, int) {}

int main()
{
    int x = 42;
    foo(++x, x);
}

… you are warned:

main.cpp: In function 'int main()':
main.cpp:6:9: warning: operation on 'x' may be undefined [-Wsequence-point]
     foo(++x, x);
         ^~~
main.cpp:6:9: warning: operation on 'x' may be undefined [-Wsequence-point]

(* not a real race but you know what I mean)

But it's just too tough for the compiler to "know" that your operations on the iterator are a read and a write respectively.

Ultimately I'm afraid you will have to rely on testing and wits and gumption. :)

Lightness Races in Orbit
  • 358,771
  • 68
  • 593
  • 989
  • I had hoped for something that checked for multiple calls to non-const functions on the same object. A few false positives would be an acceptable price. I was also surprised the address sanitizer didn't catch when the iterator went out of bounds. – LKlevin Dec 14 '18 at 15:37
  • @LKlevin I think compiler devs have decided that said false positives would not be an acceptable price. But it might be interesting to explore more. – Lightness Races in Orbit Dec 14 '18 at 15:43
2

What you have initially is not undefined behavior but unspecified behavior. The compiler is not required to issue any diagnostic for unspecified behavior.

Order of evaluation of the operands of almost all C++ operators (including the order of evaluation of function arguments in a function-call expression and the order of evaluation of the subexpressions within any expression) is unspecified. The compiler can evaluate operands in any order, and may choose another order when the same expression is evaluated again.

But the result of this unspecified behavior in this case leads to dereferencing of end iterator which in turn leads to undefined behavior.

GCC and Clang do not have any general compiler option to issue diagnostics for unspecified behavior.

In GCC there is the option fstrong-eval-order which does the following:

Evaluate member access, array subscripting, and shift expressions in left-to-right order, and evaluate assignment in right-to-left order, as adopted for C++17. Enabled by default with -std=c++17. -fstrong-eval-order=some enables just the ordering of member access and shift expressions, and is the default without -std=c++17.

There is also the option -Wreorder (C++ and Objective-C++ only) which does this:

Warn when the order of member initializers given in the code does not match the order in which they must be executed

But I do not think these options will be helpful in your particular case.

So in this particular case, you can perform operations in the intended order.

P.W
  • 24,743
  • 6
  • 32
  • 69