14

While testing some functionality with std::thread, a friend encountered a problem with GCC and we thought it's worth asking if this is a GCC bug or perhaps there's something wrong with this code (the code prints (for example) "7 8 9 10 1 2 3", but we expect every integer in [1,10] to be printed):

#include <algorithm>
#include <iostream>
#include <iterator>
#include <thread>

int main() {
    int arr[10];
    std::iota(std::begin(arr), std::end(arr), 1);
    using itr_t = decltype(std::begin(arr));

    // the function that will display each element
    auto f = [] (itr_t first, itr_t last) {
        while (first != last) std::cout<<*(first++)<<' ';};

    // we have 3 threads so we need to figure out the ranges for each thread to show
    int increment = std::distance(std::begin(arr), std::end(arr)) / 3;
    auto first    = std::begin(arr);
    auto to       = first + increment;
    auto last     = std::end(arr);
    std::thread threads[3] = {
        std::thread{f, first, to},
        std::thread{f, (first = to), (to += increment)},
        std::thread{f, (first = to), last} // go to last here to account for odd array sizes
    };
    for (auto&& t : threads) t.join();
}

The following alternate code works:

int main()
{
    std::array<int, 10> a;
    std::iota(a.begin(), a.end(), 1);
    using iter_t = std::array<int, 10>::iterator;
    auto dist = std::distance( a.begin(), a.end() )/3;
    auto first = a.begin(), to = first + dist, last = a.end();
    std::function<void(iter_t, iter_t)> f =
        []( iter_t first, iter_t last ) {
            while ( first != last ) { std::cout << *(first++) << ' '; }
        };
    std::thread threads[] {
            std::thread { f,  first, to },
            std::thread { f, to, to + dist },
            std::thread { f, to + dist, last }
    };
    std::for_each(
        std::begin(threads),std::end(threads),
        std::mem_fn(&std::thread::join));
    return 0;
}

We thought maybe its got something to do with the unsequenced evaluation of function's arity or its just the way std::thread is supposed to work when copying non-std::ref-qualified arguments. We then tested the first code with Clang and it works (and so started to suspect a GCC bug).

Compiler used: GCC 4.7, Clang 3.2.1

EDIT: The GCC code gives the wrong output with the first version of the code, but with the second version it gives the correct output.

Mankarse
  • 39,818
  • 11
  • 97
  • 141
iamOgunyinka
  • 1,040
  • 1
  • 9
  • 17
  • 1
    I don't understand the problem. Which code is problematic, the one in the pastebin or the one posted here? In any case, please post the one that is currently on pastebin here. – filmor Aug 04 '14 at 09:15
  • I'm sorry I didn't specify well. The pastebin code is the one with the problem. I'm using my cellphone and its difficult to type with it – iamOgunyinka Aug 04 '14 at 09:19
  • 1
    And what output did you get, and what did you expect? – Useless Aug 04 '14 at 09:20
  • The pastebin code outputs-> "7 8 9 10 1 2 3" so "4 5 6" went missing – iamOgunyinka Aug 04 '14 at 09:22
  • Have you tried to pass the arguments to the thread constructor in the same way as you do it in the posted code? It could be that the comma inside an array constructor is not a sequence point, so it could be that in both the second and the third thread `first` points to `to + dist`. – filmor Aug 04 '14 at 09:27
  • I have tried this, this is actually the problem, I'll write up an answer for that. – filmor Aug 04 '14 at 09:34
  • @filmor There are no sequence points in C++11. The order of evaluation inside a braced-init-list is fully defined though, as if there were a sequence point at each of the (top-level) `,`. – dyp Aug 04 '14 at 09:42
  • 5
    Possibly [this bug](https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51253), which would make this question a rough duplicate of [this one](http://stackoverflow.com/q/14060264) – dyp Aug 04 '14 at 09:44
  • 3
    @dyp is correct -- it's a bug in gcc, it's fixed in the latest release – Jonathan Wakely Aug 04 '14 at 13:25
  • I'd also suspect `std::cout` not being locked when used in `f`. When you have multiple threads using it for output in parallel, you might get strange results. – jotik Oct 13 '14 at 18:51
  • It outputs `1 2 3 4 5 6 7 8 9 10` as exepected when compiled with gcc-4.9.2. – Maxim Egorushkin Nov 10 '14 at 15:02

1 Answers1

1

From this modified program:

#include <algorithm>
#include <iostream>
#include <iterator>
#include <thread>
#include <sstream>

int main()
{
    int arr[10];
    std::iota(std::begin(arr), std::end(arr), 1);
    using itr_t = decltype(std::begin(arr));

    // the function that will display each element
    auto f = [] (itr_t first, itr_t last) {
        std::stringstream ss;
        ss << "**Pointer:" << first  << " | " << last << std::endl;
        std::cout << ss.str();
        while (first != last) std::cout<<*(first++)<<' ';};

    // we have 3 threads so we need to figure out the ranges for each thread to show
    int increment = std::distance(std::begin(arr), std::end(arr)) / 3;
    auto first    = std::begin(arr);
    auto to       = first + increment;
    auto last     = std::end(arr);
    std::thread threads[3] = {
        std::thread{f, first, to},
#ifndef FIX
        std::thread{f, (first = to), (to += increment)},
        std::thread{f, (first = to), last} // go to last here to account for odd array sizes
#else
        std::thread{f,  to,  to+increment},
        std::thread{f,  to+increment, last} // go to last here to account for odd array sizes
#endif
    };
    for (auto&& t : threads) {
        t.join();
    }
}

I add the prints of the first and last pointer for lambda function f, and find this interesting results (when FIX is undefined):

**Pointer:0x28abd8 | 0x28abe4
1 2 3 **Pointer:0x28abf0 | 0x28abf0
**Pointer:0x28abf0 | 0x28ac00
7 8 9 10

Then I add some code for the #ELSE case for the #ifndef FIX. It works well.

- Update: This conclusion, the original post below, is wrong. My fault. See Josh's comment below -

I believe the 2nd line std::thread{f, (first = to), (to += increment)}, of threads[] contains a bug: The assignment inside the two pairs of parenthesis, can be evaluated in any order, by the parser. Yet the assignment order of 1st, 2nd and 3rd argument of the constructor needs to keep the order as given.

--- Update: corrected ---

Thus the above debug printing results suggest that GCC4.8.2 (my version) is still buggy (not to say GCC4.7), but GCC 4.9.2 fixes this bug, as reported by Maxim Yegorushkin (see comment above).

Robin Hsu
  • 4,164
  • 3
  • 20
  • 37
  • Similarly, for an expression (a+b)*(c+d), the two parenthesis pairs can be executed in any order as the compiler prefers. – Robin Hsu Nov 10 '14 at 10:34
  • Robin Hsu, according to the C++ standard and [CppReference](http://en.cppreference.com/w/cpp/language/eval_order), it states that `In list-initialization, every value computation and side effect of a given initializer clause is sequenced before every value computation and side effect associated with any initializer clause that follows it in the comma-separated list of the initializer list.` So, saying `The compiler can choose which parenthesis to evaluate first` is incorrect. – iamOgunyinka Nov 13 '14 at 09:53
  • Josh, you are right. I have corrected it in my answer. Sorry for everyone. – Robin Hsu Nov 14 '14 at 02:06